[fix](fe) Fix semi join constraint matching for leading hint#65205
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run feut |
FE UT Coverage ReportIncrement line coverage |
|
run buildall |
TPC-H: Total hot run time: 29320 ms |
TPC-DS: Total hot run time: 174370 ms |
ClickBench: Total hot run time: 25.25 s |
FE Regression Coverage ReportIncrement line coverage |
|
/review |
There was a problem hiding this comment.
Automated review result: request changes.
I found two correctness gaps in the leading-hint semi/anti constraint fix. The new unit test still misses a semi-join order where the RHS is joined before the original LHS exists, and the code guard still does not cover anti joins or direct right semi/right anti constraints with the correct exact side.
Critical checkpoint conclusions:
- Goal and tests: the PR is small and focused, and it adds a targeted JUnit test, but the current change does not fully accomplish the stated leading-hint constraint goal because related semi/anti paths remain valid matches.
- Parallel paths: not complete. Left anti and direct right semi/right anti paths remain uncovered by the new check.
- Concurrency, lifecycle, config, persistence, data writes, FE-BE protocol, and observability: not applicable to this local FE planner logic change.
- Test validation: I did not run FE tests because this checkout is missing thirdparty/installed/bin/protoc and thirdparty/installed, so the repo instructions make local FE build/test execution invalid here.
- User focus: no additional user-provided review focus was supplied.
Subagent conclusions: TSC-1 was accepted as M-1 and submitted as an inline test coverage/correctness comment. OR-1 and OR-2 were accepted and merged into M-2 as one inline code comment. Convergence round 1 found OR-2, so the ledger/comment set was updated; convergence round 2 ended with both optimizer-rewrite and tests-session-config reporting NO_NEW_VALUABLE_FINDINGS.
| if (joinConstraint.getJoinType().isSemiJoin()) { | ||
| if (LongBitmap.isSubset(joinConstraint.getRightHand(), leftTableBitmap) | ||
| && !LongBitmap.isSubset(joinConstraint.getRightHand(), leftTableBitmap)) { | ||
| && !joinConstraint.getRightHand().equals(leftTableBitmap)) { |
There was a problem hiding this comment.
This guard is still too narrow. First, it only runs for isSemiJoin(), so A LEFT ANTI JOIN B can still match a leading step like (B JOIN C) JOIN A, skip this block, take the reversed branch below, and become a RIGHT_ANTI_JOIN that tests absence from B JOIN C instead of absence from B. Second, direct right semi/right anti constraints need the exact check on the original left/filtering side, not rightHand: for A RIGHT SEMI JOIN B, getJoinConstraint(A | B | C, A | C, B) passes this check because rightHand == B, then applies the semi test against A JOIN C. Please make the exact-side check cover anti joins and choose the constrained side by join direction, with left/right semi and anti unit cases.
6d96b4b to
a703e99
Compare
|
/review |
|
run buildall |
There was a problem hiding this comment.
Automated review result: requesting changes.
I reviewed the two changed files plus the relevant LeadingHint, CollectJoinConstraint, bitmap, and join-output paths. The patch fixes the earlier one-child constrained-side cases, but one remaining semi/anti leading-hint shape can still produce a semantically invalid plan; see the inline comment.
Critical checkpoint conclusions:
- Goal/test: the goal is to make semi/anti leading constraints match only valid side shapes. The added unit tests cover simple one-table sides, but not the composite full-side/min-side divergence that remains broken.
- Scope: the change is narrow and focused on LeadingHint matching plus a unit test.
- Concurrency, lifecycle, config, persistence, compatibility, and data-write paths: not involved.
- Parallel paths: checked left/right semi and anti matching. The remaining issue is a parallel composite constrained-side path where full side and min side differ.
- Tests/validation: did not run FE UT because this runner lacks thirdparty/installed and executable thirdparty/installed/bin/protoc.
git diff --checkon the changed files passed. - User focus: no additional user-provided review focus.
Subagent conclusions: optimizer-rewrite produced OPT-001, accepted as MAIN-001 and submitted inline. tests-session-config produced TSC-001, a duplicate test-coverage note folded into MAIN-001. Convergence round C1 ended with both optimizer-rewrite and tests-session-config reporting NO_NEW_VALUABLE_FINDINGS for the same final ledger/comment set.
|
|
||
| Long constrainedSide = joinConstraint.getJoinType().isRightSemiOrAntiJoin() | ||
| ? joinConstraint.getLeftHand() : joinConstraint.getRightHand(); | ||
| if (LongBitmap.isSubset(constrainedSide, leftTableBitmap) |
There was a problem hiding this comment.
This still accepts cases where the constrained semi/anti side is split across both candidate children. For example, a collected constraint from (A CROSS JOIN C) RIGHT SEMI JOIN B ON A.k = B.k has full leftHand = A|C and rightHand = B, but minLeftHand can be only A. With a leading step getJoinConstraint(A|B|C, A, B|C), constrainedSide is A|C; it is not a subset of either child, so both exact checks are skipped and the later minLeftHand/minRightHand match returns RIGHT_SEMI_JOIN. That rewrites the join as A RIGHT SEMI (B CROSS C), whose output is the whole right child B|C, while the original right semi join outputs only B and uses all of A|C only for existence testing. Please reject any child that partially overlaps the constrained side without equaling it, and add unit coverage where leftHand/rightHand is composite but the corresponding min*Hand is narrower.
TPC-H: Total hot run time: 29836 ms |
TPC-DS: Total hot run time: 174467 ms |
ClickBench: Total hot run time: 25.4 s |
FE Regression Coverage ReportIncrement line coverage |
a703e99 to
858a26a
Compare
|
/review |
|
run buildall |
There was a problem hiding this comment.
Automated review result: requesting changes.
I reviewed the current head 858a26a10a8dd5a62bb5a0e36ec61d960d572f12, the two changed files, existing review threads, and the surrounding CollectJoinConstraint / LeadingHint generation path. The latest patch fixes the earlier split constrained-side cases, but it also over-constrains semi/anti matching by requiring the full retained side to be present before a constraint can match. That rejects valid leading orders where only the retained side's minimal hand is needed and unrelated retained-side inner joins can be applied later; see the inline comment.
Critical checkpoint conclusions:
- Goal and tests: the PR is focused on semi/anti leading-hint constraint matching and adds targeted JUnit coverage, but the current code does not fully accomplish the goal because composite retained-side/min-hand cases are not covered and can now be rejected incorrectly.
- Scope: the code change is small and localized to
LeadingHint.getJoinConstraintplus a focused unit test. - Parallel paths: checked left/right semi and anti paths. Existing prior threads cover missing retained-side presence, anti/right-side exactness, and split constrained-side matching; this remaining issue is a distinct retained-side over-rejection path.
- Concurrency, lifecycle, config/session propagation, persistence, data writes, FE-BE protocol, and observability: not involved in this local FE planner logic change.
- Test validation: I did not run FE UTs because this runner lacks
thirdparty/installedand executablethirdparty/installed/bin/protoc, andfe/AGENTS.mdsays to stop FE builds when protoc is missing. I checked the GitHub patch for added-line trailing whitespace and >120-character added lines; both checks passed. - User focus: no additional user-provided review focus was supplied.
Subagent conclusions: optimizer-rewrite and tests-session-config produced no initial candidate findings. The main pass accepted MAIN-001 as the only inline comment. Convergence round C1 sent the final ledger/comment set containing MAIN-001 to both live subagents, and both replied NO_NEW_VALUABLE_FINDINGS for that same unchanged set.
| if (LongBitmap.isSubset(joinConstraint.getRightHand(), leftTableBitmap) | ||
| && !LongBitmap.isSubset(joinConstraint.getRightHand(), leftTableBitmap)) { | ||
| if (joinConstraint.getJoinType().isSemiOrAntiJoin()) { | ||
| if (!LongBitmap.isSubset(joinConstraint.getLeftHand(), joinTableBitmap) |
There was a problem hiding this comment.
This check now requires both full original hands to be present before a semi/anti constraint can match, which rejects legal orders where the retained side contains tables that can be joined later. A reduced plan is:
(A INNER JOIN C) LEFT SEMI JOIN B ON A.k = B.k
CollectJoinConstraint records this as leftHand = A|C, rightHand = B, but the minimal hands are only minLeftHand = A and minRightHand = B. The existing min-hand branch below would allow the valid leading step getJoinConstraint(A|B, A, B) so the generated plan can do (A LEFT SEMI B) INNER JOIN C; the A-C filter is still applied at the later join. With this new guard, leftHand = A|C is not a subset of the current joinTableBitmap = A|B, so the constraint is skipped and the leading hint is unnecessarily treated as unavailable. Please keep the full/exact requirement on the constrained filtering side, but let the retained side continue to use the existing minLeftHand/minRightHand logic, with unit coverage where the retained side is composite.
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary: Leading hint may match a semi join constraint when the semi join right-hand side is only a subset of one candidate child. The left-side check used the same subset predicate on both sides of the condition, making it always false. This can let an invalid leading order apply or reverse a semi join constraint even when the semi right-hand side is already joined with extra tables. Fix the check to require the semi right-hand side to match the child bitmap exactly, and add a unit test for the exact-match requirement.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- ./run-fe-ut.sh --run org.apache.doris.nereids.hint.LeadingHintTest
- Behavior changed: No
- Does this need documentation: No
858a26a to
2ca7ced
Compare
|
/review |
There was a problem hiding this comment.
Automated review completed for head 2ca7ced274ca3bd29bbe275c3cd39e1f1e6b5080.
I reviewed the changed leading-hint constraint logic and the new LeadingHintTest coverage. I did not find any new inline issue to add beyond the existing review threads; the current patch appears to address the previously raised semi/anti constrained-side cases.
Critical checkpoints:
- Goal/test: The change targets semi/anti leading-hint constraint matching. The code now uses min hands for presence and exact full constrained-side overlap, and the added unit tests cover left/right semi and anti joins, composite constrained-side splits, and retained-side min-hand matching.
- Scope: The patch is narrow and focused on
LeadingHint.getJoinConstraintplus direct unit coverage. - Concurrency/lifecycle/config/persistence/protocol/security: Not involved.
- Parallel paths: Left/right semi and anti paths were checked; the earlier anti/right-direction and composite-side concerns are covered by existing threads and by the current head.
- Test status: I did not run local FE UT because this checkout is missing
thirdparty/installed/bin/protoc, which the FE instructions require before building. PRCheckStyleis passing; several external regression checks are still pending. - Performance/observability: The added bitmap checks are small per-constraint operations; no additional observability is needed for this planner-only fix.
User focus: No additional user-provided focus points were supplied.
Subagent conclusions: optimizer-rewrite and tests-session-config both found no new valuable candidates beyond existing threads. Convergence round 1 ended with both subagents replying NO_NEW_VALUABLE_FINDINGS for the same ledger and empty final inline comment set.
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 29123 ms |
TPC-DS: Total hot run time: 180399 ms |
ClickBench: Total hot run time: 24.94 s |
FE Regression Coverage ReportIncrement line coverage |
Related PR: #65205 Problem Summary: PR #65205 introduced a correctness issue in leading hint planning by changing how semi/anti join constraints are matched. This PR reverts merge commit `cbccdafa41adfff73e683ac1dfcae46638c16d48` in full, restoring the previous planner behavior while a correct fix is prepared.
What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary:
A muted FE UT failure in
DistributeHintTest.testLeadingexposed that leading hint can match a semi/anti join constraint too early or with extra tables on the constrained side. The old left-side check inLeadingHint#getJoinConstraintused the same subset predicate on both sides of the condition, making that guard always false, and the check only covered semi joins with the original right side.This patch makes semi/anti constraints match only after the minimal left and right hands are both present in the current join. It also chooses the constrained side by join direction: left semi/anti requires the original right side to match a child exactly, while right semi/anti requires the original left side to match a child exactly. Any child that partially overlaps the constrained side without equaling it is rejected, so a composite constrained side can not be split across both join children. The retained side can still match by its minimal hand, allowing unrelated retained-side joins to be applied later. Unit tests cover left/right semi/anti cases, including early constrained-side matches, extra-table matches, composite constrained-side splits, and composite retained-side/min-hand matches.
Release note
None
Check List (For Author)
./run-fe-ut.sh --run org.apache.doris.nereids.hint.LeadingHintTestBehavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)