github-actions[bot] commented on code in PR #65467:
URL: https://github.com/apache/doris/pull/65467#discussion_r3568048250
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/CollectJoinConstraint.java:
##########
@@ -84,8 +82,7 @@ public List<Rule> buildRules() {
if (join.getJoinType().isLeftJoin()) {
filterBitMap = LongBitmap.or(filterBitMap, rightHand);
}
- leading.getFilters().add(Pair.of(filterBitMap,
expression));
- leading.putConditionJoinType(expression,
join.getJoinType());
+ leading.addFilter(filterBitMap, expression,
join.getJoinType());
Review Comment:
`filterBitMap` is still just the predicate's input tables here, so one-sided
`otherJoinConjuncts` can be consumed as scan filters before the outer join
level where they are semantically valid.
One case is a lower preserved-side ON predicate: `a FULL OUTER JOIN b ON a.v
> 0` records the predicate as `{a}`, so `makeFilterPlanIfExist()` turns it into
`Filter(a.v > 0, a)` before the full outer join; rows from `a` with `a.v <= 0`
should be preserved as unmatched full-outer rows, but the generated plan drops
them. The same applies to right-preserving joins such as `a RIGHT OUTER JOIN b
ON b.v > 0` or `a RIGHT ANTI JOIN b ON b.v > 0`.
A second case is an upper predicate over the nullable side of a lower outer
join: for `(a LEFT OUTER JOIN b ON a.k = b.k) INNER JOIN c ON b.v > 0`,
`leading(b a c)` visits scan `b` first and consumes `{b}` as a scan filter. The
regenerated `b RIGHT OUTER JOIN a` then preserves `a` rows that the original
upper inner predicate would eliminate when `b` is NULL or fails `b.v > 0`.
Please encode these outer-join dependencies in the collected
bitmap/constraint state, or otherwise block `makeFilterPlanIfExist()` from
consuming predicates that are only valid after the outer join has been applied.
`PushDownJoinOtherCondition` already models these side restrictions, and this
path needs the same semantic guard plus end-to-end leading-hint coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/CollectJoinConstraint.java:
##########
@@ -72,8 +71,7 @@ public List<Rule> buildRules() {
if (join.getJoinType().isLeftJoin()) {
filterBitMap = LongBitmap.or(filterBitMap, rightHand);
}
- leading.getFilters().add(Pair.of(filterBitMap,
expression));
- leading.putConditionJoinType(expression,
join.getJoinType());
+ leading.addFilter(filterBitMap, expression,
join.getJoinType());
Review Comment:
This still lets an upper inner predicate be consumed before a lower right
outer join has introduced its preserved side. In a reduced plan like:
```text
InnerJoin ON a.x = c.x
RightOuterJoin ON a.k = b.k
a
b
c
```
`CollectJoinConstraint` records the upper hash predicate as `{a,c}`. With
`leading(a c b)`, the `{a,c}` join is built first; the right-outer constraint
is skipped there because `{b}` is not in the join bitmap yet, so
`computeJoinType()` uses the `{a,c}` condition for an inner join. The final
join then matches the original right outer constraint and preserves `b` rows.
That is not equivalent: if `a` is empty, the original upper inner join
eliminates the unmatched `b` rows, while the generated `(a JOIN c) RIGHT OUTER
JOIN b` returns them. Please make the right-outer constraint block this
premature `{a,c}` consumption, for example by propagating upper inner
predicates that reference the nullable left side into the constraint or adding
the symmetric partial-overlap guard needed for right outer joins, and cover it
with an end-to-end leading-hint test.
--
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]
