[fix](fe) Move SemiJoinCommute to rewrite phase - #66182
Open
seawinde wants to merge 5 commits into
Open
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: SemiJoinCommute structurally normalizes RIGHT OUTER, RIGHT SEMI, and RIGHT ANTI joins. Running it during analysis made analyzed plan shapes depend on join-reorder state, so consumers that identify analyzed plans could observe inconsistent signatures for the same query. Move the rule to both regular and CTE rewrite pipelines before the semi-join transpose rules, keeping analysis stable while preserving rewrite assumptions.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- ReorderJoinTest
- SaltJoinTest
- Behavior changed: Yes. Analyze preserves RIGHT JOIN shape and Rewrite performs the existing normalization.
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: SemiJoinCommute now runs in rewrite pipelines instead of analysis. Add coverage for the disable_join_reorder boundary, the materialized-view pre-rewrite pipeline, and the ordering between SemiJoinCommute and semi-join transpose rules.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- ReorderJoinTest
- SaltJoinTest
- Behavior changed: No
- Does this need documentation: No
seawinde
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
July 28, 2026 12:13
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29177 ms |
Contributor
TPC-DS: Total hot run time: 175672 ms |
Contributor
ClickBench: Total hot run time: 24.68 s |
### What problem does this PR solve? Issue Number: None Related PR: apache#66182 Problem Summary: Moving SemiJoinCommute out of analysis exposed that CollectJoinConstraint assumed right outer, semi, and anti joins had already been normalized to left joins. A preserved-side ON predicate could then be treated as a single-table filter and pushed below the join, changing query results. Canonicalize right-join constraint metadata to the existing left-join model and update unit-test expectations for the equivalent child order produced by the new rewrite stage. ### Release note Fix incorrect LEADING query results for right outer, semi, and anti joins. ### Check List (For Author) - Test: Regression test / Unit Test - InferTest, ReorderJoinTest, SaltJoinTest - query_p0/hint/fix_leading - query_p0/hint/test_leading - Behavior changed: Yes, LEADING no longer pushes preserved-side right join ON predicates below the join. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66182 Problem Summary: Moving SemiJoinCommute out of analysis exposes a pre-existing LEADING dependency on Analyzer normalizing right joins to left joins. The attempted hint fix is independent of the rewrite-stage migration and needs dedicated coverage for right outer, semi, and anti joins. Remove that hint implementation from this PR and temporarily disable only the failing select4_2 regression case until the follow-up fix is ready. ### Release note None ### Check List (For Author) - Test: Regression test / Unit Test - ./run-fe-ut.sh --run org.apache.doris.nereids.sqltest.InferTest,org.apache.doris.nereids.rules.rewrite.ReorderJoinTest,org.apache.doris.nereids.rules.rewrite.SaltJoinTest - ./run-regression-test.sh --run -d query_p0 -s fix_leading - Behavior changed: Yes, defer LEADING handling for original right joins to a follow-up PR and temporarily skip its failing regression case. - Does this need documentation: No
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29607 ms |
Contributor
TPC-DS: Total hot run time: 178328 ms |
Contributor
ClickBench: Total hot run time: 25.1 s |
Member
Author
|
run buildall |
### What problem does this PR solve? Issue Number: None Related PR: apache#66182 Problem Summary: Moving SemiJoinCommute out of analysis exposes a pre-existing LEADING dependency on Analyzer normalizing right joins. Temporarily disable the corresponding select4_3 shape assertion alongside select4_2 until the independent LEADING fix is ready. ### Release note None ### Check List (For Author) - Test: Regression test - Verified the P0 failure is isolated to select4_3; select2_2 and select2_5 complete before it - Behavior changed: No - Does this need documentation: No
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 30118 ms |
Contributor
TPC-DS: Total hot run time: 176953 ms |
Contributor
ClickBench: Total hot run time: 25 s |
Contributor
TPC-H: Total hot run time: 29422 ms |
Contributor
TPC-DS: Total hot run time: 177122 ms |
Contributor
ClickBench: Total hot run time: 24.87 s |
Contributor
|
/review |
Contributor
FE Regression Coverage ReportIncrement line coverage |
![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 data-correctness regression remains.
The stage move does make analyzed RIGHT OUTER/SEMI/ANTI plans independent of the mutable disable_join_reorder state, and both the normal and MV pre-rewrite pipelines now commute them before the TransposeSemiJoin* rules. However, LEADING constraint collection still assumes the analyzer has normalized outer joins. For the inline query it pushes a predicate from the preserved side of a RIGHT OUTER JOIN below Scan(t3), changing the result; the PR comments out the existing result and shape checks that expose this.
Critical checkpoint conclusions:
- Goal and proof: the intended analyzed-plan stability is implemented and the new unit tests cover the causal analysis/rewrite boundary, but the PR does not accomplish the goal safely while this LEADING wrong-result path remains.
- Scope and clarity: the production change is small and focused; disabling an existing correctness regression instead of fixing the parallel LEADING path is not an acceptable completion of that scope.
- Concurrency: no new thread or shared-data concurrency is introduced. The relevant mutable state is a sequential per-connection
SessionVariablereused across nested planner calls. - Lifecycle: the two-planner invalid-statistics lifecycle was traced. Moving the commute out of Analyzer stabilizes both analyzed trees, while rewrite remains flag-dependent as intended. The LEADING analysis lifecycle is the unresolved parallel path.
- Configuration: no new configuration is added; the existing session variable remains dynamically observed during rewrite.
- Compatibility: there is no FE/BE protocol, storage-format, symbol, rolling-upgrade, or persistence-format change. There is a user-visible planner-hint/result compatibility regression described inline.
- Parallel paths: both standard and MV pre-rewrite job lists were updated, and right-oriented handling before the commute is otherwise symmetric.
CollectJoinConstraint/LeadingHintis the missed path. - Conditional checks: the
disable_join_reorder, LEADING, and distribution-hint guards are intentional, but postponing normalization exposes the left-only constraint bitmap logic. - Test coverage and results: the new FE tests cover normal rewrite, the disabled flag, and MV pre-rewrite placement. The existing LEADING result/shape tests must remain enabled. No local build or test was run in this review-only runner. Live CI showed compile, Checkstyle, and P0 regression passing; the FE UT job failed on
ForwardToMasterTest.testAddBeDropBeand an execution timeout outside the changed files, so it is not positive evidence for these new tests. - Observability: no new runtime path requires logs or metrics.
- Transactions, persistence, and data writes: not applicable; this is logical planning only.
- FE/BE variable propagation: no new transmitted variable is introduced.
- Performance: the added rewrite pass is linear over the logical tree and no separate performance defect was substantiated; the broken LEADING path can also prevent the requested join order.
- Other issues: no additional correctness issue survived independent verification and deduplication.
User focus: no additional focus was supplied; the whole PR was reviewed.
| qt_select4_1 """select count(*) from t1 left join t2 on c1 > 500 and c2 >500 right join t3 on c3 > 500 and c1 < 200;""" | ||
| qt_select4_2 """select /*+ leading(t1 t2 t3)*/ count(*) from t1 left join t2 on c1 > 500 and c2 >500 right join t3 on c3 > 500 and c1 < 200;""" | ||
| qt_select4_3 """explain shape plan select /*+ leading(t1 t2 t3)*/ count(*) from t1 left join t2 on c1 > 500 and c2 >500 right join t3 on c3 > 500 and c1 < 200;""" | ||
| // TODO: Re-enable after LEADING handles right join constraints without Analyzer normalization. |
Contributor
There was a problem hiding this comment.
[P1] Preserve right-join ON semantics before disabling this test
After normalization is removed from Analyzer, CollectJoinConstraint now sees this top RIGHT_OUTER_JOIN directly. It only pins ON predicates to both inputs for isLeftJoin(); there is no symmetric right-join case. Consequently the preserved-side predicate c3 > 500 is recorded against {t3} alone, and LeadingHint.makeFilterPlanIfExist can move it below Scan(t3). Rows with c3 <= 500 must still survive this right outer join as null-extended rows, but that pushed filter drops them. The existing qt_select4_2 count (1000) and qt_select4_3 shape/Used: leading assertions cover exactly this behavior, so commenting them out only hides the wrong-result regression. Please make LEADING encode right-join constraints symmetrically (the same concern applies to preserved-side RIGHT ANTI predicates) and keep these assertions enabled.
Member
Author
There was a problem hiding this comment.
EADING encode right-join constraints symmetrically (the same concern applies to preserved-side RIGHT ANTI predicates) would be in another pr
Contributor
FE UT Coverage ReportIncrement line coverage |
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
3 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.
What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary:
An incremental materialized view COMPLETE refresh may plan the same
INSERT OVERWRITEstatement twice using oneSessionVariable. If the firstoptimizer pass finds invalid statistics, it temporarily enables
disable_join_reorder. The second planning pass can therefore produce adifferent analyzed join structure and IVM signature from the first pass or
a later incremental refresh.
Root cause:
SemiJoinCommuteran in the Analyzer and readdisable_join_reorder. This made analyzed RIGHT OUTER, RIGHT SEMI, andRIGHT ANTI join structures depend on planner state that can change between
planning passes.
Change Summary:
Analyzer.javaSemiJoinCommutefrom analysis so analyzed plans remain stableRewriter.javaSemiJoinCommutein both normal and MV pre-rewrite pipelines beforeTransposeSemiJoin*ReorderJoinTest.javagraph LR A[Analyze RIGHT JOIN] --> B[Stable IVM signature] A --> C[Rewrite] C --> D{disable_join_reorder} D -->|false| E[Normalize to LEFT JOIN] D -->|true| F[Preserve RIGHT JOIN]This keeps IVM signature generation independent of mutable optimizer state,
while preserving the existing join-reorder switch, LEADING hint, and
distribution-hint behavior during rewrite.
Release note
Fix inconsistent incremental materialized view refresh planning when invalid
statistics change join-reorder state between COMPLETE refresh planning passes.
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)