github-actions[bot] commented on code in PR #67796:
URL: https://github.com/apache/doris/pull/67796#discussion_r4002355934
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownTopNDistinctThroughJoin.java:
##########
@@ -197,6 +201,31 @@ private List<OrderKey> getPushedOrderKeys(Set<Slot>
groupBySlots, Set<Slot> join
notFound = true;
}
}
- return pushedOrderKeys.build();
+ List<OrderKey> pushedOrderKeyList = pushedOrderKeys.build();
+ if (pushedOrderKeyList.size() == orderKeys.size()
+ || isOrderKeyPrefixUniqueAfterDistinct(joinChild,
pushedOrderKeyList)) {
+ return pushedOrderKeyList;
+ }
+ return ImmutableList.of();
+ }
+
+ /**
+ * A partial order-key prefix is safe for a hard limit only when it
uniquely orders the rows produced by
+ * {@link PlanUtils#distinct(Plan)}. This is true when a leading part of
the prefix either is already a
+ * non-null unique key, covers every child output, or functionally
determines every remaining child output.
+ */
+ private boolean isOrderKeyPrefixUniqueAfterDistinct(Plan joinChild,
List<OrderKey> orderKeyPrefix) {
+ Set<Slot> childOutput = joinChild.getOutputSet();
+ DataTrait childTrait = joinChild.getLogicalProperties().getTrait();
+ Set<Slot> prefixSlots = new HashSet<>();
+ for (OrderKey orderKey : orderKeyPrefix) {
+ prefixSlots.add((Slot) orderKey.getExpr());
+ if (prefixSlots.containsAll(childOutput) ||
childTrait.isUniqueAndNotNull(prefixSlots)
+ || childOutput.stream().allMatch(slot ->
prefixSlots.contains(slot)
Review Comment:
[P2] Apply FD augmentation to the accumulated prefix
For a child `Project(a, b, a + 1 AS c)`, the trait records `{a} -> {c}`.
With final order `(a, b, right.d)`, the accumulated child prefix `{a,b}`
therefore determines the full `DISTINCT(a,b,c)` row and is safe to push. This
check nevertheless asks `isDependent({a,b}, {c})`, while `FuncDeps.isFuncDeps`
only matches the exact stored determinant `{a}`, so the advertised safe
optimization is skipped. Please make this proof augmentation/closure-aware (for
example, accept a valid determinant contained in the prefix) and add the
augmented-prefix rule test.
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/PushDownTopNDistinctThroughJoinTest.java:
##########
@@ -0,0 +1,99 @@
+// 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.
+
+package org.apache.doris.nereids.rules.rewrite;
+
+import org.apache.doris.nereids.properties.OrderKey;
+import org.apache.doris.nereids.trees.expressions.Alias;
+import org.apache.doris.nereids.trees.expressions.Expression;
+import org.apache.doris.nereids.trees.expressions.NamedExpression;
+import org.apache.doris.nereids.trees.expressions.Slot;
+import org.apache.doris.nereids.trees.plans.Plan;
+import org.apache.doris.nereids.trees.plans.logical.LogicalOlapScan;
+import org.apache.doris.nereids.trees.plans.logical.LogicalProject;
+import org.apache.doris.nereids.util.PlanConstructor;
+import org.apache.doris.qe.ConnectContext;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import java.util.List;
+import java.util.Set;
+
+class PushDownTopNDistinctThroughJoinTest {
+ private static final PushDownTopNDistinctThroughJoin RULE = new
PushDownTopNDistinctThroughJoin();
+ private static final LogicalOlapScan LEFT_SCAN =
PlanConstructor.newLogicalOlapScan(0, "t1", 0);
+ private static final LogicalOlapScan RIGHT_SCAN =
PlanConstructor.newLogicalOlapScan(1, "t2", 0);
+
+ @BeforeEach
+ void setUp() {
+ new ConnectContext().setThreadLocalInfo();
+ }
+
+ @AfterEach
+ void tearDown() {
+ ConnectContext.remove();
+ }
+
+ @Test
+ void rejectNonUniquePartialOrderKeys() {
Review Comment:
[P2] Exercise the registered rewrites, not only the helper
All four tests enter production code only through `getPushedOrderKeys`, so
they can pass even if either `TopN -> DISTINCT -> Join` rule shape fails to
match or installs the child TopN incorrectly. They also use DUP-key scans,
leaving the new `isUniqueAndNotNull` acceptance (and nullable-unique rejection)
untested. Please add `PlanChecker`/actual-rewrite cases for the direct and
all-slots-project shapes, including a non-null unique prefix and its nullable
negative, and assert presence or absence of `TopN(DISTINCT(child))`. This also
lets the helper remain private.
##########
regression-test/suites/nereids_rules_p0/push_down_top_n/push_down_top_n_distinct_through_join.groovy:
##########
@@ -67,4 +67,61 @@ suite("push_down_top_n_distinct_through_join") {
qt_push_down_topn_through_join_data """
select distinct * from (select t1.id from table_join t1 cross join
table_join t2) t order by id limit 10;
"""
+
+ sql "DROP TABLE IF EXISTS topn_distinct_left"
+ sql "DROP TABLE IF EXISTS topn_distinct_right"
+ sql """
+ CREATE TABLE topn_distinct_left (
+ k INT NOT NULL,
+ id INT NOT NULL
+ ) DUPLICATE KEY(k, id)
+ DISTRIBUTED BY HASH(id) BUCKETS 1
+ PROPERTIES("replication_num" = "1")
+ """
+ sql """
+ CREATE TABLE topn_distinct_right (
+ id INT NOT NULL,
+ s INT NOT NULL
+ ) DUPLICATE KEY(id)
+ DISTRIBUTED BY HASH(id) BUCKETS 1
+ PROPERTIES("replication_num" = "1")
+ """
+ sql """
+ INSERT INTO topn_distinct_left VALUES
+ (0, 1), (0, 2), (0, 3), (0, 4), (0, 5), (0, 6), (0, 7), (0, 8)
+ """
+ sql """
+ INSERT INTO topn_distinct_right VALUES
+ (1, 10), (2, 20), (3, 30), (4, 40), (5, 50), (6, 60), (7, 70), (8,
80)
+ """
+
+ test {
Review Comment:
[P2] Record these deterministic results through the generated oracle
These are fixed expected result sets, but both cases hand-code
`test/check/assertEquals` and the PR does not update the suite's generated
`.out` file. Doris's regression contract requires determined results to use
named `qt`/`order_qt` cases and have their outputs generated by the regression
runner. Please convert both queries and commit the generated output; keep a
separate plan assertion if you want to prove that the unsafe child TopN is
absent.
--
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]
