github-actions[bot] commented on code in PR #66681:
URL: https://github.com/apache/doris/pull/66681#discussion_r3782115072
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/postprocess/PushDownFilterThroughProjectTest.java:
##########
@@ -156,4 +161,98 @@ public void testNotPushFilterWithNonfoldable() {
Assertions.assertTrue(newPlan.child(0) instanceof PhysicalProject);
Assertions.assertTrue(newPlan.child(0).child(0) instanceof
PhysicalProject);
}
+
+ /**
+ * A conjunct that itself contains a NoneMovableFunction (assert_true) may
still be pushed
+ * through plain row-preserving projects: it is evaluated on exactly the
same rows, and the
+ * row-changing operators below already refuse NoneMovableFunction
conjuncts.
+ */
+ @Test
+ public void testPushFilterWithNoneMovableFunctionConjunct() {
+ LogicalProperties placeHolder = Mockito.mock(LogicalProperties.class);
+ CascadesContext ctx = Mockito.mock(CascadesContext.class);
+ OlapTable t1 = PlanConstructor.newOlapTable(0, "t1", 0,
KeysType.DUP_KEYS);
+ List<String> qualifier = new ArrayList<>();
+ qualifier.add("test");
+ List<Slot> t1Output = new ArrayList<>();
+ SlotReference a = new SlotReference("a", IntegerType.INSTANCE);
+ SlotReference b = new SlotReference("b", IntegerType.INSTANCE);
+ SlotReference c = new SlotReference("c", IntegerType.INSTANCE);
+ t1Output.add(a);
+ t1Output.add(b);
+ t1Output.add(c);
+ LogicalProperties t1Properties = new LogicalProperties(() -> t1Output,
() -> DataTrait.EMPTY_TRAIT);
+ PhysicalOlapScan scan = new
PhysicalOlapScan(RelationId.createGenerator().getNextId(), t1,
+ qualifier, 0L, Collections.emptyList(),
Collections.emptyList(), null,
+ PreAggStatus.on(), ImmutableList.of(), Optional.empty(),
t1Properties,
+ Optional.empty(), new ArrayList<>(), ImmutableList.of(),
ImmutableList.of(), Optional.empty(),
+ Optional.empty(), ImmutableList.of(), Optional.empty());
+ Alias x = new Alias(a, "x");
+ List<NamedExpression> projList3 = Lists.newArrayList(x, b, c);
+ PhysicalProject proj3 = new PhysicalProject(projList3, placeHolder,
scan);
+ Alias y = new Alias(x.toSlot(), "y");
+ Alias z = new Alias(b, "z");
+ List<NamedExpression> projList2 = Lists.newArrayList(y, z, c);
+ PhysicalProject proj2 = new PhysicalProject(projList2, placeHolder,
proj3);
+ Set<Expression> conjuncts = Sets.newHashSet();
+ conjuncts.add(new AssertTrue(new EqualTo(y.toSlot(), Literal.of(0)),
new StringLiteral("msg")));
+ PhysicalFilter filter = new PhysicalFilter(conjuncts,
proj2.getLogicalProperties(), proj2);
+
+ PushDownFilterThroughProject processor = new
PushDownFilterThroughProject();
Review Comment:
**[P2] Test the logical rule changed by this PR**
These additions instantiate
`org.apache.doris.nereids.processor.post.PushDownFilterThroughProject`, but the
production hunk is in the different
`rules.rewrite.PushDownFilterThroughProject`. The physical processor already
(1) pushes an assertion through plain projects and (2) rejects any project
containing a NoneMovable expression, so both tests pass without this PR. Please
add a logical-rule test in `rules/rewrite/PushDowFilterThroughProjectTest` that
fails when the changed hunk is reverted; if the existing project-wide matcher
intentionally makes the new alias-local NoneMovable check unreachable, remove
or refactor that dead admission instead of testing the unrelated layer.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownFilterThroughSetOperation.java:
##########
@@ -87,7 +87,11 @@ public Rule build() {
pushableConjuncts = new LinkedHashSet<>();
Set<Expression> kept = new LinkedHashSet<>();
for (Expression c : origFilter.getConjuncts()) {
- if (c.containsVolatileExpression()) {
+ // a NoneMovableFunction (e.g. assert_true) must stay
above the set
+ // operation just like a volatile expression: for
INTERSECT/EXCEPT/
+ // UNION DISTINCT the set-op semantics depend on the
full branch row
+ // sets, so evaluating assert_true in each branch
changes its domain.
+ if (c.containsNoneMovableOrVolatile()) {
Review Comment:
**[P1] Do not clone volatility across UNION ALL branches**
The `canPushVolatileExpr` fast path bypasses this new fence. For
```text
Filter(random(1) > c)
UnionAll
ScanA(one row)
ScanB(one row)
```
the original filter has one seeded function context and consumes `r1, r2`;
`addFiltersToNewChildren` creates separate branch filters, so both contexts
restart at `r1`. Choosing `c` between the first two values changes the returned
rows, and `assert_true(random(1) > c, 'bad')` likewise changes error behavior.
The 1:1 row-count argument does not preserve state across cloned contexts.
Please keep volatile/NoneMovable conjuncts above UNION ALL too, or materialize
one shared evaluation, with a two-branch seeded regression.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownFilterThroughProject.java:
##########
@@ -123,9 +123,16 @@ private static Pair<Set<Expression>, Set<Expression>>
splitConjunctsByChildOutpu
// If filter slot is alias and its expression contains
non-foldable expression, it can't push down, example:
// `filter(a > 1) -> project(b + random(1, 10) as a)`, if push
down filter, it got
// `project(b + random(1, 10) as a) -> filter(b + random(1, 10) >
1)`, it contains two distinct RANDOM.
+ // The same applies to NoneMovableFunction (e.g. assert_true): if
the referenced
+ // project alias computes a NoneMovableFunction, pushing the
filter below would
+ // duplicate its evaluation (once in the project, once in the
filter). a conjunct
+ // that itself contains a NoneMovableFunction may still be pushed
through a plain
+ // slot project: the project is row-preserving, so the expression
is evaluated on
+ // exactly the same rows, and the row-changing operators below
(joins,
+ // aggregations, ...) already refuse to accept NoneMovableFunction
conjuncts.
if (childOutputs.containsAll(conjunctSlots)
&&
conjunctSlots.stream().map(childAlias::get).filter(Objects::nonNull)
-
.noneMatch(Expression::containsVolatileExpression)) {
+
.noneMatch(Expression::containsNoneMovableOrVolatile)) {
Review Comment:
**[P1] Keep volatile project outputs on their original row domain**
A filter that references only `k` still passes this alias-local check:
```text
Filter(k = 1)
Project(k, random(1) AS r)
Scan(k = 0, 1)
```
The rewrite produces `Project(random(1) AS r) -> Filter(k=1) -> Scan`. BE
seeds one generator per function context and advances it once per project input
row, so the original surviving row carries the second seeded value while the
rewritten plan returns the first. This is distinct from the existing Limit
thread: it is the active filter/project rule, plus its physical postprocessor
parallel, changing a retained volatile output. Please make any project
containing `containsNoneMovableOrVolatile()` a boundary in both paths and add a
seeded-random regression.
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/ReorderJoinTest.java:
##########
@@ -350,4 +356,63 @@ private void testAsofJoinHelper(JoinType joinType) {
.whenNot(join ->
join.getJoinType().isCrossJoin()))
.printlnTree();
}
+
+ /**
+ * A filter containing a NoneMovableFunction (assert_true) must prevent
ReorderJoin from
+ * collecting the filter conjuncts into the join and redistributing them
below the join:
+ * the child would evaluate assert_true on a superset of rows.
+ */
+ @Test
+ public void testNotReorderJoinWithNoneMovableFunction() {
+ Expression assertTrueExpr = new AssertTrue(
Review Comment:
**[P2] Use a reorderable cluster to test the new filter guard**
With only A and B, the old `findInnerJoin` cannot attach `assert_true(A.x >
0)` to the join because its slots are wholly covered by A. It remains in
`joinFilter`, and `PlanUtils.filterOrSelf` recreates the same outer filter, so
this generic two-scan matcher passes without the new early return. Use a
three-relation cluster where the old reorder moves a cross-side assertion onto
an earlier edge, then assert scan identities/ExprIds and the exact assertion
edge so the test fails on the base implementation.
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/InferPredicatesTest.java:
##########
@@ -907,4 +907,29 @@ void inferPredicatesLeftAsofInner() {
);
}
+ @Test
+ public void testDoNotInferNoneMovableFunction() {
Review Comment:
**[P2] Exercise the changed inference helpers**
This assertion is a WHERE filter above the join.
`InferPredicates.visitLogicalFilter` never calls either changed
`inferNewPredicate*` helper, while the separately changed
`PushDownFilterThroughJoin` rule is sufficient to leave it in exactly the
matched shape. Reverting the `InferPredicates` hunk therefore still passes this
test. Please apply `InferPredicates` directly to a join input/ON or, more
directly, an EXCEPT/INTERSECT branch-substitution case and assert that no
sibling filter is synthesized; cover both helper sites with a base-failing test.
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/JoinExtractOrFromCaseWhenTest.java:
##########
@@ -0,0 +1,93 @@
+// 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.rules.RuleFactory;
+import org.apache.doris.nereids.trees.expressions.Add;
+import org.apache.doris.nereids.trees.expressions.And;
+import org.apache.doris.nereids.trees.expressions.EqualTo;
+import org.apache.doris.nereids.trees.expressions.Expression;
+import org.apache.doris.nereids.trees.expressions.GreaterThan;
+import org.apache.doris.nereids.trees.expressions.Slot;
+import org.apache.doris.nereids.trees.expressions.functions.scalar.AssertTrue;
+import org.apache.doris.nereids.trees.expressions.functions.scalar.If;
+import org.apache.doris.nereids.trees.expressions.literal.IntegerLiteral;
+import org.apache.doris.nereids.trees.expressions.literal.StringLiteral;
+import org.apache.doris.nereids.trees.plans.JoinType;
+import org.apache.doris.nereids.trees.plans.logical.LogicalOlapScan;
+import org.apache.doris.nereids.trees.plans.logical.LogicalPlan;
+import org.apache.doris.nereids.util.LogicalPlanBuilder;
+import org.apache.doris.nereids.util.MemoPatternMatchSupported;
+import org.apache.doris.nereids.util.MemoTestUtils;
+import org.apache.doris.nereids.util.PlanChecker;
+import org.apache.doris.nereids.util.PlanConstructor;
+
+import com.google.common.collect.ImmutableList;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests for {@link JoinExtractOrFromCaseWhen}.
+ */
+class JoinExtractOrFromCaseWhenTest implements MemoPatternMatchSupported {
+
+ private final LogicalOlapScan scan1 =
PlanConstructor.newLogicalOlapScan(0, "t1", 0);
+ private final LogicalOlapScan scan2 =
PlanConstructor.newLogicalOlapScan(1, "t2", 0);
+
+ private RuleFactory joinExtractOrFromCaseWhenRule() {
+ return new JoinExtractOrFromCaseWhen();
+ }
+
+ /**
+ * A join condition mixing both sides with a case-when-like expression is
normally rewritten
+ * into an OR-expansion condition; but when the condition also contains a
NoneMovableFunction
+ * (assert_true), the rewrite must be skipped so the join is left
untouched.
+ */
+ @Test
+ void testNoneMovableFunctionSkipsRewrite() {
+ Slot leftA = scan1.getOutput().get(0);
+ Slot leftB = scan1.getOutput().get(1);
+ Slot rightA = scan2.getOutput().get(0);
+ Slot rightB = scan2.getOutput().get(1);
+ // (case when leftA > 0 then rightA else rightB end) = leftA + leftB
+ Expression caseWhen = new If(new GreaterThan(leftA, new
IntegerLiteral(0)), rightA, rightB);
+ Expression extractable = new EqualTo(caseWhen, new Add(leftA, leftB));
+
+ // control: without assert_true the rewrite fires and adds an
OR-expansion condition.
+ // (the condition must be an other join conjunct:
OrExpansion.needRewriteJoin only
+ // accepts a join with empty hash conjuncts, i.e. a nested loop join)
+ LogicalPlan control = new LogicalPlanBuilder(scan1).join(scan2,
JoinType.INNER_JOIN,
+ ImmutableList.of(), ImmutableList.of(extractable)).build();
+ PlanChecker.from(MemoTestUtils.createConnectContext(), control)
+ .applyTopDown(joinExtractOrFromCaseWhenRule())
+ .matches(
+ logicalJoin().when(join ->
!join.getOtherJoinConjuncts().isEmpty())
+ );
+
+ // guard: with assert_true the join condition must be left untouched.
+ Expression guarded = new And(extractable,
Review Comment:
**[P2] Make the guard case reach CASE/OR extraction**
Wrapping the extractable equality and `assert_true` in a top-level `And`
makes both direct children reference both join sides. The base
`extractExpression` therefore satisfies neither one-side branch, and `And` is
not an `EqualPredicate`, so OR expansion also does nothing; the guarded matcher
passes before this change. The control is non-discriminating too because it
only checks that the already non-empty other-conjunct list stays non-empty. Put
the assertion inside a CASE/IF equality that the base rule demonstrably
expands, and assert the exact derived OR conjunct for the control versus
exactly one unchanged conjunct for the guarded case.
--
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]
