github-actions[bot] commented on code in PR #68019:
URL: https://github.com/apache/doris/pull/68019#discussion_r4023398059
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/processor/post/materialize/LazyMaterializeTopN.java:
##########
@@ -292,6 +298,69 @@ private void collectProjectExprInputSlots(Plan plan,
Set<Slot> requiredMateriali
}
}
+ /**
+ * Keep the columns read by the TopN order keys materialized during the
scan.
+ *
+ * <p>{@link MaterializeProbeVisitor} only protects the order key slot
itself, because an order key
+ * slot is in {@code TopN.getInputSlots()}; it never resolves an identity
alias down to the column
+ * that the alias reads. For
+ *
+ * <pre>
+ * TopN(order by x)
+ * Project(lazy_col AS x, lazy_col AS y)
+ * OlapScan
+ * </pre>
+ *
+ * probing the 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 TopN still reads it. The plan then references a slot its
child no longer produces and
+ * the final {@link Validator} rejects it. Resolving the order keys
through the alias chain lets the
+ * probe reject every output backed by those columns, keeping the plan
valid.
+ *
+ * <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 order keys have to be resolved per set
operation branch instead.
+ */
+ private void collectOrderKeyColumns(PhysicalTopN<? extends Plan> topN,
Set<Slot> requiredMaterializedSlots) {
+ Map<Slot, Slot> aliasToChild = new HashMap<>();
+ collectIdentityAliasMap(topN.child(), aliasToChild);
+ for (OrderKey orderKey : topN.getOrderKeys()) {
Review Comment:
**[P1] Protect alias bases consumed below this TopN** Resolving only this
TopN's order keys leaves the same invalid-plan bug for descendant TopNs. For
example, `select y from (select lazy_col AS x, lazy_col AS y, other_col AS z
from topn_lazy_order_by_alias_tbl order by x limit 2) s order by z limit 1`
preserves an outer `TopN(z) -> ... -> TopN(x) -> Project(a AS x, a AS y, b AS
z) -> Scan(a,b)`. This loop protects only `z -> b`; probing `y` can still mark
`a` lazy, after which pruning removes scan slot `a` while retaining `a AS x`
for the inner TopN, and the final `Validator` rejects the plan when
`fe_debug=false`. Please close identity-alias dependencies for retained
descendant consumers/already-required slots (stopping at the same boundaries)
and add this nested regression.
##########
regression-test/suites/query_p0/topn_lazy/order_by_alias/topn_lazy_order_by_alias.groovy:
##########
@@ -0,0 +1,82 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+suite("topn_lazy_order_by_alias") {
+ // TopN lazy materialization only skips the rewrite when a plan validation
fails, and that
+ // validation only runs with fe_debug on. Pin it off so an invalid plan
surfaces as an error.
+ sql """ set fe_debug = false; """
+
+ sql """
+ drop table if exists topn_lazy_order_by_alias_tbl;
+ create table topn_lazy_order_by_alias_tbl (
+ sort_col int,
+ lazy_col int,
+ other_col int
+ ) duplicate key(sort_col)
+ distributed by hash(sort_col) buckets 1
+ properties('replication_num' = '1');
+ """
+ sql """
+ insert into topn_lazy_order_by_alias_tbl values
(3,30,300),(1,10,100),(2,20,200);
+ """
+
+ // Sorting by an alias of lazy_col: lazy_col feeds the sort key, so it
must stay materialized
+ // instead of being pruned from the scan while `lazy_col AS x` below the
TopN still reads it.
+ order_qt_repeated_alias_of_sort_key """
+ select lazy_col as x, lazy_col as y
+ from topn_lazy_order_by_alias_tbl order by x limit 1;
+ """
+
+ // Same shape ordered by the other alias.
+ order_qt_repeated_alias_reversed """
+ select lazy_col as x, lazy_col as y
+ from topn_lazy_order_by_alias_tbl order by y limit 1;
+ """
+
+ // A bare column plus an alias of it, sorted by the bare column.
+ order_qt_bare_column_and_alias """
+ select lazy_col, lazy_col as y
+ from topn_lazy_order_by_alias_tbl order by lazy_col limit 1;
+ """
+
+ // Only the column the order key reads is forced materialized; other_col
is still lazily fetched.
+ order_qt_other_column_still_lazy """
+ select lazy_col as x, other_col as y
+ from topn_lazy_order_by_alias_tbl order by x limit 1;
+ """
+
+ // Control: sorting by a column that is not aliased keeps both columns
lazily fetched.
+ order_qt_order_by_plain_column """
+ select lazy_col as x, other_col as y
+ from topn_lazy_order_by_alias_tbl order by sort_col limit 1;
+ """
+
+ // The same shapes through the inverted-index filter path.
+ sql """ set topn_lazy_materialization_using_index = true; """
+
+ order_qt_using_index_repeated_alias """
Review Comment:
**[P2] Exercise an index-mode shape that failed before this patch** These
two index-mode queries are pre-fix-insensitive: when
`topn_lazy_materialization_using_index` is enabled,
`MaterializeProbeVisitor.visitPhysicalProject` refuses an identity `Alias` as a
lazy source and eagerly requires its child, so both alias-heavy shapes already
keep `lazy_col` materialized on the old code. A sensitive case is `select
lazy_col AS x, lazy_col, other_col from topn_lazy_order_by_alias_tbl where
sort_col > 0 order by x limit 1`: before this change the bare `lazy_col` can be
pruned while the retained alias still reads it, whereas the fix should protect
it and leave `other_col` lazy. Please add this shape (with the validator
enabled), plus a plan assertion if this suite is intended to prove selective
laziness rather than only successful execution.
--
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]
