Repository navigation
Conversation
### What problem does this PR solve?
Group commit scans inherit `ScanOperatorX::get_block_after_projects()`, which skips operator projections because ordinary scans already evaluate them in `Scanner`. `GroupCommitOperatorX` instead dequeues producer blocks directly and never runs a Scanner. Its downstream row descriptor can therefore describe projected columns while the returned block still has the producer's layout.
For example, a queued block containing `[base columns, mv_int, mv_string]` can be consumed using a projection descriptor for `[base columns, mv_string, mv_int]`, causing a bad cast or incorrect values. Intermediate projections can also be skipped.
Override the group commit scan entry point to call `OperatorXBase::get_block_after_projects()`. This applies the consumer's intermediate and final projections and preserves the existing no-projection, empty-block, error propagation, and output-counter behavior. Ordinary scans are unchanged.
### Release note
Fix group commit inserts into tables with synchronous materialized views failing with column type mismatches or producing incorrect MV values when consumer projections differ from the queued block layout.
### Check List (For Author)
- Test:
- [x] Unit Test added: `GroupCommitScanOperatorTest` covers reordered MV columns, nullable values, intermediate projections, repeated blocks and EOS, no-projection forwarding, and queue errors.
- [x] Regression test added: `group_commit_mv_projection` inserts with sync group commit into a table with two aliased MVs and compares each physical MV index with aggregation over the base index.
- Validation passed: clang-format 16 check, `build-support/check-build-hygiene.sh`, and `git diff --check`.
- Execution pending: both `run-be-ut.sh --run --filter='GroupCommitScanOperatorTest.*' -j4` and `run-regression-test.sh --run -d insert_p0 -s group_commit_mv_projection` were attempted, but environment initialization failed because the local JDK 17 cannot start (`Failed setting boot class path`). Neither test suite was executed locally; BE compilation and clang-tidy are also unverified.
- Behavior changed:
- [x] Yes. Group commit scans execute the projections specified by their consumer plan.
- Does this need documentation?
- [x] No.
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
sollhui
changed the title
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Request changes: two verified regressions remain in generated-column group commit inserts: [P1] a pruned raw slot shifts positional reads and silently changes later ordinary values; [P2] reevaluating a volatile stored generated expression replaces the producer statement's value with the internal consumer's value. Both are detailed inline. There were no existing inline findings to retain or duplicate, and no additional user-provided focus.
Critical checkpoint conclusions:
- Goal and proof: the override applies final/intermediate consumer projections and addresses the intended MV ordering defect, but the two generated-column cases prevent complete correctness. Added tests target the original failure; execution is unverified.
- Scope and reuse: the production change is small and focused and reuses the established operator projection implementation.
- Concurrency: producers and consumer retain the existing mutex-protected queue. Projection runs after queue unlock using task-local contexts; no additional race, lock-order, deadlock, or dependency-release issue was found.
- Lifecycle and static initialization: queue Block ownership and COW column clearing preserve shared output lifetimes. No new reference cycle, static initialization dependency, or lifecycle owner was introduced.
- Configuration: no configuration item or dynamic-update behavior changed.
- Compatibility: no storage/wire format or exchanged ABI changed, and no new libc/system dependency was introduced. No release ELF or mixed-version runtime validation was performed.
- Thrift/protobuf: no definitions changed; cross-branch field-ID checks are inapplicable.
- Parallel paths: ordinary scans continue projecting in Scanner. Insert, stream/prepared, and cloud group commit producers reach the common queued consumer; the findings apply there.
- Conditional checks: no production conditional branch was added or changed; the new comment explains the MV ordering scenario.
- Test coverage: four BE unit cases cover nullable reordered MV outputs, intermediate projections, repeated blocks/EOS, no-projection forwarding/counters, and queue failure. The SQL regression compares two physical MV indexes with ordered base aggregations. Generated-column layouts are missing, as are direct projection-error and empty nonterminal queue cases; the latter control paths were traced statically. Fixture/mock APIs and CMake registration were inspected.
- Expected results: no result file was added or modified. Unit expectations and ordered SQL comparison oracles are coherent by inspection; no runtime result is claimed.
- Observability: existing projection timing and output counters cover the added work; no additional instrumentation need was identified.
- Transaction/persistence/failover: WAL, commit/abort, EditLog/replay, and FE failover protocols are unchanged. No new persistence obligation was introduced.
- Data writes, atomicity, and crashes: the two findings affect values supplied to the OLAP sink. Existing queue synchronization and error-to-abort cleanup remain intact; crash recovery was not executed.
- FE/BE variables: no variable was added. Existing projection/descriptor fields are reused; their physical-slot interpretation is the P1 finding.
- Performance: slot projections preserve shared columns through COW; no distinct avoidable copy or new asymptotic cost was found beyond evaluating required expressions. No benchmark was run.
- Other concerns: no additional verified defect remained after the complete scope and missed-case sweeps.
Validation was static only. The pure-text build-support/check-build-hygiene.sh passed. No build, unit/regression test, clang-tidy, benchmark, or runtime/ELF validation was run. No source changes were made. Review convergence was reached after two bounded rounds; execution coverage limits above remain.
Review converged after 2 round(s).
Contributor
TPC-H: Total hot run time: 28462 ms |
Contributor
TPC-DS: Total hot run time: 150896 ms |
Contributor
ClickBench: Total hot run time: 24.2 s |
…consumers ### What problem does this PR solve? Related PR: apache#69660 Problem Summary: Enabling group commit consumer projections exposes two issues: recomputing a generated base column leaves its queued slot unused, allowing scan pruning to shift subsequent input positions, and volatile generated expressions can produce values different from the producer's values. Forward generated base columns only for internal GROUP_COMMIT sinks and preserve the full GroupCommitScanNode input tuple. MV expressions still use these base values. ### Release note Preserve generated column values and column positions in group commit inserts. ### Check List (For Author) - Test: Add FE binding and scan-slot unit tests and extend the regression suite with middle generated columns, an MV using generated values, and now(6). Checkstyle passed using JDK 20; git diff --check passed. FE unit tests and regression execution were attempted but blocked by the unusable local JDK 17. - Behavior changed: Yes; internal consumers preserve producer-generated values. - Does this need documentation: No.
sollhui
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
October 27, 2026 04:46
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review of head 0e65c69a130ee461c6c44656d2ffea8397740e2b covered all eight changed files and the producer, queue, projection, binding, descriptor and sink paths. No new actionable finding was identified, and no previously reported P0/P1 finding remains applicable to this head.
Both existing concerns are fixed by the current changes:
- 4384969133: generated-middle slot alignment: the consumer forwards generated base inputs and retains the full scan tuple. For queued
[k,g,v]=[1,2,99],c3remains at position 2, sovremains 99. - 4384969143: volatile generated values: internal GROUP_COMMIT binding forwards the producer's generated value instead of rebuilding
now(6)with the consumer's query time. MV expressions can still reference that forwarded value.
Critical checkpoint conclusions:
- Goal and evidence: The queue consumer now uses the established final/intermediate projection engine, with FE changes preserving generated values and positional inputs. Targeted BE, FE and regression tests are present; execution was not performed.
- Scope and reuse: Four production files implement three focused mechanisms; four test files cover them. The implementation reuses existing projection, binding and descriptor machinery.
- Concurrency: Queue mutation and WAL serialization/dequeue remain serialized by the existing mutex. Projection runs after dequeue with task-local expression clones; no new lock order or heavy work inside queue locks was found.
- Lifecycle and ownership: Inherited preparation/open clones final and intermediate expressions. Distinct queued Block objects, local scratch storage and existing recursive COW clearing preserve shared outputs across repeated blocks. No new static initialization, thread or ownership cycle is introduced.
- Configuration: No configuration item is added or changed.
- Compatibility: Existing projection and tuple fields are reused; no wire or storage-format change was identified. Mixed-version execution was not tested.
- Thrift/protobuf IDs: Neither definition directory changes, so cross-branch field-ID auditing is not applicable.
- Parallel paths: Ordinary scans retain scanner-side projection and pruning. Sync/async queue producers share this consumer. Stream/prepared producers still compute generated values. WAL replay forwards surviving generated inputs through explicit header/name mapping; supported ALTER rejects adding generated columns. Cloud sink selection remains downstream.
- Conditional checks: The GROUP_COMMIT generated-expression bypass and GroupCommitScanNode pruning exception have comments explaining the concrete failure scenarios. Ordinary producer binding and scan pruning retain their existing behavior.
- Test coverage: BE cases cover reordered MV outputs, nullable values, repeated blocks/EOS, CSE, no-projection forwarding and queue errors. FE cases cover generated/volatile forwarding, producer distinction and retained versus ordinary-pruned slots. Regression cases compare both physical MV indexes with base aggregations and cover generated-middle, generated-MV and timestamp preservation.
- Expected results: No result file changes. Regression comparisons are ordered and use the existing two-query comparison helper; no handwritten output is introduced.
- Observability: Existing projection timing, output counters and queue/status logging cover this execution; no new logging burden or distributed operation requires additional telemetry.
- Transactions, persistence and failover: No new persistent metadata, EditLog entry or commit/replay format is introduced. Queue finish/failure and WAL replay interactions were traced; no new gap was substantiated. Version visibility and delete-bitmap logic are unchanged.
- Writes, atomicity and crash behavior: Projected blocks enter the existing OLAP sink/transaction lifecycle. Queue and projection failures propagate through Status/exception boundaries; no new partial-commit path or resource cycle was identified statically.
- FE-to-BE variables: No new variable is transmitted. Existing output tuples, projections and intermediate descriptors supply the generic execution path.
- Performance and memory: Forwarded columns use existing COW sharing and mutable-output reuse. Consumer MV computation is necessary for the requested schema; no avoidable deep copy or new per-row synchronization was identified. Existing pipeline memory tracking/reservation applies.
- Additional concerns: Rechecked earlier TVF pruning, materialized descriptor order, generated dependencies, session guards, hidden/shadow values, MV suffix ordering, WAL schema evolution, nullable handling and test scaffolding. All candidates and initial risk items have evidence-based dispositions; none remains unresolved.
User focus: the supplied focus file contains no additional concerns.
Validation limits: this was static inspection only. No builds, tests, benchmarks, cluster execution, mixed-version rollout or crash/replay experiment was run. The BE unit fixture uses mock slot contexts and bypasses production scanner initialization; real prepare/open, function failures, concurrent cancellation and WAL/cloud combinations were traced in source rather than executed. These execution limits remain for CI/runtime validation; no source-review coverage gap or unresolved correctness candidate remains.
Review converged after 2 round(s).
Contributor
TPC-H: Total hot run time: 28521 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 28548 ms |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 28435 ms |
Contributor
TPC-DS: Total hot run time: 150318 ms |
Contributor
ClickBench: Total hot run time: 24.42 s |
sollhui
changed the title
liaoxin01
added a commit
to liaoxin01/doris
that referenced
this pull request
Nov 6, 2026
### Background Group commit producers and consumers can be planned on different FEs. Their block layouts follow the table's full schema. ### What problem does this PR solve? Issue Number: None Related PR: apache#69660 (same layout issue), apache#70160 (upstream WAL replay compatibility) PR Type: Bugfix Problem Summary: Different index-map iteration orders can produce different MV column layouts across FEs, causing group commit cast errors or swapped values. ### Details - **Frequency:** Multiple synchronous MVs with producer and consumer plans generated on different FEs. - **Root cause:** Full-schema construction follows map iteration order, which can differ after metadata recovery. - **Why existing tests missed it:** Coverage missed recovery followed by adding another MV across FEs. - **Fix:** Keep base columns first, then visit indexes in ascending ID order and deduplicate columns by definition name. - **Other paths and branches:** Rollup, Schema Change and Gson recovery share this method. Mixed-version FEs can still generate different layouts during rollout. - **Behavior change:** Identical index metadata produces identical full-schema layouts across FEs. ### Testing - 44 FE unit tests passed. Both new regression tests fail before the fix and pass afterward. - FE server build with Checkstyle passed (UI disabled); baseline ASAN BE rebuilt. - Six-MV `multiple_ssb` passed through both FEs. All 54 output columns matched, including after follower restart. - Base-only, multi-MV and Schema Change WAL replay regressions passed through the follower. The initial ASAN run hit the local 16 GB memory cap; rerunning with 32 GB passed. ### Release note Fix group commit failures and incorrect MV values caused by inconsistent column ordering across FEs. ### Check List (For Author) - Behavior changed: Yes (see Details) - Does this need documentation: No - Pre-submission status: Ready; local checks passed, CI pending. ### AI Disclosure - Tool: Codex - Generated: Code, tests, and description - Human review: Pending author review
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.
What problem does this PR solve?
Group commit scans dequeue producer blocks directly, but inherit a scan entry point that skips projections because ordinary scanners have already applied them. The consumer can therefore interpret a block using a different column layout, causing type mismatches or incorrect MV values.
This change runs the consumer's intermediate and final projections through
OperatorXBase. It also preserves the producer's generated base-column values in internalGROUP_COMMITsink binding, and keeps the full group commit scan tuple in queue order. For example, inserting(k,v)=(1,99)intot(k,g AS(k+1),v)must consume[1,2,99]without pruning the middle input slot or readinggasv. A generatednow(6)must retain the producer's timestamp rather than being evaluated again with the consumer's query time. MV expressions continue to be built from the base columns.Release note
Fix group commit inserts producing column type mismatches or incorrect values when consumer projections differ from queued blocks. Preserve generated-column values and input column positions.
Check List (For Author)
now(6)values matching producer timestamps.git diff --check.Failed setting boot class path). Compilation and clang-tidy remain unverified. These tests require CI validation.Check List (For Reviewer who merge this PR)