Repository navigation
planner: support semi join rewrite as an alternative logical plan - #69279
ti-chi-bot[bot] merged 2 commits into
Conversation
|
Skipping CI for Draft Pull Request. |
📝 WalkthroughWalkthroughAdds a new "semi-join-rewrite" alternative logical plan round to the optimizer. A new ChangesSemi-join rewrite alternative logical plan round
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Command failed Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
e8d3d20 to
5e977c6
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69279 +/- ##
================================================
- Coverage 76.3199% 74.5896% -1.7304%
================================================
Files 2041 2034 -7
Lines 562128 575119 +12991
================================================
- Hits 429016 428979 -37
- Misses 132205 145609 +13404
+ Partials 907 531 -376
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
5e977c6 to
56bfb2d
Compare
|
/test mysql-test |
|
/retest |
|
/retest-required |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qw4990, terry1purcell The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/planner/optimize.go`:
- Around line 637-640: The package-level variable `savedEnableSemiJoinRewrite`
creates a shared mutable state that can cause race conditions when multiple
sessions call the optimize function concurrently, as different sessions' cleanup
operations can overwrite each other's saved values. Move the
`savedEnableSemiJoinRewrite` variable from package scope to be local within the
optimize function or the specific invocation scope where it is used, and update
the cleanup code (around lines 678-679) that restores the
`EnableSemiJoinRewrite` value to reference the invocation-local variable instead
of the global one. This ensures each session maintains its own isolated state
during the semi-join-rewrite round.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ae47a475-38b3-461d-a896-5fddc2804e8c
📒 Files selected for processing (7)
pkg/planner/core/casetest/correlated/correlated_test.gopkg/planner/core/casetest/rule/rule_outer_to_semi_join_test.gopkg/planner/core/casetest/rule/testdata/correlate_suite_out.jsonpkg/planner/core/casetest/rule/testdata/correlate_suite_xut.jsonpkg/planner/core/logical_plan_builder.gopkg/planner/optimize.gopkg/sessionctx/stmtctx/stmtctx.go
| // savedEnableSemiJoinRewrite holds the pre-round value of | ||
| // EnableSemiJoinRewrite so setup/cleanup can restore it after the | ||
| // semi-join-rewrite round. Safe because optimize is single-threaded per session. | ||
| var savedEnableSemiJoinRewrite bool |
There was a problem hiding this comment.
Scope EnableSemiJoinRewrite restore state to the current invocation.
Line [637] introduces a package-level mutable savedEnableSemiJoinRewrite; concurrent optimize calls across sessions can race and restore the wrong session value in cleanup (Lines [678-679]). This should be invocation-local, not global state.
Suggested fix
-// savedEnableSemiJoinRewrite holds the pre-round value of
-// EnableSemiJoinRewrite so setup/cleanup can restore it after the
-// semi-join-rewrite round. Safe because optimize is single-threaded per session.
-var savedEnableSemiJoinRewrite bool
-
var alternativeRounds = []alternativeRound{
@@
{
name: "semi-join-rewrite",
enabled: shouldTrySemiJoinRewriteRound,
setup: func(sv *variable.SessionVars) {
- savedEnableSemiJoinRewrite = sv.EnableSemiJoinRewrite
sv.EnableSemiJoinRewrite = true
},
cleanup: func(sv *variable.SessionVars) {
- sv.EnableSemiJoinRewrite = savedEnableSemiJoinRewrite
+ sv.EnableSemiJoinRewrite = false
},
},Also applies to: 675-680
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/planner/optimize.go` around lines 637 - 640, The package-level variable
`savedEnableSemiJoinRewrite` creates a shared mutable state that can cause race
conditions when multiple sessions call the optimize function concurrently, as
different sessions' cleanup operations can overwrite each other's saved values.
Move the `savedEnableSemiJoinRewrite` variable from package scope to be local
within the optimize function or the specific invocation scope where it is used,
and update the cleanup code (around lines 678-679) that restores the
`EnableSemiJoinRewrite` value to reference the invocation-local variable instead
of the global one. This ensures each session maintains its own isolated state
during the semi-join-rewrite round.
|
/test check-dev2 |
What problem does this PR solve?
Issue Number: close #67467
Problem Summary:
The optimizer can use
SEMI_JOIN_REWRITEwhen it is forced by hint or session variable, but alternative logical plan search did not try that rewrite as a competing logical plan when alternative logical plans are enabled.What changed and how does it work?
This PR records when the first logical plan build sees a semi join that can try
SEMI_JOIN_REWRITE, and adds an alternative logical-plan round that temporarily enables semi join rewrite for that build+optimize round. The new round follows the existingalternativeRoundsetup/cleanup pattern and restoresEnableSemiJoinRewriteafter the round finishes.Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Release Notes
Tests
Query Optimization