Add multi-stage late materialization - #10999
Conversation
ti-chi-bot
Bot
added
do-not-merge/release-note-label-needed
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds conditional multi-stage late materialization for DeltaMerge reads, including residual-filter evaluation, adaptive Late/Direct modes, runtime row statistics, executor row overrides, read-path propagation, tests, and design documentation. ChangesMulti-stage late materialization
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant DAGStorageInterpreter
participant StorageDeltaMerge
participant SegmentReadTaskPool
participant Segment
participant MultiStageLateMaterializationBlockInputStream
DAGStorageInterpreter->>StorageDeltaMerge: create filter and runtime stats
StorageDeltaMerge->>SegmentReadTaskPool: pass DMReadOptions
SegmentReadTaskPool->>Segment: initialize staged input stream
Segment->>MultiStageLateMaterializationBlockInputStream: provide stage0, stage1, and final-rest streams
MultiStageLateMaterializationBlockInputStream->>MultiStageLateMaterializationBlockInputStream: evaluate residual filter and materialize output
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
dbms/src/Interpreters/Settings.h (1)
229-229: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider defaulting new heuristic-gated feature to
false.
dt_enable_multi_stage_late_materializationdefaults totrue, enabling a non-trivial read-path behavior change (skipping Selection execution and relying on storage to apply the residual filter — see the cross-file comment onDAGStorageInterpreter.cpp) for all queries matching the heuristic, without an opt-in period. Comparable recent flags in this file (e.g.dt_enable_relevant_place) default tofalse.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbms/src/Interpreters/Settings.h` at line 229, Change the default value of the SettingBool entry dt_enable_multi_stage_late_materialization in Settings.h from true to false, keeping its description and setting name unchanged.dbms/src/Storages/DeltaMerge/Segment.cpp (1)
3735-3742: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPer-segment
LOG_INFOon multi-stage LM adoption may be noisy at scale.This fires once per segment whenever the multi-stage path is taken, which for a wide scan touching many segments could generate a large volume of INFO-level logs by default (the feature defaults to enabled). Consider
LOG_DEBUGhere, consistent with other per-segment stream-construction logs in this file (e.g.getReadInfo's "Begin segment create input stream" usesLOG_TRACE/LOG_DEBUG).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbms/src/Storages/DeltaMerge/Segment.cpp` around lines 3735 - 3742, Change the multi-stage late-materialization adoption log in the segment stream-construction path to LOG_DEBUG instead of LOG_INFO, keeping the existing message and arguments unchanged.dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationRuntimeStats.h (1)
17-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the project-relative types header.
Replace
<common/types.h>with<Core/Types.h>to follow the header include convention.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationRuntimeStats.h` at line 17, Update the include in MultiStageLateMaterializationRuntimeStats.h to use the project-relative Core/Types.h header instead of common/types.h, preserving the existing type dependencies.Source: Coding guidelines
dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationBlockInputStream.h (1)
76-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
std::string_viewfor stream names.These parameters are read-only labels. Update the declarations and matching
.cppdefinitions to takestd::string_viewinstead ofconst char *.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationBlockInputStream.h` around lines 76 - 82, Update readWithOptionalFilter and skipNextBlockOrRead in MultiStageLateMaterializationBlockInputStream to accept stream_name as std::string_view instead of const char *, and apply the same signature change to their matching .cpp definitions and compatible call sites.Source: Coding guidelines
dbms/src/Storages/SelectQueryInfo.h (1)
63-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffUse camelCase consistently for the new feature API.
Rename the introduced snake_case members and parameters throughout the propagation chain.
dbms/src/Storages/SelectQueryInfo.h#L63-L64: rename query feature fields to camelCase.dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationRuntimeStats.h#L27-L28: rename runtime counters to camelCase.dbms/src/Flash/Coprocessor/DAGStorageInterpreter.h#L171-L171: rename interpreter state to camelCase.dbms/src/Storages/DeltaMerge/DeltaMergeStore.h#L208-L209: rename read-option fields to camelCase.dbms/src/Storages/DeltaMerge/Segment.h#L232-L233: rename propagated parameters, including the corresponding declarations at Lines 795-796 and Lines 808-809.dbms/src/Operators/DMSegmentThreadSourceOp.h#L41-L42: rename constructor parameters and backing members, then updatedbms/src/Operators/DMSegmentThreadSourceOp.cpp.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbms/src/Storages/SelectQueryInfo.h` around lines 63 - 64, Rename the new multi-stage late-materialization API identifiers from snake_case to camelCase throughout the propagation chain: update fields in dbms/src/Storages/SelectQueryInfo.h lines 63-64, counters in dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationRuntimeStats.h lines 27-28, interpreter state in dbms/src/Flash/Coprocessor/DAGStorageInterpreter.h line 171, read options in dbms/src/Storages/DeltaMerge/DeltaMergeStore.h lines 208-209, and propagated parameters plus declarations in dbms/src/Storages/DeltaMerge/Segment.h lines 232-233, 795-796, and 808-809. Rename constructor parameters and backing members in dbms/src/Operators/DMSegmentThreadSourceOp.h lines 41-42 and update all corresponding uses in dbms/src/Operators/DMSegmentThreadSourceOp.cpp.Source: Coding guidelines
dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationBlockInputStream.cpp (1)
57-82: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMember-initializer-list dereferences
residual_filter_before the null check can run.
residual_filter_action(...)'s arguments includebuildResidualFilterHeader(residual_filter_)(which internally doesRUNTIME_CHECK(residual_filter != nullptr)) alongside direct dereferencesresidual_filter_->before_whereandresidual_filter_->filter_column_name. Argument evaluation order within this single call is unspecified/indeterminately-sequenced, so the direct dereferences could execute before the internal check insidebuildResidualFilterHeader, and the constructor body's ownRUNTIME_CHECK(residual_filter != nullptr)(L79) only runs after all member initializers have already completed. If a caller ever passed a nullresidual_filter_(contrary to today's guaranteed-non-null call sites), this would be a null-pointer dereference rather than a cleanRUNTIME_CHECKfailure.In practice all current call sites (e.g.
buildMultiStageLateMaterializationFilterinStorageDeltaMerge.cpp) already guarantee non-null before construction, so this is narrow, but worth hardening given it's an easy, cheap fix (e.g., validate via a comma-operator-sequenced helper, or a static factory that checks first and returns validated fields).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationBlockInputStream.cpp` around lines 57 - 82, Validate residual_filter_ before any member-initializer dereference in MultiStageLateMaterializationBlockInputStream. Move or restructure the residual_filter_action initialization so null validation is sequenced before accessing before_where or filter_column_name, while preserving the existing RUNTIME_CHECK behavior and initialization for valid filters.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dbms/src/Flash/Coprocessor/DAGContext.cpp`:
- Around line 298-307: Protect both accesses in
DAGContext::getExecutorRowsOverride with operator_profile_infos_map_mu, using a
const-compatible lock guard and making the mutex mutable if required. Keep the
existing empty/missing-key null returns and returned override behavior
unchanged.
In `@dbms/src/Storages/DeltaMerge/Remote/RNWorkerPrepareStreams.h`:
- Around line 46-47: The RNWorkers path assembled by packSegmentReadTasks must
not select multi-stage late materialization while passing null multi-stage
inputs. Disable multi-stage late materialization for dt_enable_read_thread=false
paths that produce RNWorkers, or propagate the filter and runtime stats through
RNWorkerPrepareStreams::doWork into SegmentReadTask::initInputStream; ensure
executor row overrides registered by DAGStorageInterpreter are preserved.
---
Nitpick comments:
In `@dbms/src/Interpreters/Settings.h`:
- Line 229: Change the default value of the SettingBool entry
dt_enable_multi_stage_late_materialization in Settings.h from true to false,
keeping its description and setting name unchanged.
In
`@dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationBlockInputStream.cpp`:
- Around line 57-82: Validate residual_filter_ before any member-initializer
dereference in MultiStageLateMaterializationBlockInputStream. Move or
restructure the residual_filter_action initialization so null validation is
sequenced before accessing before_where or filter_column_name, while preserving
the existing RUNTIME_CHECK behavior and initialization for valid filters.
In
`@dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationBlockInputStream.h`:
- Around line 76-82: Update readWithOptionalFilter and skipNextBlockOrRead in
MultiStageLateMaterializationBlockInputStream to accept stream_name as
std::string_view instead of const char *, and apply the same signature change to
their matching .cpp definitions and compatible call sites.
In `@dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationRuntimeStats.h`:
- Line 17: Update the include in MultiStageLateMaterializationRuntimeStats.h to
use the project-relative Core/Types.h header instead of common/types.h,
preserving the existing type dependencies.
In `@dbms/src/Storages/DeltaMerge/Segment.cpp`:
- Around line 3735-3742: Change the multi-stage late-materialization adoption
log in the segment stream-construction path to LOG_DEBUG instead of LOG_INFO,
keeping the existing message and arguments unchanged.
In `@dbms/src/Storages/SelectQueryInfo.h`:
- Around line 63-64: Rename the new multi-stage late-materialization API
identifiers from snake_case to camelCase throughout the propagation chain:
update fields in dbms/src/Storages/SelectQueryInfo.h lines 63-64, counters in
dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationRuntimeStats.h lines
27-28, interpreter state in dbms/src/Flash/Coprocessor/DAGStorageInterpreter.h
line 171, read options in dbms/src/Storages/DeltaMerge/DeltaMergeStore.h lines
208-209, and propagated parameters plus declarations in
dbms/src/Storages/DeltaMerge/Segment.h lines 232-233, 795-796, and 808-809.
Rename constructor parameters and backing members in
dbms/src/Operators/DMSegmentThreadSourceOp.h lines 41-42 and update all
corresponding uses in dbms/src/Operators/DMSegmentThreadSourceOp.cpp.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a55c75d4-6aae-4811-885c-55a128d74722
📒 Files selected for processing (31)
dbms/src/Flash/Coprocessor/DAGContext.cppdbms/src/Flash/Coprocessor/DAGContext.hdbms/src/Flash/Coprocessor/DAGStorageInterpreter.cppdbms/src/Flash/Coprocessor/DAGStorageInterpreter.hdbms/src/Flash/Statistics/ExecutorStatistics.hdbms/src/Interpreters/Settings.hdbms/src/Operators/DMSegmentThreadSourceOp.cppdbms/src/Operators/DMSegmentThreadSourceOp.hdbms/src/Storages/DeltaMerge/DMSegmentThreadInputStream.hdbms/src/Storages/DeltaMerge/DeltaMergeStore.cppdbms/src/Storages/DeltaMerge/DeltaMergeStore.hdbms/src/Storages/DeltaMerge/MultiStageLateMaterializationBlockInputStream.cppdbms/src/Storages/DeltaMerge/MultiStageLateMaterializationBlockInputStream.hdbms/src/Storages/DeltaMerge/MultiStageLateMaterializationRuntimeStats.hdbms/src/Storages/DeltaMerge/Remote/RNWorkerPrepareStreams.hdbms/src/Storages/DeltaMerge/Segment.cppdbms/src/Storages/DeltaMerge/Segment.hdbms/src/Storages/DeltaMerge/SegmentReadTask.cppdbms/src/Storages/DeltaMerge/SegmentReadTask.hdbms/src/Storages/DeltaMerge/SegmentReadTaskPool.cppdbms/src/Storages/DeltaMerge/SegmentReadTaskPool.hdbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cppdbms/src/Storages/DeltaMerge/tests/gtest_dm_vector_index.cppdbms/src/Storages/DeltaMerge/tests/gtest_segment_read_task.cppdbms/src/Storages/DeltaMerge/tests/gtest_segment_read_task_pool.cppdbms/src/Storages/DeltaMerge/tests/gtest_skippable_block_input_stream.cppdbms/src/Storages/SelectQueryInfo.cppdbms/src/Storages/SelectQueryInfo.hdbms/src/Storages/StorageDeltaMerge.cppdbms/src/Storages/StorageDisaggregatedRemote.cppdocs/design/2026-07-25-multi-stage-late-materialization.md
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dbms/src/Flash/Coprocessor/DAGStorageInterpreter.cpp (1)
92-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
std::string_viewfor the read-only executor ID.
executor_idis only used synchronously for lookup and is not mutated. As per coding guidelines, usestd::string_viewfor read-only string parameters.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbms/src/Flash/Coprocessor/DAGStorageInterpreter.cpp` around lines 92 - 109, Update addProfileRowsToExecutorRowsOverride to accept the read-only executor_id parameter as std::string_view instead of const String &, preserving the existing synchronous lookup and row aggregation behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@dbms/src/Flash/Coprocessor/DAGStorageInterpreter.cpp`:
- Around line 92-109: Update addProfileRowsToExecutorRowsOverride to accept the
read-only executor_id parameter as std::string_view instead of const String &,
preserving the existing synchronous lookup and row aggregation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 94489609-7e61-46f3-b5af-786d91732c06
📒 Files selected for processing (3)
dbms/src/Debug/MockStorage.cppdbms/src/Flash/Coprocessor/DAGStorageInterpreter.cppdbms/src/Flash/tests/gtest_executors_with_dm.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- dbms/src/Flash/tests/gtest_executors_with_dm.cpp
- dbms/src/Debug/MockStorage.cpp
0b18458 to
85f5a69
Compare
|
/test pull-integration-test |
|
/run pull-integration-test |
| remote_builder.getCurIOProfileInfos(), | ||
| /*is_append=*/true); | ||
| auto remote_profile_infos = remote_builder.getCurProfileInfos(); | ||
| dag_context.addProfileRowsToExecutorRowsOverride(table_scan.getTableScanExecutorID(), remote_profile_infos); |
There was a problem hiding this comment.
addProfileRowsToExecutorRowsOverride snapshots remote_profile_infos[*]->rows here while the remote pipeline has only been built, not executed, so the value is still zero. Later operator updates do not propagate to the atomic override, and ExecutorStatistics replaces the normally aggregated rows with that override. Remote-only queries therefore report actRows=0, while mixed local/remote queries omit remote rows. Please retain and aggregate the remote profiles at statistics collection time, or update the override from the remote completion path; the regression test should use real remote-only and mixed execution paths instead of calling the helper after setting synthetic rows.
Source: rule/WJHUANG2016-RE-002 | Second Opinion
ti-chi-bot
Bot
added
release-note
windtalker
changed the title
|
/test pull-unit-test |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gengliqi, JaySon-Huang The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
/hold |
ti-chi-bot
Bot
added
the
do-not-merge/hold
|
/hold cancel |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
|
/test pull-unit-test |
|
/hold |
ti-chi-bot
Bot
added
the
do-not-merge/hold
|
/test pull-integration-test |
1 similar comment
|
/test pull-integration-test |
|
/hold cancel |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
8a01c28
into
pingcap:feature/release-8.5-materialized-view
4 participants
What problem does this PR solve?
Issue Number: ref #11003
Problem Summary:
For wide DeltaMerge table scans, the existing late materialization path can still read many output columns for rows that are later removed by residual Selection filters. This is inefficient when the residual filter only depends on a small subset of columns and has good selectivity.
What is changed and how it works?
This PR adds multi-stage late materialization for DeltaMerge TableScan when both pushed-down TableScan filters and residual Selection filters exist. The read path is split into:
The implementation is guarded by
dt_enable_multi_stage_late_materializationand disables the optimization for unsupported or unsafe cases such as generated columns, keep-order scan, fast scan,force_push_down_all_filters_to_scan, and stage1 filters referencing virtual_tidb_tid. It also keeps TableScan / SelectionactRowscorrect for local, remote-only, and mixed local/remote execution by reporting rows through MSLM runtime stats and delayed remote profile aggregation.Check List
Tests
Unit tests run:
git diff --checkninja -C /Users/feixu/dev/pingcap/tiflash/cmake-build-debug dbms/gtests_dbms/Users/feixu/dev/pingcap/tiflash/cmake-build-debug/dbms/gtests_dbms --gtest_filter='ExecutorsWithDMTestRunner.MultiStageLateMaterialization*'/Users/feixu/dev/pingcap/tiflash/cmake-build-debug/dbms/gtests_dbms --gtest_filter='SkippableBlockInputStreamTest.MultiStageLateMaterialization*'Side effects
Documentation
Release note