morrySnow commented on code in PR #67796:
URL: https://github.com/apache/doris/pull/67796#discussion_r4004493432
##########
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:
Fixed in 5cdb4caae7b. The rule now obtains the child’s valid functional
dependencies for its output and accepts a determinant contained in the
accumulated order-key prefix, so `{a} -> {c}` proves that `{a, b}` determines
`c`. Added an actual rewrite test for the augmented-prefix project case; the
targeted FE tests pass (3/3).
##########
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)
+ || childTrait.isDependent(prefixSlots,
ImmutableSet.of(slot)))) {
Review Comment:
已在 5cdb4caae7b 修复。现在会从 child output 获取有效 FD，并判断 FD determinant 是否为累计前缀的子集，因此
`a -> c` 可以证明 `{a, b} -> c`。对应的实际 rewrite 用例已补充，定向 FE UT 3/3 通过。
##########
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:
Fixed in 5cdb4caae7b. The helper is now private, and the tests exercise the
registered rule through `PlanChecker.applyTopDown(RULE)` for the direct and
all-slots-project shapes, including a non-null unique-prefix positive case and
a nullable-unique negative case. The targeted FE tests pass (3/3).
##########
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:
Fixed in 5cdb4caae7b. The deterministic result checks now use labeled
`order_qt` cases, and the `.out` oracle was generated by the regression runner.
Both the `-forceGenOut` and normal regression runs pass.
--
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]
