Repository navigation
planner: discount equality-bound clustered-key prefix in index pruning | tidb-test=pr/2768 (#69726) - #70206
planner: discount equality-bound clustered-key prefix in index pruning | tidb-test=pr/2768 (#69726)#70206ti-chi-bot wants to merge 1 commit into
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
do-not-merge/hold
|
This cherry pick PR is for a release branch and has not yet been approved by triage owners. To merge this cherry pick:
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. |
|
@terry1purcell This PR has conflicts, I have hold it. |
|
@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide. 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 ti-community-infra/tichi repository. |
|
[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 |
📝 WalkthroughWalkthroughThe PR revises clustered-handle index paths, index-pruning scoring and selection, ORDER BY matching, JSON selectivity handling, and planner regression coverage. It also adds shared-prefix benchmarks and updates expected plan outputs. ChangesIndex pruning and planner behavior
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant QueryPlanner
participant DataSource
participant Stats
participant IndexPruner
QueryPlanner->>DataSource: build effective clustered-handle columns
DataSource->>Stats: fill index path and build ranges
Stats-->>IndexPruner: effective keys and access statistics
IndexPruner->>IndexPruner: score, dominate, and select indexes
IndexPruner-->>QueryPlanner: retained access paths
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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. 🔧 Biome (2.5.5)pkg/executor/testdata/slow_query_suite_out.jsonFile contains syntax errors that prevent linting: Line 23: unexpected character 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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: 9
🤖 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/casetest/index/index_test.go`:
- Line 498: Update the callback type in the failpoint.EnableCall invocation to
use the file’s existing plannerutil.AccessPath alias instead of util.AccessPath,
matching the callback near line 412 and the configured import alias.
In `@pkg/planner/core/casetest/integration_test.go`:
- Line 314: Resolve the cherry-pick conflicts in
pkg/planner/core/casetest/integration_test.go at lines 314-314, 576-576, and
363-455: remove all conflict markers and retain one cascades-wrapped
TestTiFlashFineGrainedShuffle body while deleting the orphaned loop; keep only
one TestIssue52023/TestIssue56915 block; remove duplicate TestFixControl43817,
TestFixControl45132, TestTiFlashExtraColumnPrune, and
TestIndexMergeJSONMemberOf2FlakyPart declarations; and consolidate CIStr to a
single package import so the file compiles.
- Line 346: Update the testkit helper integration used by integrationSuiteData
so the calls to LoadTestCases and testkit.RunTestUnderCascadesWithDomain compile
with their current arguments, either by porting the required helper
implementations/signatures or adding backwards-compatible wrappers. Preserve
compatibility with the existing LoadTestCases(t, in, out) and
RunTestUnderCascades APIs.
- Around line 350-358: Replace the `set global tidb_redact_log` statements in
the test flow with session-scoped updates, preserving the existing off/on
sequencing around each `MustQuery` and validation. Keep the changes limited to
the `tidb_redact_log` setup in this test so state does not leak across sibling
subtests.
In `@pkg/planner/core/find_best_task.go`:
- Around line 933-984: Resolve all cherry-pick conflict markers and preserve the
intended current behavior: in pkg/planner/core/find_best_task.go:933-984,
reconcile matchProperty around idxCols and prop.SortItems into one buildable
implementation; in pkg/executor/testdata/slow_query_suite_out.json:23-27, select
the intended warning/plan-note output and restore valid JSON; in
tests/integrationtest/r/planner/core/indexmerge_path.result:706-714 and
tests/integrationtest/r/planner/core/issuetest/planner_issue.result:712-732,
retain one deterministic expected plan output at each site. Remove every
conflict marker and do not leave duplicate or ambiguous alternatives.
- Around line 946-983: Update the property-matching loop following the idxCols
length check to use the effective idxCols and idxColLens variables, rather than
path.IdxCols and path.IdxColLens, so appended CommonHandle columns participate
in ORDER BY prefix matching.
In `@pkg/planner/core/operator/logicalop/logical_datasource.go`:
- Around line 691-724: Handle a nil ds.TableInfo before the primary-key-handle
fallback in the surrounding datasource column logic. Add an early return for nil
TableInfo, or otherwise align the condition with GetPKIsHandleCol so it cannot
dereference missing table metadata; preserve the existing common-handle behavior
and non-nil fallback path.
In `@pkg/planner/core/stats.go`:
- Around line 203-254: Remove all unresolved cherry-pick conflict markers across
the five affected sites. In pkg/planner/core/stats.go lines 203-254, retain
pathRangesIncludeAppendedHandle and adjustCountAfterAccess, ensuring no
duplicate adjustCountAfterAccess remains; in
pkg/planner/core/rule/rule_prune_indexes.go lines 469-481, retain the
bounds-checked tableColumns[col.Offset] branch; in
pkg/planner/core/rule/BUILD.bazel lines 24-63, resolve dependencies and the
go_test block, then run make bazel_prepare; in
pkg/planner/core/casetest/index/index_test.go lines 451-452, retain both new
tests; and in pkg/planner/core/casetest/index/BUILD.bazel lines 13-17, retain
shard_count = 13.
- Around line 219-252: Resolve the merge conflict in adjustCountAfterAccess so
stats.go contains valid Go without conflict markers and only one consistent
implementation. Verify that pathRangesIncludeAppendedHandle and all
CountAfterAccess fields match the current util.AccessPath definition, retaining
the branch’s intended adjustment behavior.
🪄 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 Plus
Run ID: 23b334e9-77f9-4626-b55b-64a9c4a8745d
📒 Files selected for processing (15)
pkg/executor/test/analyzetest/analyze_test.gopkg/executor/testdata/slow_query_suite_out.jsonpkg/planner/cardinality/selectivity.gopkg/planner/core/casetest/index/BUILD.bazelpkg/planner/core/casetest/index/index_prune_bench_test.gopkg/planner/core/casetest/index/index_test.gopkg/planner/core/casetest/integration_test.gopkg/planner/core/find_best_task.gopkg/planner/core/operator/logicalop/logical_datasource.gopkg/planner/core/rule/BUILD.bazelpkg/planner/core/rule/rule_prune_indexes.gopkg/planner/core/rule/rule_prune_indexes_internal_test.gopkg/planner/core/stats.gotests/integrationtest/r/planner/core/indexmerge_path.resulttests/integrationtest/r/planner/core/issuetest/planner_issue.result
| // callbacks identify relevance via the tenant_ws leading column. | ||
| var keptIndexes []string | ||
| seen := false | ||
| require.NoError(t, failpoint.EnableCall(fpName, func(paths []*util.AccessPath) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Import alias mismatch: util.AccessPath vs plannerutil.AccessPath.
The existing callback at Line 412 uses plannerutil.AccessPath, so pkg/planner/util is imported under the plannerutil alias in this file. The new callback references util.AccessPath, which will not resolve.
🐛 Proposed fix
- require.NoError(t, failpoint.EnableCall(fpName, func(paths []*util.AccessPath) {
+ require.NoError(t, failpoint.EnableCall(fpName, func(paths []*plannerutil.AccessPath) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| require.NoError(t, failpoint.EnableCall(fpName, func(paths []*util.AccessPath) { | |
| require.NoError(t, failpoint.EnableCall(fpName, func(paths []*plannerutil.AccessPath) { |
🤖 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/casetest/index/index_test.go` at line 498, Update the
callback type in the failpoint.EnableCall invocation to use the file’s existing
plannerutil.AccessPath alias instead of util.AccessPath, matching the callback
near line 412 and the configured import alias.
| tk.MustExec("drop table if exists t1;") | ||
| tk.MustExec("create table t1(c1 int, c2 int)") | ||
|
|
||
| <<<<<<< HEAD |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Unresolved cherry-pick conflict leaves the file uncompilable. Both sides of the merge are committed verbatim, so conflict markers, duplicate test declarations, and mixed pmodel/ast identifiers all stem from the same unresolved resolution.
pkg/planner/core/casetest/integration_test.go#L314-L314: remove the<<<<<<< HEAD/=======/>>>>>>> 22fd1ac756emarkers aroundTestTiFlashFineGrainedShuffleand keep a single body (the cascades variant wrapped intestkit.RunTestUnderCascades, discarding the orphaned loop at Lines 464-469).pkg/planner/core/casetest/integration_test.go#L576-L576: resolve the second conflict region inTestIssue52023, choosing either the HEAD assertions plus standaloneTestIssue56915or the cherry-pickedt_issue52023/t_issue56915/issue-63290 block, not both.pkg/planner/core/casetest/integration_test.go#L363-L455: delete the now-duplicated HEAD copies ofTestFixControl43817,TestFixControl45132,TestTiFlashExtraColumnPrune, andTestIndexMergeJSONMemberOf2FlakyPart(Lines 512-552, 599-650) and normalize theCIStrimport to a single package.
📍 Affects 1 file
pkg/planner/core/casetest/integration_test.go#L314-L314(this comment)pkg/planner/core/casetest/integration_test.go#L576-L576pkg/planner/core/casetest/integration_test.go#L363-L455
🤖 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/casetest/integration_test.go` at line 314, Resolve the
cherry-pick conflicts in pkg/planner/core/casetest/integration_test.go at lines
314-314, 576-576, and 363-455: remove all conflict markers and retain one
cascades-wrapped TestTiFlashFineGrainedShuffle body while deleting the orphaned
loop; keep only one TestIssue52023/TestIssue56915 block; remove duplicate
TestFixControl43817, TestFixControl45132, TestTiFlashExtraColumnPrune, and
TestIndexMergeJSONMemberOf2FlakyPart declarations; and consolidate CIStr to a
single package import so the file compiles.
| Redact []string | ||
| } | ||
| integrationSuiteData := GetIntegrationSuiteData() | ||
| integrationSuiteData.LoadTestCases(t, &input, &output, cascades, caller) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'testdata.go' pkg/testkit | xargs rg -nP -A6 'func \(td \*TestData\) LoadTestCases'
rg -nP -A4 'func RunTestUnderCascades(WithDomain)?\(' pkg/testkitRepository: pingcap/tidb
Length of output: 1146
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Target file context:"
sed -n '320,440p' pkg/planner/core/casetest/integration_test.go | cat -n
echo
echo "Search for updated helper names/usages:"
rg -n "LoadTestCases\(|RunTestUnderCascades(WithDomain)?\(" pkg testdata pkg/testkit pkg/planner/core/casetest 2>/dev/null || true
echo
echo "Locate all testkit-related files mentioning TestData:"
fd -t f 'testdata|mockstore' pkg/testkit | xargs -r sed -n '1,220p'Repository: pingcap/tidb
Length of output: 50368
Cherry-pick the testkit helper changes or port the calls.
pkg/testkit/testdata/testdata.go only defines LoadTestCases(t, in, out) and RunTestUnderCascades(t, ...), while this file calls LoadTestCases(t, &input, &output, cascades, caller) at lines 346 and 423 and testkit.RunTestUnderCascadesWithDomain(t, ...) at line 406. Port the helper signatures/backwards-compat wrappers along with these tests so it compiles.
🤖 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/casetest/integration_test.go` at line 346, Update the
testkit helper integration used by integrationSuiteData so the calls to
LoadTestCases and testkit.RunTestUnderCascadesWithDomain compile with their
current arguments, either by porting the required helper
implementations/signatures or adding backwards-compatible wrappers. Preserve
compatibility with the existing LoadTestCases(t, in, out) and
RunTestUnderCascades APIs.
| tk.MustExec("set global tidb_redact_log=off") | ||
| output[i].Plan = testdata.ConvertRowsToStrings(tk.MustQuery(tt).Rows()) | ||
| tk.MustExec("set global tidb_redact_log=on") | ||
| output[i].Redact = testdata.ConvertRowsToStrings(tk.MustQuery(tt).Rows()) | ||
| }) | ||
| tk.MustExec("set global tidb_redact_log=off") | ||
| tk.MustQuery(tt).Check(testkit.Rows(output[i].Plan...)) | ||
| tk.MustExec("set global tidb_redact_log=on") | ||
| tk.MustQuery(tt).Check(testkit.Rows(output[i].Redact...)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prefer session-scoped tidb_redact_log over set global.
set global doesn't apply deterministically to the current session (it propagates via the sysvar cache), so the immediately following MustQuery may still observe the previous value — a flake source. It also leaks state into the sibling cascades subtest since each subtest builds its own store/session. The prior code used set session; keep that unless global scope is required.
♻️ Suggested change
- tk.MustExec("set global tidb_redact_log=off")
+ tk.MustExec("set session tidb_redact_log=off")
output[i].Plan = testdata.ConvertRowsToStrings(tk.MustQuery(tt).Rows())
- tk.MustExec("set global tidb_redact_log=on")
+ tk.MustExec("set session tidb_redact_log=on")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tk.MustExec("set global tidb_redact_log=off") | |
| output[i].Plan = testdata.ConvertRowsToStrings(tk.MustQuery(tt).Rows()) | |
| tk.MustExec("set global tidb_redact_log=on") | |
| output[i].Redact = testdata.ConvertRowsToStrings(tk.MustQuery(tt).Rows()) | |
| }) | |
| tk.MustExec("set global tidb_redact_log=off") | |
| tk.MustQuery(tt).Check(testkit.Rows(output[i].Plan...)) | |
| tk.MustExec("set global tidb_redact_log=on") | |
| tk.MustQuery(tt).Check(testkit.Rows(output[i].Redact...)) | |
| tk.MustExec("set session tidb_redact_log=off") | |
| output[i].Plan = testdata.ConvertRowsToStrings(tk.MustQuery(tt).Rows()) | |
| tk.MustExec("set session tidb_redact_log=on") | |
| output[i].Redact = testdata.ConvertRowsToStrings(tk.MustQuery(tt).Rows()) | |
| }) | |
| tk.MustExec("set session tidb_redact_log=off") | |
| tk.MustQuery(tt).Check(testkit.Rows(output[i].Plan...)) | |
| tk.MustExec("set session tidb_redact_log=on") | |
| tk.MustQuery(tt).Check(testkit.Rows(output[i].Redact...)) |
🤖 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/casetest/integration_test.go` around lines 350 - 358,
Replace the `set global tidb_redact_log` statements in the test flow with
session-scoped updates, preserving the existing off/on sequencing around each
`MustQuery` and validation. Keep the changes limited to the `tidb_redact_log`
setup in this test so state does not leak across sibling subtests.
| <<<<<<< HEAD | ||
| // Basically, if `prop.SortItems` is the prefix of `path.IdxCols`, then the property is matched. | ||
| ======= | ||
| // For non-unique secondary indexes on CommonHandle tables, the index physically | ||
| // stores (index_cols, PK_cols). Build an effective column list that includes the | ||
| // PK suffix so that ORDER BY on PK columns can be recognised. | ||
| // | ||
| // Skip this when CommonHandleVersion == 0 with new collation enabled and | ||
| // the handle contains non-binary string columns. In v0, string handle columns | ||
| // are stored as sortKey bytes (collation weight), not original values. If we | ||
| // extend idxCols and the query triggers PropMatchedNeedMergeSort (e.g. IN | ||
| // predicate), the merge-sort comparator would apply the column's collation to | ||
| // sortKey bytes, producing incorrect ordering. | ||
| idxCols := path.IdxCols | ||
| idxColLens := path.IdxColLens | ||
| if path.Index != nil && !path.Index.Unique && !path.Index.Primary && | ||
| ds.TableInfo.IsCommonHandle && len(ds.CommonHandleCols) > 0 && | ||
| len(ds.CommonHandleLens) == len(ds.CommonHandleCols) && | ||
| len(path.Index.Columns) == len(path.IdxCols) && | ||
| !ds.HasV0NewCollationStringHandle() { | ||
| extended := false | ||
| for i, handleCol := range ds.CommonHandleCols { | ||
| if handleCol == nil { | ||
| continue | ||
| } | ||
| alreadyInIndex := false | ||
| for _, col := range idxCols { | ||
| if col != nil && col.EqualColumn(handleCol) { | ||
| alreadyInIndex = true | ||
| break | ||
| } | ||
| } | ||
| if !alreadyInIndex { | ||
| if !extended { | ||
| idxCols = make([]*expression.Column, len(path.IdxCols), len(path.IdxCols)+len(ds.CommonHandleCols)) | ||
| copy(idxCols, path.IdxCols) | ||
| idxColLens = make([]int, len(path.IdxColLens), len(path.IdxColLens)+len(ds.CommonHandleCols)) | ||
| copy(idxColLens, path.IdxColLens) | ||
| extended = true | ||
| } | ||
| idxCols = append(idxCols, handleCol) | ||
| idxColLens = append(idxColLens, ds.CommonHandleLens[i]) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if len(idxCols) < len(prop.SortItems) { | ||
| return property.PropNotMatched | ||
| } | ||
|
|
||
| // Basically, if `prop.SortItems` is the prefix of `idxCols`, then the property is matched. | ||
| >>>>>>> 22fd1ac756e (planner: discount equality-bound clustered-key prefix in index pruning (#69726)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Resolve the remaining cherry-pick conflicts before merge. These markers leave Go source unbuildable and test fixtures invalid or ambiguous.
pkg/planner/core/find_best_task.go#L933-L984: reconcile the old and cherry-pickedmatchPropertyimplementations and remove all conflict markers.pkg/executor/testdata/slow_query_suite_out.json#L23-L27: choose the intended warning/plan-note output and restore valid JSON.tests/integrationtest/r/planner/core/indexmerge_path.result#L706-L714: retain one deterministic expected plan output and remove the markers.tests/integrationtest/r/planner/core/issuetest/planner_issue.result#L712-L732: retain one deterministic expected plan output and remove the markers.
📍 Affects 4 files
pkg/planner/core/find_best_task.go#L933-L984(this comment)pkg/executor/testdata/slow_query_suite_out.json#L23-L27tests/integrationtest/r/planner/core/indexmerge_path.result#L706-L714tests/integrationtest/r/planner/core/issuetest/planner_issue.result#L712-L732
🤖 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/find_best_task.go` around lines 933 - 984, Resolve all
cherry-pick conflict markers and preserve the intended current behavior: in
pkg/planner/core/find_best_task.go:933-984, reconcile matchProperty around
idxCols and prop.SortItems into one buildable implementation; in
pkg/executor/testdata/slow_query_suite_out.json:23-27, select the intended
warning/plan-note output and restore valid JSON; in
tests/integrationtest/r/planner/core/indexmerge_path.result:706-714 and
tests/integrationtest/r/planner/core/issuetest/planner_issue.result:712-732,
retain one deterministic expected plan output at each site. Remove every
conflict marker and do not leave duplicate or ambiguous alternatives.
Source: Linters/SAST tools
| idxCols := path.IdxCols | ||
| idxColLens := path.IdxColLens | ||
| if path.Index != nil && !path.Index.Unique && !path.Index.Primary && | ||
| ds.TableInfo.IsCommonHandle && len(ds.CommonHandleCols) > 0 && | ||
| len(ds.CommonHandleLens) == len(ds.CommonHandleCols) && | ||
| len(path.Index.Columns) == len(path.IdxCols) && | ||
| !ds.HasV0NewCollationStringHandle() { | ||
| extended := false | ||
| for i, handleCol := range ds.CommonHandleCols { | ||
| if handleCol == nil { | ||
| continue | ||
| } | ||
| alreadyInIndex := false | ||
| for _, col := range idxCols { | ||
| if col != nil && col.EqualColumn(handleCol) { | ||
| alreadyInIndex = true | ||
| break | ||
| } | ||
| } | ||
| if !alreadyInIndex { | ||
| if !extended { | ||
| idxCols = make([]*expression.Column, len(path.IdxCols), len(path.IdxCols)+len(ds.CommonHandleCols)) | ||
| copy(idxCols, path.IdxCols) | ||
| idxColLens = make([]int, len(path.IdxColLens), len(path.IdxColLens)+len(ds.CommonHandleCols)) | ||
| copy(idxColLens, path.IdxColLens) | ||
| extended = true | ||
| } | ||
| idxCols = append(idxCols, handleCol) | ||
| idxColLens = append(idxColLens, ds.CommonHandleLens[i]) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if len(idxCols) < len(prop.SortItems) { | ||
| return property.PropNotMatched | ||
| } | ||
|
|
||
| // Basically, if `prop.SortItems` is the prefix of `idxCols`, then the property is matched. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the effective columns during property matching.
The subsequent loop still reads path.IdxCols/path.IdxColLens, so appended CommonHandle suffix columns only pass the length check and can never match an ORDER BY. Iterate over idxCols and idxColLens in the matching loop.
Proposed fix
- for ; colIdx < len(path.IdxCols); colIdx++ {
- if path.IdxColLens[colIdx] == types.UnspecifiedLength && sortItem.Col.EqualColumn(path.IdxCols[colIdx]) {
+ for ; colIdx < len(idxCols); colIdx++ {
+ if idxColLens[colIdx] == types.UnspecifiedLength && sortItem.Col.EqualColumn(idxCols[colIdx]) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| idxCols := path.IdxCols | |
| idxColLens := path.IdxColLens | |
| if path.Index != nil && !path.Index.Unique && !path.Index.Primary && | |
| ds.TableInfo.IsCommonHandle && len(ds.CommonHandleCols) > 0 && | |
| len(ds.CommonHandleLens) == len(ds.CommonHandleCols) && | |
| len(path.Index.Columns) == len(path.IdxCols) && | |
| !ds.HasV0NewCollationStringHandle() { | |
| extended := false | |
| for i, handleCol := range ds.CommonHandleCols { | |
| if handleCol == nil { | |
| continue | |
| } | |
| alreadyInIndex := false | |
| for _, col := range idxCols { | |
| if col != nil && col.EqualColumn(handleCol) { | |
| alreadyInIndex = true | |
| break | |
| } | |
| } | |
| if !alreadyInIndex { | |
| if !extended { | |
| idxCols = make([]*expression.Column, len(path.IdxCols), len(path.IdxCols)+len(ds.CommonHandleCols)) | |
| copy(idxCols, path.IdxCols) | |
| idxColLens = make([]int, len(path.IdxColLens), len(path.IdxColLens)+len(ds.CommonHandleCols)) | |
| copy(idxColLens, path.IdxColLens) | |
| extended = true | |
| } | |
| idxCols = append(idxCols, handleCol) | |
| idxColLens = append(idxColLens, ds.CommonHandleLens[i]) | |
| } | |
| } | |
| } | |
| if len(idxCols) < len(prop.SortItems) { | |
| return property.PropNotMatched | |
| } | |
| // Basically, if `prop.SortItems` is the prefix of `idxCols`, then the property is matched. | |
| for ; colIdx < len(idxCols); colIdx++ { | |
| if idxColLens[colIdx] == types.UnspecifiedLength && sortItem.Col.EqualColumn(idxCols[colIdx]) { |
🤖 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/find_best_task.go` around lines 946 - 983, Update the
property-matching loop following the idxCols length check to use the effective
idxCols and idxColLens variables, rather than path.IdxCols and path.IdxColLens,
so appended CommonHandle columns participate in ORDER BY prefix matching.
| if ds.TableInfo != nil && ds.TableInfo.IsCommonHandle { | ||
| if len(ds.CommonHandleCols) == 0 || len(ds.CommonHandleLens) != len(ds.CommonHandleCols) { | ||
| return nil, nil | ||
| } | ||
| // Global indexes (V1+) encode the partition ID between the index columns and the | ||
| // handle, and MV/columnar indexes build their ranges specially, so appended columns | ||
| // would not align with the physical key layout. V0-new-collation string handles are | ||
| // stored as sortKey bytes without restored data and cannot be ranged over. | ||
| if path.Index.Global || path.Index.MVIndex || path.Index.IsColumnarIndex() || | ||
| ds.HasV0NewCollationStringHandle() { | ||
| return nil, nil | ||
| } | ||
| for _, handleCol := range ds.CommonHandleCols { | ||
| if handleCol == nil { | ||
| return nil, nil | ||
| } | ||
| for _, col := range declaredCols { | ||
| if col.EqualColumn(handleCol) { | ||
| return nil, nil | ||
| } | ||
| } | ||
| } | ||
| return ds.CommonHandleCols, ds.CommonHandleLens | ||
| } | ||
| handleCol := ds.GetPKIsHandleCol() | ||
| if handleCol == nil || mysql.HasUnsignedFlag(handleCol.RetType.GetFlag()) { | ||
| return nil, nil | ||
| } | ||
| for _, col := range declaredCols { | ||
| if col.ID == model.ExtraHandleID || col.EqualColumn(handleCol) { | ||
| return nil, nil | ||
| } | ||
| } | ||
| return []*expression.Column{handleCol}, []int{types.UnspecifiedLength} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Inconsistent ds.TableInfo nil handling.
Line 691 explicitly tolerates ds.TableInfo == nil, but the fallthrough at Line 715 calls ds.GetPKIsHandleCol(), which dereferences ds.TableInfo.PKIsHandle. Either drop the nil check (matching the surrounding convention, e.g. GetPKIsHandleCol itself) or bail out early.
🛡️ Proposed fix
- if ds.TableInfo != nil && ds.TableInfo.IsCommonHandle {
+ if ds.TableInfo == nil {
+ return nil, nil
+ }
+ if ds.TableInfo.IsCommonHandle {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ds.TableInfo != nil && ds.TableInfo.IsCommonHandle { | |
| if len(ds.CommonHandleCols) == 0 || len(ds.CommonHandleLens) != len(ds.CommonHandleCols) { | |
| return nil, nil | |
| } | |
| // Global indexes (V1+) encode the partition ID between the index columns and the | |
| // handle, and MV/columnar indexes build their ranges specially, so appended columns | |
| // would not align with the physical key layout. V0-new-collation string handles are | |
| // stored as sortKey bytes without restored data and cannot be ranged over. | |
| if path.Index.Global || path.Index.MVIndex || path.Index.IsColumnarIndex() || | |
| ds.HasV0NewCollationStringHandle() { | |
| return nil, nil | |
| } | |
| for _, handleCol := range ds.CommonHandleCols { | |
| if handleCol == nil { | |
| return nil, nil | |
| } | |
| for _, col := range declaredCols { | |
| if col.EqualColumn(handleCol) { | |
| return nil, nil | |
| } | |
| } | |
| } | |
| return ds.CommonHandleCols, ds.CommonHandleLens | |
| } | |
| handleCol := ds.GetPKIsHandleCol() | |
| if handleCol == nil || mysql.HasUnsignedFlag(handleCol.RetType.GetFlag()) { | |
| return nil, nil | |
| } | |
| for _, col := range declaredCols { | |
| if col.ID == model.ExtraHandleID || col.EqualColumn(handleCol) { | |
| return nil, nil | |
| } | |
| } | |
| return []*expression.Column{handleCol}, []int{types.UnspecifiedLength} | |
| if ds.TableInfo == nil { | |
| return nil, nil | |
| } | |
| if ds.TableInfo.IsCommonHandle { | |
| if len(ds.CommonHandleCols) == 0 || len(ds.CommonHandleLens) != len(ds.CommonHandleCols) { | |
| return nil, nil | |
| } | |
| // Global indexes (V1+) encode the partition ID between the index columns and the | |
| // handle, and MV/columnar indexes build their ranges specially, so appended columns | |
| // would not align with the physical key layout. V0-new-collation string handles are | |
| // stored as sortKey bytes without restored data and cannot be ranged over. | |
| if path.Index.Global || path.Index.MVIndex || path.Index.IsColumnarIndex() || | |
| ds.HasV0NewCollationStringHandle() { | |
| return nil, nil | |
| } | |
| for _, handleCol := range ds.CommonHandleCols { | |
| if handleCol == nil { | |
| return nil, nil | |
| } | |
| for _, col := range declaredCols { | |
| if col.EqualColumn(handleCol) { | |
| return nil, nil | |
| } | |
| } | |
| } | |
| return ds.CommonHandleCols, ds.CommonHandleLens | |
| } | |
| handleCol := ds.GetPKIsHandleCol() | |
| if handleCol == nil || mysql.HasUnsignedFlag(handleCol.RetType.GetFlag()) { | |
| return nil, nil | |
| } | |
| for _, col := range declaredCols { | |
| if col.ID == model.ExtraHandleID || col.EqualColumn(handleCol) { | |
| return nil, nil | |
| } | |
| } | |
| return []*expression.Column{handleCol}, []int{types.UnspecifiedLength} |
🤖 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/operator/logicalop/logical_datasource.go` around lines 691 -
724, Handle a nil ds.TableInfo before the primary-key-handle fallback in the
surrounding datasource column logic. Add an early return for nil TableInfo, or
otherwise align the condition with GetPKIsHandleCol so it cannot dereference
missing table metadata; preserve the existing common-handle behavior and non-nil
fallback path.
| <<<<<<< HEAD | ||
| ======= | ||
| // pathRangesIncludeAppendedHandle reports whether the ranges of a non-unique index path | ||
| // extend past the declared index columns into the appended handle columns. | ||
| func pathRangesIncludeAppendedHandle(path *util.AccessPath) bool { | ||
| if path.Index == nil || len(path.IdxCols) <= len(path.Index.Columns) { | ||
| return false | ||
| } | ||
| for _, ran := range path.Ranges { | ||
| if len(ran.LowVal) > len(path.Index.Columns) || len(ran.HighVal) > len(path.Index.Columns) { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| // adjustCountAfterAccess adjusts the CountAfterAccess when it's less than the estimated table row count. | ||
| func adjustCountAfterAccess(ds *logicalop.DataSource, path *util.AccessPath) { | ||
| // If the `CountAfterAccess` is less than `stats.RowCount`, it means that paths were estimated using | ||
| // different assumptions regarding individual or compound selectivity estimates. | ||
| // We prefer the `stats.RowCount` to provide consistency in estimation across all paths. | ||
| // Add an arbitrary tolerance factor to account for comparison with floating point | ||
| if (path.CountAfterAccess + cost.ToleranceFactor) < ds.StatsInfo().RowCount { | ||
| // When the ranges include the appended handle columns, the handle predicates were | ||
| // credited with deliberately damped exponential backoff, so falling below the | ||
| // independence-leaning stats.RowCount is expected rather than a sign of | ||
| // inconsistent assumptions. Align to stats.RowCount without the SelectionFactor | ||
| // penalty so the credited path is not made more expensive than an uncredited one. | ||
| if pathRangesIncludeAppendedHandle(path) { | ||
| if path.MinCountAfterAccess > 0 { | ||
| path.MinCountAfterAccess = min(path.MinCountAfterAccess, path.CountAfterAccess) | ||
| } else { | ||
| path.MinCountAfterAccess = path.CountAfterAccess | ||
| } | ||
| path.CountAfterAccess = ds.StatsInfo().RowCount | ||
| path.MaxCountAfterAccess = max(path.CountAfterAccess, path.MaxCountAfterAccess) | ||
| return | ||
| } | ||
| // Store the MinCountAfterAccess "before" adjusting the "CountAfterAccess". This can be used to differentiate | ||
| // the "Min" estimate for each index/inthandle path when CountAfterAccess has been equalized. | ||
| if path.MinCountAfterAccess > 0 { | ||
| path.MinCountAfterAccess = min(path.MinCountAfterAccess, path.CountAfterAccess) | ||
| } else { | ||
| path.MinCountAfterAccess = path.CountAfterAccess | ||
| } | ||
| path.CountAfterAccess = min(ds.StatsInfo().RowCount/cost.SelectionFactor, float64(ds.StatisticTable.RealtimeCount)) | ||
| // Ensure MaxCountAfterAccess is updated to reflect that "after" result | ||
| path.MaxCountAfterAccess = max(path.CountAfterAccess, path.MaxCountAfterAccess) | ||
| } | ||
| } | ||
|
|
||
| >>>>>>> 22fd1ac756e (planner: discount equality-bound clustered-key prefix in index pruning (#69726)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Cherry-pick committed with unresolved conflict markers across five files. The automated cherry-pick of #69726 left <<<<<<< HEAD / ======= / >>>>>>> markers in place, so neither the Go packages nor the Bazel files parse. Each site must be resolved before /unhold.
pkg/planner/core/stats.go#L203-L254: keep the incomingpathRangesIncludeAppendedHandle/adjustCountAfterAccessdefinitions and delete the markers, confirming no duplicateadjustCountAfterAccessexists on this branch.pkg/planner/core/rule/rule_prune_indexes.go#L469-L481: keep the bounds-checkedtableColumns[col.Offset]branch and drop the HEAD variant plus markers.pkg/planner/core/rule/BUILD.bazel#L24-L63: resolve the deps/go_testblock and regenerate withmake bazel_preparesosrcs/depsmatch this branch.pkg/planner/core/casetest/index/index_test.go#L451-L452: keep the two new tests and remove the surrounding markers.pkg/planner/core/casetest/index/BUILD.bazel#L13-L17: keep the regeneratedshard_count = 13and remove the markers.
📍 Affects 5 files
pkg/planner/core/stats.go#L203-L254(this comment)pkg/planner/core/rule/rule_prune_indexes.go#L469-L481pkg/planner/core/rule/BUILD.bazel#L24-L63pkg/planner/core/casetest/index/index_test.go#L451-L452pkg/planner/core/casetest/index/BUILD.bazel#L13-L17
🤖 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.go` around lines 203 - 254, Remove all unresolved
cherry-pick conflict markers across the five affected sites. In
pkg/planner/core/stats.go lines 203-254, retain pathRangesIncludeAppendedHandle
and adjustCountAfterAccess, ensuring no duplicate adjustCountAfterAccess
remains; in pkg/planner/core/rule/rule_prune_indexes.go lines 469-481, retain
the bounds-checked tableColumns[col.Offset] branch; in
pkg/planner/core/rule/BUILD.bazel lines 24-63, resolve dependencies and the
go_test block, then run make bazel_prepare; in
pkg/planner/core/casetest/index/index_test.go lines 451-452, retain both new
tests; and in pkg/planner/core/casetest/index/BUILD.bazel lines 13-17, retain
shard_count = 13.
| // adjustCountAfterAccess adjusts the CountAfterAccess when it's less than the estimated table row count. | ||
| func adjustCountAfterAccess(ds *logicalop.DataSource, path *util.AccessPath) { | ||
| // If the `CountAfterAccess` is less than `stats.RowCount`, it means that paths were estimated using | ||
| // different assumptions regarding individual or compound selectivity estimates. | ||
| // We prefer the `stats.RowCount` to provide consistency in estimation across all paths. | ||
| // Add an arbitrary tolerance factor to account for comparison with floating point | ||
| if (path.CountAfterAccess + cost.ToleranceFactor) < ds.StatsInfo().RowCount { | ||
| // When the ranges include the appended handle columns, the handle predicates were | ||
| // credited with deliberately damped exponential backoff, so falling below the | ||
| // independence-leaning stats.RowCount is expected rather than a sign of | ||
| // inconsistent assumptions. Align to stats.RowCount without the SelectionFactor | ||
| // penalty so the credited path is not made more expensive than an uncredited one. | ||
| if pathRangesIncludeAppendedHandle(path) { | ||
| if path.MinCountAfterAccess > 0 { | ||
| path.MinCountAfterAccess = min(path.MinCountAfterAccess, path.CountAfterAccess) | ||
| } else { | ||
| path.MinCountAfterAccess = path.CountAfterAccess | ||
| } | ||
| path.CountAfterAccess = ds.StatsInfo().RowCount | ||
| path.MaxCountAfterAccess = max(path.CountAfterAccess, path.MaxCountAfterAccess) | ||
| return | ||
| } | ||
| // Store the MinCountAfterAccess "before" adjusting the "CountAfterAccess". This can be used to differentiate | ||
| // the "Min" estimate for each index/inthandle path when CountAfterAccess has been equalized. | ||
| if path.MinCountAfterAccess > 0 { | ||
| path.MinCountAfterAccess = min(path.MinCountAfterAccess, path.CountAfterAccess) | ||
| } else { | ||
| path.MinCountAfterAccess = path.CountAfterAccess | ||
| } | ||
| path.CountAfterAccess = min(ds.StatsInfo().RowCount/cost.SelectionFactor, float64(ds.StatisticTable.RealtimeCount)) | ||
| // Ensure MaxCountAfterAccess is updated to reflect that "after" result | ||
| path.MaxCountAfterAccess = max(path.CountAfterAccess, path.MaxCountAfterAccess) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP 'M(in|ax)CountAfterAccess' pkg/planner/util/path.go
rg -nP '\badjustCountAfterAccess\s*\(' -g '*.go'
rg -nP '\bpathRangesIncludeAppendedHandle\s*\(' -g '*.go'Repository: pingcap/tidb
Length of output: 150
🏁 Script executed:
#!/bin/bash
set -u
echo "== files =="
git ls-files | rg '(^|/)path\.go$|stats\.go$' || true
echo "== search MinCountAfterAccess =="
rg -n 'MinCountAfterAccess|MaxCountAfterAccess' . || true
echo "== search helpers =="
rg -n '\badjustCountAfterAccess\s*\(' -g '*.go' || true
rg -n '\bpathRangesIncludeAppendedHandle\s*\(' -g '*.go' || true
rg -n '\bpathRangesIncludes|HandleColumns|Ranges|CountAfterAccess' pkg/planner -g '*.go' | head -200 || true
echo "== path.go list =="
git ls-files | rg 'pkg/planner/.*/.*path\.go$|pkg/planner/util/.*\.go$' | sort | sed -n '1,120p'
echo "== stats.go snippet if present =="
if [ -f pkg/planner/core/stats.go ]; then wc -l pkg/planner/core/stats.go; fi
if [ -f pkg/planner/core/stats.go ]; then sed -n '180,270p' pkg/planner/core/stats.go; fi
echo "== AccessPath definition locations =="
rg -n 'type AccessPath struct|CountAfterAccess|MaxCountAfterAccess|MinCountAfterAccess' -g '*.go' | head -300 || trueRepository: pingcap/tidb
Length of output: 1879
🏁 Script executed:
[Your shell script here]Repository: pingcap/tidb
Length of output: 193
🏁 Script executed:
#!/bin/bash
set -u
echo "== AccessPath definitions / relevant fields =="
rg -n 'type AccessPath struct|CountAfterAccess|MaxCountAfterAccess|MinCountAfterAccess' -g '*.go' | head -200
echo "== helper references =="
rg -n '\badjustCountAfterAccess\s*\(' -g '*.go' || true
rg -n '\bpathRangesIncludeAppendedHandle\s*\(' -g '*.go' || true
echo "== stats.go current snippet =="
if [ -f pkg/planner/core/stats.go ]; then
sed -n '200,265p' pkg/planner/core/stats.go
fi
echo "== path.go AccessPath excerpt if present =="
if [ -f pkg/planner/util/path.go ]; then
rg -n 'type AccessPath struct|struct{' pkg/planner/util/path.go || true
# print a compact vicinity if AccessPath is found
python3 - <<'PY'
from pathlib import Path
p=Path('pkg/planner/util/path.go')
text=p.read_text()
lines=text.splitlines()
for i,l in enumerate(lines,1):
if 'type AccessPath struct' in l:
start=max(1,i-5); end=min(len(lines), i+220)
print(f"--- pkg/planner/util/path.go:{start}-{end} ---")
for n in range(start,end+1):
print(f"{n:5d}\t{lines[n-1]}")
break
PY
fi
echo "== git diff stat and relevant hunks =="
git diff --stat || true
git diff -- pkg/planner/util/path.go pkg/planner/core/stats.go | sed -n '1,260p' || trueRepository: pingcap/tidb
Length of output: 373
🏁 Script executed:
#!/bin/bash
set -u
echo "== files in repository =="
git ls-files | sed -n '1,200p'
echo "== AccessPath and helper references across repo =="
rg -n 'type AccessPath struct|CountAfterAccess|MaxCountAfterAccess|MinCountAfterAccess|adjustCountAfterAccess|pathRangesIncludeAppendedHandle' . -g '*.go' || true
echo "== stats.go current relevant content =="
if [ -f pkg/planner/core/stats.go ]; then
sed -n '190,260p' pkg/planner/core/stats.go
fi
echo "== path.go relevant content =="
if [ -f pkg/planner/util/path.go ]; then
sed -n '1,180p' pkg/planner/util/path.go
fi
echo "== all functions/call sites in stats.go =="
if [ -f pkg/planner/core/stats.go ]; then
rg -n '\w+\s*\(' pkg/planner/core/stats.go
fi
echo "== git diff stat and core hunks =="
git diff --stat || true
git diff -- pkg/planner/core/stats.go pkg/planner/util/path.go | sed -n '1,260p' || trueRepository: pingcap/tidb
Length of output: 49139
Resolve the adjustCountAfterAccess conflict before merging.
pkg/planner/core/stats.go is in a merge conflict between this branch and the cherry-picked helper; with the HEAD side unresolved, the file will not compile. Keep only one version, or resolve the conflict so the helper’s references are valid and consistent with the branch’s util.AccessPath.
🤖 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.go` around lines 219 - 252, Resolve the merge conflict
in adjustCountAfterAccess so stats.go contains valid Go without conflict markers
and only one consistent implementation. Verify that
pathRangesIncludeAppendedHandle and all CountAfterAccess fields match the
current util.AccessPath definition, retaining the branch’s intended adjustment
behavior.
|
@ti-chi-bot: 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. |
This branch has not been deployed
2 participants
This is an automated cherry-pick of #69726
In multi-tenant schemas every secondary index leads with the tenant column (also the clustered-key prefix), so absolute coverage scoring keeps many redundant indexes: each one scores for covering the tenant column even though the table path serves it directly and it cannot differentiate one index from another.
Index pruning now discounts leading clustered-key columns bound by equality/IN constants: they still extend an index's usable consecutive prefix but contribute nothing to its score, so prefix-only indexes fall to the existing zero-score prune. In addition, the phase-2 ordering-key diversity check becomes a full coverage signature (consecutive prefix plus remaining covered columns) enforced from the first selection slot, deduplicating indexes with identical interesting-column coverage.
For a 54-index tenant table this reduces kept paths from 11 to 6 for a typical filtered ORDER BY LIMIT query, and from 11 to 2 when only the tenant column is bound, cutting index stats sync-load, range building, and skyline work per query. Forced (use/force index) and IndexMerge- hinted paths still bypass pruning.
What problem does this PR solve?
Issue Number: ref #63856
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
Performance Improvements
Bug Fixes
Tests