planner: add trivial plan fast path for simple full-table scans#67376
planner: add trivial plan fast path for simple full-table scans#67376terry1purcell wants to merge 11 commits into
Conversation
For queries like "SELECT * FROM t" on tables with no secondary indexes, no TiFlash replicas, no predicates, and no complex clauses, the plan is predetermined as a full table scan. This adds a fast path that constructs the PhysicalTableReader → PhysicalTableScan directly, skipping all 29 logical optimization rules, synchronous stats loading, cost estimation, and post-optimization passes. The fast path is skipped during EXPLAIN so the normal optimizer produces full plan metadata for diagnostic output. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ti-chi-bot
Bot
added
the
release-note-none
|
Review Complete Findings: 6 issues ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
size/XL
|
Hi @terry1purcell. 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 a trivial-plan fast-path to the planner that can directly produce a PhysicalTableReader for constrained single-table SELECTs, integrates it into optimizeNoCache with binding checks and early-exit, adds session flags and a read-only sysvar to track usage, and includes tests and build updates. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Optimizer as "optimizeNoCache\n(Optimizer)"
participant Core as "TryTrivialPlan\n(Core)"
participant Stats as "Stats Cache / Realtime"
participant Scan as "PhysicalTableScan\n(Scan)"
participant Reader as "PhysicalTableReader\n(Reader)"
Client->>Optimizer: Submit SELECT
Optimizer->>Core: TryTrivialPlan(ctx, node)
alt Trivial conditions met and no binding match
Core->>Core: Validate SELECT shape & table
Core->>Stats: Read cached realtime / pseudo counts
Stats-->>Core: Row estimate & hist placeholder
Core->>Scan: Build PhysicalTableScan (pruned cols, ranges)
Scan-->>Reader: Wrap into PhysicalTableReader (apply stats)
Reader-->>Optimizer: Return physical plan
Optimizer-->>Client: Execute returned plan
else Not trivial or binding matched
Core-->>Optimizer: Return nil
Optimizer->>Optimizer: Continue full optimization pipeline
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
Views need the full optimizer to expand their definition into the underlying query. Without this check, SELECT * FROM a_view would produce an empty result because the fast path constructs a table scan against the view's metadata rather than its underlying tables. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/planner/core/tests/pointget/trivial_plan_test.go (1)
56-104: Consider adding direct verification that trivial plan path is (not) taken.The fallback tests verify correct query results but don't directly assert that the trivial plan fast path was skipped. While the correctness tests are valuable, consider adding a mechanism (e.g., a metric counter or session variable) to directly verify the optimization path taken. This would make the tests more robust against future regressions where the trivial path might incorrectly activate.
That said, the current tests adequately verify end-to-end correctness, which is the primary goal.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/tests/pointget/trivial_plan_test.go` around lines 56 - 104, Add an assertion in TestTrivialPlanFallback to directly verify the trivial-plan fast path was not used by checking a test-only observable (e.g., a planner metric counter or session variable). Instrument the planner (e.g., increment a planner.TrivialPlanCounter or set a boolean flag when trivial path is taken) and in the test read that observable before/after each query in TestTrivialPlanFallback to assert it did not increase (or the flag is false); reference TestTrivialPlanFallback and the planner observable (e.g., TrivialPlanCounter / trivialPathTaken) when adding the check so future regressions are detected explicitly.
🤖 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/core/tests/pointget/trivial_plan_test.go`:
- Around line 56-104: Add an assertion in TestTrivialPlanFallback to directly
verify the trivial-plan fast path was not used by checking a test-only
observable (e.g., a planner metric counter or session variable). Instrument the
planner (e.g., increment a planner.TrivialPlanCounter or set a boolean flag when
trivial path is taken) and in the test read that observable before/after each
query in TestTrivialPlanFallback to assert it did not increase (or the flag is
false); reference TestTrivialPlanFallback and the planner observable (e.g.,
TrivialPlanCounter / trivialPathTaken) when adding the check so future
regressions are detected explicitly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 63ee5eed-bac7-4366-867a-ace7fd2aedf0
📒 Files selected for processing (5)
pkg/planner/core/BUILD.bazelpkg/planner/core/tests/pointget/BUILD.bazelpkg/planner/core/tests/pointget/trivial_plan_test.gopkg/planner/core/trivial_plan.gopkg/planner/optimize.go
Tables with foreign keys have cascade/restrict behavior that needs the full optimizer. Tables with hidden columns (used by expression indexes) can cause schema mismatches in the fast path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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/core/trivial_plan.go`:
- Around line 253-261: In trivialRowCountEstimate, don't treat RealtimeCount==0
as pseudo; instead check the stats table's pseudo flag and return PseudoRowCount
only when statsTbl.IsPseudo is true; otherwise return
float64(statsTbl.RealtimeCount) (which will be 0 for genuinely empty tables).
Update the logic around domain.GetDomain(ctx).StatsHandle(),
statsHandle.GetPhysicalTableStats(tblInfo.ID, tblInfo), statsTbl.RealtimeCount
and statsTbl.IsPseudo so empty tables yield 0 and only truly pseudo stats yield
statistics.PseudoRowCount.
- Around line 170-183: PhysicalTableScan constructed in trivial_plan.go is
missing the PhysicalTableID assignment; when isTrivialTable ensures
non-partitioned tables you must set PhysicalTableID to the table ID. Update the
PhysicalTableScan initialization (the struct literal used to build ts in
trivial_plan.go) to include PhysicalTableID: tblInfo.ID so it matches other
construction sites (see planbuilder usage) and retains consistency with
ts.Init/SetSchema/SetStats flows.
🪄 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: 428d5672-bb8c-4626-a464-492d568b8be7
📒 Files selected for processing (1)
pkg/planner/core/trivial_plan.go
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
| if col.State == model.StatePublic { | ||
| allColumns = append(allColumns, col) | ||
| tblCols = append(tblCols, colInfoToColumn(col, i)) | ||
| } |
There was a problem hiding this comment.
[P1] Trivial plan breaks SELECT output column mapping
Why: PhysicalTableScan.Columns is derived from an ID-set in table order, dropping select-list ordering/duplicates/pseudo columns. This causes queries like SELECT c,a, SELECT a,a, SELECT _tidb_rowid, or SELECT *,a to return wrong column order or fail with schema/codec mismatches.
Evidence: pkg/planner/core/trivial_plan.go:129 builds Columns by deduping on col.ID and iterating in table order:
schemaColIDs[col.ID] = struct{}{}
...
for _, col := range allColumns {
if _, ok := schemaColIDs[col.ID]; ok {
columns = append(columns, col)
}
}This loses SELECT field ordering and duplicates. The scan output won't match the schema positions, causing wrong results.
| // This avoids all logical optimization rules, stats loading, cost estimation, | ||
| // and post-optimization passes that are unnecessary when the plan is predetermined. | ||
| // It is designed for execution performance, not EXPLAIN accuracy. | ||
| func TryTrivialPlan(ctx base.PlanContext, node *resolve.NodeW) (base.Plan, types.NameSlice) { |
There was a problem hiding this comment.
[P1] Trivial plan skips UnionScan requirements
Why: Returns a plain PhysicalTableReader even when dirty-txn reads or local temp/cache tables require UnionScan. For local temp tables, the TableReader runs in dummy mode and returns no rows. For dirty transactions, this breaks read-your-writes semantics.
Evidence: TryTrivialPlan returns before logical plan building that would add UnionScan at pkg/planner/core/logical_plan_builder.go:5276:
if dirty || tableInfo.TempTableType == model.TempTableLocal ||
tableInfo.TableCacheStatusType == model.TableCacheStatusEnable {
... LogicalUnionScan ...
}The early return at pkg/planner/optimize.go:264 bypasses this critical wrapping.
| // Try the trivial plan fast path for simple full-table scans where the | ||
| // plan is predetermined (no secondary indexes, no TiFlash, no predicates). | ||
| // This skips logical optimization, stats loading, and cost estimation. | ||
| if tp, tpNames := core.TryTrivialPlan(pctx, node); tp != nil { |
There was a problem hiding this comment.
[P2] Trivial plan ignores sql_select_limit
Why: core.TryAddExtraLimit is applied only after the fast-path returns, so trivial plan can return unlimited rows when sql_select_limit is set (non-default configuration).
Evidence: pkg/planner/optimize.go:264 returns early:
if tp, tpNames := core.TryTrivialPlan(pctx, node); tp != nil {
return tp, tpNames, nil
}vs pkg/planner/optimize.go:276 which applies the limit:
stmtNode = core.TryAddExtraLimit(sctx, stmtNode)The fast path skips this rewrite entirely.
| if tnW == nil { | ||
| return nil, nil | ||
| } | ||
| tblInfo := tnW.TableInfo |
There was a problem hiding this comment.
[P1] Trivial plan bypasses table-source semantics validation
Why: Can scan VIEWs, SEQUENCEs, or explicit PARTITION clauses instead of erroring/expanding/converting, changing results or suppressing expected errors.
Evidence: pkg/planner/core/trivial_plan.go:83 never checks tblInfo.IsView()/IsSequence()/IsBaseTable() before building a PhysicalTableScan. Normal planner at pkg/planner/core/logical_plan_builder.go:5006 expands views via BuildDataSourceFromView and at :5041 converts sequences to TableDual. The fast path bypasses all of this, producing empty/wrong results for SELECT * FROM view.
| require.Len(t, rows, 3) | ||
|
|
||
| // Verify EXPLAIN shows a table scan plan. | ||
| tk.MustQuery("explain format = 'brief' select * from t_simple").Check(testkit.Rows( |
There was a problem hiding this comment.
[P2] Tests don't prove the fast path is used
Why: TestTrivialPlan validates EXPLAIN output, but TryTrivialPlan explicitly returns nil in EXPLAIN mode (pkg/planner/core/trivial_plan.go:55), so the test can pass even if the fast path never triggers for actual queries.
Evidence: pkg/planner/core/trivial_plan.go:55:
if ctx.GetSessionVars().StmtCtx.InExplainStmt {
return nil, nil
}The test at pkg/planner/core/tests/pointget/trivial_plan_test.go:50 checks explain format='brief' select * from t_simple, which will always use the normal optimizer path, not the trivial fast path.
terry1purcell
changed the title
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/planner/core/trivial_plan.go (1)
164-171: Consider adding a defensive nil check when building scan columns.The
allColumnsslice (lines 153-158) filters columns byStatePublic, butbuildSchemaFromFieldsdoesn't apply this filter when expanding wildcards. During concurrent DDL (column addition in progress), a schema column could have an ID that doesn't exist incolInfoByID, causing a nil entry in thecolumnsslice.While MDL should prevent most concurrent DDL interference, a defensive check would prevent potential panics in edge cases.
Optional defensive fix
columns := make([]*model.ColumnInfo, 0, schema.Len()) for _, col := range schema.Columns { - columns = append(columns, colInfoByID[col.ID]) + colInfo := colInfoByID[col.ID] + if colInfo == nil { + // Column not in public state; fall back to full optimizer. + return nil, nil + } + columns = append(columns, colInfo) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/trivial_plan.go` around lines 164 - 171, The code builds columns from schema.Columns using colInfoByID but doesn't guard against missing entries, which can produce nils and panic; in the block that iterates schema.Columns (using colInfoByID, allColumns, and the columns slice), add a defensive nil check for colInfoByID[col.ID] and either skip nil entries or return a clear error (depending on the surrounding function's error handling) instead of appending a nil pointer; ensure any chosen behavior preserves caller expectations and consider logging or wrapping the error with context mentioning the missing column ID.pkg/sessionctx/variable/session.go (1)
1416-1419: Add SQL integration coverage forlast_plan_from_trivialbehavior.This is a user-visible session-variable/protocol behavior change. Please add SQL integration tests for statement rollover semantics (current vs previous statement) and EXPLAIN bypass behavior.
I can draft a compact test matrix for these scenarios if you want.
Based on learnings: In pingcap/tidb,for session variables or protocol behavior changes, perform targeted package tests plus SQL integration tests for user-visible behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/sessionctx/variable/session.go` around lines 1416 - 1419, Tests are missing for the user-visible session-variable/protocol behavior introduced by FoundInTrivialPlan and PrevFoundInTrivialPlan (exposed as last_plan_from_trivial); add targeted unit/package tests and SQL integration tests that validate statement rollover semantics (current vs previous statement) and EXPLAIN bypass behavior: write package-level tests exercising the session state transitions in the code paths that set FoundInTrivialPlan and PrevFoundInTrivialPlan, and add SQL integration tests that run sequences of statements and EXPLAIN queries to assert last_plan_from_trivial toggles as expected across consecutive statements and that EXPLAIN does not update the trivial-plan flag. Ensure the tests reference the session variable name last_plan_from_trivial and the internal flags FoundInTrivialPlan / PrevFoundInTrivialPlan so failures point to the correct code paths.
🤖 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/variable/session.go`:
- Around line 1416-1419: The new boolean fields FoundInTrivialPlan and
PrevFoundInTrivialPlan must be persisted during session encoding/decoding:
update EncodeSessionStates to write these two flags into the session state blob
(same area where plan-cache/bindings flags are serialized) and update
DecodeSessionStates to read them back and assign to the corresponding fields on
the session variable struct; reference the FoundInTrivialPlan and
PrevFoundInTrivialPlan fields when adding serialization/deserialization, and
ensure defaulting/validation matches the existing pattern used for the
neighboring plan-cache/binding flags.
In `@pkg/sessionctx/variable/sysvar.go`:
- Around line 421-423: The new session-visible sysvar TiDBFoundInTrivial
(GetSession uses s.PrevFoundInTrivialPlan) isn't persisted in session
snapshot/restore paths; update the session state serialization to include
PrevFoundInTrivialPlan alongside FoundInPlanCache and FoundInBinding.
Concretely: add a PrevFoundInTrivialPlan field to the session-states struct,
include it in the logic that ShowSessionStates/SetSessionStates (or the
functions that emit/consume session-states) use to serialize/deserialize session
state, and ensure the restore path assigns the value back to
SessionVars.PrevFoundInTrivialPlan so the sysvar remains consistent across
SHOW/SET SESSION_STATES.
---
Nitpick comments:
In `@pkg/planner/core/trivial_plan.go`:
- Around line 164-171: The code builds columns from schema.Columns using
colInfoByID but doesn't guard against missing entries, which can produce nils
and panic; in the block that iterates schema.Columns (using colInfoByID,
allColumns, and the columns slice), add a defensive nil check for
colInfoByID[col.ID] and either skip nil entries or return a clear error
(depending on the surrounding function's error handling) instead of appending a
nil pointer; ensure any chosen behavior preserves caller expectations and
consider logging or wrapping the error with context mentioning the missing
column ID.
In `@pkg/sessionctx/variable/session.go`:
- Around line 1416-1419: Tests are missing for the user-visible
session-variable/protocol behavior introduced by FoundInTrivialPlan and
PrevFoundInTrivialPlan (exposed as last_plan_from_trivial); add targeted
unit/package tests and SQL integration tests that validate statement rollover
semantics (current vs previous statement) and EXPLAIN bypass behavior: write
package-level tests exercising the session state transitions in the code paths
that set FoundInTrivialPlan and PrevFoundInTrivialPlan, and add SQL integration
tests that run sequences of statements and EXPLAIN queries to assert
last_plan_from_trivial toggles as expected across consecutive statements and
that EXPLAIN does not update the trivial-plan flag. Ensure the tests reference
the session variable name last_plan_from_trivial and the internal flags
FoundInTrivialPlan / PrevFoundInTrivialPlan so failures point to the correct
code paths.
🪄 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: 5bee4f70-dc7c-4f38-a439-0fad5f6c1a5f
📒 Files selected for processing (7)
pkg/executor/select.gopkg/planner/core/tests/pointget/trivial_plan_test.gopkg/planner/core/trivial_plan.gopkg/planner/optimize.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/session.gopkg/sessionctx/variable/sysvar.go
✅ Files skipped from review due to trivial changes (1)
- pkg/planner/core/tests/pointget/trivial_plan_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/planner/optimize.go
| // FoundInTrivialPlan indicates whether this statement used the trivial plan fast path. | ||
| FoundInTrivialPlan bool | ||
| // PrevFoundInTrivialPlan indicates whether the last statement used the trivial plan fast path. | ||
| PrevFoundInTrivialPlan bool |
There was a problem hiding this comment.
Persist the new trivial-plan flag through session state encode/decode.
FoundInTrivialPlan / PrevFoundInTrivialPlan are introduced here, but unlike the neighboring plan-cache/binding flags, they are not wired into EncodeSessionStates / DecodeSessionStates in this file. That can lose last_plan_from_trivial state across session migration.
💡 Proposed patch
@@ func (s *SessionVars) EncodeSessionStates(_ context.Context, sessionStates *sessionstates.SessionStates) (err error) {
sessionStates.FoundInPlanCache = s.PrevFoundInPlanCache
+ sessionStates.FoundInTrivialPlan = s.PrevFoundInTrivialPlan
sessionStates.FoundInBinding = s.PrevFoundInBinding
@@ func (s *SessionVars) DecodeSessionStates(_ context.Context, sessionStates *sessionstates.SessionStates) (err error) {
s.FoundInPlanCache = sessionStates.FoundInPlanCache
+ s.FoundInTrivialPlan = sessionStates.FoundInTrivialPlan
s.FoundInBinding = sessionStates.FoundInBinding🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/sessionctx/variable/session.go` around lines 1416 - 1419, The new boolean
fields FoundInTrivialPlan and PrevFoundInTrivialPlan must be persisted during
session encoding/decoding: update EncodeSessionStates to write these two flags
into the session state blob (same area where plan-cache/bindings flags are
serialized) and update DecodeSessionStates to read them back and assign to the
corresponding fields on the session variable struct; reference the
FoundInTrivialPlan and PrevFoundInTrivialPlan fields when adding
serialization/deserialization, and ensure defaulting/validation matches the
existing pattern used for the neighboring plan-cache/binding flags.
| {Scope: vardef.ScopeSession, Name: vardef.TiDBFoundInTrivialPlan, Value: BoolToOnOff(false), Type: vardef.TypeBool, ReadOnly: true, GetSession: func(s *SessionVars) (string, error) { | ||
| return BoolToOnOff(s.PrevFoundInTrivialPlan), nil | ||
| }}, |
There was a problem hiding this comment.
Serialize trivial-plan state in session snapshot/restore paths.
This new previous-statement sysvar is session-visible, but the session-state flow currently serializes/restores FoundInPlanCache / FoundInBinding only. @@last_plan_from_trivial can be dropped or become inconsistent after SHOW SESSION_STATES + SET SESSION_STATES.
Suggested follow-up (outside this hunk)
--- a/pkg/sessionctx/variable/session.go
+++ b/pkg/sessionctx/variable/session.go
@@
sessionStates.FoundInPlanCache = s.PrevFoundInPlanCache
+ sessionStates.FoundInTrivialPlan = s.PrevFoundInTrivialPlan
sessionStates.FoundInBinding = s.PrevFoundInBinding
@@
s.FoundInPlanCache = sessionStates.FoundInPlanCache
+ s.FoundInTrivialPlan = sessionStates.FoundInTrivialPlan
s.FoundInBinding = sessionStates.FoundInBindingAlso add the corresponding field in the session-states struct if it does not exist yet.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/sessionctx/variable/sysvar.go` around lines 421 - 423, The new
session-visible sysvar TiDBFoundInTrivial (GetSession uses
s.PrevFoundInTrivialPlan) isn't persisted in session snapshot/restore paths;
update the session state serialization to include PrevFoundInTrivialPlan
alongside FoundInPlanCache and FoundInBinding. Concretely: add a
PrevFoundInTrivialPlan field to the session-states struct, include it in the
logic that ShowSessionStates/SetSessionStates (or the functions that
emit/consume session-states) use to serialize/deserialize session state, and
ensure the restore path assigns the value back to
SessionVars.PrevFoundInTrivialPlan so the sysvar remains consistent across
SHOW/SET SESSION_STATES.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/planner/optimize.go (1)
266-280:⚠️ Potential issue | 🟠 MajorStill bypasses
sql_select_limiton the trivial return path.
core.TryAddExtraLimitis only applied at Lines 291-293, so returning here can still ignore a non-defaultsql_select_limitand scan/return the full table. Please run this fast path after the extra-limit rewrite, or suppress it when an implicit limit would be injected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/optimize.go` around lines 266 - 280, The trivial-plan fast path in the TryTrivialPlan branch returns before applying the extra-limit rewrite, so a non-default sql_select_limit can be bypassed; modify the code in the TryTrivialPlan handling (where TryTrivialPlan, CheckTableMode and sessVars.FoundInTrivialPlan are used) to invoke the same extra-limit rewrite logic (i.e., call core.TryAddExtraLimit or equivalent used later) before returning the trivial plan, or alternatively detect when an implicit limit would be injected and suppress the trivial-path return in that case so the extra-limit rewrite runs; ensure the behavior matches the later branch that currently applies TryAddExtraLimit.
🤖 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 266-280: The trivial-plan fast path in the TryTrivialPlan branch
returns before applying the extra-limit rewrite, so a non-default
sql_select_limit can be bypassed; modify the code in the TryTrivialPlan handling
(where TryTrivialPlan, CheckTableMode and sessVars.FoundInTrivialPlan are used)
to invoke the same extra-limit rewrite logic (i.e., call core.TryAddExtraLimit
or equivalent used later) before returning the trivial plan, or alternatively
detect when an implicit limit would be injected and suppress the trivial-path
return in that case so the extra-limit rewrite runs; ensure the behavior matches
the later branch that currently applies TryAddExtraLimit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 09fa004c-4fa8-4b15-a0a3-d3a555332c9c
📒 Files selected for processing (1)
pkg/planner/optimize.go
Previously the trivial plan fast path bailed on any table with a secondary index and on any query with a WHERE clause. That left a lot of provably- table-scan queries paying the full optimizer cost. This change replaces the table-level "no indexes" rule with a per-query gate: the fast path now applies whenever no index could plausibly be chosen — i.e. no index's leading column is referenced by a predicate and no index covers the SELECT projection. References to the primary key (handle or clustered) are still treated as range-scan candidates and disqualify the fast path. WHERE clauses are supported by walking the predicate AST to collect referenced columns, extending the scan column set, rewriting the predicate against that schema, and wrapping a PhysicalSelection above the PhysicalTableScan. Subqueries, aggregates, window functions, and session variables in WHERE keep the fast path out. A PhysicalProjection is added only when WHERE-only columns need to be stripped from the output. Because every condition is handed to the coprocessor via the Selection's PB serialization (the trivial path doesn't split conditions between TiKV-side and TiDB-side evaluation), bail when any condition isn't TiKV-pushable so we don't surface a runtime ErrInternal. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ti-chi-bot
Bot
added
size/XXL
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/planner/core/trivial_plan.go (1)
459-497: 💤 Low valueConsider disallowing
ParamMarkerExprin the WHERE clause visitor.Prepared statement parameters (
?placeholders) are not explicitly checked inwhereColumnVisitor. While they would likely still produce correct results (pushdown check should pass), the trivial path may have subtle interactions with prepared statement execution that haven't been tested.If prepared statements are intended to use the trivial path, add test coverage. Otherwise, adding
*ast.ParamMarkerExprto the disallowed cases would be a safe conservative choice.🤖 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/trivial_plan.go` around lines 459 - 497, The WHERE-clause AST visitor whereColumnVisitor currently doesn't treat prepared-statement parameter nodes as disallowed, which may let queries with ParamMarkerExprs take the trivial fast path; update the Enter method of whereColumnVisitor to include *ast.ParamMarkerExpr in the disallowed case list (alongside SubqueryExpr, ExistsSubqueryExpr, CompareSubqueryExpr, AggregateFuncExpr, WindowFuncExpr, VariableExpr) so v.disallowed is set and traversal stops for parameterized WHEREs, or alternatively add tests proving safe acceptance if you intend to allow them.
🤖 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.
Nitpick comments:
In `@pkg/planner/core/trivial_plan.go`:
- Around line 459-497: The WHERE-clause AST visitor whereColumnVisitor currently
doesn't treat prepared-statement parameter nodes as disallowed, which may let
queries with ParamMarkerExprs take the trivial fast path; update the Enter
method of whereColumnVisitor to include *ast.ParamMarkerExpr in the disallowed
case list (alongside SubqueryExpr, ExistsSubqueryExpr, CompareSubqueryExpr,
AggregateFuncExpr, WindowFuncExpr, VariableExpr) so v.disallowed is set and
traversal stops for parameterized WHEREs, or alternatively add tests proving
safe acceptance if you intend to allow them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2c9fd73a-f871-45b8-afcc-8cc81dcd7171
📒 Files selected for processing (3)
pkg/planner/core/tests/pointget/BUILD.bazelpkg/planner/core/tests/pointget/trivial_plan_test.gopkg/planner/core/trivial_plan.go
✅ Files skipped from review due to trivial changes (1)
- pkg/planner/core/tests/pointget/BUILD.bazel
The full optimizer's CollectPredicateColumnsPoint rule registers WHERE- referenced columns for sync or async stats loading. The trivial path skipped this rule entirely, so queries that should have triggered a sync wait (or queued an async load) silently bypassed it. Use the same Table.ColumnIsLoadNeeded check the rule uses: if any WHERE column has analyzed stats that are evicted or partially loaded, fall back to the full planner so the load registration happens. Fixes TestLoadAnalyzeV1StatsJSONFromV855 in tests/realtikvtest/statisticstest, which exercises both sync (tidb_stats_load_sync_wait=60000) and async (tidb_stats_load_sync_wait=0) paths on a query whose WHERE column has analyzed-but-evicted stats. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The full optimizer's CollectPredicateColumnsPoint rule also calls UpdateColStatsUsage so ANALYZE PREDICATE COLUMNS knows which columns were referenced by predicates. The trivial path skipped this side effect, so a SELECT … WHERE col = X that took the fast path stopped contributing to the session's predicate-column collector. The previous commit's load-needed gate only addresses the subset where column stats are evicted; predicate tracking happens for all WHERE references regardless of load state. Fixes TestCleanupPredicateColumns, TestAnalyzeTableWithPredicateColumns, TestAnalyzeTableWithTiDBPersistAnalyzeOptionsDisabled, TestPredicateUsage_FirstTouchCreatesRow, TestPredicateUsage_NoBumpWithinThrottle in pkg/statistics/handle/usage and TestInitStatsVer2 in pkg/statistics/handle/handletest/statstest. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
genBriefPlanWithSCtx is called by the binding system (EXPLAIN EXPLORE,
auto-binding suggestions) to produce plan variants whose hints can be
extracted via GenHintsFromFlatPlan. Optimizer fast paths — TryFastPlan
for point gets and TryTrivialPlan for full table scans — produce bare
operator trees with no hint metadata, which breaks downstream hint
parsing ("Optimizer hint syntax error" from an empty /*+ */ comment).
Set StmtCtx.InExplainStmt = true before invoking Optimize so the
existing InExplainStmt gate in TryTrivialPlan (and any future fast
path) skips itself and lets the full planner produce the richer
metadata the binding consumer expects.
Fixes TestExplainExploreAnalyze and TestExplainExploreVerifyAndBind
in pkg/bindinfo.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PlanBuilder.buildSelection (logical_plan_builder.go:1386) detects when a CNF predicate's eval type is string-typed (enum, set, json, hybrid-typed control functions) and wraps it via TryPushCastIntoControlFunctionForHybridType so the runtime evaluates truthiness against the underlying numeric representation. The trivial path did not apply that transformation, which made WHERE clauses like `if(e>1, e, e)` on an enum column always evaluate to false — the raw enum value would be read as 0 instead of its index. Result: trivial path returned zero rows where the full planner correctly returned all matching rows. Mirror the same cast on the CNF items before pushdown. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@terry1purcell: The following tests 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. |
1 participant
For queries like "SELECT * FROM t" on tables with no secondary indexes, no TiFlash replicas, no predicates, and no complex clauses, the plan is predetermined as a full table scan. This adds a fast path that constructs the PhysicalTableReader → PhysicalTableScan directly, skipping all 29 logical optimization rules, synchronous stats loading, cost estimation, and post-optimization passes.
The fast path is skipped during EXPLAIN so the normal optimizer produces full plan metadata for diagnostic output.
What problem does this PR solve?
Issue Number: ref #67351
Problem Summary:
What changed and how does it work?
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
New Features
Tests