github-actions[bot] commented on code in PR #67919:
URL: https://github.com/apache/doris/pull/67919#discussion_r4045976796
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownJoinOtherCondition.java:
##########
@@ -78,7 +78,7 @@ public Rule build() {
// child changes their evaluation granularity from per
joined row to per
// input row. Repeated volatile occurrences are
materialized later by
// AddProjectForVolatileExpression.
- if (otherConjunct.containsVolatileExpression()) {
+ if
(otherConjunct.containsVolatileOrNoneMovableExpression()) {
Review Comment:
[P1] Fence the later nested-loop ON projector
This guard keeps the whole non-movable ON conjunct at join scope, but the
later `ProjectOtherJoinConditionForNestedLoopJoin.AliasReplacer` still checks
only `containsVolatileExpression()`. For
`LeftOuterJoin(bitmap_count(to_bitmap_with_check(l.a)) > r.b) -> [L(a=-1),
Empty R]`, the original nested-loop join has no pair on which to evaluate ON
and emits the unmatched left row. The aliaser instead creates `Project(...,
bitmap_count(to_bitmap_with_check(l.a)) AS x) -> L`, which evaluates `-1` while
producing that row and raises `InvalidArgument`. Please use the combined fence
in `AliasReplacer` and add an empty-opposite-side outer-join regression. This
is distinct from the existing set-operation thread because it starts in ON and
runs after the fixed pushdown family.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownFilterThroughJoin.java:
##########
@@ -162,7 +162,7 @@ private boolean convertJoinCondition(Expression predicate,
Set<Slot> leftOutputs
if (!(predicate instanceof EqualTo)) {
return false;
}
- if (predicate.containsVolatileExpression()) {
+ if (predicate.containsVolatileOrNoneMovableExpression()) {
Review Comment:
[P1] Apply the same fence to original ON hash classification
This protects an equality converted from a filter, but an equivalent
equality written directly in `ON` reaches `JoinUtils.JoinSlotCoverageChecker`,
which still rejects only volatile expressions. For
`LeftOuterJoin(bitmap_count(to_bitmap_with_check(l.a)) = r.b) -> [L(a=-1),
Empty R]`, keeping it as an other conjunct gives the nested-loop executor no
pair on which to evaluate ON; classifying it as a hash conjunct moves it to
per-input key evaluation and the preserved left row now errors. Please use the
combined fence in `isHashJoinCondition` and add a direct-ON regression. That
same gate also prevents `OrExpansion` from hash-expanding a non-movable
disjunct; this entry path is distinct from the filter-derived equality fixed
here.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/ExpressionTrait.java:
##########
@@ -114,4 +114,9 @@ default boolean isVolatile() {
default boolean containsVolatileExpression() {
return containsType(VolatileExpression.class) && anyMatch(expr ->
((ExpressionTrait) expr).isVolatile());
}
+
+ default boolean containsVolatileOrNoneMovableExpression() {
Review Comment:
[P1] Keep skipped-consumer filters out of a shared CTE producer
`CollectFilterAboveConsumer` also retains the old volatile-only gate. With a
materialized two-consumer CTE over `x={-1,1}`, put
`bitmap_count(to_bitmap_with_check(x)) > 0` above consumer 1 on the probe side
of an INNER hash join whose build is runtime-empty, and `x > 0` above active
consumer 2. The empty build short-circuits consumer 1 before it is pulled, but
collection synthesizes `risky(x) OR x>0` below the shared producer. Consumer 2
keeps that producer running, and on `x=-1` the safe term is false, so the
previously skipped risky filter raises. Please apply the combined fence in
`CollectFilterAboveConsumer` and add a two-consumer materialization regression.
This combines separate CTE-consumer filters rather than cloning a filter
through a set operation.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/ExpressionTrait.java:
##########
@@ -114,4 +114,9 @@ default boolean isVolatile() {
default boolean containsVolatileExpression() {
return containsType(VolatileExpression.class) && anyMatch(expr ->
((ExpressionTrait) expr).isVolatile());
}
+
+ default boolean containsVolatileOrNoneMovableExpression() {
Review Comment:
[P1] Fence CASE branch extraction with this helper
`JoinExtractOrFromCaseWhen.isConditionNeedRewrite` still uses only the
volatile check. With `short_circuit_evaluation=true`, `CASE WHEN l.flag=1 THEN
bitmap_count(to_bitmap_with_check(r.bad)) ELSE r.safe END > 0` is safe for
`flag=0,bad=-1,safe=1`, because only the ELSE result is evaluated. The rewrite
additionally ANDs in `(bitmap_count(to_bitmap_with_check(r.bad)) > 0) OR
(r.safe > 0)`, whose first disjunct evaluates the invalid, previously
unselected branch. Please use the combined fence in that rule and cover this
short-circuit CASE configuration. This creates a new ON conjunct rather than
pushing the existing set-operation filter from thread `4044589990`.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/ReorderJoin.java:
##########
@@ -153,7 +153,7 @@ public Plan joinToMultiJoin(Plan plan, Map<Plan,
DistributeHint> planToHintType)
// (t1 join t2) join t3 where t1.a = t3.x + random()
// if reorder, then may have ((t1 join t3) on t1.a = t3.x +
random()) join t2,
// then the reorder result will less rows than origin.
- if (conjunct.containsVolatileExpression()) {
+ if (conjunct.containsVolatileOrNoneMovableExpression()) {
Review Comment:
[P1] Treat non-movable ON conjuncts as reorder boundaries
This check sees only a wrapping filter; `joinToMultiJoin` still
unconditionally flattens the inner joins' hash/other conjuncts. In
`Filter(A.id+C.id>0) -> Join(A.id=C.id,
Join(bitmap_count(to_bitmap_with_check(A.a)) > B.x, A(a=-1,id=1), B),
C(id=2))`, the original lower A-B join evaluates the invalid value. After
flattening, `findInnerJoin` prefers the available A-C hash edge, makes that
subtree empty, and attaches the risky A-B condition only above it, suppressing
the error. Please make a non-movable hash/other conjunct a boundary in both
this RBO path and the hypergraph join-order path, with a three-input
regression. This changes join association rather than cloning the set-operation
filter from the existing thread.
--
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]
