github-actions[bot] commented on code in PR #66681:
URL: https://github.com/apache/doris/pull/66681#discussion_r3840389128
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/TransposeSemiJoinAgg.java:
##########
@@ -51,6 +52,15 @@ public Rule build() {
*/
public static boolean canTranspose(LogicalAggregate<? extends Plan>
aggregate,
LogicalJoin<? extends Plan, ? extends Plan> join) {
+ // the transpose evaluates the aggregate (and its arguments) on rows
the semi join would
+ // otherwise prune, and changes a volatile join conjunct from
per-input-row to
+ // per-aggregate-group evaluation. a NoneMovableFunction (e.g.
assert_true) or volatile
+ // expression in either would change error behavior or results, so the
transpose must be
+ // rejected.
+ if
(aggregate.getExpressions().stream().anyMatch(Expression::containsNoneMovableOrVolatile)
+ ||
join.getExpressions().stream().anyMatch(Expression::containsNoneMovableOrVolatile))
{
Review Comment:
**[P1] Fence the registered PK/FK aggregate pushdown too**
`PushDownAggThroughJoinOnPkFk` moves a foreign-only Aggregate below the
inner join without checking `agg.getExpressions()`. For `Aggregate(group F.fk,
count(assert_true(F.v>0,'bad'))) -> InnerJoin(P.pk=F.fk)` with nullable `F.fk`,
`P={1}`, and `F={(NULL,0),(1,1)}`, the original join removes the NULL-FK row
before aggregation; the rewrite aggregates F first and raises `bad` on that
group even though the rebuilt join later removes it. Please apply the same
aggregate-expression fence to this registered factory (or null-reject below the
pushed Aggregate) and add direct-Join and slot-Project registered-stage tests.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/join/InnerJoinLAsscomProject.java:
##########
@@ -63,7 +64,14 @@ public Rule build() {
.when(topJoin -> checkReorder(topJoin,
topJoin.left().child(),
enableLeftZigZag))
.whenNot(join -> join.hasDistributeHint() ||
join.left().child().hasDistributeHint())
- .when(join -> join.left().isAllSlots()))
+ .when(join -> join.left().isAllSlots())
+ // the reorder redistributes every conjunct of both
joins onto a
+ // different edge: a NoneMovableFunction (assert_true)
or volatile
+ // conjunct referencing B moves from (A join B)'s A x
B evaluation to
+ // (A join C) join B, where A rows pruned by C no
longer reach it and
+ // its required error is suppressed. reject the
reorder then.
+ .whenNot(topJoin ->
JoinUtils.hasSensitiveConjunct(topJoin)
+ ||
JoinUtils.hasSensitiveConjunct(topJoin.left().child())))
Review Comment:
**[P1] Fence the registered outer-join counterparts too**
This new owner-level guard is still absent from `OuterJoinLAsscomProject`
and `OuterJoinAssocProject`, which are registered separately. For example, the
former can rewrite `InnerJoin(LeftOuterJoin(A,B, other=assert_true(A.v+B.v>0)),
C)` into `LeftOuterJoin(InnerJoin(A,C),B)`: when A has a matching B but no
matching C, the original evaluates the assertion before C prunes A, while the
rewrite prunes A first and suppresses the error. The Assoc rule has the inverse
exposure with `(INNER, LEFT_OUTER)`. Please apply the same two-owner fence to
both outer-join Project factories and add base-failing tests for sensitive top
and bottom owners.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownFilterThroughJoin.java:
##########
@@ -120,7 +120,13 @@ public Rule build() {
Set<Expression> rightPredicates = Sets.newLinkedHashSet();
Set<Expression> remainingPredicates = Sets.newLinkedHashSet();
for (Expression p : filterPredicates) {
- if (p.containsVolatileExpression()) {
+ /*
+ * a NoneMovableFunction (e.g. assert_true) or a volatile
expression must not
+ * be pushed through the join: pushing it to a child changes
which rows it is
+ * evaluated on (the child sees a superset of the join
output), which changes
+ * its error behavior or results. keep such predicates above
the join.
+ */
+ if (p.containsNoneMovableOrVolatile()) {
Review Comment:
**[P1] Preserve sensitive subtrees during join elimination**
This filter guard is bypassed when `EliminateJoinByUnique` removes the whole
right side. A reachable shape is `Project(A.k) -> LeftOuterJoin(A,
Filter(assert_true(B.v>0,'bad')) -> unique-key B)`:
`LogicalFilter.computeUnique` propagates B's uniqueness, so the registered
elimination replaces the join with A. With matching keys and a failing B row,
the original plan raises the assertion while the rewritten plan returns A
without evaluating B. The registered `EliminateJoinByFK` sibling has the same
deletion failure: equivalent sensitive filters on PK and FK sides satisfy its
predicate-compatibility test, but an extra failing PK row is evaluated only
before the primary subtree is removed. Please reject both eliminations when the
discarded side recursively contains a NoneMovable or volatile expression and
add full-stage unique-right and PK/FK regressions.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/TransposeSemiJoinAgg.java:
##########
@@ -51,6 +52,15 @@ public Rule build() {
*/
public static boolean canTranspose(LogicalAggregate<? extends Plan>
aggregate,
LogicalJoin<? extends Plan, ? extends Plan> join) {
+ // the transpose evaluates the aggregate (and its arguments) on rows
the semi join would
+ // otherwise prune, and changes a volatile join conjunct from
per-input-row to
+ // per-aggregate-group evaluation. a NoneMovableFunction (e.g.
assert_true) or volatile
+ // expression in either would change error behavior or results, so the
transpose must be
+ // rejected.
+ if
(aggregate.getExpressions().stream().anyMatch(Expression::containsNoneMovableOrVolatile)
Review Comment:
**[P1] Fence the inverse logical-join transposes too**
The same owner-level check is still missing from the registered
`TransposeSemiJoinLogicalJoin` and `TransposeSemiJoinLogicalJoinProject` paths.
For example:
```text
LeftSemiJoin(hash A.k=C.k, other assert_true(A.v>0))
Project(all slots)
InnerJoin(hash A.k=B.k)
A
B
C
```
With `A=(1,0)`, `B=(2)`, and `C=(1)`, the original bottom inner join removes
A before the assertion. The inverse rewrite builds
`InnerJoin(LeftSemiJoin(A,C),B)`, so the A-C match evaluates the assertion
first and errors. A sensitive bottom-join conjunct has the converse suppression
failure. Please apply this aggregate/join expression-list fence to both inverse
factories and cover left/right, plain/Project, and both expression owners.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/JoinUtils.java:
##########
@@ -478,4 +482,66 @@ public static boolean
checkReorderPrecondition(LogicalJoin<?, ?> top, LogicalJoi
return AdjustNullable.doVisitLogicalJoin(
join, equalConjunctsSlotMap, false, false);
}
+
+ /**
+ * whether any hash or other conjunct of the join contains a
NoneMovableFunction (e.g.
+ * assert_true) or a volatile expression. such conjuncts must not be moved
onto a different
+ * join edge by join reorder rules: they would be evaluated on a different
(superset or
+ * pruned) row set, which changes their error behavior or results.
+ */
+ public static boolean hasSensitiveConjunct(LogicalJoin<?, ?> join) {
Review Comment:
**[P1] Apply this fence to the scalar AssertNumRows transpose**
`PushDownJoinOnAssertNumRows` is registered separately and moves the old
bottom join above a newly lower scalar join without checking either owner. A
deterministic top scalar predicate can reject an A row while the old bottom
join owns `assert_true(A.v>0)`: originally that bottom ON expression is
evaluated before the top scalar join rejects the row, but after the transpose
the new `Join(A, AssertNumRows)` rejects it first and the reconstructed bottom
join never evaluates the assertion. This suppresses a required error. Please
reject the transpose when either old join has a sensitive conjunct and add
left/right registered-stage tests for both owners.
--
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]
