planner, sessionctx: support alternative logical plans | tidb-test=pr/2752#68646
planner, sessionctx: support alternative logical plans | tidb-test=pr/2752#68646AilinKid wants to merge 4 commits into
Conversation
…p#66677) ref pingcap#66676 (cherry picked from commit d59e531)
…pingcap#66743) ref pingcap#66651 (cherry picked from commit dcbd120)
…gcap#66995) ref pingcap#65710 (cherry picked from commit 0c4df2e)
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
do-not-merge/cherry-pick-not-approved
do-not-merge/work-in-progress
📝 WalkthroughWalkthroughThis PR implements a multi-round logical plan optimization framework to find better physical plans by exploring alternative logical plans. It refactors hint state management to use per-build snapshots, adds decorrelation tracking to logical and physical joins, and introduces ChangesAlternative Logical Plans Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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 |
ti-chi-bot
Bot
added
the
size/XXL
|
Hi @AilinKid. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
AilinKid
added
the
type/cherry-pick-for-release-8.5
ti-chi-bot
Bot
removed
the
type/cherry-pick-for-release-8.5
|
@AilinKid: You cannot manually add or delete the cherry pick branch category labels. It will be added automatically by bot when the PR is created. DetailsIn response to adding label named type/cherry-pick-for-release-8.5. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
AilinKid
added
type/cherry-pick-for-release-8.5
ti-chi-bot
Bot
removed
the
type/cherry-pick-for-release-8.5
|
@AilinKid: You cannot manually add or delete the cherry pick branch category labels. It will be added automatically by bot when the PR is created. DetailsIn response to adding label named type/cherry-pick-for-release-8.5. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
ti-chi-bot
Bot
added
the
do-not-merge/release-note-label-needed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/planner/core/planbuilder.go (1)
493-495: ⚡ Quick winClear
hintStatewhen no processor is provided.At Line 493,
hintStateis only assigned in the non-nil branch. If the builder is re-initialized without a processor, stale state can survive from a prior build.Suggested diff
b.hintProcessor = processor if processor != nil { b.hintState = processor.NewBuildState() + } else { + b.hintState = nil }🤖 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/core/planbuilder.go` around lines 493 - 495, The code only sets b.hintState when processor != nil, which can leave stale state when re-initialized without a processor; update the initialization in the function that uses processor so that if processor is nil you explicitly clear b.hintState (set it to nil) else set it to processor.NewBuildState(); reference b.hintState, processor and NewBuildState to locate and fix the logic.
🤖 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/core/task.go`:
- Around line 170-172: The decorrelated-apply same-order mark is only invoked in
one code path so PhysicalIndexHashJoin and PhysicalIndexMergeJoin never call
MarkAlternativeLogicalPlanSameOrderIndexJoin() and still trigger
shouldTryAlternativeLogicalPlanRound(); fix by ensuring the same-order mark runs
for index-based joins: either move the FromDecorrelatedApply check and the call
to
SCtx().GetSessionVars().StmtCtx.MarkAlternativeLogicalPlanSameOrderIndexJoin()
into the shared PhysicalIndexJoin code path that both PhysicalIndexHashJoin and
PhysicalIndexMergeJoin use, or explicitly add the same FromDecorrelatedApply
guard and MarkAlternativeLogicalPlanSameOrderIndexJoin() call inside the
PhysicalIndexHashJoin and PhysicalIndexMergeJoin implementations so that when
FromDecorrelatedApply is true those join types also mark same-order INLHJ/INLMJ
candidates.
In `@pkg/planner/optimize.go`:
- Around line 515-516: The deferred call to builder.HandleUnusedViewHints()
causes unused-view-hint diagnostics to be added after bestLogicalPlanCtx was
captured, so when optimize() later restores bestLogicalPlanCtx those warnings
are lost; to fix, ensure you materialize unused-view hints before capturing or
restoring the winner snapshot: invoke builder.HandleUnusedViewHints() (or flush
its results) and then update/capture bestLogicalPlanCtx (the snapshot used by
optimize()) so the saved StmtCtx includes those diagnostics; apply the same
change to the other occurrences referenced (around the blocks at the other two
locations).
---
Nitpick comments:
In `@pkg/planner/core/planbuilder.go`:
- Around line 493-495: The code only sets b.hintState when processor != nil,
which can leave stale state when re-initialized without a processor; update the
initialization in the function that uses processor so that if processor is nil
you explicitly clear b.hintState (set it to nil) else set it to
processor.NewBuildState(); reference b.hintState, processor and NewBuildState to
locate and fix the logic.
🪄 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: 520640d5-20ae-4d44-a2d6-52e6fb1c6fd9
📒 Files selected for processing (24)
pkg/executor/test/simpletest/BUILD.bazelpkg/parser/ast/BUILD.bazelpkg/planner/BUILD.bazelpkg/planner/core/casetest/correlated/BUILD.bazelpkg/planner/core/casetest/correlated/correlated_test.gopkg/planner/core/exhaust_physical_plans.gopkg/planner/core/logical_plan_builder.gopkg/planner/core/operator/logicalop/logical_join.gopkg/planner/core/physical_plans.gopkg/planner/core/planbuilder.gopkg/planner/core/rule_decorrelate.gopkg/planner/core/task.gopkg/planner/optimize.gopkg/sessionctx/stmtctx/BUILD.bazelpkg/sessionctx/stmtctx/stmtctx.gopkg/sessionctx/stmtctx/stmtctx_test.gopkg/sessionctx/variable/session.gopkg/sessionctx/variable/setvar_affect.gopkg/sessionctx/variable/sysvar.gopkg/sessionctx/variable/tidb_vars.gopkg/sessionctx/variable/varsutil_test.gopkg/statistics/handle/handletest/BUILD.bazelpkg/util/context/plancache.gopkg/util/hint/hint_query_block.go
| if p.FromDecorrelatedApply { | ||
| p.SCtx().GetSessionVars().StmtCtx.MarkAlternativeLogicalPlanSameOrderIndexJoin() | ||
| } |
There was a problem hiding this comment.
Count same-order INLHJ/INLMJ candidates too.
PhysicalIndexHashJoin and PhysicalIndexMergeJoin embed PhysicalIndexJoin, but they bypass this method, so their FromDecorrelatedApply plans never call MarkAlternativeLogicalPlanSameOrderIndexJoin(). When the decorrelated plan only produces a same-order INLHJ/INLMJ, shouldTryAlternativeLogicalPlanRound() still fires and we pay the extra optimization round anyway.
🤖 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/core/task.go` around lines 170 - 172, The decorrelated-apply
same-order mark is only invoked in one code path so PhysicalIndexHashJoin and
PhysicalIndexMergeJoin never call MarkAlternativeLogicalPlanSameOrderIndexJoin()
and still trigger shouldTryAlternativeLogicalPlanRound(); fix by ensuring the
same-order mark runs for index-based joins: either move the
FromDecorrelatedApply check and the call to
SCtx().GetSessionVars().StmtCtx.MarkAlternativeLogicalPlanSameOrderIndexJoin()
into the shared PhysicalIndexJoin code path that both PhysicalIndexHashJoin and
PhysicalIndexMergeJoin use, or explicitly add the same FromDecorrelatedApply
guard and MarkAlternativeLogicalPlanSameOrderIndexJoin() call inside the
PhysicalIndexHashJoin and PhysicalIndexMergeJoin implementations so that when
FromDecorrelatedApply is true those join types also mark same-order INLHJ/INLMJ
candidates.
| // TODO: when buildRound > 1, only emit unused view-hint warnings for the winner build. | ||
| defer builder.HandleUnusedViewHints() |
There was a problem hiding this comment.
Save the winner snapshot after unused-view hints are materialized.
builder.HandleUnusedViewHints() runs in the defer, but bestLogicalPlanCtx is captured before that defer fires. When optimize() restores bestLogicalPlanCtx at the end, it rewinds StmtCtx to a state that predates those warnings, so the winning round can silently lose its unused-view-hint diagnostics.
Also applies to: 582-584, 705-706
🤖 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 515 - 516, The deferred call to
builder.HandleUnusedViewHints() causes unused-view-hint diagnostics to be added
after bestLogicalPlanCtx was captured, so when optimize() later restores
bestLogicalPlanCtx those warnings are lost; to fix, ensure you materialize
unused-view hints before capturing or restoring the winner snapshot: invoke
builder.HandleUnusedViewHints() (or flush its results) and then update/capture
bestLogicalPlanCtx (the snapshot used by optimize()) so the saved StmtCtx
includes those diagnostics; apply the same change to the other occurrences
referenced (around the blocks at the other two locations).
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #68646 +/- ##
================================================
Coverage ? 55.6833%
================================================
Files ? 1824
Lines ? 662733
Branches ? 0
================================================
Hits ? 369032
Misses ? 266879
Partials ? 26822
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
AilinKid
changed the title
|
/test mysql-test |
|
@AilinKid: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test mysql-test |
|
@AilinKid: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
AilinKid
added
cherry-pick-approved
|
@AilinKid: You cannot manually add or delete the cherry pick approval state labels, only I and the tursted members have permission to do so. You can approve it in internal platform. DetailsIn response to adding label named cherry-pick-approved. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
ti-chi-bot
Bot
added
do-not-merge/cherry-pick-not-approved
and removed
cherry-pick-approved
AilinKid
added
release-note
ti-chi-bot
Bot
added
do-not-merge/release-note-label-needed
AilinKid
added
the
release-note
ti-chi-bot
Bot
removed
the
release-note
AilinKid
added
release-note
ti-chi-bot
Bot
added
do-not-merge/release-note-label-needed
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hawkingrei, qw4990 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
ti-chi-bot
Bot
added
cherry-pick-approved
3 participants
Issue Number: ref #66676
What changed
Cherry-pick the alternative logical plan support onto
release-8.5:The correlated planner test was adapted without copying upstream
testdata; it uses inline test cases on this branch.Tests
GOCACHE=/tmp/index-join-refactor-go-build go test -tags=intest ./pkg/sessionctx/stmtctx ./pkg/util/context ./pkg/util/hint ./pkg/plannerGOCACHE=/tmp/index-join-refactor-go-build go test -tags=intest ./pkg/planner/core -run ^GOCACHE=/tmp/index-join-refactor-go-build go test -tags=intest ./pkg/planner -run ^GOCACHE=/tmp/index-join-refactor-go-build go test -tags=intest ./pkg/sessionctx/variable -run 'TestNewSessionVars|TestVarsutil'GOCACHE=/tmp/index-join-refactor-go-build go test -tags=intest ./pkg/planner/core/casetest/correlatedGOCACHE=/tmp/index-join-refactor-go-build go test -run TestOptimizeHintOnPartitionTable -tags=intest,deadlock ./pkg/planner/core/casetest/hintRelease note
Summary by CodeRabbit
New Features
tidb_opt_enable_alternative_logical_planssession variable to enable alternative logical optimization plans for subquery decorrelation scenarios.Tests
NATURAL JOINwith correlated subqueries.Chores