[fix](fe) Relax nullable slot check in outer join to anti join#65110
Open
morrySnow wants to merge 1 commit into
Open
[fix](fe) Relax nullable slot check in outer join to anti join#65110morrySnow wants to merge 1 commit into
morrySnow wants to merge 1 commit into
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: ConvertOuterJoinToAntiJoin only allowed conversion when the null-supplying side slot tested by IS NULL was non-nullable. This missed valid cases where the slot is nullable but appears in a null-rejecting binary comparison between left and right join slots, because a NULL value cannot satisfy those join predicates. The fix records slots from non-null-safe comparison predicates between join children, matches child slots to the corresponding outer join output slots, and permits conversion for those null-rejected nullable slots while keeping <=> excluded.
### Release note
None
### Check List (For Author)
- Test:
- Unit Test: ./run-fe-ut.sh --run org.apache.doris.nereids.rules.rewrite.ConvertOuterJoinToAntiJoinTest
- Regression test: ./run-regression-test.sh --run -d nereids_syntax_p0 -s transform_outer_join_to_anti
- FE Checkstyle: mvn checkstyle:check -pl fe-core
- Behavior changed: Yes. The optimizer can now convert eligible outer joins with nullable null-supplying slots to anti joins when the slot is null-rejected by a non-null-safe binary comparison join condition.
- Does this need documentation: No
morrySnow
requested review from
924060929,
englefly and
starocean999
as code owners
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the FE Nereids rewrite rule ConvertOuterJoinToAntiJoin to allow a broader (but still safe) class of outer-join → anti-join conversions when the IS NULL-tested slot on the null-supplying side is nullable in schema but is null-rejected by a non-null-safe comparison join predicate (excluding <=>). This improves rewrite coverage without changing behavior in null-safe cases.
Changes:
- Extend the rewrite’s eligibility check by collecting “null-rejected” join slots from non-null-safe
ComparisonPredicates between left/right children and allowing conversion for those nullable slots. - Add FE unit tests covering nullable-slot conversions for
=and<, and a non-conversion case for<=>. - Add regression
EXPLAINcoverage for the new nullable-slot cases and a<=>non-conversion case.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/ConvertOuterJoinToAntiJoin.java |
Relaxes the nullable-slot restriction by recognizing null-rejected slots from non-null-safe comparisons, while keeping <=> excluded. |
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/ConvertOuterJoinToAntiJoinTest.java |
Adds unit tests validating conversion for nullable join slots under null-rejecting predicates and preventing conversion for null-safe equality. |
regression-test/suites/nereids_syntax_p0/transform_outer_join_to_anti.groovy |
Adds EXPLAIN assertions ensuring the planner emits ANTI JOIN for eligible nullable-slot cases and retains OUTER JOIN for <=>. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I reviewed the full PR scope: ConvertOuterJoinToAntiJoin, its FE unit tests, and the regression explain suite. I did not find a blocking issue or a substantiated inline comment to leave.
Critical checkpoint conclusions:
- Goal/test proof: The PR broadens outer-join-to-anti conversion for nullable schema slots only when the join predicate is a non-null-safe comparison between direct child slots, and adds FE unit plus regression explain coverage for
=,<, right-join symmetry, and<=>non-conversion. - Scope/focus: The implementation is focused on the eligibility check and keeps the existing rewrite structure, projection/null-alias behavior, ASOF exclusion, and rule registration unchanged.
- Semantic correctness: The new
getNullRejectedJoinSlotspath is conservative: it requiresComparisonPredicate, excludesNullSafeEqual, and only accepts direct slot comparisons across the two join children. This matches SQL null rejection for matched rows and leaves nullable non-join-keyIS NULLcases unconverted. - Parallel paths: Left and right outer joins are both covered; hash and other join conjuncts are both considered. I did not find an aggregate, mark-join, or rule-stage parallel path requiring an additional change for this PR.
- Compatibility/config/concurrency/persistence: No new config, persistence, protocol, storage format, or concurrency behavior is introduced.
- Tests: The new tests cover the changed nullable-slot behavior and existing regression style is preserved. I could not run FE unit/regression tests in this checkout because
thirdparty/installedand executablethirdparty/installed/bin/protocare missing; per local FE instructions, build/test invocation must stop in that state.git diff --checkpassed for the changed files.
User focus points: No additional user-provided review focus was present.
Subagent conclusions: optimizer-rewrite and tests-session-config both found no valuable candidates. After the main pass recorded an empty proposed inline comment set, convergence round 1 ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the same current ledger/comment set.
Contributor
TPC-H: Total hot run time: 29879 ms |
Contributor
TPC-DS: Total hot run time: 174562 ms |
Contributor
ClickBench: Total hot run time: 25.2 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Related PR: #23833
Problem Summary: ConvertOuterJoinToAntiJoin only allowed conversion when the null-supplying side slot tested by IS NULL was non-nullable. This missed valid cases where the slot is nullable but appears in a null-rejecting binary comparison between left and right join slots, because a NULL value cannot satisfy those join predicates. The fix records slots from non-null-safe comparison predicates between join children, matches child slots to the corresponding outer join output slots, and permits conversion for those null-rejected nullable slots while keeping <=> excluded.
Release note
None
Check List (For Author)