[branch-4.1][fix](be) Preserve nested paths for lazy rowid fetch#65260
[branch-4.1][fix](be) Preserve nested paths for lazy rowid fetch#65260mrhhsg wants to merge 2 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
[fix](be) Preserve nested paths for lazy rowid fetch
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.
None
- Test:
- Build:
~/.codex/skills/doris-local-regression/scripts/doris-local-regression.sh
--network 10.26.20.3/24 build
- Build: ./build.sh --fe
- Unit Test: ./run-fe-ut.sh --run
org.apache.doris.nereids.glue.translator.PhysicalPlanTranslatorTest,org.apache.doris.nereids.processor.post.materialize.MaterializeProbeVisitorTest
- 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
---------
Co-authored-by: lihangyu <lihangyu@selectdb.com>
(cherry picked from commit 6a7fc86)
mrhhsg
force-pushed
the
backport-64242-branch-4.1
branch
from
1bbed3d to
be09eb6
Compare
|
/review |
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I found three blocking issues in this backport.
Critical checkpoints:
- Existing inline review threads: none were present, so no accepted comment duplicates an existing thread.
- Required AGENTS.md files: the provided required list was empty.
- Repo-local code-review skill: not found in this checkout after hidden-path and content searches, so I followed the review prompt and shared-ledger workflow directly.
- Validation:
git diff --check 27eda17673797a507474b04263d26681b4cd1f0a be09eb6f6d9220a1dc68b9e670c2a108722dd8d7passed.thirdparty/installedandthirdparty/installed/bin/protocare absent in this runner, so I did not run full FE/BE tests; two comments are direct source-proven Java compile blockers.
User focus: no additional user-provided focus points were supplied.
Subagent conclusions: OPT-1 became inline comment MAIN-1. TSC-1 and TSC-2 became inline comments MAIN-2 and MAIN-3. No candidates were dismissed or merged as duplicates. Convergence round 1 ended with both optimizer-rewrite and tests-session-config replying NO_NEW_VALUABLE_FINDINGS for the same MAIN-1/MAIN-2/MAIN-3 comment set.
|
|
||
| Set<Slot> requiredMaterializedSlots = new HashSet<>(); | ||
| requiredMaterializedSlots.add(baseSlot); | ||
| MaterializeProbeVisitor.ProbeContext context = new MaterializeProbeVisitor.ProbeContext( |
There was a problem hiding this comment.
This test class will not compile: ProbeContext only has the single-argument constructor ProbeContext(SlotReference slot), but this and two later tests call new ProbeContext(slot, requiredMaterializedSlots). Please either add the intended constructor/state to MaterializeProbeVisitor.ProbeContext or rewrite these tests to use the current API before landing the UT.
|
|
||
| package org.apache.doris.nereids.processor.post.materialize; | ||
|
|
||
| import org.apache.doris.analysis.ColumnAccessPath; |
There was a problem hiding this comment.
This import is unresolved in this branch. There is no org.apache.doris.analysis.ColumnAccessPath class or ColumnAccessPath.data(...) helper in the repo; the existing access-path representation is the thrift TColumnAccessPath/TDataAccessPath/TAccessPathType types. As written, the new UT fails at compilation before it can exercise the PR.
| return Optional.empty(); | ||
| } | ||
| return Optional.of(new MaterializeSource(scan, context.slot)); | ||
| return Optional.of( |
There was a problem hiding this comment.
This still does not get the nested access paths to BE. The visitor now returns the relation output slot, but the slot that becomes the materialization tuple descriptor is rebuilt later from the original lazy slot: PhysicalLazyMaterialize does ((SlotReference) lazySlot).withColumn(originalColumn), and withColumn() does not copy allAccessPaths/predicateAccessPaths. generateTupleDesc(materialize.getOutput(), ...) therefore creates a SlotDescriptor with empty access paths, so the new set_slot_access_paths() calls in BE are a no-op for the targeted rowid fetch. Please carry MaterializeSource.baseSlot's access paths onto the lazy output slot that is serialized for the fetch request, and add a test that checks the materialization tuple descriptor contains those paths.
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run compile |
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
2 participants
What problem does this PR solve?
Issue Number: None
Related PR: #64242
Problem Summary:
Backport #64242 to branch-4.1. Preserve nested access paths for TopN lazy materialization rowid fetch, avoid row-store fetch when lazy slots carry nested access paths, and keep struct iterators readable when child fields are pruned.
Backport note: branch-4.1 still uses the single-row
seek_and_read_by_rowid(...)API in one rowid-fetch path, so the cherry-pick conflict was resolved by setting slot access paths before the existing per-row fetch loop.Release note
None
Check List (For Author)
Test
git diff --check HEAD^ HEADbranch-4.1; diff scope is limited to the [fix](be) Preserve nested paths for lazy rowid fetch #64242 backport files.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)