planner: index pruning using existing infra (#64999)#68988
Closed
qw4990 wants to merge 12 commits into
Closed
Conversation
ti-chi-bot
Bot
added
do-not-merge/cherry-pick-not-approved
release-note-none
|
[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 |
|
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 threshold-controlled index pruning: new ChangesIndex pruning with threshold control and interesting column tracking
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 |
|
Hi @qw4990. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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.
Inline comments:
In `@pkg/planner/core/collect_column_stats_usage.go`:
- Around line 263-307: The code currently passes
currentJoinCols/currentOrderingCols to children before schema-changing ops
rewrite outputs back to child inputs, so aliases/computed columns aren't mapped
to source columns; update collectFromPlan to, when lp is a schema-changing
operator (at least logicalop.LogicalProjection, logicalop.LogicalAggregation,
logicalop.LogicalWindow), build the same output->input col map you use to
translate AskedColumnGroup/curColGroups and apply that translation to
currentJoinCols and currentOrderingCols (i.e. replace output Columns with their
mapped input Columns) before recursing; keep the rest of the logic (use
ExtractColGroups, the existing colMap/translation helper you already use for
curColGroups) and pass the mapped join/ordering cols into c.collectFromPlan for
children.
- Around line 274-280: The join-column collection for logicalop.LogicalJoin and
logicalop.LogicalApply omits outer/semi join filters; update the handlers for
LogicalJoin and LogicalApply to also extract columns from x.LeftConditions and
x.RightConditions (in addition to x.EqualConditions and x.OtherConditions) by
calling expression.ExtractColumnsFromExpressions on those slices and appending
the results to currentJoinCols so Left/RightConditions are considered during
pruning.
In `@pkg/planner/core/rule_collect_plan_stats.go`:
- Around line 267-280: The bug is that keptIndexIDs (map keyed by
ds.PhysicalTableID) is only merged when the current alias was pruned
(len(prunedPaths) < len(ds.AllPossibleAccessPaths)), so candidate indexes from
aliases that were not pruned never get merged and their indexes may be skipped;
always merge tableKeptIndexes into keptIndexIDs for ds.PhysicalTableID
regardless of whether this alias was pruned. Update the logic around
keptIndexIDs, ds.PhysicalTableID, tableKeptIndexes and prunedPaths (currently
inside the if checking len(prunedPaths) < len(ds.AllPossibleAccessPaths)) so
that the union/merge into keptIndexIDs happens unconditionally (or add an else
branch that performs the same merge) to ensure all aliases contributing
candidate indexes are aggregated.
In `@pkg/planner/core/rule/rule_prune_indexes.go`:
- Around line 267-287: The fallback handling in the else branch for
path.Index.Columns only increments score.interestingCount and omits
consecutive-prefix tracking; update the loop over path.Index.Columns (using
idxCol.Offset, tableColumns and colInfo.ID) to also detect consecutive matches
in index order: keep a prevMatchedIndexPos (init e.g. -2), on each matched
idxCol (if req.interestingColIDs contains colInfo.ID) increment
score.interestingCount and if prevMatchedIndexPos+1 == current index position
then append the col ID to score.consecutiveColumnIDs (or increment the existing
consecutive-prefix counter on score), otherwise reset prevMatchedIndexPos to
current position (and start a new consecutive run); this restores
consecutive-prefix scoring while reusing path.Index.Columns,
req.interestingColIDs, score.interestingCount and consecutiveColumnIDs.
In `@pkg/planner/core/stats_test.go`:
- Around line 285-291: The helper mutates session vars via
se.GetSessionVars().SetSystemVarWithOldValAsRet and records restores with
StmtCtx.AddSetVarHintRestore but never consumes that restore list, leaking hints
into later tests; fix by immediately consuming/restoring those entries inside
the helper after setting them (either by invoking the same cleanup routine the
normal statement path uses or by iterating the recorded restores and calling
SetSystemVarWithOldValAsRet for each saved old value and then clearing the
restore list), ensuring StmtCtx.AddSetVarHintRestore entries are applied/cleared
before the helper returns.
🪄 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: 1af25ff9-4e54-465b-9d4c-2b7a4b95faf3
📒 Files selected for processing (29)
pkg/executor/testdata/slow_query_suite_out.jsonpkg/planner/core/BUILD.bazelpkg/planner/core/casetest/dag/testdata/plan_suite_out.jsonpkg/planner/core/collect_column_stats_usage.gopkg/planner/core/find_best_task.gopkg/planner/core/logical_plan_builder.gopkg/planner/core/operator/logicalop/logical_datasource.gopkg/planner/core/operator/logicalop/logical_index_scan.gopkg/planner/core/operator/logicalop/logical_plans_misc.gopkg/planner/core/rule/BUILD.bazelpkg/planner/core/rule/rule_prune_indexes.gopkg/planner/core/rule_collect_plan_stats.gopkg/planner/core/rule_derive_topn_from_window.gopkg/planner/core/rule_generate_column_substitute.gopkg/planner/core/rule_join_elimination.gopkg/planner/core/rule_max_min_eliminate.gopkg/planner/core/rule_partition_processor.gopkg/planner/core/rule_predicate_push_down.gopkg/planner/core/stats.gopkg/planner/core/stats_test.gopkg/session/bootstraptest/BUILD.bazelpkg/sessionctx/variable/session.gopkg/sessionctx/variable/setvar_affect.gopkg/sessionctx/variable/sysvar.gopkg/sessionctx/variable/tidb_vars.gopkg/sessionctx/variable/varsutil_test.gopkg/statistics/handle/handletest/BUILD.bazelpkg/statistics/handle/handletest/handle_test.gotests/integrationtest/r/planner/core/casetest/integration.result
💤 Files with no reviewable changes (1)
- tests/integrationtest/r/planner/core/casetest/integration.result
Comment on lines
+263
to
+307
| func (c *columnStatsUsageCollector) collectFromPlan(askedColGroups [][]*expression.Column, lp base.LogicalPlan, accumulatedJoinCols []*expression.Column, accumulatedOrderingCols []*expression.Column) { | ||
| // derive the new current op's new asked column groups accordingly. | ||
| curColGroups := lp.ExtractColGroups(askedColGroups) | ||
|
|
||
| // Track columns for index pruning as we traverse | ||
| currentJoinCols := accumulatedJoinCols | ||
| currentOrderingCols := accumulatedOrderingCols | ||
|
|
||
| // Extract join and ordering columns from this node before visiting children | ||
| if c.interestingColsByDS != nil { | ||
| switch x := lp.(type) { | ||
| case *logicalop.LogicalJoin: | ||
| // Extract join columns from EqualConditions and OtherConditions | ||
| currentJoinCols = append(currentJoinCols, expression.ExtractColumnsFromExpressions(nil, expression.ScalarFuncs2Exprs(x.EqualConditions), nil)...) | ||
| currentJoinCols = append(currentJoinCols, expression.ExtractColumnsFromExpressions(nil, x.OtherConditions, nil)...) | ||
| case *logicalop.LogicalApply: | ||
| currentJoinCols = append(currentJoinCols, expression.ExtractColumnsFromExpressions(nil, expression.ScalarFuncs2Exprs(x.EqualConditions), nil)...) | ||
| currentJoinCols = append(currentJoinCols, expression.ExtractColumnsFromExpressions(nil, x.OtherConditions, nil)...) | ||
| case *logicalop.LogicalSort: | ||
| for _, item := range x.ByItems { | ||
| currentOrderingCols = append(currentOrderingCols, expression.ExtractColumns(item.Expr)...) | ||
| } | ||
| case *logicalop.LogicalTopN: | ||
| for _, item := range x.ByItems { | ||
| currentOrderingCols = append(currentOrderingCols, expression.ExtractColumns(item.Expr)...) | ||
| } | ||
| case *logicalop.LogicalWindow: | ||
| for _, item := range x.OrderBy { | ||
| currentOrderingCols = append(currentOrderingCols, item.Col) | ||
| } | ||
| case *logicalop.LogicalAggregation: | ||
| // GROUP BY columns benefit from indexes (similar to ordering) | ||
| currentOrderingCols = append(currentOrderingCols, expression.ExtractColumnsFromExpressions(nil, x.GroupByItems, nil)...) | ||
| // MIN/MAX aggregates can benefit from ordered indexes | ||
| for _, aggFunc := range x.AggFuncs { | ||
| if aggFunc.Name == ast.AggFuncMin || aggFunc.Name == ast.AggFuncMax { | ||
| currentOrderingCols = append(currentOrderingCols, expression.ExtractColumnsFromExpressions(nil, aggFunc.Args, nil)...) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for _, child := range lp.Children() { | ||
| c.collectFromPlan(child) | ||
| // passing the new asked column groups down, along with accumulated join/ordering columns | ||
| c.collectFromPlan(curColGroups, child, currentJoinCols, currentOrderingCols) |
There was a problem hiding this comment.
Back-propagate interesting columns through schema-changing operators.
currentJoinCols/currentOrderingCols are passed to children before LogicalProjection/LogicalAggregation/LogicalWindow rewrite their output columns back to child inputs. That means aliases and computed outputs stop matching the child DataSource schema, so queries like SELECT * FROM (SELECT a AS x FROM t) s ORDER BY x never mark t.a as interesting and the new pruner can drop the (a) index. This needs the same kind of output-to-input translation you already do for AskedColumnGroup/colMap before the recursive descent.
🤖 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/collect_column_stats_usage.go` around lines 263 - 307, The
code currently passes currentJoinCols/currentOrderingCols to children before
schema-changing ops rewrite outputs back to child inputs, so aliases/computed
columns aren't mapped to source columns; update collectFromPlan to, when lp is a
schema-changing operator (at least logicalop.LogicalProjection,
logicalop.LogicalAggregation, logicalop.LogicalWindow), build the same
output->input col map you use to translate AskedColumnGroup/curColGroups and
apply that translation to currentJoinCols and currentOrderingCols (i.e. replace
output Columns with their mapped input Columns) before recursing; keep the rest
of the logic (use ExtractColGroups, the existing colMap/translation helper you
already use for curColGroups) and pass the mapped join/ordering cols into
c.collectFromPlan for children.
Comment on lines
+274
to
+280
| case *logicalop.LogicalJoin: | ||
| // Extract join columns from EqualConditions and OtherConditions | ||
| currentJoinCols = append(currentJoinCols, expression.ExtractColumnsFromExpressions(nil, expression.ScalarFuncs2Exprs(x.EqualConditions), nil)...) | ||
| currentJoinCols = append(currentJoinCols, expression.ExtractColumnsFromExpressions(nil, x.OtherConditions, nil)...) | ||
| case *logicalop.LogicalApply: | ||
| currentJoinCols = append(currentJoinCols, expression.ExtractColumnsFromExpressions(nil, expression.ScalarFuncs2Exprs(x.EqualConditions), nil)...) | ||
| currentJoinCols = append(currentJoinCols, expression.ExtractColumnsFromExpressions(nil, x.OtherConditions, nil)...) |
There was a problem hiding this comment.
Include LeftConditions and RightConditions in join-interesting columns.
Outer/semi join filters stored in LeftConditions/RightConditions are currently invisible to pruning. For plans like t1 LEFT JOIN t2 ON t1.a = t2.a AND t2.b > 1, that can make t2.b look irrelevant and prune the only useful t2(b, ...) access path before costing.
Suggested change
case *logicalop.LogicalJoin:
// Extract join columns from EqualConditions and OtherConditions
currentJoinCols = append(currentJoinCols, expression.ExtractColumnsFromExpressions(nil, expression.ScalarFuncs2Exprs(x.EqualConditions), nil)...)
+ currentJoinCols = append(currentJoinCols, expression.ExtractColumnsFromExpressions(nil, x.LeftConditions, nil)...)
+ currentJoinCols = append(currentJoinCols, expression.ExtractColumnsFromExpressions(nil, x.RightConditions, nil)...)
currentJoinCols = append(currentJoinCols, expression.ExtractColumnsFromExpressions(nil, x.OtherConditions, nil)...)
case *logicalop.LogicalApply:
currentJoinCols = append(currentJoinCols, expression.ExtractColumnsFromExpressions(nil, expression.ScalarFuncs2Exprs(x.EqualConditions), nil)...)
+ currentJoinCols = append(currentJoinCols, expression.ExtractColumnsFromExpressions(nil, x.LeftConditions, nil)...)
+ currentJoinCols = append(currentJoinCols, expression.ExtractColumnsFromExpressions(nil, x.RightConditions, nil)...)
currentJoinCols = append(currentJoinCols, expression.ExtractColumnsFromExpressions(nil, x.OtherConditions, nil)...)🤖 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/collect_column_stats_usage.go` around lines 274 - 280, The
join-column collection for logicalop.LogicalJoin and logicalop.LogicalApply
omits outer/semi join filters; update the handlers for LogicalJoin and
LogicalApply to also extract columns from x.LeftConditions and x.RightConditions
(in addition to x.EqualConditions and x.OtherConditions) by calling
expression.ExtractColumnsFromExpressions on those slices and appending the
results to currentJoinCols so Left/RightConditions are considered during
pruning.
Comment on lines
+267
to
+280
| // Only update if pruning actually occurred (paths changed) | ||
| if len(prunedPaths) < len(ds.AllPossibleAccessPaths) { | ||
| // Union kept indexes instead of overwriting, to handle cases where the same physical table | ||
| // appears multiple times (e.g., self-joins, repeated subqueries). Each alias may keep different | ||
| // indexes, and we need to load stats for all indexes kept by any alias. | ||
| if existingKeptIndexes, exists := keptIndexIDs[ds.PhysicalTableID]; exists { | ||
| // Merge new indexes into existing set | ||
| for idxID := range tableKeptIndexes { | ||
| existingKeptIndexes[idxID] = struct{}{} | ||
| } | ||
| } else { | ||
| // First time seeing this physical table, create new set | ||
| keptIndexIDs[ds.PhysicalTableID] = tableKeptIndexes | ||
| } |
There was a problem hiding this comment.
Kept-index aggregation is incomplete for mixed pruned/unpruned aliases of the same table.
At Line 268, keptIndexIDs is only updated when the current alias is pruned. If another alias for the same PhysicalTableID is not pruned, its candidate indexes are never merged, but Line 501-505 still enforces table-level filtering. This can skip sync-loading stats for indexes still selectable by that unpruned alias.
💡 Suggested fix
- // Only update if pruning actually occurred (paths changed)
- if len(prunedPaths) < len(ds.AllPossibleAccessPaths) {
+ pruned := len(prunedPaths) < len(ds.AllPossibleAccessPaths)
+ // If this table already has a kept-index filter (from another alias), we still
+ // need to merge this alias's kept set even when this alias itself is not pruned.
+ if pruned || keptIndexIDs[ds.PhysicalTableID] != nil {
// Union kept indexes instead of overwriting, to handle cases where the same physical table
// appears multiple times (e.g., self-joins, repeated subqueries). Each alias may keep different
// indexes, and we need to load stats for all indexes kept by any alias.
if existingKeptIndexes, exists := keptIndexIDs[ds.PhysicalTableID]; exists {
// Merge new indexes into existing set
for idxID := range tableKeptIndexes {
existingKeptIndexes[idxID] = struct{}{}
}
} else {
// First time seeing this physical table, create new set
keptIndexIDs[ds.PhysicalTableID] = tableKeptIndexes
}
+ }
+ if pruned {
ds.AllPossibleAccessPaths = prunedPaths
// Make a copy for PossibleAccessPaths to avoid sharing the same slice
ds.PossibleAccessPaths = make([]*util.AccessPath, len(ds.AllPossibleAccessPaths))
copy(ds.PossibleAccessPaths, ds.AllPossibleAccessPaths)
}🤖 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/rule_collect_plan_stats.go` around lines 267 - 280, The bug
is that keptIndexIDs (map keyed by ds.PhysicalTableID) is only merged when the
current alias was pruned (len(prunedPaths) < len(ds.AllPossibleAccessPaths)), so
candidate indexes from aliases that were not pruned never get merged and their
indexes may be skipped; always merge tableKeptIndexes into keptIndexIDs for
ds.PhysicalTableID regardless of whether this alias was pruned. Update the logic
around keptIndexIDs, ds.PhysicalTableID, tableKeptIndexes and prunedPaths
(currently inside the if checking len(prunedPaths) <
len(ds.AllPossibleAccessPaths)) so that the union/merge into keptIndexIDs
happens unconditionally (or add an else branch that performs the same merge) to
ensure all aliases contributing candidate indexes are aggregated.
Comment on lines
+267
to
+287
| } else if path.Index != nil && tableColumns != nil { | ||
| // Fallback path: use Index.Columns (for static pruning mode when FullIdxCols is nil) | ||
| // Map IndexColumn.Offset to column ID via tableColumns | ||
| for _, idxCol := range path.Index.Columns { | ||
| if idxCol.Offset < 0 || idxCol.Offset >= len(tableColumns) { | ||
| continue | ||
| } | ||
| colInfo := tableColumns[idxCol.Offset] | ||
| if colInfo == nil { | ||
| continue | ||
| } | ||
| idxColID := colInfo.ID | ||
|
|
||
| // Check if this index column matches an interesting column | ||
| if _, found := req.interestingColIDs[idxColID]; found { | ||
| score.interestingCount++ | ||
| // Note: We cannot track consecutiveColumnIDs here because we don't have | ||
| // the full column information needed to determine if columns are consecutive | ||
| // in the index. This is acceptable as the user indicated. | ||
| } | ||
| } |
There was a problem hiding this comment.
Recover consecutive-prefix scoring in the FullIdxCols == nil fallback.
This branch already has the index order via path.Index.Columns, but it only bumps interestingCount. For static-pruned partition paths that strips both the prefix-match bonus and the phase-2 ordering key, so an (a, b) index can be ranked/pruned differently from the exact same non-partitioned path.
Suggested change
} else if path.Index != nil && tableColumns != nil {
// Fallback path: use Index.Columns (for static pruning mode when FullIdxCols is nil)
// Map IndexColumn.Offset to column ID via tableColumns
- for _, idxCol := range path.Index.Columns {
+ for i, idxCol := range path.Index.Columns {
if idxCol.Offset < 0 || idxCol.Offset >= len(tableColumns) {
continue
}
colInfo := tableColumns[idxCol.Offset]
if colInfo == nil {
continue
}
idxColID := colInfo.ID
// Check if this index column matches an interesting column
if _, found := req.interestingColIDs[idxColID]; found {
score.interestingCount++
- // Note: We cannot track consecutiveColumnIDs here because we don't have
- // the full column information needed to determine if columns are consecutive
- // in the index. This is acceptable as the user indicated.
+ if i == len(score.consecutiveColumnIDs) {
+ score.consecutiveColumnIDs = append(score.consecutiveColumnIDs, idxColID)
+ }
}
}
}🤖 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/rule/rule_prune_indexes.go` around lines 267 - 287, The
fallback handling in the else branch for path.Index.Columns only increments
score.interestingCount and omits consecutive-prefix tracking; update the loop
over path.Index.Columns (using idxCol.Offset, tableColumns and colInfo.ID) to
also detect consecutive matches in index order: keep a prevMatchedIndexPos (init
e.g. -2), on each matched idxCol (if req.interestingColIDs contains colInfo.ID)
increment score.interestingCount and if prevMatchedIndexPos+1 == current index
position then append the col ID to score.consecutiveColumnIDs (or increment the
existing consecutive-prefix counter on score), otherwise reset
prevMatchedIndexPos to current position (and start a new consecutive run); this
restores consecutive-prefix scoring while reusing path.Index.Columns,
req.interestingColIDs, score.interestingCount and consecutiveColumnIDs.
Comment on lines
+285
to
+291
| for name, val := range se.GetSessionVars().StmtCtx.StmtHints.SetVars { | ||
| oldV, err := se.GetSessionVars().SetSystemVarWithOldValAsRet(name, val) | ||
| if err != nil { | ||
| se.GetSessionVars().StmtCtx.AppendWarning(err) | ||
| } | ||
| se.GetSessionVars().StmtCtx.AddSetVarHintRestore(name, oldV) | ||
| } |
There was a problem hiding this comment.
Restore hinted session variables inside the helper.
Line 285 mutates session vars for SET_VAR, but this helper never runs the normal statement cleanup path that consumes the restore list. That can leak hint values into later helper calls and make assertions order-dependent.
Suggested fix
- // Apply SET_VAR hints to session variables
+ // Apply SET_VAR hints to session variables and restore them before returning.
+ restoreSetVars := make(map[string]string, len(se.GetSessionVars().StmtCtx.StmtHints.SetVars))
for name, val := range se.GetSessionVars().StmtCtx.StmtHints.SetVars {
oldV, err := se.GetSessionVars().SetSystemVarWithOldValAsRet(name, val)
if err != nil {
se.GetSessionVars().StmtCtx.AppendWarning(err)
}
se.GetSessionVars().StmtCtx.AddSetVarHintRestore(name, oldV)
+ restoreSetVars[name] = oldV
}
+ defer func() {
+ for name, oldV := range restoreSetVars {
+ _, _ = se.GetSessionVars().SetSystemVarWithOldValAsRet(name, oldV)
+ }
+ }()As per coding guidelines, “Test files: Prefer extending existing test suites and fixtures over creating new scaffolding; keep test changes minimal and deterministic.”
🤖 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/stats_test.go` around lines 285 - 291, The helper mutates
session vars via se.GetSessionVars().SetSystemVarWithOldValAsRet and records
restores with StmtCtx.AddSetVarHintRestore but never consumes that restore list,
leaking hints into later tests; fix by immediately consuming/restoring those
entries inside the helper after setting them (either by invoking the same
cleanup routine the normal statement path uses or by iterating the recorded
restores and calling SetSystemVarWithOldValAsRet for each saved old value and
then clearing the restore list), ensuring StmtCtx.AddSetVarHintRestore entries
are applied/cleared before the helper returns.
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/core/collect_column_stats_usage.go (1)
135-164:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore
AskedColumnGrouppropagation toDataSource.Line 262 still derives
curColGroups, but after dropping the parameter here nothing consumes it anymore. That silently stops populating the datasource's group-NDV requests, so multi-column cardinality estimates can regress outside the new pruning path as well.Suggested fix
-func (c *columnStatsUsageCollector) collectPredicateColumnsForDataSource(ds *logicalop.DataSource) { +func (c *columnStatsUsageCollector) collectPredicateColumnsForDataSource(ds *logicalop.DataSource, askedColGroups [][]*expression.Column) { // Skip all system tables. if filter.IsSystemSchema(ds.DBName.L) { intest.Assert(!ds.SCtx().GetSessionVars().InRestrictedSQL, "system table should have been skipped in restricted SQL mode") return } + ds.AskedColumnGroup = askedColGroups // For partition tables, no matter whether it is static or dynamic pruning mode, we use table ID rather than partition ID to // set TableColumnID.TableID. In this way, we keep the set of predicate columns consistent between different partitions and global table. tblID := ds.TableInfo.ID ... }case *logicalop.DataSource: - c.collectPredicateColumnsForDataSource(x) + c.collectPredicateColumnsForDataSource(x, curColGroups) // Collect all interesting columns (WHERE + JOIN + ORDERING) for index pruning if c.interestingColsByDS != nil { c.collectInterestingColumnsForDataSource(x, currentJoinCols, currentOrderingCols) } case *logicalop.LogicalIndexScan: - c.collectPredicateColumnsForDataSource(x.Source) + c.collectPredicateColumnsForDataSource(x.Source, curColGroups) c.addPredicateColumnsFromExpressions(x.AccessConds, true) case *logicalop.LogicalTableScan: - c.collectPredicateColumnsForDataSource(x.Source) + c.collectPredicateColumnsForDataSource(x.Source, curColGroups) c.addPredicateColumnsFromExpressions(x.AccessConds, true)Also applies to: 260-262, 307-319
🤖 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/collect_column_stats_usage.go` around lines 135 - 164, The change removed propagation of AskedColumnGroup into DataSource so curColGroups is computed but never applied, causing group-NDV requests to be dropped; restore passing and applying the column-group info: in collectPredicateColumnsForDataSource ensure the computed curColGroups (or the parameter previously supplied) is forwarded into the DataSource context (e.g., set DataSource.AskedColumnGroup or call the same helper that used to accept curColGroups) before calling addPredicateColumnsFromExpressions, and make any corresponding callers (e.g., collectFromPlan / collectInterestingColumnsForDataSource) pass the curColGroups through so AskedColumnGroup is populated for multi-column NDV estimates.
🤖 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.
Outside diff comments:
In `@pkg/planner/core/collect_column_stats_usage.go`:
- Around line 135-164: The change removed propagation of AskedColumnGroup into
DataSource so curColGroups is computed but never applied, causing group-NDV
requests to be dropped; restore passing and applying the column-group info: in
collectPredicateColumnsForDataSource ensure the computed curColGroups (or the
parameter previously supplied) is forwarded into the DataSource context (e.g.,
set DataSource.AskedColumnGroup or call the same helper that used to accept
curColGroups) before calling addPredicateColumnsFromExpressions, and make any
corresponding callers (e.g., collectFromPlan /
collectInterestingColumnsForDataSource) pass the curColGroups through so
AskedColumnGroup is populated for multi-column NDV estimates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6c1f9560-7986-42e9-aedd-c83fd41d4a2a
📒 Files selected for processing (5)
pkg/planner/core/collect_column_stats_usage.gopkg/planner/core/operator/logicalop/logical_datasource.gopkg/planner/core/rule_collect_plan_stats.gopkg/planner/core/stats.gopkg/planner/core/stats_test.go
✅ Files skipped from review due to trivial changes (2)
- pkg/planner/core/operator/logicalop/logical_datasource.go
- pkg/planner/core/stats.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/planner/core/rule_collect_plan_stats.go
- pkg/planner/core/stats_test.go
Codecov Report❌ Patch coverage is Please upload reports for the commit 9e5cd66 to get more accurate results. Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #68988 +/- ##
================================================
Coverage ? 24.5262%
================================================
Files ? 1739
Lines ? 643421
Branches ? 0
================================================
Hits ? 157807
Misses ? 465432
Partials ? 20182
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@qw4990: 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. |
ti-chi-bot
Bot
added
cherry-pick-approved
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
1 participant
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.
This is an automated cherry-pick of #64999
What problem does this PR solve?
Issue Number: close #63856
Problem Summary:
What changed and how does it work?
Prior PR title:
planner: index pruning using existing infra | tidb-test=pr/2661
Latest commit results in no mysql test changes.
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
Improvements
Tests
Behavior Changes