pkg/planner: build multiple logical plans from shared AST in optimize#66743
pkg/planner: build multiple logical plans from shared AST in optimize#66743AilinKid wants to merge 12 commits intopingcap:masterfrom
Conversation
ti-chi-bot
bot
added
the
release-note-none
|
Review Complete Findings: 2 issues ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
bot
added
size/L
|
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:
📝 WalkthroughWalkthroughRefactors optimizer to support per-round logical-plan build/optimization: adds snapshot/restore of per-build planner state, a build-and-optimize round helper, best-plan tracking, multi-round scaffolding (currently one round), timing instrumentation, and improved error handling for failed logical-plan builds. Changes
Sequence DiagramsequenceDiagram
participant AST as AST
participant Pool as PlanBuilderPool
participant Builder as PlanBuilder
participant Validator as Priv/Lock Check
participant Optimizer as core.DoOptimize
participant Tracker as BestPlanTracker
participant Restore as BaselineRestore
AST->>Pool: request PlanBuilder
Pool->>Builder: provide reusable builder
Builder->>Builder: build logical plan
Builder->>Builder: saveLogicalPlanBuildCtx
Builder->>Validator: run fast privilege/lock/mode checks
Builder->>Optimizer: submit logical plan for optimization
Optimizer->>Tracker: return optimized plan + cost
Tracker->>Tracker: compare & select best plan
Tracker->>Restore: if best, request restore of per-round context
Restore->>Builder: restoreLogicalPlanBuildCtx
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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)
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.5.0)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 |
![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
🤖 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 477-480: The DurationOptimizer.Total currently starts after the
AST hint walk and misses early returns, so change timing to cover the entire
optimize path by starting the timer before invoking
node.Node.Accept(hintProcessor) (the hintProcessor/hint.NewQBHintHandler call)
and ensure DurationOptimizer.Total is recorded on all exits (including early
returns) by using a deferred function that sets Total from the timer; apply this
to the block around PlanBuilder/optimizer flow so the timer starts before hint
handling and the defer guarantees Total is stored regardless of return path.
- Around line 441-456: The baseline currently captures only StmtCtx and
PlannerSelectBlockAsName in
captureLogicalPlanBuildBaseline/restoreLogicalPlanBuildBaseline but
buildLogicalPlan also resets per-build session state (SessionVars.MapScalarSubQ,
SessionVars.MapHashCode2UniqueID4ExtendedCol, and SessionVars.RewritePhaseInfo),
which will be left stale when needBaseline triggers multi-round selection;
update logicalPlanBuildBaseline to include those fields (or move them into a
round-local struct) and modify captureLogicalPlanBuildBaseline and
restoreLogicalPlanBuildBaseline to snapshot and restore
SessionVars.MapScalarSubQ, SessionVars.MapHashCode2UniqueID4ExtendedCol, and
SessionVars.RewritePhaseInfo so buildRound/needBaseline behavior returns a plan
with matching session-side artifacts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 37ab5b81-edfb-4b37-a54e-9c1aac3f832c
📒 Files selected for processing (1)
pkg/planner/optimize.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #66743 +/- ##
================================================
- Coverage 77.6885% 77.5464% -0.1422%
================================================
Files 2008 1930 -78
Lines 549664 540676 -8988
================================================
- Hits 427026 419275 -7751
- Misses 120978 121383 +405
+ Partials 1660 18 -1642
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}
pkg/planner/optimize.go
Outdated
| visitInfo := core.VisitInfo4PrivCheck(ctx, is, node.Node, builder.GetVisitInfo()) | ||
| if err := core.CheckPrivilege(activeRoles, pm, visitInfo); err != nil { | ||
| return nil, nil, 0, err | ||
| beginOpt := time.Now() |
There was a problem hiding this comment.
[P2] Optimize_time now includes rewrite/build time
Why: DurationOptimizer.Total is now started before buildLogicalPlan(), so slow-log Optimize_time necessarily overlaps Rewrite_time, misleading diagnostics and slow-log rule matching that treats the two as disjoint phases.
Evidence: beginOpt := time.Now() at pkg/planner/optimize.go:497 is set before buildLogicalPlan(...) is called; sessVars.DurationOptimizer.Total = time.Since(beginOpt) at :583 captures the full window including rewrite. Slow log prints both Rewrite_time (slow_log.go:398) and Optimize_time (slow_log.go:408) independently — and slow-log rule matching keys on DurationOptimizer.Total (slow_log.go:756). Previously beginOpt was set after the build step, keeping the two intervals disjoint.
Suggestion: Move beginOpt := time.Now() to after the final buildLogicalPlan(...) call (i.e. just before core.DoOptimize(...)), restoring the original semantics where Optimize_time covers only the optimization phase.
There was a problem hiding this comment.
Addressed in f89fdd9. Optimize_time now starts right before the first core.DoOptimize(...) call (not before logical build/rewrite), and is still recorded through defer only when optimization actually starts.
There was a problem hiding this comment.
⏳ @AilinKid I've received your follow-up on this review comment and will continue working on it. I'll update this comment when I have something to share.
ℹ️ Learn more details on Pantheon AI.
pkg/planner/optimize.go
Outdated
| names := p.OutputNames() | ||
| var err error | ||
| // todo: you can customize each round's special builder (like semi join rewrite or not by signal) | ||
| p, err = buildLogicalPlan(ctx, sctx, node, builder) |
There was a problem hiding this comment.
[P2] Unused view-hint (qb_name) warnings silently dropped on error paths
Why: The refactor removed the unconditional/deferred emission of unused qb_name view-hint warnings. Any early return before HandleUnusedViewHints() is reached silently loses user-visible diagnostics for failing statements — a regression from the previous behavior where emission ran unconditionally via defer.
Evidence: buildLogicalPlan(...) error at :516 returns immediately; subsequent error returns at :527, :532, :536, :554 also exit before the only builder.HandleUnusedViewHints() calls at :545/:557. PlanBuilder.HandleUnusedViewHints() (planbuilder.go:521) is the sole emission point that appends warnings into StmtCtx; if never invoked, the warning is lost entirely.
Suggestion: Add defer builder.HandleUnusedViewHints() immediately after builder is initialized inside the closure, restoring "warn even on error" semantics.
There was a problem hiding this comment.
Addressed earlier in bc1b396 by adding defer builder.HandleUnusedViewHints() inside per-round build/optimize handling, so unused qb_name warnings are emitted even on error paths.
There was a problem hiding this comment.
Thanks for the follow-up. Adding defer builder.HandleUnusedViewHints() inside the per-round closure is the right fix — it guarantees warning emission regardless of whether buildLogicalPlan, privilege checks, or DoOptimize returns an error. ✅
ℹ️ Learn more details on Pantheon AI.
|
/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. |
pkg/planner/optimize.go
Outdated
| if needBaseline { | ||
| restoreLogicalPlanBuildBaseline(sessVars, bestState) | ||
| } | ||
| sessVars.DurationOptimizer.Total = time.Since(beginOpt) |
There was a problem hiding this comment.
Old impl will update DurationOptimizer even got error, but new impl will not. So better put this in defer
There was a problem hiding this comment.
Addressed in bc1b396. DurationOptimizer.Total is now assigned in a deferred function, so it is recorded even when optimize returns with error.
pkg/planner/optimize.go
Outdated
| if err != nil { | ||
| return nil, nil, false, err | ||
| } | ||
| builder.HandleUnusedViewHints() |
There was a problem hiding this comment.
Guess we should have more fine-grained handling of HandleUnusedViewHints() , because multiple round of calling this function may cause many warnings.(But I think we can leave a TODO here and refine it later)
And also we should put it in derfer like the old code.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/planner/optimize.go (1)
471-480:⚠️ Potential issue | 🟡 MinorUnused view-hint warnings lost on error paths.
If
buildLogicalPlanor any subsequent check fails,builder.HandleUnusedViewHints()is never called, and user-visible warnings about unusedqb_nameview hints are silently dropped. The previous implementation useddeferto ensure warnings were emitted even on failure.Suggested fix: defer HandleUnusedViewHints
func buildAndOptimizeLogicalPlanRound( ... ) (base.Plan, types.NameSlice, bool, error) { builder := planBuilderPool.Get().(*core.PlanBuilder) defer planBuilderPool.Put(builder.ResetForReuse()) builder.Init(sctx, is, hintProcessor) + defer builder.HandleUnusedViewHints() // todo: you can customize each round's special builder (like semi join rewrite or not by signal) p, err := buildLogicalPlan(ctx, sctx, node, builder) if err != nil { return nil, nil, false, err } ... - // Handle the non-logical plan statement. - logic, isLogicalPlan := p.(base.LogicalPlan) - if !isLogicalPlan { - builder.HandleUnusedViewHints() - return p, names, true, nil - } + logic, isLogicalPlan := p.(base.LogicalPlan) + if !isLogicalPlan { + return p, names, true, nil + } ... - builder.HandleUnusedViewHints() if *bestPlan == nil || cost < *bestCost {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/optimize.go` around lines 471 - 480, After acquiring the builder from planBuilderPool, ensure builder.HandleUnusedViewHints() is deferred so unused view-hint warnings are emitted on all error paths; to run it before the builder is reset, keep the existing defer planBuilderPool.Put(builder.ResetForReuse()) and then immediately add defer builder.HandleUnusedViewHints() (place this before calling builder.Init and before calling buildLogicalPlan), so HandleUnusedViewHints runs prior to ResetForReuse even if buildLogicalPlan or subsequent checks return early.
🧹 Nitpick comments (1)
pkg/planner/optimize.go (1)
441-456: Incomplete state capture for multi-round restoration.When
buildRound > 1,restoreLogicalPlanBuildCtxwill be used to restore the winning round's state. However,logicalPlanBuildCtxonly capturesstmtCtxStateandplannerSelectBlockAsName, whilebuildLogicalPlanalso populatesSessionVars.MapScalarSubQ,SessionVars.MapHashCode2UniqueID4ExtendedCol, andSessionVars.RewritePhaseInfo. These fields will retain values from the last build round rather than the winning round.This is latent today since
buildRound = 1, but will cause incorrect session state when multi-round selection is enabled.Suggested fix
type logicalPlanBuildCtx struct { stmtCtxState stmtctx.LogicalPlanBuildState plannerSelectBlockAsName *[]ast.HintTable + mapScalarSubQ map[int]any + mapHashCode2UniqueID map[string]int + rewritePhaseInfo variable.RewritePhaseInfo } func saveLogicalPlanBuildCtx(sessVars *variable.SessionVars) logicalPlanBuildCtx { return logicalPlanBuildCtx{ stmtCtxState: sessVars.StmtCtx.SaveLogicalPlanBuildState(), plannerSelectBlockAsName: sessVars.PlannerSelectBlockAsName.Load(), + mapScalarSubQ: sessVars.MapScalarSubQ, + mapHashCode2UniqueID: sessVars.MapHashCode2UniqueID4ExtendedCol, + rewritePhaseInfo: sessVars.RewritePhaseInfo, } } func restoreLogicalPlanBuildCtx(sessVars *variable.SessionVars, logicalPlanCtx logicalPlanBuildCtx) { sessVars.StmtCtx.RestoreLogicalPlanBuildState(logicalPlanCtx.stmtCtxState) sessVars.PlannerSelectBlockAsName.Store(logicalPlanCtx.plannerSelectBlockAsName) + sessVars.MapScalarSubQ = logicalPlanCtx.mapScalarSubQ + sessVars.MapHashCode2UniqueID4ExtendedCol = logicalPlanCtx.mapHashCode2UniqueID + sessVars.RewritePhaseInfo = logicalPlanCtx.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 441 - 456, logicalPlanBuildCtx currently only saves stmtCtxState and plannerSelectBlockAsName, so when restoreLogicalPlanBuildCtx is used after multi-round builds it fails to revert SessionVars.MapScalarSubQ, SessionVars.MapHashCode2UniqueID4ExtendedCol and SessionVars.RewritePhaseInfo set by buildLogicalPlan; update logicalPlanBuildCtx to include fields for MapScalarSubQ, MapHashCode2UniqueID4ExtendedCol and RewritePhaseInfo, modify saveLogicalPlanBuildCtx to copy those SessionVars values into the new struct, and modify restoreLogicalPlanBuildCtx to Store/restore those three SessionVars from the saved logicalPlanBuildCtx so the winning round fully restores session state.
🤖 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 590-593: The early return for nonLogical plans skips recording
optimizer timing; before returning in the nonLogical branch of optimize (the
block that currently returns p, names, 0, nil), set
sessVars.DurationOptimizer.Total to the elapsed optimization duration by
computing time.Since(<the optimization start time>) (or capture time.Now() at
function entry into a startTime variable if none exists) so the duration is
recorded for DDL / non-logical statements; keep the existing return values
unchanged.
---
Duplicate comments:
In `@pkg/planner/optimize.go`:
- Around line 471-480: After acquiring the builder from planBuilderPool, ensure
builder.HandleUnusedViewHints() is deferred so unused view-hint warnings are
emitted on all error paths; to run it before the builder is reset, keep the
existing defer planBuilderPool.Put(builder.ResetForReuse()) and then immediately
add defer builder.HandleUnusedViewHints() (place this before calling
builder.Init and before calling buildLogicalPlan), so HandleUnusedViewHints runs
prior to ResetForReuse even if buildLogicalPlan or subsequent checks return
early.
---
Nitpick comments:
In `@pkg/planner/optimize.go`:
- Around line 441-456: logicalPlanBuildCtx currently only saves stmtCtxState and
plannerSelectBlockAsName, so when restoreLogicalPlanBuildCtx is used after
multi-round builds it fails to revert SessionVars.MapScalarSubQ,
SessionVars.MapHashCode2UniqueID4ExtendedCol and SessionVars.RewritePhaseInfo
set by buildLogicalPlan; update logicalPlanBuildCtx to include fields for
MapScalarSubQ, MapHashCode2UniqueID4ExtendedCol and RewritePhaseInfo, modify
saveLogicalPlanBuildCtx to copy those SessionVars values into the new struct,
and modify restoreLogicalPlanBuildCtx to Store/restore those three SessionVars
from the saved logicalPlanBuildCtx so the winning round fully restores session
state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5d0d7496-a53f-4049-9b75-7490d09adf29
📒 Files selected for processing (1)
pkg/planner/optimize.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/planner/optimize.go (1)
556-559:⚠️ Potential issue | 🟠 MajorKeep
DurationOptimizer.Totalscoped to the actual optimize phase.Starting the timer before the hint walk and logical-plan build makes
Optimize_timeoverlapRewrite_time, and it also reports a non-zero optimize duration on thenonLogicalpath even thoughcore.DoOptimizenever ran. Please accumulate this metric around eachcore.DoOptimizecall instead of around the whole round/build flow.Also applies to: 561-565, 601-603
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/optimize.go` around lines 556 - 559, The timer for sessVars.DurationOptimizer.Total is currently started too early (beginOpt) covering hint walk and logical-plan build and thus overlaps Rewrite_time and reports time on nonLogical paths; move the start/end timing to directly wrap each core.DoOptimize invocation (create a new begin time immediately before calling core.DoOptimize and add the elapsed duration to sessVars.DurationOptimizer.Total instead of overwriting it), ensure the nonLogical path does not touch DurationOptimizer, and apply the same change for the other similar timing sites referenced around the core.DoOptimize calls (the blocks at the other noted locations).
🧹 Nitpick comments (1)
pkg/planner/optimize.go (1)
482-482: Snapshot the winning round after unused view-hint warnings are materialized.
bestLogicalPlanCtxis saved before the deferredbuilder.HandleUnusedViewHints()runs. That is harmless withbuildRound == 1, but as soon as a second round is enabled the finalrestoreLogicalPlanBuildCtx()will revive the winner without its unusedqb_name/view-hint warnings. Either flush those warnings beforesaveLogicalPlanBuildCtx(), or persist them in the saved context.Also applies to: 531-533, 609-610
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/optimize.go` at line 482, The saved bestLogicalPlanCtx is captured before deferred builder.HandleUnusedViewHints() runs, causing the restored winner to miss its qb_name/view-hint warnings when buildRound > 1; fix by ensuring unused-view-hint warnings are materialized into the context before snapshotting—either call builder.HandleUnusedViewHints() (or otherwise flush/collect its warnings) prior to saveLogicalPlanBuildCtx(), or extend saveLogicalPlanBuildCtx()/bestLogicalPlanCtx to persist the builder's unused-view-hint warnings so restoreLogicalPlanBuildCtx() includes them; update the same pattern around the other occurrences noted (lines referenced near 531-533 and 609-610).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/planner/optimize.go`:
- Around line 556-559: The timer for sessVars.DurationOptimizer.Total is
currently started too early (beginOpt) covering hint walk and logical-plan build
and thus overlaps Rewrite_time and reports time on nonLogical paths; move the
start/end timing to directly wrap each core.DoOptimize invocation (create a new
begin time immediately before calling core.DoOptimize and add the elapsed
duration to sessVars.DurationOptimizer.Total instead of overwriting it), ensure
the nonLogical path does not touch DurationOptimizer, and apply the same change
for the other similar timing sites referenced around the core.DoOptimize calls
(the blocks at the other noted locations).
---
Nitpick comments:
In `@pkg/planner/optimize.go`:
- Line 482: The saved bestLogicalPlanCtx is captured before deferred
builder.HandleUnusedViewHints() runs, causing the restored winner to miss its
qb_name/view-hint warnings when buildRound > 1; fix by ensuring unused-view-hint
warnings are materialized into the context before snapshotting—either call
builder.HandleUnusedViewHints() (or otherwise flush/collect its warnings) prior
to saveLogicalPlanBuildCtx(), or extend
saveLogicalPlanBuildCtx()/bestLogicalPlanCtx to persist the builder's
unused-view-hint warnings so restoreLogicalPlanBuildCtx() includes them; update
the same pattern around the other occurrences noted (lines referenced near
531-533 and 609-610).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8bc83890-e134-4cc9-bd3c-1d8f48ec0f89
📒 Files selected for processing (1)
pkg/planner/optimize.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/planner/optimize.go (1)
482-534: Snapshot the winner context after unused view-hint warnings are applied.Once
buildRoundis raised above1,saveLogicalPlanBuildCtx()will captureStmtCtxbefore the deferredbuilder.HandleUnusedViewHints()runs. AfterrestoreLogicalPlanBuildCtx(), the winning round comes back without its own unusedqb_name/view-hint warnings. Please either snapshot after those warnings are appended, or delay emitting them until after the final winner is restored.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/optimize.go` around lines 482 - 534, The snapshot of the winning round's StmtCtx is taken before deferred builder.HandleUnusedViewHints() runs, so saved bestLogicalPlanCtx misses the winner's unused view-hint warnings; ensure the winner context is captured after those warnings are applied by invoking builder.HandleUnusedViewHints() (or moving the save) before calling saveLogicalPlanBuildCtx() when updating *bestLogicalPlanCtx (references: builder.HandleUnusedViewHints, saveLogicalPlanBuildCtx, restoreLogicalPlanBuildCtx, and the section that updates *bestPlan/*bestCost/*bestLogicalPlanCtx).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/planner/optimize.go`:
- Around line 482-534: The snapshot of the winning round's StmtCtx is taken
before deferred builder.HandleUnusedViewHints() runs, so saved
bestLogicalPlanCtx misses the winner's unused view-hint warnings; ensure the
winner context is captured after those warnings are applied by invoking
builder.HandleUnusedViewHints() (or moving the save) before calling
saveLogicalPlanBuildCtx() when updating *bestLogicalPlanCtx (references:
builder.HandleUnusedViewHints, saveLogicalPlanBuildCtx,
restoreLogicalPlanBuildCtx, and the section that updates
*bestPlan/*bestCost/*bestLogicalPlanCtx).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5989fe9c-c6c2-49dd-97ff-4575d440fce2
📒 Files selected for processing (1)
pkg/planner/optimize.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/planner/optimize.go (1)
467-481: Consider reducing parameter count for readability.The function has 14 parameters, several of which are output pointers. While functional, this makes the signature hard to read and reason about. When multi-round selection is enabled, consider grouping related state into structs (e.g., a
roundContextfor timing/checked flags, abestPlanStatefor winner tracking).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/optimize.go` around lines 467 - 481, The function buildAndOptimizeLogicalPlanRound has too many parameters; refactor by creating grouping structs to reduce the signature: introduce a roundContext (containing checked *bool, optimizeStarted *bool, beginOpt *time.Time, needRestoreLogicalPlanCtx bool) for per-round timing/flags and a bestPlanState (containing bestPlan *base.PhysicalPlan, bestNames *types.NameSlice, bestCost *float64, bestLogicalPlanCtx *logicalPlanBuildCtx) for winner tracking; replace the corresponding parameters in buildAndOptimizeLogicalPlanRound with these two structs (keeping ctx, sctx, node, is, hintProcessor as explicit inputs), update all call sites to construct and pass the new structs, and ensure internal uses reference the new struct fields instead of the previous standalone parameters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/planner/optimize.go`:
- Around line 467-481: The function buildAndOptimizeLogicalPlanRound has too
many parameters; refactor by creating grouping structs to reduce the signature:
introduce a roundContext (containing checked *bool, optimizeStarted *bool,
beginOpt *time.Time, needRestoreLogicalPlanCtx bool) for per-round timing/flags
and a bestPlanState (containing bestPlan *base.PhysicalPlan, bestNames
*types.NameSlice, bestCost *float64, bestLogicalPlanCtx *logicalPlanBuildCtx)
for winner tracking; replace the corresponding parameters in
buildAndOptimizeLogicalPlanRound with these two structs (keeping ctx, sctx,
node, is, hintProcessor as explicit inputs), update all call sites to construct
and pass the new structs, and ensure internal uses reference the new struct
fields instead of the previous standalone parameters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dc6904ef-4e95-4563-86e6-ba0fd87e8b29
📒 Files selected for processing (1)
pkg/planner/optimize.go
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: guo-shaoge 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:
|
|
@AilinKid: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
3 participants
What problem does this PR solve?
Issue Number: ref #66651
Problem Summary:
This PR implements step 2 of #66651.
Step 1 has already been merged; this step adds the actual multi-build flow in
Optimizefor rebuilding logical plans from a shared AST.What changed and how does it work?
pkg/planner/optimize.go.PlanBuilder.Check List
Tests
Unit test command:
go test -run TestOptimizeHintOnPartitionTable -tags=intest,deadlock ./pkg/planner/core/casetest/hintAdditional validation:
make bazel_lint_changedSide effects
Documentation
Release note
Summary by CodeRabbit
Performance Improvements
Reliability