github-actions[bot] commented on code in PR #66681:
URL: https://github.com/apache/doris/pull/66681#discussion_r3800322687
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PullUpProjectUnderTopN.java:
##########
@@ -48,7 +48,10 @@ public Rule build() {
return logicalTopN(
logicalProject(logicalJoin().when(j ->
j.getJoinType().isLeftRightOuterOrCrossJoin()
|| j.getJoinType().isAsofOuterJoin()))
- .whenNot(p -> p.isAllSlots()))
+ // a project computing a NoneMovableFunction (e.g.
assert_true) or a volatile
+ // expression must not be pulled above the top-N: rows
pruned by the top-N
+ // would stop evaluating the expression, changing its
error behavior or results.
+ .whenNot(p -> p.isAllSlots() ||
p.containsNoneMovableOrVolatile()))
Review Comment:
**[P1] Fence row pruning before it is synthesized below this project**
This guard runs after the project-bearing
`PushDownTopNThroughJoin`/`PushDownTopNThroughWindow` rules. For `TopN(1,k
DESC) -> Project(k, assert_true(k>0) AS ok) -> CrossJoin(left k={1,0}, right
one)`, the earlier Join rule keeps the outer `TopN -> Project` but inserts
`TopN(1,k DESC)` in the left child. The original Project evaluates
`assert_true` on `k=0` and errors; the rewritten plan drops that row before the
Project and returns. Because the Project never moves, the pull-up fences from
the prior thread cannot prevent this. Please gate every project-bearing
row-pruning pushdown with `containsNoneMovableOrVolatile()` (including the
analogous Limit and scan-level TopN paths) and add a full-stage regression
proving no child pruning is synthesized.
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/PushDownFilterThroughJoinTest.java:
##########
@@ -226,4 +228,24 @@ private void bothSideToRight(JoinType joinType) {
)
);
}
+
+ @Test
+ void testNotPushNoneMovableFunctionThroughJoin() {
+ // assert_true(student.id > 18) references only the left side but must
NOT be pushed
+ // to the left child: the child evaluates it on a superset of rows
(before the join),
+ // changing its error behavior.
+ Expression assertTrueExpr = new AssertTrue(
Review Comment:
**[P2] Exercise the join-condition conversion guard**
This predicate is a top-level `GreaterThan`, so `convertJoinCondition`
returns at its existing non-`EqualTo` check and this test reaches only the
later child-push guard. The patch also adds a separate guard inside
`convertJoinCondition`; please add a cross-side `EqualTo` containing
`AssertTrue` and assert the exact `Filter -> Join` tree, so the base
implementation moves it into `otherJoinConjuncts` while this head retains it
above the join.
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/PushProjectIntoUnionTest.java:
##########
@@ -118,6 +122,42 @@ public void
testConstantExprIdsDistinctFromUnionOutputAndAcrossRows() {
}
}
+ /**
+ * A constant UNION row holding a NoneMovableFunction (assert_true(false))
must never be
+ * dropped by pushing a parent project that does not reference it: the
push-down would turn a
+ * required assertion/error into plain returned rows. the rule must not
fire.
+ */
+ @Test
+ public void testDoNotPushProjectIntoUnionWithNoneMovableConst() {
+ SlotReference s = new SlotReference(new ExprId(10), "s",
+ IntegerType.INSTANCE, true, ImmutableList.of());
+ SlotReference x = new SlotReference(new ExprId(11), "x",
+ BooleanType.INSTANCE, true, ImmutableList.of());
+ // constant row: s = 1, x = assert_true(false) — a required assertion
that throws.
+ NamedExpression rowS = new Alias(new ExprId(1), new IntegerLiteral(1),
"1");
+ NamedExpression rowX = new Alias(new ExprId(2), new AssertTrue(
+ BooleanLiteral.of(false), new StringLiteral("msg")), "x");
+ LogicalUnion union = new LogicalUnion(Qualifier.ALL,
+ ImmutableList.of(s, x),
+ ImmutableList.of(),
+ ImmutableList.of(ImmutableList.of(rowS, rowX)),
+ false,
+ ImmutableList.of());
+ // parent project selects only s, dropping x: pushing it into the
union would drop the assertion.
Review Comment:
**[P2] Cover the newly guarded duplication branch**
This project never references `x`, so it proves only the final
unreferenced-NoneMovable check. It cannot reach the other changed branch where
`guardedSlots` rejects a second syntactic reference. Please add a direct case
that projects the assertion-backed UNION slot twice and verifies the `Project
-> Union` boundary remains; that case fails on the base volatile-only
implementation and protects the new no-duplication contract.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/algebra/Project.java:
##########
@@ -154,4 +154,14 @@ default boolean containsNoneMovableFunction() {
}
return false;
}
+
+ /** containsNoneMovableOrVolatile */
+ default boolean containsNoneMovableOrVolatile() {
Review Comment:
**[P1] Apply this boundary to join-exploration project moves**
Registered `PushDownProjectThroughInnerOuterJoin` and
`PushDownProjectThroughSemiJoin` still move arbitrary side-local Projects below
joins. For `TopJoin(Project(A.k, assert_true(A.k > 0) AS ok, A LEFT SEMI JOIN
B), C)`, let A contain a failing row with no B match: originally the semi join
removes it before the Project, but the exploration alternative copies the
Project into A, so the assertion now runs and turns returned rows into an
error. The inner-join factory has the same unmatched-row failure, and both are
active before/after DPHyper. Please reject these moves when the Project
`containsNoneMovableOrVolatile()`, audit `TransposeAggSemiJoinProject`, and add
exploration/runtime regressions.
--
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]
