Repository navigation
planner: discount equality-bound clustered-key prefix in index pruning | tidb-test=pr/2768 - #69726
Merged
Merged
Conversation
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. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ti-chi-bot
Bot
added
release-note-none
|
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:
📝 WalkthroughWalkthroughIndex pruning now discounts clustered-key prefix columns bound by constant equality or ChangesIndex pruning discount logic
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Query
participant PruneIndexesByWhereAndOrder
participant IndexSelection
participant Plan
Query->>PruneIndexesByWhereAndOrder: submit predicates and ordering
PruneIndexesByWhereAndOrder->>IndexSelection: score effective keys and discounted prefixes
IndexSelection->>Plan: retain non-dominated access paths
Plan-->>Query: produce explain plan
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![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/rule/rule_prune_indexes.go (1)
625-655: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winApply coverage-key deduplication to non-consecutive coverage too.
Line 631 only checks duplicate coverage keys when
hasConsecutiveis true, and Line 654 only records keys for consecutive prefixes. An index withconsecutiveColumnIDs == nilbutcoveredColumnIDs == [c]still has a meaningfulbuildCoverageKey("|c"), so duplicate non-consecutive coverage can consume phase-1 slots and keep extra paths.♻️ Proposed adjustment
func shouldAddIndex(entry scoredIndex, path *util.AccessPath, req columnRequirements, state *indexSelectionState) bool { hasConsecutive := len(entry.info.consecutiveColumnIDs) > 0 + hasCoverage := len(entry.info.coveredColumnIDs) > 0 if state.phase1Count < state.phase1Limit { @@ - if hasConsecutive { + if hasCoverage { if _, seen := state.seenCoverageKeys[buildCoverageKey(entry.info)]; seen { return false } } @@ func recordCoverage(info indexWithScore, state *indexSelectionState) { @@ - if len(info.consecutiveColumnIDs) > 0 { + if len(info.coveredColumnIDs) > 0 { state.seenCoverageKeys[buildCoverageKey(info)] = struct{}{} } }🤖 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 625 - 655, Phase 1 in the index pruning logic is only deduplicating coverage keys for indexes with consecutive columns, so non-consecutive coverage can still be selected multiple times. Update the phase-1 path in the selection logic around shouldAddIndexWithConsecutive/shouldAddIndexWithoutConsecutive to check buildCoverageKey(entry.info) for all indexes, not just when hasConsecutive is true. Also adjust recordCoverage so it always stores the coverage key for any index that has a meaningful covered-column set, while still tracking consecutiveColumnIDs separately.
🤖 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/rule/rule_prune_indexes.go`:
- Around line 625-655: Phase 1 in the index pruning logic is only deduplicating
coverage keys for indexes with consecutive columns, so non-consecutive coverage
can still be selected multiple times. Update the phase-1 path in the selection
logic around shouldAddIndexWithConsecutive/shouldAddIndexWithoutConsecutive to
check buildCoverageKey(entry.info) for all indexes, not just when hasConsecutive
is true. Also adjust recordCoverage so it always stores the coverage key for any
index that has a meaningful covered-column set, while still tracking
consecutiveColumnIDs separately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 04370861-599e-4fef-ae3c-ddef21a10e39
📒 Files selected for processing (3)
pkg/planner/core/casetest/index/BUILD.bazelpkg/planner/core/casetest/index/index_test.gopkg/planner/core/rule/rule_prune_indexes.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69726 +/- ##
================================================
- Coverage 76.3192% 73.5474% -2.7718%
================================================
Files 2041 2060 +19
Lines 559910 587747 +27837
================================================
+ Hits 427319 432273 +4954
- Misses 131690 154636 +22946
+ Partials 901 838 -63
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
Author
|
/retest-required |
…width tiebreak Two refinements to the clustered-prefix discount: The coverage-signature dedup now includes discounted columns at their declared chain position. idx(ws, a) and idx(a) on a clustered (ws, id) table score identically, but ranger builds ranges only over declared index columns: idx(ws, a) reaches a two-column range while idx(a) ranges on a alone with ws as an index filter, scanning every tenant's matching entries. Conflating them could prune the strictly better index; both now survive to skyline. Indexes that differ only in unused trailing columns (idx(a) vs idx(a, b)) still deduplicate. The fewer-columns tiebreak no longer requires a single consecutive column. When indexes tie on score, chain, and covering, the narrower index wins at any chain length instead of falling through to index-ID order: its entries are narrower and, on clustered tables, the PK suffix (usable for index-side filtering and ordering) starts earlier. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…uning Benchmark the 54-index multi-tenant schema that motivated the clustered-prefix discount, planning a filtered ORDER BY LIMIT query and an order-only variant at the default prune threshold and with pruning disabled (threshold=-1). Measured medians on this change (vs master, 6 runs x 30 plans, mockstore with analyzed stats): full query 1084us -> 889us (-18%), order-only 552us -> 324us (-41%); the disabled-pruning variants are unchanged, confirming the discount costs nothing when inactive. Relative to no stage-1 pruning at all, the default threshold saves 43% and 67% respectively on this schema. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ti-chi-bot
Bot
added
size/XXL
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/planner/core/casetest/index/index_prune_bench_test.go (1)
178-187: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the redundant
rowsvariable.
rowsis always equal toisince the loop starts at 0, making it a redundant tracker. Usei > 0directly for the comma-separator check.♻️ Proposed simplification
- rows := 0 for i := range 900 { ws := tenants[i%len(tenants)] - if rows > 0 { + if i > 0 { sb.WriteString(",") } fmt.Fprintf(&sb, "(UUID_TO_BIN(UUID()), '%s', %d, 'label-%d', UUID_TO_BIN('%s'), UUID_TO_BIN(UUID()), %d, %d)", ws, i, i%50, typeUUIDs[i%len(typeUUIDs)], i%3, 15+(i%4)) - rows++ }🤖 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_prune_bench_test.go` around lines 178 - 187, The loop in the benchmark setup uses a redundant rows counter in the index_prune bench test. Simplify the separator logic in the for loop by removing rows entirely and using i > 0 for the comma check, while keeping the fmt.Fprintf row generation unchanged; this is in the code that builds the VALUES string for the test input.
🤖 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/casetest/index/index_prune_bench_test.go`:
- Around line 178-187: The loop in the benchmark setup uses a redundant rows
counter in the index_prune bench test. Simplify the separator logic in the for
loop by removing rows entirely and using i > 0 for the comma check, while
keeping the fmt.Fprintf row generation unchanged; this is in the code that
builds the VALUES string for the test input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5f82b369-db9c-4674-9500-fcf9318810e7
📒 Files selected for processing (2)
pkg/planner/core/casetest/index/BUILD.bazelpkg/planner/core/casetest/index/index_prune_bench_test.go
There was a problem hiding this comment.
Pull request overview
This PR refines TiDB planner index-pruning heuristics to reduce planning overhead in schemas with many secondary indexes that share a clustered-key (tenant-style) leading prefix, while also deduplicating indexes that provide equivalent “interesting-column” coverage for a given query.
Changes:
- Discount equality/IN-bound leading clustered-key prefix columns from index coverage scoring (while still treating them as extending the usable prefix for range-building identity).
- Replace phase-2 “ordering diversity” with a full coverage signature (usable prefix chain + remaining covered interesting columns) and apply deduplication starting from the first selection slot.
- Add planner casetests and a benchmark to validate and measure pruning behavior on shared-prefix tenant schemas.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/planner/core/rule/rule_prune_indexes.go | Implements discounted clustered-prefix scoring and coverage-signature-based dedup during index selection. |
| pkg/planner/core/casetest/index/index_test.go | Adds casetests covering shared clustered-prefix discounting, dedup behavior, and hint bypass behavior. |
| pkg/planner/core/casetest/index/index_prune_bench_test.go | Adds benchmarks to measure planning-time impact for shared-prefix schemas with many indexes. |
| pkg/planner/core/casetest/index/BUILD.bazel | Registers the new benchmark file in the Bazel test target and adjusts sharding. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Selection could rebuild the coverage key (with a clone and sort of the non-consecutive covered columns) up to twice per candidate across the phase-1 duplicate check, recordCoverage, and phase-2 checks. Compute it once when the candidate is scored, and skip the clone/sort when the non-consecutive remainder has at most one element. Addresses a Copilot review comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
AilinKid
approved these changes
Jul 14, 2026
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
qw4990
reviewed
Jul 16, 2026
Previously the index-prune rule discounted the equality-bound leading clustered-key (tenant) prefix from an index's score. That conflated two concerns: dropping indexes whose only access is the handle prefix, and ranking the survivors. The discount flattened genuinely different indexes to the same score - e.g. k1(tenant_id, a) and k2(a, b) under tenant_id = 1 AND a = 1 - and could not see the handle columns TiKV appends to a secondary index's key, so an index binding a trailing handle column as an access condition scored no better than one that did not. Rank instead by the index's effective key: the declared columns plus the appended clustered handle (mirroring the int-handle append and its common-handle counterpart). Handle columns count toward the score like any other, so the consecutive-from-start metric reflects the real contiguous access-condition prefix. The discount is reduced to a single redundancy flag: an index whose only covered access is the handle prefix is dropped, since the clustered table path already serves those rows. Coverage dedup now uses precise domination: an index is pruned when another covers the identical interesting-column set through a consecutive prefix that extends it. Covering a larger set does not dominate, so a narrower index survives as an alternative for the cost model. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VF9S5ZEJHAZ9s9w5epgbH1
…uilding Index pruning derived the effective index key (declared columns plus the clustered handle TiKV appends to a non-unique secondary index) with its own copy of the append rules. That copy diverged from fillIndexPath: it appended the missing handle columns even when a primary key column already appeared in the index, and it ignored the global/MV/columnar and V0-new-collation skips. On a common-handle table this overcredited the usable prefix — e.g. an index (c4, c2) under c1 = 1 AND c4 = 1 was scored as if c1 were an access condition, when the real key does not append it, so pruning could drop an index the ranger would still range differently. Extract the layout into DataSource.HandleColsToAppend (with HasV0NewCollationStringHandle moved alongside it) as the single source of truth. fillIndexPath mutates the path through it for range building; effectiveIndexColumnIDs reads it, before fillIndexPath runs, to score access conditions. Scoring can no longer credit an access condition the ranger cannot realize. Also refresh two now-stale comments: the discountedColIDs field no longer subtracts from the score (it only flags redundant indexes), and the shared-prefix test no longer relies on the ranger ignoring appended handle columns. Regenerate TestWarningsInSlowQuery, whose remaining-paths note changed with the appended handle from pingcap#69749. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VF9S5ZEJHAZ9s9w5epgbH1
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
pkg/planner/core/rule/rule_prune_indexes.go:313
- The comment for discountedHandlePrefixCols says the bound clustered-key prefix columns “do not contribute to the score”, but the current implementation still increments interestingCount and contributes to the score (and relies on droppable/coversNonDiscounted instead). This comment should be updated to avoid misleading future maintainers.
// still extend an index's usable prefix, so scoring lets them anchor consecutive-column
// tracking without contributing to the score. Only a bound leading prefix qualifies:
// discounting stops at the first clustered-key column without an equality/IN predicate.
…ning collectEqOrInBoundColIDs, which detects the equality-bound clustered-key prefix for redundant-index pruning, only recognized = and IN. The ranger also builds equality access ranges for the null-safe <=> (ast.NullEQ), so a query like WHERE tenant <=> 'x' left the tenant prefix undetected and kept prefix-only indexes the table path already serves. Include ast.NullEQ alongside ast.EQ. Add a <=> regression case to TestIndexPruneWithSharedClusteredPrefix; it keeps ix_tenant_label without the fix and prunes to the table path with it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VF9S5ZEJHAZ9s9w5epgbH1
JSON columns carry no column statistics: ANALYZE builds no histogram or CMSketch for them (only multi-valued indexes over them, e.g. cast(`j` as signed array), are analyzed). But selectivity estimation still iterated over them, found no stats, and recorded a used-item status that surfaced in EXPLAIN as stats:partial[<json col>:missing|unInitialized]. That status is meaningless (no stats are ever used) and, worse, its value depends on whether the table's stats existence map happened to be loaded at plan time. Which items get sync-loaded is influenced by index pruning, so the annotation flips between missing and unInitialized depending on stats-load timing and pruning decisions, making plan-output tests flaky (this is the tail that made TestIndexMergeJSONMemberOf2FlakyPart flaky). Skip JSON columns in the selectivity column loop. They contribute no selectivity, so estimation is unaffected; only the spurious status recording is removed, which drops JSON columns from stats:partial[...] and makes the annotation deterministic. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VF9S5ZEJHAZ9s9w5epgbH1
…ts status Follow-up to skipping JSON columns when recording used-column stats status: the multi-valued-index EXPLAIN outputs in TestAnalyzeMVIndex no longer list the base JSON column `j` in stats:partial[...]. The multi-valued index and other index entries are unchanged; only the meaningless j:unInitialized column entry is removed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VF9S5ZEJHAZ9s9w5epgbH1
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
pkg/planner/core/rule/rule_prune_indexes.go:315
- The comment on discountedHandlePrefixCols says the equality-bound clustered-key prefix “anchors consecutive-column tracking without contributing to the score”, but the current implementation does count these columns in interestingCount and consecutiveColumnIDs (and score) and instead uses discountedColIDs/coversNonDiscounted to drop handle-prefix-only redundant indexes. Please update the comment to match the actual behavior to avoid misleading future changes.
// with the tenant column, so covering it says nothing about which index is better —
// and the table path (clustered key) already provides the same access. Such columns
// still extend an index's usable prefix, so scoring lets them anchor consecutive-column
// tracking without contributing to the score. Only a bound leading prefix qualifies:
// discounting stops at the first clustered-key column without an equality/IN predicate.
qw4990
approved these changes
Jul 22, 2026
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
Contributor
|
/retest |
terry1purcell
changed the title
FullIdxCols can carry nil placeholders for index columns that cannot be resolved to schema columns (see util.IndexInfo2FullCols). Index pruning passes FullIdxCols into HandleColsToAppend, whose duplicate-handle checks dereferenced those entries and could panic. Suppress the append when any declared column is unresolved: the physical key layout past that point is unknowable, and fillIndexPath's resolved-prefix length check would skip the append for such indexes anyway. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015aNN5EVKBHknee2aCZNzze
…dle extension Bound the partial-index constraint-column lookup in scoreIndexPath by the table columns actually passed in (treating an unresolvable offset as not found, which only prunes the index — always safe since the table path is kept), and gate matchProperty's common-handle extension on CommonHandleLens matching CommonHandleCols, the same invariant HandleColsToAppend checks. Both spots previously relied on construction-time invariants alone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015aNN5EVKBHknee2aCZNzze
time-and-fate
approved these changes
Jul 29, 2026
time-and-fate
left a comment
time-and-fate
left a comment
Member
There was a problem hiding this comment.
pkg/executor/test/analyzetest/analyze_test.go part LGTM.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AilinKid, qw4990, time-and-fate The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Member
|
In response to a cherrypick label: new pull request created to branch |
1 of 13 tasks
2 of 13 tasks
This was referenced Aug 12, 2026
5 tasks done
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
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.
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
New Features
Bug Fixes
Tests / Benchmarks