[fix](join) Bound outer join null-reject inference to nullable outputs - #65250
Merged
Conversation
foxtail463
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
July 6, 2026 07:41
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29409 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-DS: Total hot run time: 173640 ms |
Contributor
ClickBench: Total hot run time: 25.41 s |
morrySnow
reviewed
Jul 6, 2026
morrySnow
left a comment
morrySnow
left a comment
Contributor
There was a problem hiding this comment.
Thanks for this PR! The bounding of null-reject inference to nullable-side outputs is a clean optimization. I reviewed all angles (line-by-line, removed-behavior audit, cross-file tracer, reuse, simplification, efficiency, altitude, conventions) and found the implementation correct and well-tested.
One code organization observation (not a bug): The new getNullableSideOutput() in LogicalJoin re-encodes the join-type-to-nullable-side mapping (LEFT_OUTER→right nullable, RIGHT_OUTER→left nullable, FULL_OUTER→both) that already exists in JoinUtils.getJoinOutput() lines 417-433. If a new join type with different null-extension semantics is added, the mapping would need updating in multiple places. Consider whether a utility method on JoinType (e.g., isLeftSideNullable()/isRightSideNullable()) could consolidate this knowledge.
LGTM overall — the new test coverage is thorough (IN+OR-FALSE elimination, FULL OUTER degradation, early-return guard, negative cases).
morrySnow
reviewed
Jul 6, 2026
Contributor
FE Regression Coverage ReportIncrement line coverage |
yx-keith
reviewed
Jul 7, 2026
yx-keith
left a comment
yx-keith
left a comment
Contributor
There was a problem hiding this comment.
Bounding the inference to getNullableSideOutput() is the right target set. Note this isn't purely skipping useless work — it changes elimination behavior: skipping mark-join slots as inference targets can drop an elimination, and the smaller target set interacts with the expression-complexity limit (a set that previously bailed on complexity may now fit and eliminate more).
P0 Regression / cloud_p0 are red. Since outer-join elimination affects results, please confirm these are expected plan changes (regenerated .out) rather than wrong results, and that skipping mark-join slots is always safe for eliminating the current join.
Contributor
Author
|
run buildall |
foxtail463
force-pushed
the
fix/outer-join-null-reject
branch
from
July 7, 2026 13:14
1722a9b to
1e9ee9c
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
run compile |
Contributor
Author
|
run feut |
Contributor
Author
|
run performance |
Contributor
TPC-H: Total hot run time: 29716 ms |
Contributor
TPC-DS: Total hot run time: 181190 ms |
Contributor
ClickBench: Total hot run time: 25.04 s |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29453 ms |
Contributor
TPC-DS: Total hot run time: 180248 ms |
Contributor
ClickBench: Total hot run time: 24.98 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Jul 30th, 2026 7:58 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
morrySnow
reviewed
Jul 24, 2026
foxtail463
force-pushed
the
fix/outer-join-null-reject
branch
from
July 24, 2026 12:07
7be65e9 to
eecf697
Compare
Contributor
Author
|
run buidall |
Contributor
Author
|
run buidall |
Contributor
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Request changes: one P1 correctness blocker and one P2 required test gap.
Review completeness: Incomplete under the mandatory three-round cap. The final round identified, and the main review independently verified, a remediation-scope correction to the P1: the evaluation-sensitive guard must cover the whole filter set. That correction is included in the inline comment and no candidate remains unclassified, but the corrected payload could not receive a fourth convergence round.
Critical checkpoint conclusions:
- Goal and proof: Bounding outer-join null-reject inference to nullable outputs implements the intended optimization for ordinary deterministic predicates, and the LEFT/RIGHT/FULL/ASOF weakening matrix is sound. It is not correct for evaluation-sensitive filter sets, as described in the P1. The new aggregate partial-inference path is safe but lacks its required boundary test (P2).
- Scope and clarity: The production change is focused, the removed helper has no remaining repository caller, and the new join-type helpers centralize the nullable-side mapping.
- Concurrency: Not involved; these are single-query planner rewrites with no shared mutable state or locking changes.
- Lifecycle: The rewrite strengthens join types monotonically, returns null for unchanged types, and generated not-null markers are later removed; no loop or non-intuitive resource lifecycle was found.
- Configuration: No configuration item is added. The supported
disable_nereids_rules='INFER_FILTER_NOT_NULL'path exposes the P1 and was included in the review. - Compatibility: No function symbol, protocol, storage-format, or rolling-upgrade contract changes.
- Parallel paths: All
inferNotNullSlots/inferNotNullcallers, mark-join handling, MV/exploration consumers, and LEFT/RIGHT/FULL/ASOF paths were traced. They are otherwise conservative or semantics-preserving. - Conditional logic: The nullable-side intersection and join weakening conditions are correct for ordinary predicates; the missing filter-wide volatile/
NoneMovableFunctioncondition is the P1. - Test coverage: Added tests cover positive and negative join-shape rewrites, including cascades, but not evaluation-sensitive result/error semantics. The newly reachable >32-slot aggregate boundary also lacks focused and row-result coverage.
- Test results: The modified explain outputs are consistent with the traced join transformations. No build or test was run in this review because the runner instructions prohibit it.
- Observability: No new runtime subsystem needs logs or metrics; explain-plan output is the relevant existing signal.
- Transactions and persistence: Not involved; there are no EditLog, failover, or persisted-state changes.
- Data writes: Not involved; there is no transactionality, atomicity, or crash-recovery path.
- FE/BE variables: No new cross-module variable or thrift propagation is introduced.
- Performance: Width, depth, and cumulative 32-slot limits keep fold work bounded; target restriction removes unrelated folds. No additional performance defect was found.
- Other issues: Existing inline threads about mark-slot targeting and JoinUtils enumeration were deduplicated; no other substantiated issue remains.
User focus: No additional focus was supplied, so the entire PR was reviewed.
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29765 ms |
Contributor
TPC-DS: Total hot run time: 177587 ms |
Contributor
ClickBench: Total hot run time: 24.95 s |
Contributor
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29578 ms |
Contributor
TPC-DS: Total hot run time: 177326 ms |
Contributor
ClickBench: Total hot run time: 25.21 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
run cloud_p0 |
Contributor
Author
|
run external |
Contributor
|
/review |
Contributor
FE Regression Coverage ReportIncrement line coverage |
1 similar comment
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
There was a problem hiding this comment.
Request changes: one new P1 correctness blocker was found on the reviewed head. The existing evaluation-sensitive outer-join P1 remains unresolved, and the new 33-column unit test is still plan-shape-only rather than the full-rewrite/result coverage requested by the existing P2.
Critical checkpoint conclusions:
- Goal and proof: Restricting null-reject inference to the current join's nullable outputs works for ordinary deterministic predicates, and the LEFT/RIGHT/FULL/ASOF weakening matrix is sound. The newly reachable partial aggregate path is not generally safe because temporary inferred filters can cross a volatile argument project and make a persistent cardinality rewrite (
MAIN-1). - Scope and clarity: The join-targeting production change is focused, but removing the aggregate-wide cheapness gate broadens behavior beyond that goal and creates the new blocker.
- Concurrency: Not involved; these are single-query planner rewrites with no shared mutable state or locking change.
- Lifecycle: Join-type transitions are monotonic and the unchanged-type guard prevents loops. The defect is an intermediate-lifecycle interaction: generated filters are later removed, but the outer-to-inner rewrite they trigger is not reversed.
- Configuration: No item is added. The supported
disable_nereids_rules='INFER_FILTER_NOT_NULL'path still exposes the already-reported evaluation-sensitive outer-join issue. - Compatibility: No protocol, storage format, function symbol used outside this internal planner surface, or rolling-upgrade contract changes.
- Parallel paths and conditional logic: All shared inference callers, mark-slot behavior, LEFT/RIGHT/FULL/ASOF branches, aggregate variants, and rewrite stages were traced. Their ordinary deterministic behavior is conservative or semantics-preserving;
MAIN-1is the distinct evaluation-sensitive exception. - Test coverage and results: Unit and explain tests cover the ordinary join-shape branches, and the modified expected plans are consistent. The new >32 test checks only an isolated rule shape; it does not cover normalization, pushdown, persistent join elimination, volatile evaluation, NULL-row results, or multiple-aggregate intersection. No local build/test was run because this review runner explicitly forbids builds.
- Observability: No new runtime subsystem needs logs or metrics; explain plans are the relevant existing signal.
- Transactions, persistence, and data writes: Not involved.
- FE/BE propagation: No new cross-module variable or thrift field is introduced. BE variadic aggregate NULL handling and seeded-random evaluation were traced to validate
MAIN-1. - Performance: Width, depth, and cumulative slot limits bound fold work, and target restriction removes irrelevant folds. No additional performance defect was substantiated.
- Other issues: Existing threads about mark-slot targeting and JoinUtils enumeration were deduplicated; no other unresolved suspicious point remains.
User focus: No additional focus was supplied, so the entire PR was reviewed.
morrySnow
approved these changes
Aug 1, 2026
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
4 participants
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.
related PR: #63318
Problem Summary:
Outer join elimination uses fold-based null-reject inference to decide whether
nullable-side rows from the current outer join can be filtered away. This check
only needs slots from that join's nullable-side outputs, but the previous flow
could also test unrelated predicate inputs. When a filter predicate references a
mark slot produced by another join, folding that slot is useless for eliminating
the current outer join and can be costly when the fixed-point rewrite batch
revisits the same predicate multiple times.
Solution:
Add an API to infer null-rejecting slots only for a given target slot set, and
use the current join's nullable-side outputs as that target in
EliminateOuterJoin. Skip mark-join slots as inference targets, preserve the
existing expression complexity limits, and avoid rewriting when the join type
does not change.