planner: build multi alternative logical plan from shared AST#66677
Conversation
ti-chi-bot
Bot
added
release-note-none
|
@AilinKid I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
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. |
📝 WalkthroughWalkthroughThreads a per-build QB hint state through planner/view resolution, moves transient hint bookkeeping out of handler fields into QBHintBuildState, adds snapshot/restore APIs for planner and plan-cache state, and updates tests and init flows to use the new state. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PlanBuilder
participant HintHandler
participant ViewResolver
participant StmtCtx
Client->>PlanBuilder: Start plan build
PlanBuilder->>HintHandler: NewBuildState() -> hintState
PlanBuilder->>StmtCtx: SaveLogicalPlanBuildState()
PlanBuilder->>ViewResolver: BuildDataSourceFromView(hintState)
ViewResolver->>HintHandler: GetCurrentStmtHints(hints, level, hintState)
ViewResolver->>HintHandler: MarkViewQBNameUsed(qbName, hintState)
ViewResolver-->>PlanBuilder: View plan
PlanBuilder->>HintHandler: HandleUnusedViewHints(hintState) (deferred)
PlanBuilder->>StmtCtx: RestoreLogicalPlanBuildState() (if needed)
PlanBuilder-->>Client: Complete plan build
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
|
Review Complete Findings: 1 issues ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
size/XL
AilinKid
changed the title
![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)
477-479: Defensively clear/guardhintStateto avoid stale state reuse.If
Init(...)is called withprocessor == nilafter a prior initialized run,b.hintStatecan remain stale.HandleUnusedViewHints()also assumes non-nil state when processor exists. A small defensive reset/guard avoids accidental leakage paths.Suggested patch
func (b *PlanBuilder) Init(sctx base.PlanContext, is infoschema.InfoSchema, processor *hint.QBHintHandler) (*PlanBuilder, []ast.HintTable) { @@ b.ctx = sctx b.is = is b.hintProcessor = processor + b.hintState = nil if processor != nil { b.hintState = processor.NewBuildState() } @@ func (b *PlanBuilder) HandleUnusedViewHints() { - if b.hintProcessor == nil { + if b.hintProcessor == nil || b.hintState == nil { return } b.hintProcessor.SetWarns(b.hintProcessor.HandleUnusedViewHints(b.hintState, nil)) }Also applies to: 521-527
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/planbuilder.go` around lines 477 - 479, Init currently only sets b.hintState when processor != nil, risking stale state if Init is later called with processor == nil; ensure b.hintState is defensively cleared when processor is nil by setting b.hintState = nil in the Init path where processor == nil, and similarly guard or clear before/after any code that uses hintState (e.g., HandleUnusedViewHints) so callers cannot assume non-nil state; reference the Init method, b.hintState field, processor.NewBuildState(), and HandleUnusedViewHints() when making the change.
🤖 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 555-560: The selection logic only updates winner when cost <
bestCost, which can leave bestPlan nil if the first successful candidate has a
non-finite/edge cost; change the update condition in the blocks that assign
bestCost/bestPlan/bestNames/bestState (the code that currently sets bestCost =
cost; bestPlan = finalPlan; bestNames = names; bestState =
captureLogicalPlanBuildBaseline(sessVars)) to always take the first successful
candidate or when cost < bestCost — e.g., if bestPlan == nil || cost < bestCost
then assign bestCost, bestPlan, bestNames and call
captureLogicalPlanBuildBaseline(sessVars); apply the same change to the other
similar block that currently uses only cost < bestCost so a valid plan is
selected even if its cost is non-finite.
In `@pkg/util/hint/hint_query_block.go`:
- Around line 269-273: The SetWarns method on QBHintHandler can panic if
p.warnHandler is nil; update SetWarns to guard before calling
p.warnHandler.SetHintWarning by checking if p.warnHandler == nil and returning
early (or skip warnings) when nil, so iterate warns only when warnHandler is
non-nil; reference QBHintHandler.SetWarns and the warnHandler field and
SetHintWarning method when making the change.
---
Nitpick comments:
In `@pkg/planner/core/planbuilder.go`:
- Around line 477-479: Init currently only sets b.hintState when processor !=
nil, risking stale state if Init is later called with processor == nil; ensure
b.hintState is defensively cleared when processor is nil by setting b.hintState
= nil in the Init path where processor == nil, and similarly guard or clear
before/after any code that uses hintState (e.g., HandleUnusedViewHints) so
callers cannot assume non-nil state; reference the Init method, b.hintState
field, processor.NewBuildState(), and HandleUnusedViewHints() when making the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2a11a716-ee81-4aed-bb8c-5dc8e3495185
📒 Files selected for processing (8)
pkg/planner/core/logical_plan_builder.gopkg/planner/core/planbuilder.gopkg/planner/optimize.gopkg/sessionctx/stmtctx/BUILD.bazelpkg/sessionctx/stmtctx/stmtctx.gopkg/sessionctx/stmtctx/stmtctx_test.gopkg/util/context/plancache.gopkg/util/hint/hint_query_block.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #66677 +/- ##
================================================
+ Coverage 77.6714% 77.6809% +0.0094%
================================================
Files 2008 1929 -79
Lines 549230 538988 -10242
================================================
- Hits 426595 418691 -7904
+ Misses 120964 120285 -679
+ Partials 1671 12 -1659
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
|
/retest-required |
|
@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. |
AilinKid
force-pushed
the
ast-multi-logical-plan
branch
from
918214e to
d7b27b5
Compare
|
Review Complete Findings: 1 issues ℹ️ Learn more details on Pantheon AI. |
|
Review Complete Findings: 0 issues ℹ️ Learn more details on Pantheon AI. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/util/hint/hint_query_block.go (1)
269-273:⚠️ Potential issue | 🟠 MajorGuard
SetWarnsagainst nil receiver/warn handler to avoid panic.At Line 272,
p.warnHandler.SetHintWarning(one)can panic whenp == nilorp.warnHandler == nil.Suggested fix
func (p *QBHintHandler) SetWarns(warns []string) { + if p == nil || p.warnHandler == nil || len(warns) == 0 { + return + } for _, one := range warns { p.warnHandler.SetHintWarning(one) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/util/hint/hint_query_block.go` around lines 269 - 273, Guard SetWarns against a nil receiver or missing warn handler: in QBHintHandler.SetWarns check if p == nil or p.warnHandler == nil and return early before iterating, then proceed to loop calling p.warnHandler.SetHintWarning(one); this prevents panics from dereferencing a nil QBHintHandler or its warnHandler.
🤖 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`:
- Line 608: The current use of maps.Clone(sc.TableStats) in
SaveLogicalPlanBuildState does a shallow copy and leaves pointers to mutable
*statistics.Table objects that can be modified later; replace the shallow clone
with a deep-copy that iterates sc.TableStats and calls
statistics.Table.CopyAs(...) for each value (using an appropriate CopyIntent) so
the saved tableStats snapshot contains independent copies; ensure the new map
stores the copied *statistics.Table instances instead of the original pointers.
---
Duplicate comments:
In `@pkg/util/hint/hint_query_block.go`:
- Around line 269-273: Guard SetWarns against a nil receiver or missing warn
handler: in QBHintHandler.SetWarns check if p == nil or p.warnHandler == nil and
return early before iterating, then proceed to loop calling
p.warnHandler.SetHintWarning(one); this prevents panics from dereferencing a nil
QBHintHandler or its warnHandler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cc763ff8-7f42-4e8b-b72d-abb32adebb46
📒 Files selected for processing (7)
pkg/planner/core/logical_plan_builder.gopkg/planner/core/planbuilder.gopkg/sessionctx/stmtctx/BUILD.bazelpkg/sessionctx/stmtctx/stmtctx.gopkg/sessionctx/stmtctx/stmtctx_test.gopkg/util/context/plancache.gopkg/util/hint/hint_query_block.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/sessionctx/stmtctx/stmtctx_test.go
- pkg/planner/core/planbuilder.go
- pkg/util/context/plancache.go
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: guo-shaoge, qw4990 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 |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
/ok-to-test |
|
/retest-required |
1 similar comment
|
/retest-required |
ti-chi-bot
Bot
added
the
needs-cherry-pick-release-8.5
|
In response to a cherrypick label: new pull request created to branch |
4 participants
What problem does this PR solve?
Issue Number: ref #66676
Problem Summary:
This work is split into two steps for shared-AST multi logical plan build support.
This PR is Step 1 only: isolate planner build state so repeated builds from the same AST can be added safely in the follow-up step.
What changed and how does it work?
Main work in this PR (Step 1):
QBHintHandlerinto:StatementContextlogical-plan-build snapshot/restore support for mutable planning fields used by planner build/optimize paths (including warnings, plan-cache tracker state, andTableStats).Not included in this PR:
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
Refactor
New Features
Tests