[fix](ivm) Resolve IVM identity keys by slot identity and materialize unprojected keys - #67669
Merged
Merged
Conversation
… of name An aggregate delta over a join of two tables that both expose a column with the same name (GROUP BY l.id, r.id) resolved its group keys through name-keyed lookups. A name index collapses same-named slots onto the last match, so both group keys were bound to one delta slot and a (10, 20) group was written as (20, 20), diverging from COMPLETE. Key changes: - IvmAggDeltaHandler resolves the delta-side group key slots by ExprId against the top delta project output and carries them as an ordered list aligned with the aggregate metadata; the apply project emits each group key output from its own delta slot, and the ivm_use_full_keys identity conjuncts resolve their delta side by slot identity first (name/sanitized fallback kept for distinct names only). - IvmDeltaRewriteHelper gains a shared findSlotByExprId helper. Unit Test: - IvmAggDeltaHandlerTest: two new cases - default and ivm_use_full_keys grouped aggregate with same-named group keys keep distinct delta slots; the full class (33 tests) passes, and the new case fails on the unfixed code. - Regression test_ivm_agg_join_same_name_group_key: same-named GROUP BY keys over a two-table join, covering plain INSERT, ivm_use_full_keys, and MOW UPDATE (delete + insert) incremental refresh, all cross-checked against COMPLETE.
…oject layer With ivm_use_full_keys, every identity key of an MV must be materialized in the stored layout, either as a visible output or under a hidden key column. The result sink judged "already projected" by column name, so a same-named key from another table (GROUP BY l.id, r.id selecting only l.id) was silently dropped from the key set and the full-keys join lost one dimension. Key changes: - The result sink now checks projected keys by slot identity (findProjectedKey), like the project layer already did, instead of by output name. - Materialization of an unprojected identity key moves from the sink down to the first project that drops it: CREATE plans (result sink) and refresh plans (olap-table sink) then produce the same hidden key layout and agree on hidden column names, which is required for refresh to match the stored layout. - Same-named keys keep their own hidden columns; the hidden materialization is shared by both layers through a single helper. Unit Test: - IvmNormalizeMTMVJoinTest: full-keys same-named unprojected key is materialized under a hidden column wrapping the right slot; the two new ivm regressions (same-named group keys and full-keys unprojected key) pass, and the full ivm regression set (85 suites) passed on the materialization change.
yujun777
requested review from
924060929,
englefly,
morrySnow,
seawinde and
starocean999
as code owners
September 8, 2026 12:34
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
…erialized layout Since the project layer materializes unprojected identity keys as hidden key columns, the same-named right-table keys of a join that only outputs the left side are no longer folded onto the same-named left outputs (which silently dropped the right dimensions from the full-keys layout). The test now expects each unprojected right key under its own hidden key column, matching the materialization behavior. Unit Test: - IvmNormalizeMTMVTest#testJoinIdentityKeysConcatenateLeftAndRight updated; the full class (52 tests) passes.
Contributor
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 16634 ms |
Contributor
TPC-DS: Total hot run time: 81180 ms |
Contributor
ClickBench: Total hot run time: 14.57 s |
Contributor
TPC-H: Total hot run time: 16865 ms |
Contributor
TPC-DS: Total hot run time: 82648 ms |
Contributor
ClickBench: Total hot run time: 14.94 s |
morrySnow
approved these changes
Sep 9, 2026
9 tasks
yujun777
added a commit
that referenced
this pull request
Sep 10, 2026
…NormalizeMTMVJoinTest (#67775) #67646 replaced `IvmRewriteContext`'s public three-argument constructor with factory methods. #67669, merged a few minutes later, added a test that still called the removed constructor, so the fe-core test sources no longer compile on master: ``` fe/fe-core/src/test/java/org/apache/doris/mtmv/ivm/IvmNormalizeMTMVJoinTest.java:1053: error: constructor IvmRewriteContext cannot be applied to given types; required: (Mode, MTMV, String, boolean, ExecutionKind, Optional<IvmDryRunLimit>, Map<...>, Optional<StreamReadMode>) found: (Mode, MTMV, boolean) ``` The two changes do not conflict textually, so each PR was green on its own and the breakage only showed up once both had landed. ### What is changed? Construct the context in `testFullKeysSinkMaterializesSameNamedUnprojectedKey` through the factory the CREATE MATERIALIZED VIEW analyze flow already uses, `IvmRewriteContext.normalize(mtmv)` (see `MTMVPlanUtil`), instead of the removed constructor. Test-only change; no production code is touched. `normalize(mtmv)` is needed here rather than `create(mtmvName)`: the test enables full keys through `mtmv.getIvmInfo().setUseFullKeys(true)`, which `IvmNormalizeMTMV.resolveUseFullKeys()` only reads on its fallback path — when the context carries a null `useFullKeys` but a non-null MTMV. `create(...)` leaves the MTMV null, which would silently resolve that setting to `false`, so the test would compile but fail. `Mode.NORMALIZE` also avoids the `Mode.INCREMENTAL` plan-signature validation, and nothing in this test asserts on the mode. ### Test - `IvmNormalizeMTMVJoinTest` — 44 tests, 0 failures, 0 errors - `mvn test-compile -pl fe-core -am` — BUILD SUCCESS, no errors; all fe-core test sources compile again
morningman
added a commit
that referenced
this pull request
Sep 11, 2026
…on PRs from master in merge order (#67533 #67594 #67646 #67669 #67665 #67710 #67775 #67575 #67480) (#67830) Cherry-picked from #67533, #67594, #67646, #67669, #67665, #67710, #67775, #67575, #67480 Batch pick of every merged PR carrying the `incremental-computation` label that `branch-incremental-computation` does not have yet, in the order they landed on master (`git log --first-parent`). One commit per PR, each created with `git cherry-pick -x` so the message ends with `(cherry picked from commit <master sha>)`. | # | Master commit | PR | Title | |---|---|---|---| | 1 | 300d532 | #67533 | [fix](table stream) Preserve table stream offsets during cleanup | | 2 | bfe46ec | #67594 | [fix](row binlog) make time-based incremental reads use a safe fence | | 3 | c1bff0d | #67646 | [fix](ivm) Fail aggregate IVM refresh when the previous refresh txn is not visible yet | | 4 | 4f3abce | #67669 | [fix](ivm) Resolve IVM identity keys by slot identity and materialize unprojected keys | | 5 | 7129a3e | #67665 | [fix](ivm) Propagate and compensate failures of the IVM excluded-trigger-tables ALTER | | 6 | a565aca | #67710 | [fix](regression) Stop MTMV task waits from latching onto the previous task | | 7 | 8565db2 | #67775 | [fix](ivm) Repair the removed rewrite-context constructor call in IvmNormalizeMTMVJoinTest | | 8 | 0ded66a | #67575 | [feature](ivm) Support incremental refresh for array_agg and collect_list aggregates | | 9 | efc929a | #67480 | [fix](table stream) fix table stream TSO boundary semantics | Not included on purpose: - #62606 (IVM feature) is already in the branch's fork point (`efedf10c7e3`). - #67508 already landed on this branch via #67712. ### Cherry-pick notes - All nine picks applied without conflicts. Each pick's diff is identical to its master commit (compared with `index`/`@@` lines stripped). - Every touched file is byte-identical to master at `efc929aa7af` except `Env.java`, `FrontendServiceImpl.java` and `FrontendService.thrift`, whose remaining differences come only from unrelated master commits that are not part of this label (#67708, #66770, #67572, #67520). - #67594 is the safe-read-fence prerequisite of #67480; picking both in master order is what makes #67480 apply cleanly here (it replaces the earlier stand-alone attempt #67828, which had to hand-adapt around the missing fence). - #67775 is needed because #67646 (removes the `IvmRewriteContext` constructor) and #67669 (test that still used it) are both picked. ### Verification - `be/src/exec/scan/olap_scanner.cpp` (the only BE change): syntax-only compile with the Release flags, clean. The new thrift RPC `acquireTimeBasedChangeReadFence` is FE-only; nothing in `be/` or `cloud/` references it. - FE: `run-fe-ut.sh --run` on this branch (regenerates thrift, compiles fe-core main + test) with the 19 test classes touched by the picks: 19 classes, 450 tests, 0 failures, 0 errors, BUILD SUCCESS — `TableStreamManagerCleanupTest` 9, `CloudGlobalTransactionMgrTest` 22, `AlterMTMVTest` 24, `IvmAggArrayAggProcessorTest` 2, `IvmAggCollectListProcessorTest` 1, `IvmAggDeltaHandlerTest` 33, `IvmDeltaRewriterTest` 20, `IvmJoinDeltaHandlerTest` 23, `IvmLinearDeltaHandlerTest` 39, `IvmNormalizeMTMVJoinTest` 44, `PhysicalPlanTranslatorTest` 17, `IvmIncrRefreshMTMVTest` 13, `IvmNormalizeMTMVTest` 52, `CreateMTMVCommandTest` 94, `ExplainTableStreamPlanTest` 23, `OlapScanNodeTest` 12, `TimeBasedChangeVisibleWaiterTest` 7, `TransactionIdGeneratorTest` 1, `TSOTimestampTest` 14. - Regression suites touched parse cleanly (groovy parser check). 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_018m1ARNXtGSWucJTy34uwe1 --------- Co-authored-by: TsukiokaKogane <cby141994@gmail.com> Co-authored-by: Luwei <814383175@qq.com> Co-authored-by: yujun <yujun@selectdb.com>
morningman
added a commit
that referenced
this pull request
Sep 23, 2026
…on PRs from master in merge order (#67347 #68193 #68336 #68170 #68180) (#68405) Cherry-picked from #67347, #68193, #68336, #68170, #68180 Batch pick of every merged PR carrying the `incremental-computation` label that `branch-incremental-computation` does not have yet (no `incremental-computation-picked` label), in the order they landed on master (`git log --first-parent`). One commit per PR, each ending with `(cherry picked from commit <master sha>)`. Follows the same convention as #67830, #68017, #68073, #68151, #68236 and #68303. | # | Master commit | PR | Title | |---|---|---|---| | 1 | a7c0a78 | #67347 | [fix](cloud) bind the packed slice location lifetime to its writer | | 2 | addf0c8 | #68193 | [feature](ivm) Add the per-partition refresh state and its journal channel | | 3 | e885b46 | #68336 | [refactor](ivm) Rename IvmInfo.refreshVersion to sequencePrefix | | 4 | 01efbca | #68170 | [fix](ivm) Fall back to complete refresh when the IVM stream is unusable | | 5 | d6c1a2b | #68180 | [fix](ivm) Choose IVM baseline rebuild partitions from the MV partition mapping | Not included on purpose: - The 29 labelled PRs that already carry `incremental-computation-picked` (every other closed PR with the label). This batch closes the selection query: 34 closed labelled PRs = 29 already picked + these 5. ### Prerequisite check - **The IVM series (#68193, #68336, #68170, #68180)** — one series by the same author, merged 2026-09-21/22, all tracing to issue #65418. #68193 says in its own description that it "adds the state that the following PRs need" — `MTMVPartitionState` plus its journal channel — and #68336 is the rename of the field sitting next to it. The PRs that consume that state are in this batch too, so the series is carried whole and in master merge order. Its earlier PRs (#67802, #67837, #67814, #68138, #67646, #67669, #67575) were picked in the previous rounds. - **#67347** is standalone (cloud packed-file slice lifetime). Every file that uses the APIs it changes is in the pick; `be/src/io/fs/packed_file_system.cpp`, which also reads the global slice index but is not modified, compiles unchanged against the picked headers. - **No unpicked master commit is required by any of the five.** Verified by compiling, not by inspection alone: the whole FE main + test tree compiles and the touched unit tests pass (below), and every BE/cloud file the picks touch passes a `-fsyntax-only` compile with the real build's flags. - The unlabelled master commits touching the same files (#67186 Hive partition batching, #66530 external scan task reuse, #66761 TIMESTAMP_NS, #67545 DLF, #67067 eager-agg) are *not* prerequisites — see the drift check: none of the picks' own added lines reference what they introduce. ### Drift check against master Per pick, `git show` of the branch commit against the master squash, with `index`/`@@` lines stripped: #67347, #68193 and #68336 are identical. Two differ, both mechanically: - **#68170 / `MTMVTask.java`** — master's context around `executePartitionBasedRefresh` carries #67186's `try` block and snapshot preload; this branch does not have #67186, so the auto-merge kept the branch's block. The pick's own added and removed lines are identical to master's. - **#68180 / `MTMV.java`** — master's squash also adds `import org.apache.doris.datasource.mvcc.MvccSnapshot;`, for #67186's `pinnedSnapshots` overloads of `calculatePartitionMappings` / `getEffectiveQueryUsedBaseTablePartitionMap`. Those overloads do not exist here and the pick's new code only calls the single-argument form, which exists on both sides, so the import is not needed and not carried. For every file this batch touches, `git diff upstream-apache/master -- <file>` was taken and each master-side line attributed to the unpicked commit that added it. All 42 files resolve: 37 have no master-side difference at all, and the 5 that do are fully owned by | file | master-side lines | owner | |---|---|---| | `MTMV.java` | 20 | #67186 | | `MTMVTask.java` | 46 | #67186, #66530 | | `MTMVTaskTest.java` | 44 | #67186 | | `MTMVPartitionUtil.java` | 65 | #67186, #67545 | | `MTMVRelatedPartitionDescSyncLimitGenerator.java` | 3 | #66761 | i.e. nothing belonging to the picks is missing, and no unlabelled commit has to come along. ### Verification - FE: `run-fe-ut.sh --run` on this branch (regenerates thrift/protobuf, compiles fe-core main 4480 files + test 1485 files) over the 11 touched test classes — `AlterMTMVTest` 25, `MTMVTest` 23, `MTMVTaskTest` 48, `MetaLockUtilsTest` 6, `IvmBaselineRebuildTest` 37, `IvmAggDeltaHandlerTest` 33, `IvmDeltaRewriteStateTest` 10, `IvmFailureReasonTest` 1, `IvmInfoTest` 6, `IvmSequenceCalculatorTest` 4, `DatabaseTransactionMgrTest` 20 — **213 tests, 0 failures, 0 errors, 0 skipped, BUILD SUCCESS**. - FE checkstyle on fe-core: 0 violations. - BE/cloud: `-fsyntax-only` with the Release flags of the real build and its own compiler (`/opt/homebrew/opt/llvm@20/bin/clang++`) on `be/src/io/fs/packed_file_manager.cpp`, `be/src/io/fs/packed_file_writer.cpp`, `be/src/cloud/cloud_rowset_writer.cpp` and `be/test/io/fs/packed_file_manager_test.cpp` (the last with `-DBE_TEST -fno-access-control`): no errors. - The three new regression suites parse (`test_ivm_baseline_marker_scope`, `test_ivm_chained_stream_scope`, `test_ivm_partitions_fallback_stream_unusable`); their `.out` files are the upstream ones, unmodified. --------- Co-authored-by: Xin Liao <liaoxin@selectdb.com> Co-authored-by: yujun <yujun@selectdb.com>
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
4 participants
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.
Two follow-ups from the #62606 review round, both around same-named identity keys of aggregate/full-keys MVs:
Resolve IVM aggregate group keys by slot identity instead of name — an aggregate delta over a join of two tables that both expose the same column name (
GROUP BY l.id, r.id) resolved its group keys through name-keyed lookups. A name index collapses same-named slots onto the last match, so both keys were bound to one delta slot and a(10, 20)group was written as(20, 20), diverging from COMPLETE. The delta-side group keys are now resolved and carried by slot identity (ordered with the aggregate metadata), the apply project emits each key output from its own delta slot, and theivm_use_full_keysidentity conjuncts resolve their delta side by identity first. The full-keys regression with a non-full-keys counterpart covers plain INSERT, full-keys, and MOW UPDATE refresh, each cross-checked against COMPLETE.Materialize unprojected full-keys identity keys at the project layer — with
ivm_use_full_keysevery identity key must be materialized in the stored layout, either as a visible output or under a hidden key column. The result sink judged "already projected" by column name, so a same-named key from another table (selecting onlyl.idwhile grouping byl.id, r.id) was silently dropped and the key set lost one dimension. The sink now checks projection by slot identity, and materialization moves down to the first project that drops a key, so CREATE (result sink) and refresh (olap-table sink) plans produce the same hidden-key layout. A new regression asserts the hidden key column exists (DESC with hidden columns) and that INCREMENTAL matches COMPLETE.Tests:
IvmAggDeltaHandlerTest(33) andIvmNormalizeMTMVJoinTestnew cases pass; the new regression cases pass and the full ivm regression set (85 suites) is green on the materialization change.Trace issue: #65418