pkg/planner, pkg/sessionctx: support non-decorrelate alternative#66995
Merged
ti-chi-bot[bot] merged 2 commits intoMar 25, 2026
Conversation
ti-chi-bot
Bot
added
the
release-note-none
|
Review failed due to infrastructure/execution failure after retries. Please re-trigger review. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
sig/planner
|
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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an optional second logical-plan optimization round driven by a decorrelate-change signal, snapshots/restores planner state across rounds, threads decorrelate/outer-driven-index-join signals through optimization, marks decorrelated semi-apply joins for index-join generation, and adds tests/fixtures plus a session toggle to enable alternative logical-plan rounds. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Optimizer as Optimizer (optimize)
participant LogicalOpt as Logical Optimizer
participant Decorrelate as Decorrelate Solver
participant PhysicalOpt as Physical Optimizer
Client->>Optimizer: optimize(logicalPlan)
Optimizer->>Optimizer: PrepareLogicalPlanBuildState()\nSaveLogicalPlanBuildBaseline()
Optimizer->>LogicalOpt: buildAndOptimizeLogicalPlanRound(round=0)
LogicalOpt->>Decorrelate: apply decorrelation rules
Decorrelate-->>LogicalOpt: decorrelateChanged (true/false)
LogicalOpt-->>Optimizer: (logicalPlan, decorrelateChanged, foundEquivalentOuterDrivenIndexJoin)
Optimizer->>PhysicalOpt: physical optimization (constructIndexJoin uses FromDecorrelatedSimpleSemiApply)
PhysicalOpt-->>Optimizer: physicalPlan, cost
alt decorrelateChanged && !foundEquivalentOuterDrivenIndexJoin && EnableAlternativeLogicalPlans
Optimizer->>Optimizer: RestoreLogicalPlanBuildBaseline()
Optimizer->>LogicalOpt: buildAndOptimizeLogicalPlanRound(round=withoutDecorrelate)
LogicalOpt-->>Optimizer: alternativeLogicalPlan
Optimizer->>PhysicalOpt: physical optimization (alternative)
PhysicalOpt-->>Optimizer: alternativePhysicalPlan, cost2
Optimizer->>Optimizer: choose lower-cost physical plan
end
Optimizer-->>Client: finalPhysicalPlan, cost
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Member
|
/ok-to-test |
AilinKid
changed the title
Contributor
Author
|
/test all |
hawkingrei
reviewed
Mar 13, 2026
| // allow change variables (otherwise can't unset read-only mode) | ||
| case *ast.SetStmt, | ||
| // allow analyze table | ||
| // allow analyze table |
Member
There was a problem hiding this comment.
I think you should run gofmt.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/planner/optimize.go (1)
495-550:⚠️ Potential issue | 🟡 MinorOnly the winner round should emit unused-view warnings.
Lines 495-496 still defer
HandleUnusedViewHints()for every round. Withtidb_opt_enable_alternative_logical_plans=on, the same AST is built twice, so unused view-hint warnings will be appended twice even though only one round can win. The winner snapshot at Lines 549-550 is also taken before that defer fires, so replaying warnings from the chosen round would be safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/optimize.go` around lines 495 - 550, The defer builder.HandleUnusedViewHints() is running for every optimization round and causing duplicate unused-view warnings; remove that unconditional defer and instead invoke builder.HandleUnusedViewHints() only when the round becomes the winner (inside the if block that updates *bestPlan / *bestCost / *bestNames), and also call it before returning a non-logical plan (where p is not a base.LogicalPlan) so warnings are emitted only for the chosen plan; use the existing symbols builder.HandleUnusedViewHints(), buildLogicalPlan, and the bestPlan/bestCost update path (and take care to call it after you save/clone any logical-plan context like bestLogicalPlanCtx so the winner snapshot is preserved).
🧹 Nitpick comments (1)
pkg/sessionctx/vardef/tidb_vars.go (1)
337-340: Document the optimization CPU trade-off in this variable comment.Line 337 explains behavior, but not the known extra optimization CPU overhead when enabled.
As per coding guidelines "Comments SHOULD explain non-obvious intent, constraints, invariants, concurrency guarantees, SQL/compatibility contracts, or important performance trade-offs".✏️ Suggested comment update
- // TiDBOptEnableAlternativeLogicalPlans controls whether the optimizer evaluates - // extra logical-plan alternatives and chooses the lower-cost plan. + // TiDBOptEnableAlternativeLogicalPlans controls whether the optimizer evaluates + // extra logical-plan alternatives and chooses the lower-cost plan. + // Enabling it may increase optimization CPU due to extra planning rounds. TiDBOptEnableAlternativeLogicalPlans = "tidb_opt_enable_alternative_logical_plans"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/sessionctx/vardef/tidb_vars.go` around lines 337 - 340, Update the comment for the TiDBOptEnableAlternativeLogicalPlans variable to document the CPU/performance trade-off: explicitly state that enabling extra logical-plan alternatives can increase optimizer CPU usage and planning latency (especially on complex queries or high-concurrency workloads), and note any situations where the extra planning cost is expected to outweigh runtime gains; edit the comment block above the TiDBOptEnableAlternativeLogicalPlans constant to include this guidance and any relevant caveats for operators.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/planner/optimize.go`:
- Around line 495-550: The defer builder.HandleUnusedViewHints() is running for
every optimization round and causing duplicate unused-view warnings; remove that
unconditional defer and instead invoke builder.HandleUnusedViewHints() only when
the round becomes the winner (inside the if block that updates *bestPlan /
*bestCost / *bestNames), and also call it before returning a non-logical plan
(where p is not a base.LogicalPlan) so warnings are emitted only for the chosen
plan; use the existing symbols builder.HandleUnusedViewHints(),
buildLogicalPlan, and the bestPlan/bestCost update path (and take care to call
it after you save/clone any logical-plan context like bestLogicalPlanCtx so the
winner snapshot is preserved).
---
Nitpick comments:
In `@pkg/sessionctx/vardef/tidb_vars.go`:
- Around line 337-340: Update the comment for the
TiDBOptEnableAlternativeLogicalPlans variable to document the CPU/performance
trade-off: explicitly state that enabling extra logical-plan alternatives can
increase optimizer CPU usage and planning latency (especially on complex queries
or high-concurrency workloads), and note any situations where the extra planning
cost is expected to outweigh runtime gains; edit the comment block above the
TiDBOptEnableAlternativeLogicalPlans constant to include this guidance and any
relevant caveats for operators.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 023ba60e-6618-4b2c-a7bd-5902206cd0c2
📒 Files selected for processing (17)
pkg/planner/core/casetest/correlated/correlated_test.gopkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_in.jsonpkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_out.jsonpkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_xut.jsonpkg/planner/core/casetest/correlated/testdata/fixture_a.sqlpkg/planner/core/casetest/correlated/testdata/fixture_b.sqlpkg/planner/core/casetest/correlated/testdata/fixture_c.sqlpkg/planner/core/casetest/correlated/testdata/fixture_db.fixture_a.jsonpkg/planner/core/casetest/correlated/testdata/fixture_db.fixture_b.jsonpkg/planner/core/casetest/correlated/testdata/fixture_db.fixture_c.jsonpkg/planner/core/integration_test.gopkg/planner/optimize.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/session.gopkg/sessionctx/variable/setvar_affect.gopkg/sessionctx/variable/sysvar.gopkg/sessionctx/variable/varsutil_test.go
hawkingrei
reviewed
Mar 13, 2026
hawkingrei
reviewed
Mar 13, 2026
|
|
||
| tk.MustExec("set @@tidb_opt_enable_alternative_logical_plans=on") | ||
| alternativePlan := tk.MustQuery("explain format='brief' select t1.a, (select count(*) from t2 where t2.a = t1.a) as cnt from t1 order by t1.a").Rows() | ||
| require.Equal(t, hintedPlan, alternativePlan, caller+": enabled alternative round should pick the lower-cost undecorrelated plan") |
Member
There was a problem hiding this comment.
Can we move this test case into casetest/correlated
guo-shaoge
approved these changes
Mar 13, 2026
| return optFlag &^ rule.FlagDecorrelate | ||
| } | ||
| return optFlag | ||
| } |
Collaborator
There was a problem hiding this comment.
I think we need figure out a better way so it's easier to extend to more round, instead of using hard-coded int value. We can do that in the future.
Contributor
Author
|
/retest-required |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #66995 +/- ##
================================================
- Coverage 77.7962% 77.6006% -0.1957%
================================================
Files 2022 1942 -80
Lines 554862 542444 -12418
================================================
- Hits 431662 420940 -10722
- Misses 121455 121502 +47
+ Partials 1745 2 -1743
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ti-chi-bot
Bot
added
size/XXL
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/planner/optimize.go (1)
477-492: Signature expanded with multiple boolean signals.The function now returns
(base.Plan, types.NameSlice, nonLogical bool, decorrelateChanged bool, foundEquivalentOuterDrivenIndexJoin bool, error). Consider grouping these related signals into a small result struct for clarity and future extensibility, rather than having 6 return values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/optimize.go` around lines 477 - 492, The function buildAndOptimizeLogicalPlanRound currently returns six separate values (base.Plan, types.NameSlice, two/three booleans, and error); refactor by introducing a small result struct (e.g., LogicalPlanRoundResult) that groups these outputs as named fields (Plan base.Plan, Names types.NameSlice, NonLogical bool, DecorrelateChanged bool, FoundEquivalentOuterDrivenIndexJoin bool, Err error), update the buildAndOptimizeLogicalPlanRound signature to return that struct (and remove the multi-value return), and update all call sites to use the struct fields instead of positional returns; ensure you also update any documentation/comments and preserve existing semantics and error handling when populating and returning the new struct from buildAndOptimizeLogicalPlanRound.pkg/planner/core/optimizer.go (1)
1005-1013: Magic string comparison for rule name is fragile.The condition
rule.Name() == "decorrelate"relies on a string literal. If theDecorrelateSolver.Name()method's return value changes, this logic would silently break. Consider using a constant or comparing against the rule type directly.♻️ Suggested approach
Define a constant in the rule package and use it consistently:
// In pkg/planner/core/rule/constants.go or similar const DecorrelateRuleName = "decorrelate"Then use:
-if planChanged && rule.Name() == "decorrelate" { +if planChanged && rule.Name() == rule.DecorrelateRuleName {Alternatively, use a type assertion if feasible.
Also applies to: 1041-1043
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/optimizer.go` around lines 1005 - 1013, Replace the fragile string literal check rule.Name() == "decorrelate" in the Optimize loop with a stable identifier: either compare against a package-level constant (e.g., rule.DecorrelateRuleName) defined next to DecorrelateSolver.Name(), or perform a type assertion/check against the concrete rule type (e.g., _, ok := rule.(*DecorrelateSolver)) to detect the decorrelate rule; update both occurrences (the one after rule.Optimize and the later similar check) to use the chosen approach so the logic no longer depends on a magic string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/planner/core/casetest/correlated/correlated_test.go`:
- Around line 140-169: The test fixture function prepareAlternativePlanFixture
relies on the default of the session variable
tidb_opt_enable_alternative_logical_plans, making recorded plans flaky; set the
session variable explicitly at the start of prepareAlternativePlanFixture using
the testkit (e.g. call tk.MustExec to set session
tidb_opt_enable_alternative_logical_plans=ON or =1) so the alternative-plan
behavior is enabled for the entire fixture setup and subsequent plan generation;
also consider restoring the prior value or setting it per-test if other tests
expect a different default.
In `@pkg/planner/core/integration_test.go`:
- Around line 1772-1795: Before asserting plans in
TestCorrelatedSubqueryChoosesLowerCostApplyPlan and the two nearby tests, enable
the alternative-plans feature by setting the session variable
tidb_opt_enable_alternative_logical_plans to ON; specifically, add a session set
(e.g. via tk.MustExec("set session
tidb_opt_enable_alternative_logical_plans=1")) early in the test body (before
building hintedSQL/defaultExplainSQL and running explain/select checks) and
apply the same addition to the other two tests around lines 1799-1900 so the
assertions exercise the intended alternative-plan behavior.
---
Nitpick comments:
In `@pkg/planner/core/optimizer.go`:
- Around line 1005-1013: Replace the fragile string literal check rule.Name() ==
"decorrelate" in the Optimize loop with a stable identifier: either compare
against a package-level constant (e.g., rule.DecorrelateRuleName) defined next
to DecorrelateSolver.Name(), or perform a type assertion/check against the
concrete rule type (e.g., _, ok := rule.(*DecorrelateSolver)) to detect the
decorrelate rule; update both occurrences (the one after rule.Optimize and the
later similar check) to use the chosen approach so the logic no longer depends
on a magic string.
In `@pkg/planner/optimize.go`:
- Around line 477-492: The function buildAndOptimizeLogicalPlanRound currently
returns six separate values (base.Plan, types.NameSlice, two/three booleans, and
error); refactor by introducing a small result struct (e.g.,
LogicalPlanRoundResult) that groups these outputs as named fields (Plan
base.Plan, Names types.NameSlice, NonLogical bool, DecorrelateChanged bool,
FoundEquivalentOuterDrivenIndexJoin bool, Err error), update the
buildAndOptimizeLogicalPlanRound signature to return that struct (and remove the
multi-value return), and update all call sites to use the struct fields instead
of positional returns; ensure you also update any documentation/comments and
preserve existing semantics and error handling when populating and returning the
new struct from buildAndOptimizeLogicalPlanRound.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f0204706-4db3-444f-9152-c37d15012af6
📒 Files selected for processing (13)
pkg/planner/core/casetest/correlated/correlated_test.gopkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_out.jsonpkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_xut.jsonpkg/planner/core/exhaust_physical_plans.gopkg/planner/core/integration_test.gopkg/planner/core/operator/logicalop/logical_join.gopkg/planner/core/operator/physicalop/physical_index_join.gopkg/planner/core/optimizer.gopkg/planner/core/rule_decorrelate.gopkg/planner/core/task.gopkg/planner/optimize.gopkg/sessionctx/stmtctx/stmtctx.gopkg/sessionctx/stmtctx/stmtctx_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_xut.json
- pkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_out.json
Contributor
Author
|
I reran the planner micro-benchmark after the latest tuning, against the latest Setup:
Median
Median
I also rechecked the existing failpoint regressions separately: |
Contributor
Author
|
I benchmarked two scenarios against the latest
With 200k rows in each fixture table and only 50 qualifying outer rows, execution improved from So the tradeoff is now clearer:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/planner/optimize.go (1)
542-543:⚠️ Potential issue | 🟡 MinorCapture order drops winner-round unused-view warnings.
bestLogicalPlanCtxis captured before deferredbuilder.HandleUnusedViewHints()runs. On Line 699 restore, winner warnings from that deferred call are discarded.Suggested fix direction
- defer builder.HandleUnusedViewHints() + handledUnusedViewHints := false + defer func() { + if !handledUnusedViewHints { + builder.HandleUnusedViewHints() + } + }() ... if *bestPlan == nil || cost < *bestCost { *bestCost = cost *bestPlan = finalPlan *bestNames = names + builder.HandleUnusedViewHints() + handledUnusedViewHints = true if bestLogicalPlanCtx != nil { *bestLogicalPlanCtx = saveLogicalPlanBuildCtx(sctx.GetSessionVars()) } }Also applies to: 596-599, 699-699
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/optimize.go` around lines 542 - 543, The deferred call to builder.HandleUnusedViewHints() runs after bestLogicalPlanCtx is captured, causing winner-round unused-view warnings to be lost; change the flow so HandleUnusedViewHints() executes (and its results are preserved) before capturing bestLogicalPlanCtx — either remove/defer and call builder.HandleUnusedViewHints() explicitly at the appropriate point (prior to assigning bestLogicalPlanCtx) or invoke the deferred function immediately and capture its returned warnings into a variable that is then merged into bestLogicalPlanCtx; update all similar sites (the other occurrences around the builder and restore/restoreWinner logic) to ensure HandleUnusedViewHints runs before bestLogicalPlanCtx is set.
♻️ Duplicate comments (2)
pkg/planner/core/casetest/correlated/correlated_test.go (1)
111-113:⚠️ Potential issue | 🟠 MajorSet
tidb_opt_enable_alternative_logical_plansexplicitly before asserting Apply plans.Line 111 and Line 141 tests assert automatic Apply-plan selection, but the feature is opt-in. Relying on session defaults makes this regression brittle across environments/runs.
Suggested fix
func TestCorrelatedSubqueryChoosesLowerCostApplyPlan(t *testing.T) { testkit.RunTestUnderCascades(t, func(t *testing.T, tk *testkit.TestKit, cascades, caller string) { tk.MustExec("use test") + tk.MustExec("set @@session.tidb_opt_enable_alternative_logical_plans = 1") tk.MustExec("drop table if exists t1, t2") tk.MustExec("create table t1(a int primary key)") tk.MustExec("create table t2(a int, b int, key idx_a(a))") @@ func prepareAlternativePlanFixture(t *testing.T, tk *testkit.TestKit, dom *domain.Domain) { t.Helper() + tk.MustExec("set @@session.tidb_opt_enable_alternative_logical_plans = 1") tk.MustExec("drop database if exists fixture_db") tk.MustExec("create database fixture_db")Based on learnings: Applies to
**/*_test.go— “Keep test changes minimal and deterministic; avoid broad golden/testdata churn unless required.”Also applies to: 141-147, 182-188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/casetest/correlated/correlated_test.go` around lines 111 - 113, The tests rely on the opt-in feature tidb_opt_enable_alternative_logical_plans but never enable it, making Apply-plan assertions flaky; update the test body executed by RunTestUnderCascadesWithDomain to explicitly set the session variable on the TestKit (tk) before calling prepareAlternativePlanFixture and before any Apply-plan assertions (e.g., use tk.MustExec("set @@session.tidb_opt_enable_alternative_logical_plans = 1") or the TestKit API your suite uses), and mirror this change for the other blocks referencing Apply assertions (the blocks around prepareAlternativePlanFixture and the later assertion groups) so the tests are deterministic.pkg/planner/core/integration_test.go (1)
1771-1799:⚠️ Potential issue | 🟠 MajorEnable alternative-logical-plan switch in these tests.
Both tests validate “skip extra alternative round” behavior, but they never turn on
tidb_opt_enable_alternative_logical_plans. As written, they can pass vacuously when the feature is disabled.Suggested fix
func TestSimpleExistsSkipsAlternativeRoundWhenDecorrelatedIndexJoinExists(t *testing.T) { testkit.RunTestUnderCascades(t, func(t *testing.T, tk *testkit.TestKit, cascades, caller string) { tk.MustExec("use test") + tk.MustExec("set @@session.tidb_opt_enable_alternative_logical_plans = 1") tk.MustExec("drop table if exists t1, t2")func TestScalarExistsSkipsAlternativeRoundWhenDecorrelatedIndexJoinExists(t *testing.T) { testkit.RunTestUnderCascades(t, func(t *testing.T, tk *testkit.TestKit, cascades, caller string) { tk.MustExec("use test") + tk.MustExec("set @@session.tidb_opt_enable_alternative_logical_plans = 1") tk.MustExec("drop table if exists t1, t2")Also applies to: 1801-1874
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/integration_test.go` around lines 1771 - 1799, The tests TestSimpleExistsSkipsAlternativeRoundWhenDecorrelatedIndexJoinExists (and the other test in the 1801-1874 range) never enable the alternative-logical-plan session switch, so they may pass vacuously; update each test to set tidb_opt_enable_alternative_logical_plans=1 at the start of the test session (use the TestKit instance tk, e.g. tk.MustExec to set the session variable) and restore it (or set it back to 0) in a defer if necessary so the rest of the suite is unaffected; place the tk.MustExec call before creating tables / running the EXPLAIN and before enabling the failpoint referenced by failIfAlternativeLogicalPlanRoundTriggered to ensure the alternative-plan path is active.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/sessionctx/stmtctx/stmtctx.go`:
- Around line 744-752: PrepareFreshOptimizeState currently clears many fields
but omits resetting the PlanCacheTracker and RangeFallbackHandler, allowing
plan-cache flags/reasons to leak across Optimize calls; update
StatementContext.PrepareFreshOptimizeState to reinitialize or reset the
PlanCacheTracker and RangeFallbackHandler the same way Reset does (e.g., call
the same reset/reinit logic used in Reset or set sc.PlanCacheTracker and
sc.RangeFallbackHandler to fresh instances/zero values) so that PlanCacheTracker
and RangeFallbackHandler state do not persist between fresh optimize baselines.
---
Outside diff comments:
In `@pkg/planner/optimize.go`:
- Around line 542-543: The deferred call to builder.HandleUnusedViewHints() runs
after bestLogicalPlanCtx is captured, causing winner-round unused-view warnings
to be lost; change the flow so HandleUnusedViewHints() executes (and its results
are preserved) before capturing bestLogicalPlanCtx — either remove/defer and
call builder.HandleUnusedViewHints() explicitly at the appropriate point (prior
to assigning bestLogicalPlanCtx) or invoke the deferred function immediately and
capture its returned warnings into a variable that is then merged into
bestLogicalPlanCtx; update all similar sites (the other occurrences around the
builder and restore/restoreWinner logic) to ensure HandleUnusedViewHints runs
before bestLogicalPlanCtx is set.
---
Duplicate comments:
In `@pkg/planner/core/casetest/correlated/correlated_test.go`:
- Around line 111-113: The tests rely on the opt-in feature
tidb_opt_enable_alternative_logical_plans but never enable it, making Apply-plan
assertions flaky; update the test body executed by
RunTestUnderCascadesWithDomain to explicitly set the session variable on the
TestKit (tk) before calling prepareAlternativePlanFixture and before any
Apply-plan assertions (e.g., use tk.MustExec("set
@@session.tidb_opt_enable_alternative_logical_plans = 1") or the TestKit API
your suite uses), and mirror this change for the other blocks referencing Apply
assertions (the blocks around prepareAlternativePlanFixture and the later
assertion groups) so the tests are deterministic.
In `@pkg/planner/core/integration_test.go`:
- Around line 1771-1799: The tests
TestSimpleExistsSkipsAlternativeRoundWhenDecorrelatedIndexJoinExists (and the
other test in the 1801-1874 range) never enable the alternative-logical-plan
session switch, so they may pass vacuously; update each test to set
tidb_opt_enable_alternative_logical_plans=1 at the start of the test session
(use the TestKit instance tk, e.g. tk.MustExec to set the session variable) and
restore it (or set it back to 0) in a defer if necessary so the rest of the
suite is unaffected; place the tk.MustExec call before creating tables / running
the EXPLAIN and before enabling the failpoint referenced by
failIfAlternativeLogicalPlanRoundTriggered to ensure the alternative-plan path
is active.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b397757d-f122-4bae-9997-2adc2780b562
📒 Files selected for processing (10)
pkg/planner/core/casetest/correlated/correlated_test.gopkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_in.jsonpkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_out.jsonpkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_xut.jsonpkg/planner/core/exhaust_physical_plans.gopkg/planner/core/integration_test.gopkg/planner/core/physical_plan_test.gopkg/planner/optimize.gopkg/sessionctx/stmtctx/stmtctx.gopkg/sessionctx/stmtctx/stmtctx_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_in.json
- pkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_out.json
- pkg/planner/core/exhaust_physical_plans.go
Contributor
Author
|
Addressed the latest CodeRabbit follow-ups in
I kept the round-selection design itself unchanged for now; the current |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/planner/optimize.go (1)
472-480:⚠️ Potential issue | 🟠 MajorBaseline snapshot copies slice/map references instead of cloning.
mapScalarSubQ(slice) andmapHashCode2UniqueID(map) are copied by reference, not cloned. When round 0 modifies these collections, the baseline copy is also modified since they share the same underlying data. This defeats the purpose of baseline snapshotting for multi-round optimization.Compare with
CaptureLogicalPlanBuildState()(context snippet 3) which properly usesmaps.Clone()for its map fields.🐛 Proposed fix to clone the collections
func saveLogicalPlanBuildBaseline(sessVars *variable.SessionVars) logicalPlanBuildBaseline { return logicalPlanBuildBaseline{ stmtCtxState: sessVars.StmtCtx.SaveLogicalPlanBuildBaseline(), plannerSelectBlockAsName: sessVars.PlannerSelectBlockAsName.Load(), - mapScalarSubQ: sessVars.MapScalarSubQ, - mapHashCode2UniqueID: sessVars.MapHashCode2UniqueID4ExtendedCol, + mapScalarSubQ: slices.Clone(sessVars.MapScalarSubQ), + mapHashCode2UniqueID: maps.Clone(sessVars.MapHashCode2UniqueID4ExtendedCol), rewritePhaseInfo: sessVars.RewritePhaseInfo, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/optimize.go` around lines 472 - 480, saveLogicalPlanBuildBaseline currently copies the slice/map references into the logicalPlanBuildBaseline causing shared mutation; modify saveLogicalPlanBuildBaseline so it deep-clones the collections before returning (clone mapScalarSubQ slice into a new slice and clone mapHashCode2UniqueID into a new map using the same approach as CaptureLogicalPlanBuildState(), e.g., using maps.Clone for maps and an explicit copy for slices), ensuring the returned logicalPlanBuildBaseline has independent copies of those fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/planner/optimize.go`:
- Around line 462-470: saveLogicalPlanBuildCtx currently assigns
sessVars.MapScalarSubQ and sessVars.MapHashCode2UniqueID4ExtendedCol by
reference into the returned logicalPlanBuildCtx, which can lead to
shared-mutation bugs; update saveLogicalPlanBuildCtx to deep-copy those maps
(create new maps and copy entries) when populating mapScalarSubQ and
mapHashCode2UniqueID so the logicalPlanBuildCtx owns independent copies, leaving
other fields (stmtCtxState via CaptureLogicalPlanBuildState,
plannerSelectBlockAsName, rewritePhaseInfo) unchanged.
---
Outside diff comments:
In `@pkg/planner/optimize.go`:
- Around line 472-480: saveLogicalPlanBuildBaseline currently copies the
slice/map references into the logicalPlanBuildBaseline causing shared mutation;
modify saveLogicalPlanBuildBaseline so it deep-clones the collections before
returning (clone mapScalarSubQ slice into a new slice and clone
mapHashCode2UniqueID into a new map using the same approach as
CaptureLogicalPlanBuildState(), e.g., using maps.Clone for maps and an explicit
copy for slices), ensuring the returned logicalPlanBuildBaseline has independent
copies of those fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fb34eb1f-fde5-4ce5-b8b0-2f06fc3715ab
📒 Files selected for processing (7)
pkg/planner/core/optimizer.gopkg/planner/core/physical_plan_test.gopkg/planner/core/rule_decorrelate.gopkg/planner/optimize.gopkg/sessionctx/stmtctx/stmtctx.gopkg/sessionctx/stmtctx/stmtctx_test.gopkg/sessionctx/vardef/tidb_vars.go
✅ Files skipped from review due to trivial changes (3)
- pkg/planner/core/physical_plan_test.go
- pkg/sessionctx/stmtctx/stmtctx_test.go
- pkg/sessionctx/stmtctx/stmtctx.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/sessionctx/vardef/tidb_vars.go
- pkg/planner/core/rule_decorrelate.go
Comment on lines
+462
to
+470
| func saveLogicalPlanBuildCtx(sessVars *variable.SessionVars) logicalPlanBuildCtx { | ||
| return logicalPlanBuildCtx{ | ||
| stmtCtxState: sessVars.StmtCtx.SaveLogicalPlanBuildState(), | ||
| stmtCtxState: sessVars.StmtCtx.CaptureLogicalPlanBuildState(), | ||
| plannerSelectBlockAsName: sessVars.PlannerSelectBlockAsName.Load(), | ||
| mapScalarSubQ: sessVars.MapScalarSubQ, | ||
| mapHashCode2UniqueID: sessVars.MapHashCode2UniqueID4ExtendedCol, | ||
| rewritePhaseInfo: sessVars.RewritePhaseInfo, | ||
| } | ||
| } |
There was a problem hiding this comment.
Same reference-copy issue exists in saveLogicalPlanBuildCtx.
This function also copies mapScalarSubQ and mapHashCode2UniqueID by reference. While CaptureLogicalPlanBuildState() properly clones its maps, the session-level collections are not cloned here.
🐛 Proposed fix
func saveLogicalPlanBuildCtx(sessVars *variable.SessionVars) logicalPlanBuildCtx {
return logicalPlanBuildCtx{
stmtCtxState: sessVars.StmtCtx.CaptureLogicalPlanBuildState(),
plannerSelectBlockAsName: sessVars.PlannerSelectBlockAsName.Load(),
- mapScalarSubQ: sessVars.MapScalarSubQ,
- mapHashCode2UniqueID: sessVars.MapHashCode2UniqueID4ExtendedCol,
+ mapScalarSubQ: slices.Clone(sessVars.MapScalarSubQ),
+ mapHashCode2UniqueID: maps.Clone(sessVars.MapHashCode2UniqueID4ExtendedCol),
rewritePhaseInfo: sessVars.RewritePhaseInfo,
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/planner/optimize.go` around lines 462 - 470, saveLogicalPlanBuildCtx
currently assigns sessVars.MapScalarSubQ and
sessVars.MapHashCode2UniqueID4ExtendedCol by reference into the returned
logicalPlanBuildCtx, which can lead to shared-mutation bugs; update
saveLogicalPlanBuildCtx to deep-copy those maps (create new maps and copy
entries) when populating mapScalarSubQ and mapHashCode2UniqueID so the
logicalPlanBuildCtx owns independent copies, leaving other fields (stmtCtxState
via CaptureLogicalPlanBuildState, plannerSelectBlockAsName, rewritePhaseInfo)
unchanged.
Contributor
Author
|
/retest-required |
1 similar comment
Contributor
Author
|
/retest-required |
ti-chi-bot
Bot
added
the
size/L
Contributor
Author
|
/test tidb_parser_test |
|
@AilinKid: The specified target(s) for The following commands are available to trigger optional jobs: Use 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. |
qw4990
approved these changes
Mar 25, 2026
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
yudongusa
approved these changes
Mar 25, 2026
yudongusa
left a comment
yudongusa
left a comment
There was a problem hiding this comment.
Please open a document PR to track
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: guo-shaoge, qw4990, yudongusa 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 |
Contributor
Author
|
/test-required |
Contributor
Author
|
/retest-required |
This was referenced Mar 25, 2026
13 tasks
This was referenced Apr 2, 2026
This was referenced May 7, 2026
AilinKid
added a commit
to AilinKid/tidb
that referenced
this pull request
May 14, 2026
AilinKid
added a commit
to AilinKid/tidb
that referenced
this pull request
May 14, 2026
AilinKid
added a commit
to AilinKid/tidb
that referenced
this pull request
May 15, 2026
AilinKid
added a commit
to AilinKid/tidb
that referenced
this pull request
May 15, 2026
13 tasks
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
5 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: ref #65710
Problem Summary:
The optimizer only explored one logical-plan shape, so correlated subqueries could not keep a non-decorrelated alternative and compare physical costs. The regression coverage for this path also depended on importing a full plan replayer.
What changed and how does it work?
tidb_opt_enable_alternative_logical_plansto gate alternative logical-plan explorationplanner.Optimize, when enabled, run an extra logical optimization round with decorrelate disabled, physically optimize both candidates, and keep the lower-cost planExample plans
A Customer Query Example:
tidb_opt_enable_alternative_logical_plans = OFFtidb_opt_enable_alternative_logical_plans = ONCheck List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
New Features
Tests
Chores