englefly commented on code in PR #68019:
URL: https://github.com/apache/doris/pull/68019#discussion_r4038492500
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/processor/post/materialize/LazyMaterializeTopN.java:
##########
@@ -292,6 +298,98 @@ private void collectProjectExprInputSlots(Plan plan,
Set<Slot> requiredMateriali
}
}
+ /**
+ * Keep the columns consumed below the TopN materialized during the scan,
resolved through identity
+ * aliases.
+ *
+ * <p>{@link MaterializeProbeVisitor} only protects the slot it is
tracing: a slot consumed by an
+ * operator on the way from the TopN down to the relation stops the probe,
but the columns an
+ * identity alias reads are never resolved. For
+ *
+ * <pre>
+ * OuterTopN(order by z)
+ * InnerTopN(order by x)
+ * Project(lazy_col AS x, lazy_col AS y, other_col AS z)
+ * OlapScan
+ * </pre>
+ *
+ * probing the outer output {@code y} resolves to the base column {@code
lazy_col}, so {@code lazy_col}
+ * is classified lazy and {@link LazySlotPruning} removes it from the
scan, while {@code lazy_col AS x}
+ * below the outer TopN is still read by the inner TopN. The plan then
references a slot its child no
+ * longer produces and the final {@link Validator} rejects it. The same
happens when an identity alias
+ * is consumed by a filter, a join condition or any other operator that
stays below the TopN.
+ *
+ * <p>Therefore every slot consumed below this TopN (its own order keys,
the expressions of every
+ * descendant operator and the slots that are required materialized
already) is resolved through its
+ * identity alias chain. Project expressions are handled by {@link
#collectProjectExprInputSlots},
+ * which knows that a transparent {@code Alias(Slot)} output may still be
fetched lazily.
+ *
+ * <p>A set operation is a boundary: {@link MaterializeProbeVisitor} never
reports a lazy source for a
+ * slot produced by a set operation, and {@link #collectIdentityAliasMap}
stops at it, so the aliases
+ * below a set operation are neither resolved nor reachable. If lazy
materialization is ever extended
+ * through set operations, the consumed slots have to be resolved per set
operation branch instead.
+ */
+ private void collectRequiredAliasSources(PhysicalTopN<? extends Plan> topN,
+ Set<Slot> requiredMaterializedSlots) {
+ Map<Slot, Slot> aliasToChild = new HashMap<>();
+ collectIdentityAliasMap(topN.child(), aliasToChild);
+
+ Set<Slot> consumedSlots = new HashSet<>();
+ for (OrderKey orderKey : topN.getOrderKeys()) {
+ consumedSlots.addAll(orderKey.getExpr().getInputSlots());
+ }
+ collectConsumedSlots(topN.child(), consumedSlots);
+ consumedSlots.addAll(requiredMaterializedSlots);
+ for (Slot slot : consumedSlots) {
+ collectAliasChain(slot, aliasToChild, requiredMaterializedSlots);
+ }
+ }
+
+ /** Collect the slots consumed by the expressions of the operators that
stay below the TopN. */
+ private void collectConsumedSlots(Plan plan, Set<Slot> consumedSlots) {
+ if (plan instanceof PhysicalSetOperation) {
+ // Set operations are not materialized lazily, so nothing below
them can be lazy either.
+ return;
+ }
+ if (!(plan instanceof PhysicalProject)) {
+ // Project expressions are covered by
collectProjectExprInputSlots, which keeps the input of a
+ // transparent Alias(Slot) lazy because that alias output may
still be fetched later.
+ consumedSlots.addAll(plan.getInputSlots());
Review Comment:
Fixed in e7155731e2f. You are right that an unmapped direct consumer must
not become required: the walk now resolves only the consumed slots that take
part in an identity alias chain - the alias outputs and the slots an alias
reads.
I kept one half of the "unmapped seed" case though, because dropping it
entirely reopens the same invalid-plan class through the aliases: a slot that
an identity alias reads still has to be closed, since probing an output that
aliases it resolves to that slot without passing the operator that consumes it.
Concretely
```sql
select s.y, s.w from (
select lazy_col as y, other_col as w, arr, sort_col, lazy_col from
lat_t) s
left join lateral unnest(s.arr) tt(tag) on tt.tag = s.lazy_col
order by s.sort_col limit 1;
```
is valid with the fix, and with "only mapped seeds are resolved" (I built
that variant to check) it fails with `A expression contains slot not from
children`: the conjunct reads the bare `lazy_col`, the probe of `y` resolves
through `lazy_col AS y`, and no operator below the TopN stops the probe for the
bare slot. So the condition is `aliasToChild.containsKey(slot) ||
aliasSources.contains(slot)`, which leaves every direct consumer that no alias
reads - including the index-filter predicate slot - untouched.
Verified for this comment:
- `topNLazyMaterializationUsingIndex` is back to
`materializedSlots:(t1.username) lazySlots:(t1.addr,t1.age,t1.user_id)` for
`select * from t1 where user_id = 1 order by username limit 1`, and the whole
`query_p0/topn_lazy` directory (8 suites) passes.
- The suite now also asserts an index-mode shape where the direct filter
predicate stays lazy while the alias feeding the sort key stays materialized:
`select lazy_col as x, sort_col, other_col from t where sort_col > 0 order by x
limit 1` -> `materializedSlots:(x) lazySlots:(other_col,sort_col)`, result `10
1 100`.
- The bare-conjunct lateral shape above is pinned as
`qt_lateral_generate_bare_conjunct_plan`, and
`TopnLazyMaterializeTest#testIndexFilterPredicateSlotStaysLazy` covers the
index-mode case as a unit test.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/processor/post/materialize/LazyMaterializeTopN.java:
##########
@@ -292,6 +298,98 @@ private void collectProjectExprInputSlots(Plan plan,
Set<Slot> requiredMateriali
}
}
+ /**
+ * Keep the columns consumed below the TopN materialized during the scan,
resolved through identity
+ * aliases.
+ *
+ * <p>{@link MaterializeProbeVisitor} only protects the slot it is
tracing: a slot consumed by an
+ * operator on the way from the TopN down to the relation stops the probe,
but the columns an
+ * identity alias reads are never resolved. For
+ *
+ * <pre>
+ * OuterTopN(order by z)
+ * InnerTopN(order by x)
+ * Project(lazy_col AS x, lazy_col AS y, other_col AS z)
+ * OlapScan
+ * </pre>
+ *
+ * probing the outer output {@code y} resolves to the base column {@code
lazy_col}, so {@code lazy_col}
+ * is classified lazy and {@link LazySlotPruning} removes it from the
scan, while {@code lazy_col AS x}
+ * below the outer TopN is still read by the inner TopN. The plan then
references a slot its child no
+ * longer produces and the final {@link Validator} rejects it. The same
happens when an identity alias
+ * is consumed by a filter, a join condition or any other operator that
stays below the TopN.
+ *
+ * <p>Therefore every slot consumed below this TopN (its own order keys,
the expressions of every
+ * descendant operator and the slots that are required materialized
already) is resolved through its
+ * identity alias chain. Project expressions are handled by {@link
#collectProjectExprInputSlots},
+ * which knows that a transparent {@code Alias(Slot)} output may still be
fetched lazily.
+ *
+ * <p>A set operation is a boundary: {@link MaterializeProbeVisitor} never
reports a lazy source for a
+ * slot produced by a set operation, and {@link #collectIdentityAliasMap}
stops at it, so the aliases
+ * below a set operation are neither resolved nor reachable. If lazy
materialization is ever extended
+ * through set operations, the consumed slots have to be resolved per set
operation branch instead.
+ */
+ private void collectRequiredAliasSources(PhysicalTopN<? extends Plan> topN,
+ Set<Slot> requiredMaterializedSlots) {
+ Map<Slot, Slot> aliasToChild = new HashMap<>();
+ collectIdentityAliasMap(topN.child(), aliasToChild);
+
+ Set<Slot> consumedSlots = new HashSet<>();
+ for (OrderKey orderKey : topN.getOrderKeys()) {
+ consumedSlots.addAll(orderKey.getExpr().getInputSlots());
+ }
+ collectConsumedSlots(topN.child(), consumedSlots);
+ consumedSlots.addAll(requiredMaterializedSlots);
+ for (Slot slot : consumedSlots) {
+ collectAliasChain(slot, aliasToChild, requiredMaterializedSlots);
+ }
+ }
+
+ /** Collect the slots consumed by the expressions of the operators that
stay below the TopN. */
+ private void collectConsumedSlots(Plan plan, Set<Slot> consumedSlots) {
+ if (plan instanceof PhysicalSetOperation) {
+ // Set operations are not materialized lazily, so nothing below
them can be lazy either.
+ return;
+ }
+ if (!(plan instanceof PhysicalProject)) {
+ // Project expressions are covered by
collectProjectExprInputSlots, which keeps the input of a
+ // transparent Alias(Slot) lazy because that alias output may
still be fetched later.
+ consumedSlots.addAll(plan.getInputSlots());
Review Comment:
Fixed in e7155731e2f, "here" rather than in
`PhysicalGenerate.getInputSlots()`: `collectConsumedSlots` now adds
`PlanUtils.fastGetInputSlots(generate.getConjuncts())` minus the generator
outputs, exactly like `LogicalGenerate.getInputSlots()` does.
`PhysicalGenerate.getInputSlots()` is left untouched to keep the fix contained,
since it is also read by the validator and by the other physical
post-processors.
Your shape reproduced as reported - `A expression contains slot not from
children`, with `lazy_col` pruned from the scan while `Project[lazy_col AS x,
...]` below the generate still reads it:
```sql
set fe_debug = false;
set topn_lazy_materialization_threshold = 1;
explain shape plan
select s.y, s.w from (
select lazy_col as x, lazy_col as y, other_col as w, arr, sort_col from
lat_t) s
left join lateral unnest(s.arr) tt(tag) on tt.tag = s.x
order by s.sort_col limit 1;
```
With the fix the plan is valid and still selective:
`PhysicalLazyMaterialize[materializedSlots:(s.y,s.sort_col) lazySlots:(s.w)]`
over `PhysicalLazyMaterializeOlapScan[lat_t lazySlots:(other_col)]`, result `10
100`.
`fe_debug=false` lateral-UNNEST alias regression added as
`qt_lateral_generate_conjunct_plan` /
`order_qt_lateral_generate_conjunct_result` in
`query_p0/topn_lazy/order_by_alias`, with the plan shape asserted; the conjunct
variant that reads the bare column is covered as well
(`qt_lateral_generate_bare_conjunct_plan`). Unit test:
`TopnLazyMaterializeTest#testLateralGenerateConjunctKeepsItsAliasSourceMaterialized`,
which also asserts that only `other_col` stays lazy.
--
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]
