github-actions[bot] commented on code in PR #68543:
URL: https://github.com/apache/doris/pull/68543#discussion_r4119614479
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/OrExpansion.java:
##########
@@ -159,6 +160,14 @@ public Plan visitLogicalJoin(LogicalJoin<? extends Plan, ?
extends Plan> join, O
if (join.getJoinType().isInnerJoin()) {
joins.addAll(expandInnerJoin(ctx.cascadesContext,
hashOtherConditions,
join, leftProducer, rightProducer, leftCloneToLeft,
rightCloneToRight));
Review Comment:
[P1] Preserve one evaluation of volatile ON predicates per joined pair. For
one `a`/`b` pair satisfying only `a.k1=b.k1`, consider `a RIGHT JOIN b ON
(a.k1=b.k1 OR a.k2=b.k2) AND random()<0.5`. The original outer join returns
exactly one row, matched or NULL-padded. This branch sends the residual
predicate to both `expandInnerJoin` and the swapped anti join. If the inner
draw is true and the anti draw false, `UNION ALL` returns both rows; the
reverse draws can return none. The earlier volatile-expression rewrite leaves
this single occurrence at join-pair scope. Please keep such joins unexpanded or
share one predicate result per pair, with a regression for this case.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/OrExpansion.java:
##########
@@ -75,6 +75,7 @@ public class OrExpansion extends
DefaultPlanRewriter<OrExpandsionContext> implem
.add(JoinType.INNER_JOIN)
.add(JoinType.LEFT_ANTI_JOIN)
.add(JoinType.LEFT_OUTER_JOIN)
+ .add(JoinType.RIGHT_OUTER_JOIN)
Review Comment:
[P1] Keep unselected CASE branches out of derived hash keys. With
`disable_join_reorder=true`, consider `a RIGHT JOIN b ON (CASE WHEN b.flag=1
THEN assert_true(a.ok,'bad') ELSE a.ok END)=b.ok`, where `a.ok=false`,
`b.flag=0`, and `b.ok=false`. The original CASE selects ELSE and matches
without calling `assert_true`. Adding RIGHT_OUTER_JOIN to `needRewriteJoin`
also lets `JoinExtractOrFromCaseWhen` derive `(assert_true(a.ok,'bad')=b.ok OR
a.ok=b.ok)`. `OrExpansion` treats the first equality as a hash key and
`PushDownExpressionsInHashCondition` evaluates the assertion in a child
Project, so the query throws. CASE extraction excludes volatile expressions but
not `NoneMovableFunction`. Please preserve conditional evaluation or exclude
this shape, and add a right-join regression.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/OrExpansion.java:
##########
@@ -159,6 +160,14 @@ public Plan visitLogicalJoin(LogicalJoin<? extends Plan, ?
extends Plan> join, O
if (join.getJoinType().isInnerJoin()) {
joins.addAll(expandInnerJoin(ctx.cascadesContext,
hashOtherConditions,
join, leftProducer, rightProducer, leftCloneToLeft,
rightCloneToRight));
+ } else if (join.getJoinType().isRightOuterJoin()) {
+ // right outer join = inner join union right anti join
+ // right anti join is built as left anti join with swapped
producers,
+ // so that unmatched rows of the right child are kept and left
side is padded with null
Review Comment:
[P1] Keep limited inputs shared across the expanded branches. If
`inline_cte_referenced_threshold` is raised above the generated consumer count,
`CTEInliner` copies the right-input producer separately for each inner branch.
For `a=(k1=1,k2=2)` and `b={(1,0),(0,2)}`, `a RIGHT JOIN (SELECT k1,k2 FROM b
LIMIT 1) r ON a.k1=r.k1 OR a.k2=r.k2` must return exactly one row, whichever
`b` row its one LIMIT selects. After inlining, the first equality branch can
select `(1,0)` and the second `(0,2)`, so `UNION ALL` returns two. The inliner
blocks nondeterministic functions, but an unordered `LogicalLimit` exposes no
expression to that check. Please materialize this input once or skip expansion
for this shape, and test with a raised inline threshold.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/OrExpansion.java:
##########
@@ -159,6 +160,14 @@ public Plan visitLogicalJoin(LogicalJoin<? extends Plan, ?
extends Plan> join, O
if (join.getJoinType().isInnerJoin()) {
joins.addAll(expandInnerJoin(ctx.cascadesContext,
hashOtherConditions,
join, leftProducer, rightProducer, leftCloneToLeft,
rightCloneToRight));
+ } else if (join.getJoinType().isRightOuterJoin()) {
+ // right outer join = inner join union right anti join
+ // right anti join is built as left anti join with swapped
producers,
+ // so that unmatched rows of the right child are kept and left
side is padded with null
+ joins.addAll(expandInnerJoin(ctx.cascadesContext,
hashOtherConditions,
+ join, leftProducer, rightProducer, leftCloneToLeft,
rightCloneToRight));
+ joins.add(expandLeftAntiJoin(ctx.cascadesContext,
hashOtherConditions,
Review Comment:
[P2] Do not apply the original right-side broadcast hint to the swapped anti
join. A hinted `a RIGHT OUTER JOIN [broadcast] b ON (a.k1=b.k1 OR a.k2=b.k2)`
reaches this branch because `SemiJoinCommute` leaves hinted joins in place. The
call swaps `(b,a)`, but `expandLeftAntiJoin` copies the original
`BROADCAST_RIGHT` hint onto `b LEFT ANTI JOIN a`. `RequestPropertyDeriver`
honors it there without a size check, replicating a potentially large `a` on
every BE; the original RIGHT OUTER JOIN could not honor right-side broadcast.
Please drop or remap the positional hint on this swapped branch and cover the
plan with a hinted EXPLAIN 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]
