github-actions[bot] commented on code in PR #65530:
URL: https://github.com/apache/doris/pull/65530#discussion_r3569275138
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/processor/post/materialize/LazyMaterializeTopN.java:
##########
@@ -235,6 +233,20 @@ private Plan doComputeTopN(PhysicalTopN<? extends Plan>
topN, CascadesContext ct
return result;
}
+ @VisibleForTesting
+ static List<Slot> collectRequiredOutputSlots(Map<Slot, MaterializeSource>
materializeMap,
+ Set<Slot> requiredMaterializedSlots, Set<Slot> materializedSlots) {
+ List<Slot> requiredOutputSlots = new ArrayList<>();
+ for (Map.Entry<Slot, MaterializeSource> entry :
materializeMap.entrySet()) {
+ if (requiredMaterializedSlots.contains(entry.getKey())
Review Comment:
This still misses the case where the eager output sharing the same base
column is itself another alias. A reduced shape is:
```text
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`.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/processor/post/materialize/LazySlotPruning.java:
##########
@@ -146,8 +147,8 @@ public Plan visitPhysicalFilter(PhysicalFilter<? extends
Plan> filter, Context c
filter.child().accept(this, contextForScan));
filter = (PhysicalFilter<? extends Plan>) filter
.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.
Review 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.
```text
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
