Uh oh!
There was an error while loading. Please reload this page.
[fix](join) Bound outer join null-reject inference to nullable outputs - #65250
Conversation
hello-stephen
commented
Jul 6, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
foxtail463
commented
Jul 6, 2026
run buildall |
hello-stephen
commented
Jul 6, 2026
TPC-H: Total hot run time: 29409 ms |
hello-stephen
commented
Jul 6, 2026
FE UT Coverage ReportIncrement line coverage |
hello-stephen
commented
Jul 6, 2026
TPC-DS: Total hot run time: 173640 ms |
hello-stephen
commented
Jul 6, 2026
ClickBench: Total hot run time: 25.41 s |
morrySnow
left a comment
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).
Uh oh!
There was an error while loading. Please reload this page.
hello-stephen
commented
Jul 6, 2026
FE Regression Coverage ReportIncrement line coverage |
yx-keith
left a comment
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.
foxtail463
commented
Jul 7, 2026
run buildall |
1722a9b to
1e9ee9cComparefoxtail463
commented
Jul 7, 2026
run buildall |
foxtail463
commented
Jul 8, 2026
run compile |
foxtail463
commented
Jul 8, 2026
run feut |
foxtail463
commented
Jul 8, 2026
run performance |
hello-stephen
commented
Jul 8, 2026
TPC-H: Total hot run time: 29716 ms |
hello-stephen
commented
Jul 8, 2026
TPC-DS: Total hot run time: 181190 ms |
hello-stephen
commented
Jul 8, 2026
ClickBench: Total hot run time: 25.04 s |
foxtail463
commented
Jul 8, 2026
run buildall |
hello-stephen
commented
Jul 8, 2026
TPC-H: Total hot run time: 29453 ms |
hello-stephen
commented
Jul 8, 2026
TPC-DS: Total hot run time: 180248 ms |
hello-stephen
commented
Jul 8, 2026
ClickBench: Total hot run time: 24.98 s |
hello-stephen
commented
Jul 8, 2026
FE Regression Coverage ReportIncrement line coverage |
7be65e9 to
eecf697Comparefoxtail463
commented
Jul 24, 2026
run buidall |
foxtail463
commented
Jul 27, 2026
run buidall |
morrySnow
commented
Jul 27, 2026
/review |
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
foxtail463
commented
Jul 28, 2026
run buildall |
hello-stephen
commented
Jul 28, 2026
TPC-H: Total hot run time: 29765 ms |
hello-stephen
commented
Jul 28, 2026
TPC-DS: Total hot run time: 177587 ms |
hello-stephen
commented
Jul 28, 2026
ClickBench: Total hot run time: 24.95 s |
foxtail463
commented
Jul 29, 2026
run buildall |
hello-stephen
commented
Jul 29, 2026
FE UT Coverage ReportIncrement line coverage |
hello-stephen
commented
Jul 29, 2026
TPC-H: Total hot run time: 29578 ms |
hello-stephen
commented
Jul 29, 2026
TPC-DS: Total hot run time: 177326 ms |
hello-stephen
commented
Jul 29, 2026
ClickBench: Total hot run time: 25.21 s |
hello-stephen
commented
Jul 29, 2026
FE Regression Coverage ReportIncrement line coverage |
foxtail463
commented
Jul 30, 2026
run cloud_p0 |
foxtail463
commented
Jul 30, 2026
run external |
morrySnow
commented
Jul 30, 2026
/review |
hello-stephen
commented
Jul 30, 2026
FE Regression Coverage ReportIncrement line coverage |
1 similar comment
hello-stephen
commented
Jul 30, 2026
FE Regression Coverage ReportIncrement line coverage |
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
apache#65250) related PR: apache#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. --------- Co-authored-by: yangtao555 <yangtao555@jd.com>
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.