Support running local TopN for multi-stage late materialization - #11005
Conversation
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
ti-chi-bot
Bot
added
release-note-none
|
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:
📝 WalkthroughWalkthroughThe PR changes multi-stage late materialization to use numeric modes and adds a Running Local TopN path. TopN metadata flows from physical plans through DeltaMerge execution, where typed sort keys, adaptive candidate filtering, runtime statistics, documentation, and tests are added. ChangesMulti-stage late materialization TopN
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PhysicalTopN
participant PhysicalTableScan
participant StorageDeltaMerge
participant DeltaMergeStream
participant RunningLocalTopN
participant RuntimeStats
PhysicalTopN->>PhysicalTableScan: attach TopN description
PhysicalTableScan->>StorageDeltaMerge: build TopN-aware filter
StorageDeltaMerge->>DeltaMergeStream: pass TopN metadata
DeltaMergeStream->>RunningLocalTopN: update Stage 1 block
RunningLocalTopN-->>DeltaMergeStream: return candidate filter
DeltaMergeStream->>RuntimeStats: record candidates and stream state
Possibly related issues
Possibly related PRs
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dbms/src/Flash/Planner/Plans/PhysicalMockTableScan.cpp (1)
220-254: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
mock_streamscached before TopN attachment;setMultiStageLateMaterializationTopNnever refreshes them.
setFilterConditions(lines 220-254) eagerly rebuildsmock_streamsviagetStreamFromDeltaMerge(..., multi_stage_late_materialization_topn), reading the member's value at that call time. ButsetMultiStageLateMaterializationTopN(267-272) — invoked later fromPhysicalTopN::build/tryAttachMultiStageLateMaterializationTopNonce the child (table scan + pushed-down selection) already exists — only assigns the member and never rebuildsmock_streams. SincebuildBlockInputStreamImpljust replays the already-cachedmock_streams, the stream-execution-model path silently loses the TopN optimization for any DAG shapedTableScan -> Selection -> TopN, even thoughbuildPipelineExecGroupImpl(pipeline model) reads the member live and works correctly. This asymmetry could make new unit tests validating the TopN feature pass/fail inconsistently depending on execution model.Consider rebuilding
mock_streamsinsidesetMultiStageLateMaterializationTopNwhenhasFilterConditions()is true (mirroring the rebuild already done insetFilterConditions), or deferring the initialgetStreamFromDeltaMergecall until all wiring (filter + topn) is finalized.Also applies to: 267-272
🤖 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/Planner/Plans/PhysicalMockTableScan.cpp` around lines 220 - 254, Update setMultiStageLateMaterializationTopN so that, after assigning the TopN configuration, it rebuilds mock_streams when hasFilterConditions() is true, using the same DeltaMerge stream construction and current scan/filter state as setFilterConditions. Preserve the existing behavior when filters are not attached, and ensure the rebuilt stream receives the updated multi_stage_late_materialization_topn value.
🧹 Nitpick comments (8)
dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationRuntimeStats.h (1)
35-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
tryLogCurrentExceptionover a silentcatch (...).Swallowing the exception silently hides real failures (e.g. bad format args) in the destructor path.
As per coding guidelines: "Use
tryLogCurrentException(log, "context")in broadcatch (...)paths to avoid duplicated exception-formatting code".♻️ Proposed fix
catch (...) { + tryLogCurrentException(log, "MultiStageLateMaterializationRuntimeStats::logSummary"); }🤖 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` around lines 35 - 44, Update the destructor of MultiStageLateMaterializationRuntimeStats to call tryLogCurrentException in the broad catch (...) block, passing the appropriate logger and context for the logSummary failure; do not silently swallow exceptions.Source: Coding guidelines
dbms/src/Flash/tests/gtest_executors_with_dm.cpp (2)
375-382: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse camelCase for the new helper parameters.
Rename
pushed_down_filter,residual_filter,order_by_column, andis_desctopushedDownFilter,residualFilter,orderByColumn, andisDesc, including the named arguments at Lines 623-630, 710-717, and 786-793.As per coding guidelines, method and variable names should use
camelCase.🤖 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/tests/gtest_executors_with_dm.cpp` around lines 375 - 382, Rename the parameters of buildDAGRequestWithPushedDownFilterAndTopN from pushed_down_filter, residual_filter, order_by_column, and is_desc to pushedDownFilter, residualFilter, orderByColumn, and isDesc, and update all corresponding references and named arguments at the indicated call sites.Source: Coding guidelines
623-630: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake TopN request construction independent of the default setting.
PhysicalTopN::buildreads the MSLM setting while building the request, but all three tests selectMSLMModeTopNafterward. Set the mode before each request is built.
dbms/src/Flash/tests/gtest_executors_with_dm.cpp#L623-L630: setMSLMModeTopNbeforebuildDAGRequestWithPushedDownFilterAndTopN.dbms/src/Flash/tests/gtest_executors_with_dm.cpp#L710-L717: setMSLMModeTopNbefore building the DateTime request.dbms/src/Flash/tests/gtest_executors_with_dm.cpp#L786-L793: setMSLMModeTopNbefore building the string-order request.🤖 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/tests/gtest_executors_with_dm.cpp` around lines 623 - 630, Set the MSLM mode to MSLMModeTopN before constructing each request with buildDAGRequestWithPushedDownFilterAndTopN, because PhysicalTopN::build reads the setting during construction. Apply this ordering change at dbms/src/Flash/tests/gtest_executors_with_dm.cpp lines 623-630, 710-717, and 786-793; preserve the existing request parameters and subsequent test logic.dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationTopN.h (1)
33-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the project’s C++ naming and width-type conventions for the new TopN contract.
dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationTopN.h#L33-L149: rename new variables/methods to camelCase (for example,orderByColumns,columnId,currentBlockSequence) and useInt32fordirection.dbms/src/Storages/SelectQueryInfo.h#L67-L67: rename the new metadata member to camelCase and update its consumers.dbms/src/Debug/MockStorage.h#L112-L112: rename the new parameter to camelCase.dbms/src/Debug/MockStorage.h#L127-L127: rename the new parameter to camelCase.As per coding guidelines, “Method and variable names should use
camelCase” and explicit-width types should be used fromdbms/src/Core/Types.h.🤖 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/MultiStageLateMaterializationTopN.h` around lines 33 - 149, Apply the TopN naming and type conventions across all affected sites: in dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationTopN.h:33-149, rename new variables and methods to camelCase and change direction to Int32; in dbms/src/Storages/SelectQueryInfo.h:67-67, rename the new metadata member to camelCase and update all consumers; in dbms/src/Debug/MockStorage.h:112-112 and :127-127, rename each new parameter to camelCase.Source: Coding guidelines
docs/design/2026-07-26-running-local-topn-for-mslm.md (1)
198-203: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the design pseudocode with the implemented API.
docs/design/2026-07-26-running-local-topn-for-mslm.md#L198-L203: use the actualMultiStageLateMaterializationTopNDescriptionstructure or explicitly label this as conceptual.docs/design/2026-07-26-running-local-topn-for-mslm.md#L342-L349: removestream_sequence, which is not present inHeapEntry.docs/design/2026-07-26-running-local-topn-for-mslm.md#L393-L405: updateRunningLocalTopN::updateto its actual result and 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 `@docs/design/2026-07-26-running-local-topn-for-mslm.md` around lines 198 - 203, Align the design pseudocode with the implemented API: at docs/design/2026-07-26-running-local-topn-for-mslm.md:198-203, use MultiStageLateMaterializationTopNDescription or explicitly mark MSLMTopNDescription as conceptual; at :342-349, remove stream_sequence from HeapEntry; at :393-405, update RunningLocalTopN::update to match its actual parameters and return result.dbms/src/Storages/StorageDeltaMerge.cpp (2)
846-859: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated topk/order-by bound validation.
The
topk == 0,topk > multi_stage_late_materialization_topn_max_topk, andorder_by_columns.size() > multi_stage_late_materialization_topn_max_order_by_columnschecks here duplicate the same checks intryBuildMultiStageLateMaterializationTopN(dbms/src/Flash/Planner/Plans/PhysicalTopN.cpp, lines 52-57). If the bounds ever change, both call sites must be updated in lockstep. Consider extracting a tiny shared helper (e.g., inMultiStageLateMaterializationTopN.h/.cpp) that validates(topk, order_by_size)against the two limits.🤖 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/StorageDeltaMerge.cpp` around lines 846 - 859, Extract the duplicated topk and order_by_columns bound validation from the StorageDeltaMerge path and tryBuildMultiStageLateMaterializationTopN into a shared helper in MultiStageLateMaterializationTopN.h/.cpp. Have both call sites use the helper while preserving their existing disable/error behavior and the separate empty order-by validation.
908-920: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMagic numbers for the "worth-it" heuristic.
10and the2 *ratio decide whether TopN-enhanced multi-stage late materialization is enabled, but they're unexplained literals. Extracting them into namedconstexpr(e.g.,multi_stage_late_materialization_topn_min_final_rest_columns,..._min_rest_to_stage1_ratio) with a short rationale comment would make the heuristic easier to tune/audit later.🤖 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/StorageDeltaMerge.cpp` around lines 908 - 920, Replace the unexplained literals in the worth-it heuristic within the final rest-column checks with named constexpr constants for the minimum final-rest-column count and minimum rest-to-stage1 ratio. Add a brief rationale comment explaining these thresholds, then use the constants in the checks while preserving the existing behavior.dbms/src/Flash/Planner/Plans/PhysicalTopN.cpp (1)
141-144: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the magic
2with a named late-materialization mode.The setting is raw unsigned-integer typed and documented as
0: disabled, 1: selection only, 2: TopN enhanced, butPhysicalTopN.cppuses the literal2directly. Add/use a named mode/const value such asMultiStageLateMaterializationMode::TopNso the top-N-enhanced path is self-documenting and less brittle.🤖 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/Planner/Plans/PhysicalTopN.cpp` around lines 141 - 144, Replace the literal comparison to 2 in the TopN planning branch with the named late-materialization mode representing TopN enhancement, such as MultiStageLateMaterializationMode::TopN. Reuse the existing mode definition if available, adding one only where the setting’s documented values are defined, while preserving the current enabled and disabled control flow.
🤖 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/tests/gtest_executors_with_dm.cpp`:
- Line 688: Update the DateTime fixture construction in the test so rows 24–31
derive a valid day and hour from row: use the day offset 1 + row / 24 and hour
row % 24, while preserving the existing date and other components.
In `@dbms/src/Interpreters/Settings.h`:
- Line 229: Update the dt_enable_multi_stage_late_materialization setting
definition to preserve compatibility with existing true/false configuration
values while retaining its UInt64 modes 0, 1, and 2. Use the setting’s parsing
or alias mechanism rather than requiring numeric-only input, and ensure both
boolean aliases and numeric values deserialize correctly.
In `@dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationTopN.cpp`:
- Line 277: Update the unsupported-order-type throw in the TopN-enhanced
late-materialization logic to use the fmt-style DB::Exception constructor with a
suitable ErrorCodes value, preferably NOT_IMPLEMENTED. Add the corresponding
ErrorCodes declaration near the file’s existing declarations and pass the type
name as a format argument instead of concatenating strings.
---
Outside diff comments:
In `@dbms/src/Flash/Planner/Plans/PhysicalMockTableScan.cpp`:
- Around line 220-254: Update setMultiStageLateMaterializationTopN so that,
after assigning the TopN configuration, it rebuilds mock_streams when
hasFilterConditions() is true, using the same DeltaMerge stream construction and
current scan/filter state as setFilterConditions. Preserve the existing behavior
when filters are not attached, and ensure the rebuilt stream receives the
updated multi_stage_late_materialization_topn value.
---
Nitpick comments:
In `@dbms/src/Flash/Planner/Plans/PhysicalTopN.cpp`:
- Around line 141-144: Replace the literal comparison to 2 in the TopN planning
branch with the named late-materialization mode representing TopN enhancement,
such as MultiStageLateMaterializationMode::TopN. Reuse the existing mode
definition if available, adding one only where the setting’s documented values
are defined, while preserving the current enabled and disabled control flow.
In `@dbms/src/Flash/tests/gtest_executors_with_dm.cpp`:
- Around line 375-382: Rename the parameters of
buildDAGRequestWithPushedDownFilterAndTopN from pushed_down_filter,
residual_filter, order_by_column, and is_desc to pushedDownFilter,
residualFilter, orderByColumn, and isDesc, and update all corresponding
references and named arguments at the indicated call sites.
- Around line 623-630: Set the MSLM mode to MSLMModeTopN before constructing
each request with buildDAGRequestWithPushedDownFilterAndTopN, because
PhysicalTopN::build reads the setting during construction. Apply this ordering
change at dbms/src/Flash/tests/gtest_executors_with_dm.cpp lines 623-630,
710-717, and 786-793; preserve the existing request parameters and subsequent
test logic.
In `@dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationRuntimeStats.h`:
- Around line 35-44: Update the destructor of
MultiStageLateMaterializationRuntimeStats to call tryLogCurrentException in the
broad catch (...) block, passing the appropriate logger and context for the
logSummary failure; do not silently swallow exceptions.
In `@dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationTopN.h`:
- Around line 33-149: Apply the TopN naming and type conventions across all
affected sites: in
dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationTopN.h:33-149, rename
new variables and methods to camelCase and change direction to Int32; in
dbms/src/Storages/SelectQueryInfo.h:67-67, rename the new metadata member to
camelCase and update all consumers; in dbms/src/Debug/MockStorage.h:112-112 and
:127-127, rename each new parameter to camelCase.
In `@dbms/src/Storages/StorageDeltaMerge.cpp`:
- Around line 846-859: Extract the duplicated topk and order_by_columns bound
validation from the StorageDeltaMerge path and
tryBuildMultiStageLateMaterializationTopN into a shared helper in
MultiStageLateMaterializationTopN.h/.cpp. Have both call sites use the helper
while preserving their existing disable/error behavior and the separate empty
order-by validation.
- Around line 908-920: Replace the unexplained literals in the worth-it
heuristic within the final rest-column checks with named constexpr constants for
the minimum final-rest-column count and minimum rest-to-stage1 ratio. Add a
brief rationale comment explaining these thresholds, then use the constants in
the checks while preserving the existing behavior.
In `@docs/design/2026-07-26-running-local-topn-for-mslm.md`:
- Around line 198-203: Align the design pseudocode with the implemented API: at
docs/design/2026-07-26-running-local-topn-for-mslm.md:198-203, use
MultiStageLateMaterializationTopNDescription or explicitly mark
MSLMTopNDescription as conceptual; at :342-349, remove stream_sequence from
HeapEntry; at :393-405, update RunningLocalTopN::update to match its actual
parameters and return result.
🪄 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: 5b19c44f-11d3-494c-858d-02fcb6c17ab9
📒 Files selected for processing (23)
dbms/src/Debug/MockStorage.cppdbms/src/Debug/MockStorage.hdbms/src/Flash/Coprocessor/DAGStorageInterpreter.cppdbms/src/Flash/Coprocessor/DAGStorageInterpreter.hdbms/src/Flash/Planner/Plans/PhysicalMockTableScan.cppdbms/src/Flash/Planner/Plans/PhysicalMockTableScan.hdbms/src/Flash/Planner/Plans/PhysicalTableScan.cppdbms/src/Flash/Planner/Plans/PhysicalTableScan.hdbms/src/Flash/Planner/Plans/PhysicalTopN.cppdbms/src/Flash/tests/gtest_executors_with_dm.cppdbms/src/Interpreters/Settings.hdbms/src/Storages/DeltaMerge/Filter/PushDownFilter.hdbms/src/Storages/DeltaMerge/MultiStageLateMaterializationBlockInputStream.cppdbms/src/Storages/DeltaMerge/MultiStageLateMaterializationBlockInputStream.hdbms/src/Storages/DeltaMerge/MultiStageLateMaterializationRuntimeStats.hdbms/src/Storages/DeltaMerge/MultiStageLateMaterializationTopN.cppdbms/src/Storages/DeltaMerge/MultiStageLateMaterializationTopN.hdbms/src/Storages/DeltaMerge/Segment.cppdbms/src/Storages/SelectQueryInfo.cppdbms/src/Storages/SelectQueryInfo.hdbms/src/Storages/StorageDeltaMerge.cppdocs/design/2026-07-25-multi-stage-late-materialization.mddocs/design/2026-07-26-running-local-topn-for-mslm.md
| for (size_t row = 0; row < rows; ++row) | ||
| { | ||
| c0_values.push_back(row); | ||
| ts_values.push_back(MyDateTime(2020, 1, 1, static_cast<UInt32>(row), 0, 0, 0).toPackedUInt()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the DateTime fixture values valid.
Rows 24-31 pass hours 24-31 to MyDateTime. Build the day and hour from the row index, for example day = 1 + row / 24 and hour = row % 24, so this test exercises valid DateTime values.
Suggested fix
- ts_values.push_back(MyDateTime(2020, 1, 1, static_cast<UInt32>(row), 0, 0, 0).toPackedUInt());
+ ts_values.push_back(
+ MyDateTime(
+ 2020,
+ 1,
+ static_cast<UInt32>(1 + row / 24),
+ static_cast<UInt32>(row % 24),
+ 0,
+ 0,
+ 0)
+ .toPackedUInt());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ts_values.push_back(MyDateTime(2020, 1, 1, static_cast<UInt32>(row), 0, 0, 0).toPackedUInt()); | |
| ts_values.push_back( | |
| MyDateTime( | |
| 2020, | |
| 1, | |
| static_cast<UInt32>(1 + row / 24), | |
| static_cast<UInt32>(row % 24), | |
| 0, | |
| 0, | |
| 0) | |
| .toPackedUInt()); |
🤖 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/tests/gtest_executors_with_dm.cpp` at line 688, Update the
DateTime fixture construction in the test so rows 24–31 derive a valid day and
hour from row: use the day offset 1 + row / 24 and hour row % 24, while
preserving the existing date and other components.
| if (checkDataType<DataTypeMyDateTime>(type_not_null.get())) | ||
| return makeSortKeyTypeInfo<UInt64, UInt64, SortKeyKind::DateTime, ColumnVector<UInt64>>(); | ||
|
|
||
| throw Exception("Unsupported order by type for TopN-enhanced multi-stage late materialization: " + type->getName()); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use the fmt-style DB::Exception constructor with an error code.
String concatenation without an error code deviates from the repo convention for storage-engine error handling.
As per coding guidelines: "Use DB::Exception for error handling with the fmt-style constructor: throw Exception(ErrorCodes::SOME_CODE, "Message with {}", arg);" and "Use DB::Exception with appropriate error codes from ErrorCodes.cpp for error handling in storage engine code".
♻️ Proposed fix
- throw Exception("Unsupported order by type for TopN-enhanced multi-stage late materialization: " + type->getName());
+ throw Exception(
+ ErrorCodes::NOT_IMPLEMENTED,
+ "Unsupported order by type for TopN-enhanced multi-stage late materialization, type={}",
+ type->getName());Add the corresponding namespace ErrorCodes { extern const int NOT_IMPLEMENTED; } declaration (or another suitable existing code) near the top of the file.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| throw Exception("Unsupported order by type for TopN-enhanced multi-stage late materialization: " + type->getName()); | |
| throw Exception( | |
| ErrorCodes::NOT_IMPLEMENTED, | |
| "Unsupported order by type for TopN-enhanced multi-stage late materialization, type={}", | |
| type->getName()); |
🤖 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/MultiStageLateMaterializationTopN.cpp` at line
277, Update the unsupported-order-type throw in the TopN-enhanced
late-materialization logic to use the fmt-style DB::Exception constructor with a
suitable ErrorCodes value, preferably NOT_IMPLEMENTED. Add the corresponding
ErrorCodes declaration near the file’s existing declarations and pass the type
name as a format argument instead of concatenating strings.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/design/2026-07-26-running-local-topn-for-mslm.md (1)
658-660: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKeep
Selection::actRowssemantically accurate.Overwriting it with
topn_candidate_rowsmakesEXPLAIN ANALYZEreport materialized candidate rows instead of rows passing the residual Selection, misleading diagnostics and tooling that interprets operator cardinalities. PreserveactRowsand expose candidate rows through a separate, clearly labeled metric.🤖 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 `@docs/design/2026-07-26-running-local-topn-for-mslm.md` around lines 658 - 660, Remove the overwrite of the residual Selection executor’s actRows with topn_candidate_rows in the TopN-enhanced MSLM path. Preserve Selection::actRows as the number of rows passing residual Selection, and expose storage candidate rows through a separate clearly labeled runtime metric for EXPLAIN ANALYZE.dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationBlockInputStream.h (1)
93-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffUse camelCase for newly introduced C++ identifiers.
dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationBlockInputStream.h#L93-L116: rename adaptive state fields to names such asrunningTopNandtopnAdaptiveDisabled.dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationBlockInputStream.cpp#L62-L65: update the renamed member initialization.dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationBlockInputStream.cpp#L287-L331: rename parameters and locals such asresidualPassedRowsandwarmupRows.dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationBlockInputStream.cpp#L382-L459: update references to the renamed state.dbms/src/Storages/DeltaMerge/tests/gtest_skippable_block_input_stream.cpp#L99-L104: renamecandidate_rows.dbms/src/Storages/DeltaMerge/tests/gtest_skippable_block_input_stream.cpp#L287-L308: renametopkand related locals.dbms/src/Storages/DeltaMerge/tests/gtest_skippable_block_input_stream.cpp#L1042-L1178: rename new test locals such aswarmupRows,stage0Filters, andfilterValues.As per coding guidelines, “Method and variable names should use camelCase.”
🤖 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 93 - 116, Rename all newly introduced C++ methods, members, parameters, and locals to camelCase, preserving behavior: update adaptive state members such as running_topn and topn_adaptive_disabled in MultiStageLateMaterializationBlockInputStream, their initialization and references, and parameters/locals in updateTopNAdaptiveState and related logic. Also rename candidate_rows, topk-related locals, and test locals such as warmupRows, stage0Filters, and filterValues in dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationBlockInputStream.h:93-116, dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationBlockInputStream.cpp:62-65, 287-331, 382-459, and dbms/src/Storages/DeltaMerge/tests/gtest_skippable_block_input_stream.cpp:99-104, 287-308, 1042-1178.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.
Inline comments:
In `@docs/design/2026-07-26-running-local-topn-for-mslm.md`:
- Around line 575-577: Update the adaptive disable-ratio calculation to use
residual-passed rows, such as stage1_output_rows, rather than
residual_input_rows, so selective residual predicates do not dilute the ratio.
Define and document the zero-denominator behavior, and apply the same
denominator consistently to the warmup eligibility and post-warmup ratio checks
where relevant.
---
Nitpick comments:
In
`@dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationBlockInputStream.h`:
- Around line 93-116: Rename all newly introduced C++ methods, members,
parameters, and locals to camelCase, preserving behavior: update adaptive state
members such as running_topn and topn_adaptive_disabled in
MultiStageLateMaterializationBlockInputStream, their initialization and
references, and parameters/locals in updateTopNAdaptiveState and related logic.
Also rename candidate_rows, topk-related locals, and test locals such as
warmupRows, stage0Filters, and filterValues in
dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationBlockInputStream.h:93-116,
dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationBlockInputStream.cpp:62-65,
287-331, 382-459, and
dbms/src/Storages/DeltaMerge/tests/gtest_skippable_block_input_stream.cpp:99-104,
287-308, 1042-1178.
In `@docs/design/2026-07-26-running-local-topn-for-mslm.md`:
- Around line 658-660: Remove the overwrite of the residual Selection executor’s
actRows with topn_candidate_rows in the TopN-enhanced MSLM path. Preserve
Selection::actRows as the number of rows passing residual Selection, and expose
storage candidate rows through a separate clearly labeled runtime metric for
EXPLAIN ANALYZE.
🪄 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: 41c5293a-e439-43fc-a10d-803e59880da1
📒 Files selected for processing (6)
dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationBlockInputStream.cppdbms/src/Storages/DeltaMerge/MultiStageLateMaterializationBlockInputStream.hdbms/src/Storages/DeltaMerge/MultiStageLateMaterializationRuntimeStats.hdbms/src/Storages/DeltaMerge/MultiStageLateMaterializationTopN.hdbms/src/Storages/DeltaMerge/tests/gtest_skippable_block_input_stream.cppdocs/design/2026-07-26-running-local-topn-for-mslm.md
🚧 Files skipped from review as they are similar to previous changes (2)
- dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationRuntimeStats.h
- dbms/src/Storages/DeltaMerge/MultiStageLateMaterializationTopN.h
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
|
/hold |
ti-chi-bot
Bot
added
the
do-not-merge/hold
|
[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:
|
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
|
/hold cancel |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
d813287
into
pingcap:feature/release-8.5-materialized-view
4 participants
What problem does this PR solve?
Issue Number: ref #11003
Problem Summary:
This PR improves multi-stage late materialization for queries with TopN by avoiding materializing all rest columns before local TopN pruning.
What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
dt_enable_multi_stage_late_materialization: disabled (0), selection-only (1), TopN-enhanced (2).0and improved propagation of TopN configuration through delta-merge execution.