Skip to content
Merged
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
146 changes: 112 additions & 34 deletions pkg/planner/optimize.go
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,23 @@ var planBuilderPool = sync.Pool{
},
}

type logicalPlanBuildBaseline struct {
stmtCtxState stmtctx.LogicalPlanBuildState
plannerSelectBlockAsName *[]ast.HintTable
}

func captureLogicalPlanBuildBaseline(sessVars *variable.SessionVars) logicalPlanBuildBaseline {
return logicalPlanBuildBaseline{
stmtCtxState: sessVars.StmtCtx.SaveLogicalPlanBuildState(),
plannerSelectBlockAsName: sessVars.PlannerSelectBlockAsName.Load(),
}
}

func restoreLogicalPlanBuildBaseline(sessVars *variable.SessionVars, baseline logicalPlanBuildBaseline) {
sessVars.StmtCtx.RestoreLogicalPlanBuildState(baseline.stmtCtxState)
sessVars.PlannerSelectBlockAsName.Store(baseline.plannerSelectBlockAsName)
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

// optimizeCnt is a global variable only used for test.
var optimizeCnt int

Expand All @@ -457,53 +474,114 @@ func optimize(ctx context.Context, sctx planctx.PlanContext, node *resolve.NodeW
})
sessVars := sctx.GetSessionVars()

// build logical plan
// Build the logical plan from the raw AST. The hint processor only keeps
// AST-derived metadata; per-build state is allocated inside PlanBuilder.
hintProcessor := hint.NewQBHintHandler(sctx.GetSessionVars().StmtCtx)
node.Node.Accept(hintProcessor)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
builder := planBuilderPool.Get().(*core.PlanBuilder)
defer planBuilderPool.Put(builder.ResetForReuse())
builder.Init(sctx, is, hintProcessor)
defer builder.HandleUnusedViewHints()
p, err := buildLogicalPlan(ctx, sctx, node, builder)
if err != nil {
return nil, nil, 0, err
}

// build multi logical plan from raw AST.
var (
buildRound = 1
needBaseline = buildRound > 1
Comment thread
AilinKid marked this conversation as resolved.
Outdated
bestCost = math.MaxFloat64
bestPlan base.PhysicalPlan
bestNames types.NameSlice
bestState logicalPlanBuildBaseline
checked bool
)
var baseline logicalPlanBuildBaseline
if needBaseline {
baseline = captureLogicalPlanBuildBaseline(sessVars)
}
activeRoles := sessVars.ActiveRoles
// Check privilege. Maybe it's better to move this to the Preprocess, but
// we need the table information to check privilege, which is collected
// into the visitInfo in the logical plan builder.
if pm := privilege.GetPrivilegeManager(sctx); pm != nil {
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()
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

@pantheon-ai pantheon-ai Bot Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

for i := range buildRound {
if needBaseline && i > 0 {
restoreLogicalPlanBuildBaseline(sessVars, baseline)
}
}

if err := core.CheckTableLock(sctx, is, builder.GetVisitInfo()); err != nil {
return nil, nil, 0, err
}
var (
p base.Plan
names types.NameSlice
nonLogical bool
)
err := func() error {
Comment thread
AilinKid marked this conversation as resolved.
Outdated
builder := planBuilderPool.Get().(*core.PlanBuilder)
defer planBuilderPool.Put(builder.ResetForReuse())

if err := core.CheckTableMode(node); err != nil {
return nil, nil, 0, err
}
builder.Init(sctx, is, hintProcessor)

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)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

@pantheon-ai pantheon-ai Bot Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if err != nil {
return err
}

// Handle the non-logical plan statement.
logic, isLogicalPlan := p.(base.LogicalPlan)
if !isLogicalPlan {
return p, names, 0, nil
}
names = p.OutputNames()
if !checked {
// Keep privilege and lock checks fail-fast. These depend on visitInfo
// produced by the logical build, but not on the later cost winner.
if pm := privilege.GetPrivilegeManager(sctx); pm != nil {
visitInfo := core.VisitInfo4PrivCheck(ctx, is, node.Node, builder.GetVisitInfo())
if err := core.CheckPrivilege(activeRoles, pm, visitInfo); err != nil {
return err
}
}

core.RecheckCTE(logic)
if err := core.CheckTableLock(sctx, is, builder.GetVisitInfo()); err != nil {
return err
}

beginOpt := time.Now()
finalPlan, cost, err := core.DoOptimize(ctx, sctx, builder.GetOptFlag(), logic)
// TODO: capture plan replayer here if it matches sql and plan digest
if err := core.CheckTableMode(node); err != nil {
return err
}
checked = true
}

// Handle the non-logical plan statement.
logic, isLogicalPlan := p.(base.LogicalPlan)
if !isLogicalPlan {
builder.HandleUnusedViewHints()
nonLogical = true
return nil
}

core.RecheckCTE(logic)

// todo: also you can customize each round's special logical opt flag here (like decorrelate rule or not)
finalPlan, cost, err := core.DoOptimize(ctx, sctx, builder.GetOptFlag(), logic)
if err != nil {
return err
}
builder.HandleUnusedViewHints()

if bestPlan == nil || cost < bestCost {
bestCost = cost
bestPlan = finalPlan
bestNames = names
if needBaseline {
bestState = captureLogicalPlanBuildBaseline(sessVars)
}
}
return nil
}()
if err != nil {
return nil, nil, 0, err
}
if nonLogical {
// keep compatible with the old.
return p, names, 0, nil
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
if bestPlan == nil {
return nil, nil, 0, errors.New("failed to build logical plan")
}
if needBaseline {
restoreLogicalPlanBuildBaseline(sessVars, bestState)
}
sessVars.DurationOptimizer.Total = time.Since(beginOpt)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Old impl will update DurationOptimizer even got error, but new impl will not. So better put this in defer

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in bc1b396. DurationOptimizer.Total is now assigned in a deferred function, so it is recorded even when optimize returns with error.

return finalPlan, names, cost, err
return bestPlan, bestNames, bestCost, nil
}

// OptimizeExecStmt to handle the "execute" statement
Expand Down