[fix](fe) Preserve required slots in TopN lazy materialization#65530
[fix](fe) Preserve required slots in TopN lazy materialization#65530morrySnow wants to merge 1 commit into
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#51329 Problem Summary: TopN lazy materialization could produce invalid plans in two slot-pruning cases: an output alias was selected as lazy while its base slot remained an eager sorting output, or an eager sorting slot used by a Filter was removed together with lazy predicate slots. Keep aliases eager when their base is eager, and remove only selected lazy slots from Filter outputs. ### Release note Fix invalid plans caused by pruning slots still required by TopN lazy materialization. ### Check List (For Author) - Test: Unit Test and Regression Test - LazyMaterializeTopNTest - LazySlotPruningTest - query_p0/topn_lazy/topn_lazy_on_data_model - query_p0/topn_lazy/usingIndex/topNLazyMaterializationUsingIndex - Behavior changed: Yes (slots required by TopN ordering remain eagerly materialized) - Does this need documentation: No
morrySnow
requested review from
924060929,
englefly and
starocean999
as code owners
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/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 two remaining issues in the TopN lazy materialization changes. The direct order-key pruning case is improved and the added tests are deterministic, but there are still unhandled slot-retention cases that can either produce an invalid plan or carry unnecessary materialized columns through TopN.
Checkpoint summary: the task goal is clear, but not fully accomplished for alias-vs-alias sharing and predicate-only filter slots; the patch is focused; no concurrency, lifecycle, persistence, config, FE/BE protocol, or cloud/transaction behavior is involved; the relevant parallel paths are the project alias and using-index filter pruning paths; test coverage was checked and should be extended for the alias-vs-alias case. I did not run builds or tests because this review environment is review-only.
| Set<Slot> requiredMaterializedSlots, Set<Slot> materializedSlots) { | ||
| List<Slot> requiredOutputSlots = new ArrayList<>(); | ||
| for (Map.Entry<Slot, MaterializeSource> entry : materializeMap.entrySet()) { | ||
| if (requiredMaterializedSlots.contains(entry.getKey()) |
There was a problem hiding this comment.
This still misses the case where the eager output sharing the same base column is itself another alias. A reduced shape is:
TopN(order by a)
Project(username AS a, username AS b)
Scan(username)
a is materialized because it is the TopN order key, but materializedSlots contains alias slot a, not the scan base slot username. For b, MaterializeProbeVisitor traces to MaterializeSource.baseSlot = username, and this check compares that base only with materializedSlots.contains(username), so b stays lazy. LazySlotPruning then maps lazy b to child slot username and removes it from the scan while keeping alias a, leaving Project(username AS a) above a child that no longer outputs username.
Please protect base slots referenced by retained simple aliases as well, and add a regression such as SELECT username AS a, username AS b FROM mow ORDER BY a LIMIT 5.
| .copyStatsAndGroupIdFrom(filter).resetLogicalProperties(); | ||
| List<Slot> filterOutput = Lists.newArrayList(filter.getOutput()); | ||
| filterOutput.removeAll(filter.getInputSlots()); | ||
| // Predicate slots that are not lazy can still be required by TopN order keys. |
There was a problem hiding this comment.
This projection now keeps every non-lazy Filter input above the Filter. In the using-index path, some predicate slots still have to be read eagerly to evaluate the Filter, but they are not necessarily needed by TopN or the final output.
Project(v, pad)
TopN(order by k1)
Filter(v IS NULL AND x > 0)
Scan(k1, v, pad, x)
Here v can be lazy and triggers this branch, k1 must stay for ordering, but x is predicate-only. Since filter.getOutput() is the child output, subtracting only context.lazySlots leaves [k1, x, row_id], so x is carried through TopN and PhysicalLazyMaterialize as materialized tuple data even though it should be dropped after the Filter. The fix should project the slots required above the Filter, for example order/materialized outputs plus row id, rather than all non-lazy Filter inputs.
TPC-H: Total hot run time: 29475 ms |
TPC-DS: Total hot run time: 180487 ms |
ClickBench: Total hot run time: 25.22 s |
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
2 participants
What problem does this PR solve?
Issue Number: None
Related PR: #51329
Problem Summary: TopN lazy materialization could produce invalid plans in two slot-pruning cases: an output alias was selected as lazy while its base slot remained an eager sorting output, or an eager sorting slot used by a Filter was removed together with lazy predicate slots. Keep aliases eager when their base is eager, and remove only selected lazy slots from Filter outputs.
Release note
Fix invalid plans caused by pruning slots still required by TopN lazy materialization.
Check List (For Author)