[fix](be) Preserve nested paths for lazy rowid fetch#64242
Open
mrhhsg wants to merge 2 commits into
Open
Conversation
mrhhsg
requested review from
924060929,
englefly,
gavinchou,
liaoxin01,
morrySnow,
starocean999 and
yiguolei
as code owners
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
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.
Review result: no blocking issues found.
Critical checkpoint conclusions:
- Goal and proof: The PR preserves nested access paths for TopN lazy rowid fetch, disables row-store lazy fetch for complex or nested-pruned slots, and adds FE unit plus regression coverage for the affected path.
- Scope: The changes are focused on lazy materialization slot propagation, row-store eligibility, and BE rowid storage reads.
- Concurrency: No new shared mutable state, thread entry, lock ordering, or lifecycle/concurrency-sensitive path was introduced.
- Lifecycle/static initialization: Only namespace-scope helper functions were added; no new static object lifecycle issue found.
- Configuration: No new config item was added.
- Compatibility: Protobuf slot descriptors already carry access paths; this change consumes them in the rowid fetch path without introducing new protocol fields.
- Parallel paths: Both PMultiGetRequest and PMultiGetRequestV2/internal rowid fetch paths are covered; row-store fallback is guarded on both FE and BE sides.
- Special checks: Complex/nested-pruned row-store rejection has a clear storage-layout reason and matching tests.
- Tests: Added FE unit assertions and a regression test with ordered output; I did not rerun tests in this review runner.
- Observability: Existing errors/logging are sufficient for this narrow read-path fix.
- Transaction/persistence/data writes: No transaction, persistence, or data write behavior is changed.
- FE-BE variable passing: Existing SlotDescriptor access-path serialization/deserialization is used; rowid fetch now passes those options to segment iterators.
- Performance: Row-store is conservatively disabled only where it cannot return the required nested/pruned layout; no obvious avoidable hot-path regression found.
User focus response: No additional user-provided review focus was present.
Residual risk: Coverage is strongest for internal rowid fetch with nested STRUCT/MAP/ARRAY pruning; external table lazy materialization paths were checked for obvious incompatibility but not executed here.
Member
Author
|
run buildall |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
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: 29446 ms |
Contributor
TPC-DS: Total hot run time: 170485 ms |
mrhhsg
force-pushed
the
fix/topn-lazy-nested-rowid-fetch
branch
from
751a4be to
a61638c
Compare
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review summary: no blocking issues found in the actual GitHub PR diff.
Critical checkpoint conclusions:
- Goal and proof: The change preserves nested access paths for TopN lazy rowid fetch, disables row-store fetch where BE cannot apply nested/complex projections, and adds FE/unit plus regression coverage for nested lazy materialization.
- Scope/focus: The actual PR diff is small and focused on lazy materialization rowid fetch, access-path propagation, row-store eligibility, and struct iterator pruning.
- Concurrency/lifecycle: No new shared mutable lifecycle or concurrency paths were introduced; existing segment/iterator ownership is preserved.
- Configuration/compatibility: No new config or incompatible storage/protocol format change was introduced.
- Parallel paths: Both old
PMultiGetRequestand V2 block-desc rowid fetch paths are guarded for row-store fallback and pass slot access paths into storage reads. - Special checks: The row-store disqualification for nested access paths and complex types is consistent with BE row-store limitations.
- Tests: Added unit coverage for relation-slot access paths and row-store eligibility, plus a regression query that exercises sparse nested STRUCT/MAP/ARRAY lazy fetch with ordered output.
- Observability: No additional observability appears necessary for this narrowly scoped correctness fix.
- Transaction/persistence/data-write concerns: Not applicable; this is a read-path/materialization change.
- Performance: Disabling row-store only for unsupported nested/complex cases is appropriate; simple scalar row-store lazy fetch remains eligible.
User focus: no additional user-provided review focus was supplied.
HappenLee
reviewed
Jun 9, 2026
| } | ||
| slot->to_protobuf(mget_req.add_slots()); | ||
| } | ||
| bool fetch_row_store = _fetch_option.t_fetch_opt.fetch_row_store; |
Contributor
There was a problem hiding this comment.
这个是不是直接在FE改就行了，有sub column prune的时候不走行存，不用在be判断了
HappenLee
reviewed
Jun 9, 2026
| } | ||
|
|
||
| static ReaderType rowid_fetch_reader_type(const SlotDescriptor& slot) { | ||
| return has_nested_access_paths(slot) || has_complex_type(slot) ? ReaderType::READER_ALTER_TABLE |
Contributor
There was a problem hiding this comment.
为什么这里要用ReaderType::READER_ALTER_TABLE这种方式呢？
HappenLee
reviewed
Jun 9, 2026
| if (!slot.all_access_paths().empty()) { | ||
| storage_read_options.all_access_paths[unique_id] = slot.all_access_paths(); | ||
| } else { | ||
| storage_read_options.all_access_paths.erase(unique_id); |
Contributor
There was a problem hiding this comment.
为什么这里要erase，哪里还会插入对应的map吗
mrhhsg
force-pushed
the
fix/topn-lazy-nested-rowid-fetch
branch
3 times, most recently
from
da7118a to
767e993
Compare
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated review summary for PR 64242:
I reviewed the current diff and the existing inline review threads. I did not find an additional distinct blocking issue beyond the already-known comments in be/src/exec/rowid_fetcher.cpp.
Critical checkpoint conclusions:
- Goal and coverage: The PR addresses nested-column lazy materialization rowid fetch by propagating access paths to BE column-store fetch and by preventing row-store lazy fetch for nested/complex slots. Added FE unit tests and a regression case cover the targeted nested pruning path.
- Scope: The change is focused on lazy materialization, row-store eligibility, and rowid fetch access-path propagation.
- Concurrency: No new shared mutable state, thread lifecycle, atomics, or lock ordering changes were introduced. Existing batched/external rowid fetch scheduling remains unchanged.
- Lifecycle/static initialization: No new static objects with cross-translation-unit initialization dependencies were introduced.
- Configuration: No new configuration item was added.
- Compatibility: No storage format or thrift schema change was introduced; the BE protobuf request already carries slot access paths through existing fields.
- Parallel paths: Both older
PMultiGetRequestand batchedPMultiGetRequestV2Doris-format rowid fetch paths now set slot access paths before creating/reusing the column iterator. External-file fetch paths are not directly affected. - Conditional checks: The FE row-store gating is conservative for complex/nested lazy slots, matching the BE row-store limitation that it does not apply nested access paths.
- Tests: The added tests cover FE row-store gating, relation-slot access-path preservation, and an end-to-end regression query for nested STRUCT/MAP/ARRAY lazy rowid fetch. I did not run the tests locally in this review session.
- Test results: The regression output uses a deterministic
ORDER BYin the query and the new expected rows match the inserted data and ordering. - Observability: No new observability appears necessary for this focused correctness fix; existing rowid fetch logging/profile counters remain in place.
- Transaction/persistence/data write: The PR does not modify transaction processing, persistence, or write paths.
- FE/BE variable passing: The existing slot access-path fields are propagated through the lazy materialize output slot and then through
SlotDescriptorprotobuf to BE. - Performance: The BE change adds small per-slot map assignments before rowid column iterator creation/reuse; no obvious hot-path regression beyond the necessary access-path setup was found.
- User focus: No additional user-provided review focus was specified.
mrhhsg
force-pushed
the
fix/topn-lazy-nested-rowid-fetch
branch
from
767e993 to
1ceb1bb
Compare
Member
Author
|
/review |
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28864 ms |
Contributor
TPC-DS: Total hot run time: 169157 ms |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve?
Issue Number: None
Problem Summary: TopN lazy materialization can fetch pruned complex columns by row id after nested-column pruning. The materialization tuple kept the pruned slot type but did not preserve the relation slot access paths, so BE could build full storage iterators and read full child layouts into pruned result columns. Row-store lazy fetch also cannot apply nested access paths, so FE now rejects row-store fetch for complex or nested-pruned lazy slots. This patch carries relation slot access paths into lazy materialization slots, passes slot access paths to storage rowid fetch, keeps rowid fetch on the normal query reader path, preserves FE's row-store fetch decision in the BE request and row-store decode path, and keeps struct iterators readable when only some child fields are pruned.
### Release note
None
### Check List (For Author)
- Test:
- Build: ~/.codex/skills/doris-local-regression/scripts/doris-local-regression.sh --network 10.26.20.3/24 build
- Unit Test: ./run-fe-ut.sh --run org.apache.doris.nereids.processor.post.materialize.MaterializeProbeVisitorTest
- Unit Test: ./run-fe-ut.sh --run org.apache.doris.nereids.glue.translator.PhysicalPlanTranslatorTest#testCanUseRowStoreForLazySlots
- Regression test: ~/.codex/skills/doris-local-regression/scripts/doris-local-regression.sh --network 10.26.20.3/24 run -d nereids_rules_p0/column_pruning -s topn_lazy_nested_column_pruning
- Format: build-support/clang-format.sh be/src/exec/rowid_fetcher.cpp
- Check: git diff --check
- Behavior changed: No
- Does this need documentation: No
mrhhsg
force-pushed
the
fix/topn-lazy-nested-rowid-fetch
branch
from
72b6fc7 to
af79aaf
Compare
mrhhsg
added a commit
to mrhhsg/doris
that referenced
this pull request
Jun 11, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#64242 Problem Summary: Lazy TopN materialization remaps a relation slot to the output slot ExprId when the source slot carries sub-column metadata. The previous remap kept subPath but dropped nested access paths, so row-store eligibility checks could not see nested pruning metadata. Preserve access paths in this local remap and keep row-store lazy fetch disabled only for slots that actually carry sub-column paths or nested access paths, instead of using a broad complex-type guard without an independent failure case. ### Release note None ### Check List (For Author) - Test: Regression test / Unit Test - ./build.sh --fe - ./run-fe-ut.sh --run org.apache.doris.nereids.glue.translator.PhysicalPlanTranslatorTest,org.apache.doris.nereids.processor.post.materialize.MaterializeProbeVisitorTest - ~/.codex/skills/doris-local-regression/scripts/doris-local-regression.sh --network 10.26.20.3/24 run -d nereids_rules_p0/column_pruning -s topn_lazy_nested_column_pruning - Behavior changed: No - Does this need documentation: No
Contributor
|
PR approved by at least one committer and no changes requested. |
github-actions
Bot
added
approved
Contributor
|
PR approved by anyone and no changes requested. |
### What problem does this PR solve? Issue Number: None Related PR: apache#64242 Problem Summary: Lazy TopN materialization needs nested access paths from the relation output slot to reach the BE rowid fetch SlotDescriptor, and row-store lazy fetch must be disabled when a lazily fetched source column has nested access paths. Preserve the relation output slot as MaterializeSource.baseSlot for lazy materialization output, and make PhysicalPlanTranslator.shouldUseRowStore check the lazy slots against the relation output before enabling row-store fetch. ### Release note None ### Check List (For Author) - Test: Unit Test / Regression test / Build - ./run-fe-ut.sh --run org.apache.doris.nereids.glue.translator.PhysicalPlanTranslatorTest,org.apache.doris.nereids.processor.post.materialize.MaterializeProbeVisitorTest - ~/.codex/skills/doris-local-regression/scripts/doris-local-regression.sh --network 10.26.20.3/24 run -d nereids_rules_p0/column_pruning -s topn_lazy_nested_column_pruning - ./build.sh --be --fe - git diff --check - Behavior changed: No - Does this need documentation: No
mrhhsg
force-pushed
the
fix/topn-lazy-nested-rowid-fetch
branch
from
af79aaf to
7d7d152
Compare
github-actions
Bot
removed
the
approved
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review opinion: no blocking issues found in the final PR diff.
Critical checkpoint conclusions:
- Goal and proof: The PR preserves nested access-path metadata through TopN lazy materialization and passes those paths into BE row-id column-store fetch. The added
MaterializeProbeVisitorTestcases and thetopn_lazy_nested_column_pruningregression cover the targeted relation-slot/access-path path and sparse nested STRUCT/MAP/ARRAY row-id fetch. - Scope: The final PR is focused to lazy materialization metadata propagation, row-store eligibility for nested-pruned lazy slots, one struct iterator pruning behavior, and matching tests.
- Concurrency: No new shared mutable state, thread entry, atomics, dependency state, or lock ordering is introduced. Existing row-id RPC/task scheduling behavior is unchanged.
- Lifecycle/static initialization: Only helper methods/functions are added; no new static object or cross-translation-unit initialization dependency was found.
- Configuration: No new configuration item was added.
- Compatibility: No thrift/protobuf/storage-format field is added. The change reuses existing
SlotDescriptoraccess-path serialization and existing BEStorageReadOptionsmaps, so mixed-format compatibility risk is low. - Parallel paths: Both Doris-format row-id fetch paths are covered: the older
PMultiGetRequestpath and the V2/internal batched path both callset_slot_access_pathsbefore iterator creation/reuse. Row-store lazy fetch is guarded in FE for relation outputs carrying nested access paths. - Special conditions: The FE nested-path check is reached only after
canUseRowStoreForLazySlotsvalidates slot shape/original columns. The struct iterator change keeps the parent readable while pruning only skipped children, matching the downstream iterator layout requirement. - Tests: I did not run the full test suite in this review runner. I verified the PR patch applies cleanly in reverse with whitespace errors enabled. The submitted PR reports FE unit/regression coverage, and the added regression query uses explicit ordering and drops the table before use.
- Test results: The new expected regression rows match the inserted data and
ORDER BY ABS(pk % 3), pkordering. - Observability: No additional logs or metrics appear necessary for this narrow correctness fix; existing row-id fetch errors/statistics remain in place.
- Transaction/persistence/data writes: Not applicable; this PR changes read-path planning/execution and tests only.
- FE-BE variable passing: Access paths flow from relation output slots to materialized output slots, then through tuple slot descriptors into BE requests, where storage iterators consume them by normalized column unique id.
- Performance: The added per-slot access-path map setup is small and tied to row-id iterator creation/reuse. Row-store is only disabled for nested-pruned lazy slots where it cannot apply access paths.
- Other issues: The current
BE UT (macOS)check fails before running tests because the job reportsERROR: The JAVA version is 25, it must be JDK-17; I did not see evidence in the failed log that this is caused by the PR code.
Existing review context: I read the existing inline threads in be/src/exec/rowid_fetcher.cpp and did not duplicate the already-known comments about BE-side row-store gating, the earlier READER_ALTER_TABLE approach, or stale access-path erasure.
User focus: No additional user-provided review focus was present.
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28887 ms |
Contributor
TPC-DS: Total hot run time: 167729 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
englefly
approved these changes
Jun 15, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
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.
fix Preserve nested paths for lazy rowid fetch
What problem does this PR solve?
Issue Number: None
Problem Summary: TopN lazy materialization can fetch nested-pruned columns by row id after nested-column pruning. The probe must keep the relation output slot metadata; otherwise the slot remapped to the TopN output ExprId can lose nested access paths. Then FE may consider a nested-pruned lazy slot safe for row-store fetch, while BE row-store fetch maps values only by column unique id and cannot apply sub-column or nested access paths. This patch preserves relation slot access paths through lazy materialization, passes slot access paths to the normal storage rowid fetch path, rejects row-store fetch only for lazy slots that actually carry sub-column paths or nested access paths, and keeps struct iterators readable when child fields are pruned.
Release note
None
Check List (For Author)