Conversation
Initial test with 1.43.0-SNAPSHOT
…ken care of at artifactory level)
…de (coming back to their original pre-1.42 state)
… is not true' due to CALCITE-7636
… is not true' due to CALCITE-7636
… is not true' due to CALCITE-7636
…29 TIMESTAMP literals are now preserved as TimestampString at declared precision instead of round-tripping through millisecond-precision runtime values, so zero-fractional literals now render with a full .000000000 nanosecond suffix (semantically identical)
…rg_orc2/4/5/6/7.q.out
… bugfix on Aggregate)
…o_query88.q (probably due to CALCITE-7722)
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Initial test with 1.43.0-SNAPSHOT to detect early any potential issue before the official 1.43 is released.
Code adjustments:
0 ) Cleanup Bug.java
CALCITE_7641_FIXED(fixed in 1.43), and related code in HiveMaterializedViewRule.java (which is now unnecessary).A) Implement new methods in
HiveRelShuttleImpldue to CALCITE-7511 . Due to the same reason, some visitors inPlanModifierForASTConvandHiveRelOptMaterializationValidatorneeded some adjustment.B) Due to CALCITE-7636 (see point 3 below), we need to adjust HiveRowIsDeletedPropagator because the visit(HiveFilter) method expected a condition with the form
OR(<(N, $t1.writeid), <(N, $t2.writeid))and now it will beIS_NOT_TRUE(AND(<=($t1.writeid, N), <=($t2.writeid, N))). Issue seen on: materialized_view_create_rewrite_6.q, materialized_view_create_rewrite_6_aggr_2joins.q, materialized_view_create_rewrite_6_aggr_3joins.q, materialized_view_create_rewrite_9.qC) Probably due to the same ticket, it seems now
ExprNodeDescUtils#orinternalsplit's flattening / deduplication may collapse the operand list, resulting in one single operand, e.g. when the expression comes from a (valid)RexCall(OR, [FALSE, FALSE]), which will lead to an exception onGenericUDFOPOr#initialize(UDFArgumentLengthException: The operator 'OR' accepts at least 2 arguments). Therefore we need to add a check for this scenario inExprNodeDescUtils#or(and alsoExprNodeDescUtils#and, where this should be theoretically possible as well). Issue seen on the same materialized_view_create_*.q files as the previous point.D) TO BE REVIEWED AlterMaterializedViewRebuildAnalyzer.java fix. Root cause: Calcite 1.43's MaterializedViewRule changed how it decides which rewrite to emit. For a rebuild query like SELECT ... FROM src WHERE ... where an MV mat already covers that exact query, 1.43 emits the trivial view-only rewrite TableScan(mat) (i.e. the plan collapses to a single scan of the target MV). 1.42 emitted a Union rewrite Union(scan(mat), scan(delta_from_src)). Why that breaks rebuild: the analyzer feeds the rewritten plan into INSERT OVERWRITE mat .... If the rewritten plan is scan(mat) alone, the statement executes as INSERT OVERWRITE mat SELECT * FROM mat, throwing away any delta that has accumulated in source tables since the previous rebuild — which is precisely the delta the rebuild was meant to fold in. The fix, two parts:
Guard that rejects the trivial rewrite (added at the point right after MV rewriting has been applied and downstream logic is about to accept the rewritten basePlan): if basePlan no longer references any of the source tables that appear in the original query (getTablesUsed(basePlan).stream().noneMatch(tablesUsedQuery::contains)), the rewrite collapsed to a scan of the MV alone. In that case, return calcitePreMVRewritingPlan so the rebuild executes the original query against source tables (full rebuild).
clearSourceSnapshotFiltershelper: before MV rewriting runs, HiveAugmentSnapshotMaterializationRule mutates the shared Table objects of source tables via setVersionIntervalFrom(snapshotId), so a subsequent MV rewrite would produce a delta-only scan. When we fall back to calcitePreMVRewritingPlan, those Table objects are still tainted, and the fallback plan would still perform a delta-only scan — silently producing the same broken behavior with a differently-shaped plan. The helper walks the fallback plan's TableScans and clears versionIntervalFrom on each underlying Table, so the fallback truly scans all source rows.The visible signature of this helper's effect is the fromVersion=[#Masked#] string disappearing from the rebuild's CBO plan — that's the diff in mv_iceberg_orc2.q.out. Related test adjustments: mv_iceberg_orc2/4/5/6/7
mv_iceberg_orc2.q.out — 1-line diff
The test creates a v1 iceberg MV mat1 from a simple SELECT b, c FROM tbl_ice WHERE c > 52, then does inserts and rebuilds. Under 1.43, explain cbo of alter materialized view mat1 rebuild was rendering HiveTableScan(..., fromVersion=[#Masked#]) because HiveAugmentSnapshotMaterializationRule had installed a version filter on the source Table before our guard fired. After clearSourceSnapshotFilters runs, that filter is gone. The regenerated golden simply drops fromVersion=[#Masked#] from the scan line. Nothing else changes — data output was already correct, only the explain plan cosmetically referenced a snapshot ID that no longer applies once we've decided to full-rebuild.
mv_iceberg_orc4.q.out — large diff, data values change
The test creates an MV over a JOIN with an aggregate, then re-inserts overlapping rows into both tbl_ice and tbl_ice_v2:
insert into tbl_ice values (1,'one',50), (2,'two',51), (3,'three',52), (4,'four',53), (5,'five',54);
insert into tbl_ice_v2 values (1,'one v2',50), (4,'four v2',53), (5,'five v2',54);
Rows (4,'four',53) and (5,'five',54) already existed in tbl_ice from the first insert; likewise (4,…) and (5,…) already existed in tbl_ice_v2. After both inserts, each of a=4 and a=5 has 2 rows in tbl_ice, and d=4/d=5 has 2 rows in tbl_ice_v2. The inner join on a=d filtered c>52 therefore has 4 rows per group (2×2 cartesian for each key). The SUM(f) aggregate over 4 rows of value 53 is 212, over 4 rows of value 54 is 216.
Under 1.42, the incremental Union rebuild produced (four,53,106) and (five,54,108) — 2× not 4×. That's because the incremental delta algebra used was essentially mat_new = mat_old + agg(delta_left ⋈ delta_right), which misses the cross-terms (old_left ⋈ delta_right) and (delta_left ⋈ old_right). When the source tables have no duplicates, those cross-terms are empty and the algebra is correct; when they do (this test), the incremental result diverges from the SQL-semantic answer.
Under 1.43 with our fix, the fallback does a full rebuild by re-executing the MV definition query against current source state, giving the SQL-semantic-correct 212/216. The plan sections of the diff also change: instead of a Union-with-delta plan they now show a straight HiveAggregate ← HiveJoin ← two HiveTableScans shape.
The regenerated golden is therefore more correct than the old one; the old golden essentially locked in the arithmetic quirk of insert-only incremental maintenance under duplicate-containing sources.
mv_iceberg_orc5.q.out — large diff, data values change
Same test structure as orc4 but the MV also carries count(f) and avg(f):
select b, c, sum(f), count(f), avg(f) from tbl_ice join tbl_ice_v2 on a=d where c>52 group by b, c;
Same duplicate-inserts pattern. Full rebuild sees 4 rows per key, so:
-- (four, 53): sum=212, count=4, avg=53.0
-- (five, 54): sum=216, count=4, avg=54.0
Old 1.42 golden had sum=106/108, count=2, avg=53.0/54.0 — again the incremental algebra missed cross-terms. The regenerated values are internally consistent (212 = 4×53, avg = 212/4 = 53.0). Same reasoning as orc4 for accepting the change.
mv_iceberg_orc6.q.out — 8-line diff, no data change
Rebuild is now a pure full rebuild. Consequence:
The rebuild statement no longer reads from mat1, so Input: default@mat1 disappears from the PREHOOK/POSTHOOK metadata.
Lineage no longer records mat1 as a source column; it goes from EXPRESSION [(tbl_ice_v2)...f..., (mat1)default.mat1.FieldSchema(name:_c2,...)] to EXPRESSION [(tbl_ice_v2)...f...] and similarly for b/c (which become SIMPLE).
The select * from mat1 at the end of the test emits the same rows in both versions — nothing about the visible query result changes; only the auditing metadata catches up with the fact that the rebuild is a straight source-side computation now.
mv_iceberg_orc7.q.out — 144-line diff, no data change
Same category as orc6: pure plan-shape change with identical output. The test's MV is a count(c) GROUP BY a — a decomposable aggregate over a single table. So even with the duplicate-insert on a=1, both 1.42's incremental and 1.43's full rebuild arrive at (1,2), (4,1), (5,1). The diff is entirely in the plan sections:
CBO plan drops the delta-merge subtree — HiveJoin(right, IS NOT DISTINCT FROM) ← [scan(mat1), aggregate(scan(tbl_ice, fromVersion=…))] with the trailing CASE that reconciled old-vs-new sums — and becomes the flat shape HiveAggregate(count) ← HiveTableScan(tbl_ice).
Tez stage graph collapses accordingly: three reducers (Reducer 2/3/5) reduce to one (Reducer 2), and the Right Outer Join merger vanishes.
Move Operator switches from replace: false (incremental insert-into) to replace: true (overwrite).
Input: default@mat1 and Version interval from markers are removed.
None of that changes the visible query output — the two data blocks at lines 59–61 and 182–184 are the pre- and post-rebuild select * from mat1 results and are byte-identical to the old golden.
Other test adjustments:
materialized_view_partitioned_2 fixed (as expected, due to fix CALCITE-7635 , see HIVE-29742)
Some "lost" simplifications came back, and some redundant IS NOT NULL were removed/simplified, due to fix CALCITE-7722. Example: auto_join13.q (which also recovered vectorized mode), interval_3.q, vector_interval_mapjoin.q, join13.q, subquery_notin.q, allcolref_in_udf.q, vectorized_dynamic_partition_pruning.q, cbo_query88.q, query88.q, merge_with_null_check_on_joining_col.q, external_jdbc_table_perf.q, pcs.q, explainuser_1.q.out, dynamic_partition_pruning.q, vector_coalesce.q, lineage2.q,lineage3.q, cbo_query72.q, cbo_query88.q,
Several test plans changed due to change in expression
A > B==>(A <= B) IS NOT TRUE:In principle this is actually a fix coming from CALCITE-7636, considering that
ROW__IDstruct is currently defined as NULLABLE (viaCalcitePlanner#genTableLogicalPlanstep "// 3.3 Add column info corresponding to virtual columns" which callsColumnInfo5-arg constructor which inside harcodesnullable=true. If we want the original plan, this nullability ofROW__IDstruct would need to be adjusted.Examples: sketches_materialized_view_cume_dist, sketches_materialized_view_ntile, sketches_materialized_view_percentile_disc, sketches_materialized_view_rank, sketches_materialized_view_rollup2,materialized_view_cluster, materialized_view_cluster, materialized_view_create_rewrite_3, materialized_view_create_rewrite_4, materialized_view_create_rewrite_5, materialized_view_create_rewrite_7, materialized_view_create_rewrite_8, materialized_view_create_rewrite_nulls, materialized_view_create_rewrite_one_key_gby, materialized_view_create_rewrite_rebuild_dummy, materialized_view_create_rewrite_time_window, materialized_view_distribute_sort, materialized_view_parquet, materialized_view_partition_cluster, materialized_view_partitioned, materialized_view_partitioned_create_rewrite_agg, materialized_view_partitioned_create_rewrite_agg_2, materialized_view_partitioned_create_rewrite_agg_3, materialized_view_rebuild_2, materialized_view_rebuild_3, cbo_query54.q, TestMaterializedViewRebuild#testSecondRebuildCanBeIncrementalAfterMajorCompaction, mv_iceberg_orc8.
Also (see more details above in B and C): materialized_view_create_rewrite_6.q, materialized_view_create_rewrite_6_aggr_2joins.q, materialized_view_create_rewrite_6_aggr_3joins.q, materialized_view_create_rewrite_9.q.
Semantically equal predicates, just in different terms order, possibly due to CALCITE-7635
Example: vector_interval_2.q
Some of the plans with predicates that were "split" into several Filter operators due to Calcite upgrade 1.42 seem to be back to their pre-1.42 state (i.e. a single Filter with the whole AND predicate).
Example: auto_join2.q,join2.q,auto_join_stats.q,auto_join_stats2.q
Probably due to CALCITE-7529 TIMESTAMP literals are now preserved as TimestampString at declared precision instead of round-tripping through millisecond-precision runtime values, so zero-fractional literals now render with a full .000000000 nanosecond suffix (semantically identical). Seen in: vector_case_when_2.q.out
Due to CALCITE-7687 (metadata selectivity bugfix on Aggregate), the plans for query64 have changed.
What changes were proposed in this pull request?
Why are the changes needed?
Does this PR introduce any user-facing change?
How was this patch tested?