This is an automated email from the ASF dual-hosted git repository.
yiguolei pushed a commit to branch branch-4.2
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/branch-4.2 by this push:
new 9fb0c9b6141 branch-4.2 [fix](topn-filter) Prevent unsafe TopN filter
pushdown through outer joins (#70106)
9fb0c9b6141 is described below
commit 9fb0c9b6141d8f3c5855c543e99390f3b1e88a22
Author: minghong <[email protected]>
AuthorDate: Wed Nov 4 08:29:57 2026 +0800
branch-4.2 [fix](topn-filter) Prevent unsafe TopN filter pushdown through
outer joins (#70106)
### What problem does this PR solve?
Issue Number: N/A
Related PR: #70100
Backport #70100 (commit b5c622238a8d25369278381222b8937cde43b362) to
`branch-4.2`.
Prevent TopN runtime filters from removing matching rows on
null-generating outer join sides when the ordering expression can turn
NULL into a non-NULL value. Apply the shared safety check to hash and
nested loop joins and recheck rewritten hash join keys.
The cherry-pick applied without conflicts. The only branch-specific
adjustment replaces `List.of` with the existing Guava `ImmutableList.of`
in the new unit tests because `branch-4.2` compiles against Java 8 APIs.
Production logic and the regression suite are unchanged from the source
PR.
### Release note
Fix incorrect TopN query results for outer joins ordered by expressions
that convert NULL inputs into non-NULL values.
### Check List (For Author)
- Test
- [x] Unit Test: `TopNRuntimeFilterTest` — 13 tests passed, 0
failures/errors/skips, including the three new visitor tests.
- [ ] Regression test: `topn_filter_outer_join_null` is included; not
rerun against a running cluster on this branch.
Validation:
- FE production sources compiled successfully.
- After the Java 8 test adjustment, reran test compilation and the
targeted suite with the existing production classes: `mvn test -pl
fe-common,fe-core -am -Dskip.clean=true -Dmaven.main.skip=true
-Dcheckstyle.skip=true -DfailIfNoTests=false
-Dtest=org.apache.doris.nereids.postprocess.TopNRuntimeFilterTest` (from
`fe/`).
- `mvn checkstyle:check -pl fe-core` — 0 violations.
- Behavior changed:
- [x] Yes. Unsafe TopN runtime filters are no longer pushed through
null-generating outer join sides.
- Does this need documentation?
- [x] No.
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label
---
.../processor/post/TopnFilterPushDownVisitor.java | 29 ++-
.../nereids/postprocess/TopNRuntimeFilterTest.java | 74 ++++++
.../topn_filter_outer_join_null.out | 247 +++++++++++++++++++++
.../topn_filter_outer_join_null.groovy | 82 +++++++
4 files changed, 422 insertions(+), 10 deletions(-)
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/nereids/processor/post/TopnFilterPushDownVisitor.java
b/fe/fe-core/src/main/java/org/apache/doris/nereids/processor/post/TopnFilterPushDownVisitor.java
index 2519e703c06..7fd3c0727ea 100644
---
a/fe/fe-core/src/main/java/org/apache/doris/nereids/processor/post/TopnFilterPushDownVisitor.java
+++
b/fe/fe-core/src/main/java/org/apache/doris/nereids/processor/post/TopnFilterPushDownVisitor.java
@@ -21,9 +21,11 @@ import
org.apache.doris.nereids.processor.post.TopnFilterPushDownVisitor.PushDow
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.NullToNonNullFunction;
import org.apache.doris.nereids.trees.expressions.Slot;
import org.apache.doris.nereids.trees.expressions.functions.scalar.Nullable;
import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitors;
+import org.apache.doris.nereids.trees.plans.JoinType;
import org.apache.doris.nereids.trees.plans.Plan;
import org.apache.doris.nereids.trees.plans.algebra.TopN;
import org.apache.doris.nereids.trees.plans.algebra.Union;
@@ -196,11 +198,7 @@ public class TopnFilterPushDownVisitor extends
PlanVisitor<Boolean, PushDownCont
@Override
public Boolean visitPhysicalHashJoin(PhysicalHashJoin<? extends Plan, ?
extends Plan> join,
PushDownContext ctx) {
- if (ctx.nullsFirst && (join.getJoinType().isOuterJoin() ||
join.getJoinType().isAsofOuterJoin())) {
- // topn-filter can be pushed down to the left child of
leftOuterJoin
- // and to the right child of rightOuterJoin,
- // but PushDownTopNThroughJoin rule already pushes topn to the
left and right side.
- // the topn-filter will be generated according to the inferred
topn node.
+ if (!canPushToJoinLeft(join.getJoinType(), ctx)) {
return false;
}
if
(join.left().getOutputSet().containsAll(ctx.probeExpr.getInputSlots())) {
@@ -215,6 +213,9 @@ public class TopnFilterPushDownVisitor extends
PlanVisitor<Boolean, PushDownCont
if (ctx.probeExpr.equals(conj.child(1))) {
// push to left child. right child is blocking operator,
do not need topn-filter
PushDownContext ctxOnLeftChild =
ctx.withNewProbeExpression(conj.child(0));
+ if (!canPushToJoinLeft(join.getJoinType(),
ctxOnLeftChild)) {
+ continue;
+ }
PushDownContext childPushDownContext =
adjustProbeExprNullableThroughOuterJoin(join.left(),
ctxOnLeftChild);
return join.left().accept(this, childPushDownContext);
@@ -229,11 +230,7 @@ public class TopnFilterPushDownVisitor extends
PlanVisitor<Boolean, PushDownCont
@Override
public Boolean visitPhysicalNestedLoopJoin(PhysicalNestedLoopJoin<?
extends Plan, ? extends Plan> join,
PushDownContext ctx) {
- if (ctx.nullsFirst && join.getJoinType().isOuterJoin()) {
- // topn-filter can be pushed down to the left child of
leftOuterJoin
- // and to the right child of rightOuterJoin,
- // but PushDownTopNThroughJoin rule already pushes topn to the
left and right side.
- // the topn-filter will be generated according to the inferred
topn node.
+ if (!canPushToJoinLeft(join.getJoinType(), ctx)) {
return false;
}
// push to left child. right child is blocking operator, do not need
topn-filter
@@ -243,6 +240,18 @@ public class TopnFilterPushDownVisitor extends
PlanVisitor<Boolean, PushDownCont
return false;
}
+ private boolean canPushToJoinLeft(JoinType joinType, PushDownContext ctx) {
+ if (ctx.nullsFirst && (joinType.isOuterJoin() ||
joinType.isAsofOuterJoin())) {
+ // PushDownTopNThroughJoin already creates a child TopN for safe
preserved-side pushdown.
+ // Its filter is generated separately; the parent filter must not
create leading NULL rows.
+ return false;
+ }
+ // Filtering a left match may make a RIGHT/FULL join emit a new
null-extended row.
+ // NULLS LAST cannot protect expressions such as COALESCE that turn
those NULLs into values.
+ return !joinType.isLeftSideNullable()
+ || !ctx.probeExpr.anyMatch(e ->
NullToNonNullFunction.canConvertNullToNonNull((Expression) e));
+ }
+
@Override
public Boolean visitPhysicalRelation(PhysicalRelation relation,
PushDownContext ctx) {
if (supportPhysicalRelations(relation) &&
relation.getOutputSet().containsAll(ctx.probeExpr.getInputSlots())) {
diff --git
a/fe/fe-core/src/test/java/org/apache/doris/nereids/postprocess/TopNRuntimeFilterTest.java
b/fe/fe-core/src/test/java/org/apache/doris/nereids/postprocess/TopNRuntimeFilterTest.java
index a137487b449..8931377cd59 100644
---
a/fe/fe-core/src/test/java/org/apache/doris/nereids/postprocess/TopNRuntimeFilterTest.java
+++
b/fe/fe-core/src/test/java/org/apache/doris/nereids/postprocess/TopNRuntimeFilterTest.java
@@ -21,6 +21,8 @@ import org.apache.doris.nereids.datasets.ssb.SSBTestBase;
import org.apache.doris.nereids.glue.translator.PlanTranslatorContext;
import org.apache.doris.nereids.processor.post.PlanPostProcessors;
import org.apache.doris.nereids.processor.post.TopnFilterContext;
+import org.apache.doris.nereids.processor.post.TopnFilterPushDownVisitor;
+import
org.apache.doris.nereids.processor.post.TopnFilterPushDownVisitor.PushDownContext;
import org.apache.doris.nereids.trees.expressions.Expression;
import org.apache.doris.nereids.trees.expressions.Slot;
import org.apache.doris.nereids.trees.expressions.SlotReference;
@@ -29,7 +31,9 @@ import
org.apache.doris.nereids.trees.expressions.functions.scalar.Substring;
import org.apache.doris.nereids.trees.expressions.literal.IntegerLiteral;
import org.apache.doris.nereids.trees.plans.Plan;
import org.apache.doris.nereids.trees.plans.SortPhase;
+import org.apache.doris.nereids.trees.plans.physical.PhysicalHashJoin;
import
org.apache.doris.nereids.trees.plans.physical.PhysicalLazyMaterializeOlapScan;
+import org.apache.doris.nereids.trees.plans.physical.PhysicalNestedLoopJoin;
import org.apache.doris.nereids.trees.plans.physical.PhysicalPlan;
import org.apache.doris.nereids.trees.plans.physical.PhysicalRelation;
import org.apache.doris.nereids.trees.plans.physical.PhysicalTopN;
@@ -279,6 +283,76 @@ public class TopNRuntimeFilterTest extends SSBTestBase
implements MemoPatternMat
}
}
+ @Test
+ public void testTopNFilterRejectsNullToNonNullThroughOuterJoin() {
+ checkTopNFilterThroughJoin(ImmutableList.of(
+ "coalesce(l.k, -1)",
+ "ifnull(l.k, -1)",
+ "if(l.k is null, -1, l.k)",
+ "case when l.k is null then -1 else l.k end",
+ "coalesce(l.k, -1) - 1",
+ "cast(l.k is null as int)",
+ "cast(l.k <=> 0 as int)",
+ "cast(to_json(array(l.k)) as string)"),
ImmutableList.of("RIGHT OUTER", "FULL OUTER"), false, false);
+ }
+
+ @Test
+ public void testTopNFilterPreservesNullPropagatingOuterJoinExpressions() {
+ checkTopNFilterThroughJoin(ImmutableList.of("l.k", "cast(l.k as
bigint)", "abs(l.k)"),
+ ImmutableList.of("RIGHT OUTER", "FULL OUTER"), false, true);
+ checkTopNFilterThroughJoin(ImmutableList.of("l.k"),
ImmutableList.of("RIGHT OUTER", "FULL OUTER"), true, false);
+ }
+
+ @Test
+ public void testTopNFilterPreservesNonNullGeneratingJoinSide() {
+ checkTopNFilterThroughJoin(ImmutableList.of("coalesce(l.k, -1)"),
+ ImmutableList.of("INNER", "LEFT OUTER"), false, true);
+ }
+
+ private void checkTopNFilterThroughJoin(List<String> expressions,
List<String> joinTypes,
+ boolean nullsFirst, boolean expectedFilter) {
+ double previousRatio =
connectContext.getSessionVariable().topnFilterRatio;
+ boolean previousJoinReorder =
connectContext.getSessionVariable().isDisableJoinReorder();
+ String previousDisabledRules = String.join(",",
+
connectContext.getSessionVariable().getDisableNereidsRuleNames());
+ connectContext.getSessionVariable().topnFilterRatio = 1000000;
+ connectContext.getSessionVariable().setDisableJoinReorder(true);
+
connectContext.getSessionVariable().setDisableNereidsRules(previousDisabledRules
+ ",PRUNE_EMPTY_PARTITION");
+ try {
+ for (String joinType : joinTypes) {
+ for (boolean hashJoin : ImmutableList.of(true, false)) {
+ String condition = hashJoin ? "l.id = r.id" : "l.id <
r.id";
+ for (String expression : expressions) {
+ String sql = "SELECT " + expression + " AS sort_key
FROM topn_safety l "
+ + joinType + " JOIN topn_safety r ON " +
condition
+ + " ORDER BY sort_key NULLS " + (nullsFirst ?
"FIRST" : "LAST") + " LIMIT 1";
+ PlanChecker checker =
PlanChecker.from(connectContext).analyze(sql).rewrite().implement();
+ PhysicalPlan plan = checker.getPhysicalPlan();
+ Assertions.assertTrue(plan.anyMatch(node -> hashJoin
+ ? node instanceof PhysicalHashJoin : node
instanceof PhysicalNestedLoopJoin),
+ sql + "\n" + plan.treeString());
+ // Exercise the visitor even when the planner chooses
a single-stage sort.
+ List<PhysicalTopN<? extends Plan>> topns =
plan.collectToList(
+ node -> node instanceof PhysicalTopN);
+ Assertions.assertFalse(topns.isEmpty(),
plan.treeString());
+ TopnFilterContext filters = new TopnFilterContext();
+ TopnFilterPushDownVisitor visitor = new
TopnFilterPushDownVisitor(filters);
+ for (PhysicalTopN<? extends Plan> topn : topns) {
+ topn.accept(visitor, new PushDownContext(topn,
topn.getOrderKeys().get(0).getExpr(),
+ topn.getOrderKeys().get(0).isNullFirst()));
+ }
+ Assertions.assertEquals(expectedFilter,
!filters.getTopnFilters().isEmpty(),
+ sql + "\n" + plan.treeString());
+ }
+ }
+ }
+ } finally {
+ connectContext.getSessionVariable().topnFilterRatio =
previousRatio;
+
connectContext.getSessionVariable().setDisableJoinReorder(previousJoinReorder);
+
connectContext.getSessionVariable().setDisableNereidsRules(previousDisabledRules);
+ }
+ }
+
@Test
public void testProbeExprNullableThroughRightOuterJoin() {
// topn node push down filter value to scan node.
diff --git
a/regression-test/data/nereids_syntax_p0/topn_filter_outer_join_null.out
b/regression-test/data/nereids_syntax_p0/topn_filter_outer_join_null.out
new file mode 100644
index 00000000000..1376f7a7ea1
--- /dev/null
+++ b/regression-test/data/nereids_syntax_p0/topn_filter_outer_join_null.out
@@ -0,0 +1,247 @@
+-- This file is automatically generated. You should know what you did if you
want to edit this
+-- !enabled --
+0 0
+
+-- !disabled --
+0 0
+
+-- !enabled --
+0 0
+
+-- !disabled --
+0 0
+
+-- !enabled --
+0 0
+
+-- !disabled --
+0 0
+
+-- !enabled --
+0 0
+
+-- !disabled --
+0 0
+
+-- !enabled --
+-1 0
+
+-- !disabled --
+-1 0
+
+-- !enabled --
+1 1
+
+-- !disabled --
+1 1
+
+-- !enabled --
+1 1
+
+-- !disabled --
+1 1
+
+-- !enabled --
+1 1
+
+-- !disabled --
+1 1
+
+-- !enabled --
+1 1
+
+-- !disabled --
+1 1
+
+-- !enabled --
+0 1
+
+-- !disabled --
+0 1
+
+-- !enabled --
+0 0
+
+-- !disabled --
+0 0
+
+-- !enabled --
+0 0
+
+-- !disabled --
+0 0
+
+-- !enabled --
+0 0
+
+-- !disabled --
+0 0
+
+-- !enabled --
+0 0
+
+-- !disabled --
+0 0
+
+-- !enabled --
+-1 0
+
+-- !disabled --
+-1 0
+
+-- !enabled --
+1 1
+
+-- !disabled --
+1 1
+
+-- !enabled --
+1 1
+
+-- !disabled --
+1 1
+
+-- !enabled --
+1 1
+
+-- !disabled --
+1 1
+
+-- !enabled --
+1 1
+
+-- !disabled --
+1 1
+
+-- !enabled --
+0 1
+
+-- !disabled --
+0 1
+
+-- !enabled --
+0 0
+
+-- !disabled --
+0 0
+
+-- !enabled --
+0 0
+
+-- !disabled --
+0 0
+
+-- !enabled --
+0 0
+
+-- !disabled --
+0 0
+
+-- !enabled --
+0 0
+
+-- !disabled --
+0 0
+
+-- !enabled --
+-1 0
+
+-- !disabled --
+-1 0
+
+-- !enabled --
+1 1
+
+-- !disabled --
+1 1
+
+-- !enabled --
+1 1
+
+-- !disabled --
+1 1
+
+-- !enabled --
+1 1
+
+-- !disabled --
+1 1
+
+-- !enabled --
+1 1
+
+-- !disabled --
+1 1
+
+-- !enabled --
+0 1
+
+-- !disabled --
+0 1
+
+-- !enabled --
+0 0
+
+-- !disabled --
+0 0
+
+-- !enabled --
+0 0
+
+-- !disabled --
+0 0
+
+-- !enabled --
+0 0
+
+-- !disabled --
+0 0
+
+-- !enabled --
+0 0
+
+-- !disabled --
+0 0
+
+-- !enabled --
+-1 0
+
+-- !disabled --
+-1 0
+
+-- !enabled --
+1 1
+
+-- !disabled --
+1 1
+
+-- !enabled --
+1 1
+
+-- !disabled --
+1 1
+
+-- !enabled --
+1 1
+
+-- !disabled --
+1 1
+
+-- !enabled --
+1 1
+
+-- !disabled --
+1 1
+
+-- !enabled --
+0 1
+
+-- !disabled --
+0 1
+
+-- !bare_key --
+0 0
+
+-- !bare_key --
+0 0
+
diff --git
a/regression-test/suites/nereids_syntax_p0/topn_filter_outer_join_null.groovy
b/regression-test/suites/nereids_syntax_p0/topn_filter_outer_join_null.groovy
new file mode 100644
index 00000000000..b93c34907cd
--- /dev/null
+++
b/regression-test/suites/nereids_syntax_p0/topn_filter_outer_join_null.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_filter_outer_join_null", "p0") {
+ sql "SET enable_sql_cache = false"
+ sql "SET enable_query_cache = false"
+ sql "SET disable_join_reorder = true"
+ sql "SET parallel_pipeline_task_num = 1"
+ sql "DROP TABLE IF EXISTS topn_filter_outer_join_null"
+ sql """CREATE TABLE topn_filter_outer_join_null (k INT)
+ DUPLICATE KEY(k)
+ DISTRIBUTED BY HASH(k) BUCKETS 1
+ PROPERTIES("replication_num" = "1")"""
+ sql "INSERT INTO topn_filter_outer_join_null VALUES (0), (1)"
+
+ for (String joinType : ["FULL OUTER", "RIGHT OUTER"]) {
+ // The inequalities match the same integer keys while requiring a
nested loop join.
+ for (String condition : ["l.k = r.k", "l.k < r.k + 1 AND l.k > r.k -
1"]) {
+ for (String direction : ["ASC", "DESC"]) {
+ int firstKey = direction == "ASC" ? 0 : 1
+ int fallback = direction == "ASC" ? -1 : 2
+ String probe = """(
+ SELECT k FROM topn_filter_outer_join_null WHERE k =
${firstKey}
+ UNION ALL
+ SELECT k FROM topn_filter_outer_join_null
+ WHERE k != ${firstKey} AND sleep(k + ${firstKey}) = 0
+ )"""
+ for (String expression : ["coalesce(l.k, ${fallback})",
+ "ifnull(l.k, ${fallback})",
+ "if(l.k is null, ${fallback}, l.k)",
+ "case when l.k is null then ${fallback} else l.k end",
+ "coalesce(l.k, ${fallback}) - 1"]) {
+ String query = """SELECT ${expression} AS sort_key,
coalesce(r.k, -1)
+ FROM ${probe} l ${joinType} JOIN
topn_filter_outer_join_null r ON ${condition}
+ ORDER BY sort_key ${direction} NULLS LAST, 2 NULLS
LAST LIMIT 1"""
+ sql "SET topn_filter_ratio = 1000000"
+ explain {
+ sql query
+ contains(condition == "l.k = r.k" ? "HASH JOIN" :
"NESTED LOOP JOIN")
+ notContains "TOPN OPT:"
+ }
+ qt_enabled query
+ sql "SET topn_filter_ratio = 0"
+ qt_disabled query
+ }
+ }
+ }
+ }
+
+ sql "SET topn_filter_ratio = 1000000"
+ for (String nullOrder : ["FIRST", "LAST"]) {
+ String query = """SELECT l.k, r.k FROM (
+ SELECT k FROM topn_filter_outer_join_null WHERE k = 0
+ UNION ALL
+ SELECT k FROM topn_filter_outer_join_null WHERE k > 0 AND sleep(k)
= 0
+ ) l FULL OUTER JOIN topn_filter_outer_join_null r ON l.k = r.k
+ ORDER BY l.k NULLS ${nullOrder}, r.k NULLS LAST LIMIT 1"""
+ explain {
+ sql query
+ if (nullOrder == "FIRST") {
+ notContains "TOPN OPT:"
+ } else {
+ contains "TOPN OPT:"
+ }
+ }
+ qt_bare_key query
+ }
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
