Skip to content

planner: index pruning using existing infra (#64999) | tidb-test=pr/2760 - #69009

Merged
ti-chi-bot[bot] merged 11 commits into
pingcap:release-8.5from
qw4990:pick-64999-4
Jun 9, 2026
Merged

planner: index pruning using existing infra (#64999) | tidb-test=pr/2760#69009
ti-chi-bot[bot] merged 11 commits into
pingcap:release-8.5from
qw4990:pick-64999-4

Conversation

@qw4990

@qw4990 qw4990 commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

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

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No need to test
    • I checked and no code files have been changed.

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

Summary by CodeRabbit

  • New Features

    • Query-driven index pruning that keeps the most relevant indexes.
    • New session/system variable to tune pruning aggressiveness (default provided).
  • Improvements

    • Planner propagates requested/interesting columns across the plan, improving index selection and TopN pushdown decisions.
    • Statistics and async histogram collection avoid loading pruned indexes to reduce overhead.
  • Tests

    • Added and updated tests and fixtures to validate pruning behavior and EXPLAIN outputs.

@ti-chi-bot ti-chi-bot Bot added do-not-merge/invalid-title do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/cherry-pick-not-approved release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. sig/planner SIG: Planner labels Jun 8, 2026
@tiprow

tiprow Bot commented Jun 8, 2026

Copy link
Copy Markdown

Hi @qw4990. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Details

Instructions 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

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a configurable index-pruning pipeline: collect per-DataSource "interesting" columns from predicates/joins/orders, rank and prune index access paths with a two-phase diversity-aware selector, propagate AllPossibleAccessPaths through planner rules, and avoid loading stats for pruned indexes.

Changes

Index Pruning Feature

Layer / File(s) Summary
Session variable and configuration setup
pkg/sessionctx/variable/tidb_vars.go, pkg/sessionctx/variable/session.go, pkg/sessionctx/variable/setvar_affect.go, pkg/sessionctx/variable/sysvar.go, pkg/sessionctx/variable/varsutil_test.go
New system variable tidb_opt_index_prune_threshold (default 20) is defined, initialized in SessionVars, marked hint-updatable, and unit-tested.
DataSource structure and access path tracking
pkg/planner/core/operator/logicalop/logical_datasource.go, pkg/planner/core/logical_plan_builder.go, pkg/planner/core/rule_partition_processor.go
DataSource adds AllPossibleAccessPaths for pre-pruning candidates plus AskedColumnGroup and InterestingColumns; builder and partition processor clone possiblePaths into AllPossibleAccessPaths.
Interesting columns collection during stats analysis
pkg/planner/core/collect_column_stats_usage.go
Extends column-stats usage traversal to gather interesting columns from WHERE predicates, joins, GROUP/BY and MIN/MAX args, and ordering operators; stores per-DataSource interesting columns when pruning is enabled.
Index pruning implementation with scoring and selection
pkg/planner/core/rule/rule_prune_indexes.go
Implements PruneIndexesByWhereAndOrder with multi-criteria scoring (interesting-column coverage, consecutive-prefix matches, single-scan bonus), two-phase selection with diversity rules, and safety fallbacks; adds ShouldPreferIndexMerge.
Integration with statistics collection pipeline
pkg/planner/core/rule_collect_plan_stats.go
Pruning is applied before synchronous stats loading; kept-index tracking is passed to collectSyncIndices to avoid loading stats for pruned indexes.
Planner rules updated for access path expansion
pkg/planner/core/find_best_task.go, pkg/planner/core/stats.go, pkg/planner/core/rule_derive_topn_from_window.go, pkg/planner/core/rule_generate_column_substitute.go, pkg/planner/core/rule_join_elimination.go, pkg/planner/core/rule_predicate_push_down.go
Multiple rules updated to use AllPossibleAccessPaths when appropriate for broader candidate consideration; skyline pruning now consults ShouldPreferIndexMerge.
Access path initialization and cloning across planner
pkg/planner/core/logical_plan_builder.go, pkg/planner/core/operator/logicalop/logical_index_scan.go, pkg/planner/core/rule_max_min_eliminate.go
Access path setup clones possiblePaths into AllPossibleAccessPaths during buildDataSource; index-scan and max/min logic iterate the expanded path set and clone it during subplan cloning.
Integration tests, handler tests, and fixtures
tests/integrationtest/r/*, pkg/planner/core/stats_test.go, pkg/statistics/handle/handletest/handle_test.go
Updated expected EXPLAIN outputs and pruning notes, added tests verifying pruning behavior and that pruned indexes are not asynchronously loaded.

Sequence Diagram

sequenceDiagram
  participant Optimizer as CollectPredicateColumnsPoint.Optimize
  participant DS as logicalop.DataSource
  participant Rule as rule.PruneIndexesByWhereAndOrder
  participant Sync as collectSyncIndices
  Optimizer->>DS: read AllPossibleAccessPaths & InterestingColumns
  Optimizer->>Rule: request pruning (threshold, interesting cols)
  Rule-->>Optimizer: return kept index IDs
  Optimizer->>Sync: pass keptIndexIDs
  Sync->>Sync: filter sync index stats by kept IDs
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • pingcap/tidb#66695: Related skyline/pruning adjustments touching skyline preference and pruning heuristics.

Suggested reviewers

  • terry1purcell
  • AilinKid
  • guo-shaoge
  • yudongusa

Poem

🐇 I nibble columns, count and score,
I hop through joins and WHEREs galore,
I trim the paths that stretch too wide,
Keep just the trails where queries glide,
Hoppity-hop — the planner's proud and spry.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 65.79% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning PR description is largely incomplete with missing required sections and unclear problem statement. Provide a complete PR description: fill in the Issue Number with a concrete issue link, add a Problem Summary explaining the business case, describe specific changes made, and ensure all testing checklists are properly verified and marked.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title 'planner: index pruning using existing infra (#64999)' clearly describes the main feature added—index pruning implementation using existing infrastructure in the planner package.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov

codecov Bot commented Jun 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.68142% with 47 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (release-8.5@de0a5bc). Learn more about missing BASE report.

Additional details and impacted files
@@               Coverage Diff                @@
##             release-8.5     #69009   +/-   ##
================================================
  Coverage               ?   55.2158%           
================================================
  Files                  ?       1829           
  Lines                  ?     660474           
  Branches               ?          0           
================================================
  Hits                   ?     364686           
  Misses                 ?     268664           
  Partials               ?      27124           
Flag Coverage Δ
integration 38.3239% <75.2212%> (?)
unit 65.1414% <91.3274%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
dumpling 55.3108% <0.0000%> (?)
parser ∅ <0.0000%> (?)
br 54.6273% <0.0000%> (?)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (1)
pkg/planner/core/rule_collect_plan_stats.go (1)

268-285: Keep ds.AllPossibleAccessPaths = prunedPaths—it’s intentional for static pruning + stats loading
The overwrite in CollectPredicateColumnsPoint is consistent with the current design: pkg/planner/core/stats.go explicitly states index pruning is done earlier in CollectPredicateColumnsPoint to avoid loading stats for pruned indexes, and the stats derivation uses ds.AllPossibleAccessPaths (e.g., fillIndexPath and deriveStatsByFilter). Keeping the full pre-pruning set here would undermine that goal, so the proposed diff should not be applied.

🤖 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 268 - 285, The
overwrite of ds.AllPossibleAccessPaths with prunedPaths is intentional and must
remain—revert any change that attempted to preserve the pre-pruned full set;
keep the current logic in this block: merge per-physical-table kept indexes into
keptIndexIDs (using the existingKeptIndexes union logic), then set
ds.AllPossibleAccessPaths = prunedPaths and copy into ds.PossibleAccessPaths (as
done now) so downstream routines like CollectPredicateColumnsPoint,
fillIndexPath, and deriveStatsByFilter operate on the pruned index set and avoid
loading stats for pruned indexes.
🤖 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/rule/rule_prune_indexes.go`:
- Around line 133-135: The pruning pass is using the possibly-stale
path.IsSingleScan field (set only when path.FullIdxCols != nil) which can be
overwritten later in stats recompute; update the code in rule_prune_indexes.go
to compute the single-scan decision locally from finalized path metadata instead
of reading path.IsSingleScan (or remove use of IsSingleScan in this pass).
Concretely, where you call ds.IsSingleScan(path.FullIdxCols,
path.FullIdxColLens) or read path.IsSingleScan to admit/rank candidates, invoke
ds.IsSingleScan with the finalized FullIdxCols/FullIdxColLens (or equivalent
finalized column info from the path) into a local bool (e.g., localIsSingleScan)
and use that within the pruning logic rather than mutating or depending on
path.IsSingleScan; ensure all other spots noted (the other blocks around the
ranges mentioned) follow the same pattern so pruning uses the computed local
value not the path field.
- Around line 42-43: The ordering diversity key currently only stores column IDs
(consecutiveColumnIDs) so different directions collapse; change the ordering
signature to include the per-column Desc bit (e.g., replace consecutiveColumnIDs
[]int64 with a slice that encodes both ID and Desc — either a small struct {ID
int64; Desc bool} or pack Desc into an unsigned int/bit of an int64) and update
all code paths that build, compare, and hash that signature (the struct fields
interestingCount and consecutiveColumnIDs and any helper that generates the
ordering key) so the Desc bit is carried into the ordering key; apply the same
change to the other places referenced in the file (the code around the other
signature constructions and comparisons) so pruning uses ID+direction rather
than ID alone.

In `@pkg/sessionctx/variable/sysvar.go`:
- Around line 298-301: The TiDBOptIndexPruneThreshold sysvar is stored to
s.OptIndexPruneThreshold but wasn't marked as hint-updatable or wired into the
SET_VAR()/hint affect plumbing; update the sysvar entry for
TiDBOptIndexPruneThreshold in pkg/sessionctx/variable/sysvar.go to set
IsHintUpdatableVerified=true and then add the corresponding wiring in
pkg/sessionctx/variable/setvar_affect.go so SET_VAR()/optimizer hint paths
recognize and update OptIndexPruneThreshold (follow the pattern used by other
hint-updatable int sysvars: register the mapping from the sysvar name
TiDBOptIndexPruneThreshold to the session field OptIndexPruneThreshold and
ensure the applyHint/update logic handles its value conversion and bounds
checks).

---

Nitpick comments:
In `@pkg/planner/core/rule_collect_plan_stats.go`:
- Around line 268-285: The overwrite of ds.AllPossibleAccessPaths with
prunedPaths is intentional and must remain—revert any change that attempted to
preserve the pre-pruned full set; keep the current logic in this block: merge
per-physical-table kept indexes into keptIndexIDs (using the existingKeptIndexes
union logic), then set ds.AllPossibleAccessPaths = prunedPaths and copy into
ds.PossibleAccessPaths (as done now) so downstream routines like
CollectPredicateColumnsPoint, fillIndexPath, and deriveStatsByFilter operate on
the pruned index set and avoid loading stats for pruned indexes.
🪄 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: a32a09ab-fa88-46e9-b156-1815f14a1399

📥 Commits

Reviewing files that changed from the base of the PR and between de0a5bc and e86bed4.

📒 Files selected for processing (19)
  • pkg/planner/core/collect_column_stats_usage.go
  • pkg/planner/core/find_best_task.go
  • pkg/planner/core/logical_plan_builder.go
  • pkg/planner/core/operator/logicalop/logical_datasource.go
  • pkg/planner/core/operator/logicalop/logical_index_scan.go
  • pkg/planner/core/operator/logicalop/logical_plans_misc.go
  • pkg/planner/core/rule/rule_prune_indexes.go
  • pkg/planner/core/rule_collect_plan_stats.go
  • pkg/planner/core/rule_derive_topn_from_window.go
  • pkg/planner/core/rule_generate_column_substitute.go
  • pkg/planner/core/rule_join_elimination.go
  • pkg/planner/core/rule_max_min_eliminate.go
  • pkg/planner/core/rule_partition_processor.go
  • pkg/planner/core/rule_predicate_push_down.go
  • pkg/planner/core/stats.go
  • pkg/sessionctx/variable/session.go
  • pkg/sessionctx/variable/setvar_affect.go
  • pkg/sessionctx/variable/sysvar.go
  • pkg/sessionctx/variable/tidb_vars.go

Comment on lines +42 to +43
interestingCount int // Total number of interesting columns covered
consecutiveColumnIDs []int64 // IDs of consecutive columns (for detecting different orderings)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Include index direction in the ordering key.

The diversity key only records column IDs, so (a, b) and (a DESC, b DESC) collapse to the same ordering. Phase 2 can then prune the only path that satisfies a DESC requirement even though later property matching distinguishes direction. Please carry the per-column Desc bit in the signature you use for ordering diversity.

Also applies to: 221-236, 484-490

🤖 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 42 - 43, The
ordering diversity key currently only stores column IDs (consecutiveColumnIDs)
so different directions collapse; change the ordering signature to include the
per-column Desc bit (e.g., replace consecutiveColumnIDs []int64 with a slice
that encodes both ID and Desc — either a small struct {ID int64; Desc bool} or
pack Desc into an unsigned int/bit of an int64) and update all code paths that
build, compare, and hash that signature (the struct fields interestingCount and
consecutiveColumnIDs and any helper that generates the ordering key) so the Desc
bit is carried into the ordering key; apply the same change to the other places
referenced in the file (the code around the other signature constructions and
comparisons) so pruning uses ID+direction rather than ID alone.

Comment on lines +133 to +135
if path.FullIdxCols != nil {
path.IsSingleScan = ds.IsSingleScan(path.FullIdxCols, path.FullIdxColLens)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don't prune on a stale IsSingleScan bit.

This function uses path.IsSingleScan to both admit and rank candidates, but it only populates that field opportunistically when FullIdxCols != nil. The later recompute in pkg/planner/core/stats.go means the value is not stable here, so a genuinely covering index can already be under-scored or excluded before that fix runs. Compute single-scan locally from finalized path metadata, or stop using it in this pruning pass.

Also applies to: 172-185, 303-321

🤖 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 133 - 135, The
pruning pass is using the possibly-stale path.IsSingleScan field (set only when
path.FullIdxCols != nil) which can be overwritten later in stats recompute;
update the code in rule_prune_indexes.go to compute the single-scan decision
locally from finalized path metadata instead of reading path.IsSingleScan (or
remove use of IsSingleScan in this pass). Concretely, where you call
ds.IsSingleScan(path.FullIdxCols, path.FullIdxColLens) or read path.IsSingleScan
to admit/rank candidates, invoke ds.IsSingleScan with the finalized
FullIdxCols/FullIdxColLens (or equivalent finalized column info from the path)
into a local bool (e.g., localIsSingleScan) and use that within the pruning
logic rather than mutating or depending on path.IsSingleScan; ensure all other
spots noted (the other blocks around the ranges mentioned) follow the same
pattern so pruning uses the computed local value not the path field.

Comment on lines +298 to +301
{Scope: ScopeGlobal | ScopeSession, Name: TiDBOptIndexPruneThreshold, Value: strconv.Itoa(DefTiDBOptIndexPruneThreshold), Type: TypeInt, MinValue: -1, MaxValue: math.MaxInt32, SetSession: func(s *SessionVars, val string) error {
s.OptIndexPruneThreshold = TidbOptInt(val, DefTiDBOptIndexPruneThreshold)
return nil
}},

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify hint-update plumbing for TiDBOptIndexPruneThreshold across related files.

set -euo pipefail

echo "== Sysvar definition =="
rg -n -C3 'TiDBOptIndexPruneThreshold|OptIndexPruneThreshold' pkg/sessionctx/variable/sysvar.go

echo
echo "== Sysvar name/default declarations =="
rg -n -C3 'TiDBOptIndexPruneThreshold|DefTiDBOptIndexPruneThreshold' pkg/sessionctx/variable/tidb_vars.go pkg/sessionctx/variable/session.go

echo
echo "== Hint/setvar affect registration =="
rg -n -C4 'TiDBOptIndexPruneThreshold|OptIndexPruneThreshold' pkg/sessionctx/variable/setvar_affect.go

echo
echo "== Any IsHintUpdatableVerified usage for this var =="
rg -n -C3 'TiDBOptIndexPruneThreshold.*IsHintUpdatableVerified|IsHintUpdatableVerified.*TiDBOptIndexPruneThreshold' pkg/sessionctx/variable/sysvar.go pkg/sessionctx/variable/setvar_affect.go

Repository: pingcap/tidb

Length of output: 3074


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== All occurrences in pkg/sessionctx/variable =="
rg -n --hidden --no-ignore-vcs 'OptIndexPruneThreshold|TiDBOptIndexPruneThreshold' pkg/sessionctx/variable || true

echo
echo "== setvar_affect candidates =="
rg -n --hidden --no-ignore-vcs 'setvar|affect|hint|IsHintUpdatable' pkg/sessionctx/variable/setvar_affect.go || true

echo
echo "== All occurrences of IsHintUpdatableVerified and TiDBOptIndexPruneThreshold =="
rg -n --hidden --no-ignore-vcs 'IsHintUpdatableVerified' pkg/sessionctx/variable || true

echo
echo "== Cross-check for setvar-affect wiring elsewhere in repo =="
rg -n --hidden --no-ignore-vcs 'OptIndexPruneThreshold|TiDBOptIndexPruneThreshold' pkg | head -n 200 || true

echo
echo "== Cross-check for any generic hint update plumbing touching optimizer vars =="
rg -n --hidden --no-ignore-vcs 'IsHintUpdatableVerified|IsHintUpdatable|hint.*setvar|setvar.*hint|affect.*setvar' pkg/sessionctx/variable pkg/planner pkg/executor pkg/session | head -n 200 || true

Repository: pingcap/tidb

Length of output: 9293


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== All occurrences in pkg/sessionctx/variable =="
rg -n --hidden --no-ignore-vcs 'OptIndexPruneThreshold|TiDBOptIndexPruneThreshold' pkg/sessionctx/variable || true

echo
echo "== setvar_affect candidates =="
rg -n --hidden --no-ignore-vcs 'setvar|affect|hint|IsHintUpdatable' pkg/sessionctx/variable/setvar_affect.go || true

echo
echo "== All occurrences of IsHintUpdatableVerified and TiDBOptIndexPruneThreshold =="
rg -n --hidden --no-ignore-vcs 'IsHintUpdatableVerified' pkg/sessionctx/variable || true

echo
echo "== Cross-check for setvar-affect wiring elsewhere in repo =="
rg -n --hidden --no-ignore-vcs 'OptIndexPruneThreshold|TiDBOptIndexPruneThreshold' pkg | head -n 200 || true

echo
echo "== Cross-check for any generic hint update plumbing touching optimizer vars =="
rg -n --hidden --no-ignore-vcs 'IsHintUpdatableVerified|IsHintUpdatable|hint.*setvar|setvar.*hint|affect.*setvar' pkg/sessionctx/variable pkg/planner pkg/executor pkg/session | head -n 200 || true

Repository: pingcap/tidb

Length of output: 9293


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== All occurrences in pkg/sessionctx/variable =="
rg -n --hidden --no-ignore-vcs 'OptIndexPruneThreshold|TiDBOptIndexPruneThreshold' pkg/sessionctx/variable || true

echo
echo "== setvar_affect candidates =="
rg -n --hidden --no-ignore-vcs 'setvar|affect|hint|IsHintUpdatable' pkg/sessionctx/variable/setvar_affect.go || true

echo
echo "== All occurrences of IsHintUpdatableVerified and TiDBOptIndexPruneThreshold =="
rg -n --hidden --no-ignore-vcs 'IsHintUpdatableVerified' pkg/sessionctx/variable || true

echo
echo "== Cross-check for setvar-affect wiring elsewhere in repo =="
rg -n --hidden --no-ignore-vcs 'OptIndexPruneThreshold|TiDBOptIndexPruneThreshold' pkg | head -n 200 || true

echo
echo "== Cross-check for any generic hint update plumbing touching optimizer vars =="
rg -n --hidden --no-ignore-vcs 'IsHintUpdatableVerified|IsHintUpdatable|hint.*setvar|setvar.*hint|affect.*setvar' pkg/sessionctx/variable pkg/planner pkg/executor pkg/session | head -n 200 || true

Repository: pingcap/tidb

Length of output: 9293


Wire TiDBOptIndexPruneThreshold into SET_VAR()/hint affect plumbing

TiDBOptIndexPruneThreshold is registered in pkg/sessionctx/variable/sysvar.go (ScopeGlobal|ScopeSession) and stored via s.OptIndexPruneThreshold, but its sysvar entry does not set IsHintUpdatableVerified, and there are no references to TiDBOptIndexPruneThreshold/OptIndexPruneThreshold in pkg/sessionctx/variable/setvar_affect.go. Add the missing affect/hint-update wiring (and mark IsHintUpdatableVerified appropriately) so optimizer hints can update this variable as intended.

🤖 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/sessionctx/variable/sysvar.go` around lines 298 - 301, The
TiDBOptIndexPruneThreshold sysvar is stored to s.OptIndexPruneThreshold but
wasn't marked as hint-updatable or wired into the SET_VAR()/hint affect
plumbing; update the sysvar entry for TiDBOptIndexPruneThreshold in
pkg/sessionctx/variable/sysvar.go to set IsHintUpdatableVerified=true and then
add the corresponding wiring in pkg/sessionctx/variable/setvar_affect.go so
SET_VAR()/optimizer hint paths recognize and update OptIndexPruneThreshold
(follow the pattern used by other hint-updatable int sysvars: register the
mapping from the sysvar name TiDBOptIndexPruneThreshold to the session field
OptIndexPruneThreshold and ensure the applyHint/update logic handles its value
conversion and bounds checks).

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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 `@tests/integrationtest/r/planner/core/plan.result`:
- Line 106: The planner shows nondeterministic selection between equivalent
indexes (seen as IndexRangeScan using index:k2 vs index:k1), indicating unstable
candidate ordering in the index ranking/pruning phase; update the index
selection logic (the index ranking/pruning codepath) to enforce a deterministic
tie-break after existing score and path-type comparisons by comparing a stable
identifier (e.g., index ID or name) to break ties so identical SQL always yields
the same chosen index (e.g., consistently prefer the smaller index ID or
lexicographically smaller name).
🪄 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: 126c11ff-7628-4198-9346-823bb3e2f3ea

📥 Commits

Reviewing files that changed from the base of the PR and between e86bed4 and 2e3fc6c.

📒 Files selected for processing (5)
  • tests/integrationtest/r/clustered_index.result
  • tests/integrationtest/r/imdbload.result
  • tests/integrationtest/r/index_merge.result
  • tests/integrationtest/r/planner/core/casetest/integration.result
  • tests/integrationtest/r/planner/core/plan.result
💤 Files with no reviewable changes (1)
  • tests/integrationtest/r/planner/core/casetest/integration.result
✅ Files skipped from review due to trivial changes (1)
  • tests/integrationtest/r/imdbload.result

Projection 0.10 root planner__core__plan.tt.a, planner__core__plan.tt.b
└─IndexLookUp 0.10 root
├─IndexRangeScan(Build) 0.10 cop[tikv] table:tt, index:k1(`a` + 1, b) range:[5 3,5 3], keep order:false, stats:pseudo
├─IndexRangeScan(Build) 0.10 cop[tikv] table:tt, index:k2(`a` + 1, b) range:[5 3,5 3], keep order:false, stats:pseudo

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Stabilize tie-break for equivalent indexes to avoid nondeterministic plans.

Line 106 picks index:k2(...), while the same statement in nearby repeated blocks still picks index:k1(...). For identical SQL in a single fixture, this one-off divergence strongly suggests unstable candidate ordering in index ranking/pruning. Please enforce a deterministic final tie-break (for example by index ID/name after score/path-type comparison) so repeated planning yields the same winner.

🤖 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 `@tests/integrationtest/r/planner/core/plan.result` at line 106, The planner
shows nondeterministic selection between equivalent indexes (seen as
IndexRangeScan using index:k2 vs index:k1), indicating unstable candidate
ordering in the index ranking/pruning phase; update the index selection logic
(the index ranking/pruning codepath) to enforce a deterministic tie-break after
existing score and path-type comparisons by comparing a stable identifier (e.g.,
index ID or name) to break ties so identical SQL always yields the same chosen
index (e.g., consistently prefer the smaller index ID or lexicographically
smaller name).

@qw4990 qw4990 changed the title [WIP] fixup planner: index pruning using existing infra (#64999) Jun 8, 2026
@ti-chi-bot ti-chi-bot Bot removed do-not-merge/invalid-title do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jun 8, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
pkg/planner/core/stats_test.go (1)

305-308: 💤 Low value

Consider using flag constants instead of magic bit positions.

The hardcoded bit shifts (1 << 15) and (1 << 19) are fragile—if the optimizer flag enum is reordered, this test will silently break. Consider importing and using the actual flag constants if they're accessible from the test package, or adding a brief comment noting which source file defines these positions to aid future maintenance.

🤖 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 305 - 308, Replace the magic bit
shifts in the test where optFlag |= (1 << 15) and optFlag |= (1 << 19) with the
actual optimizer flag constants (e.g., FlagCollectPredicateColumnsPoint and
FlagSyncWaitStatsLoadPoint) imported from the package that defines the optimizer
flags; update the test to reference those constant names when setting optFlag
(or, if those constants are not accessible from this test package, add a brief
comment pointing to the source file that defines the enum/bit positions and
define local constants with the exact names to avoid fragile magic numbers).
Ensure you update the lines that currently reference optFlag to use the named
constants so future enum reorders won’t silently break.
🤖 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/stats_test.go`:
- Around line 305-308: Replace the magic bit shifts in the test where optFlag |=
(1 << 15) and optFlag |= (1 << 19) with the actual optimizer flag constants
(e.g., FlagCollectPredicateColumnsPoint and FlagSyncWaitStatsLoadPoint) imported
from the package that defines the optimizer flags; update the test to reference
those constant names when setting optFlag (or, if those constants are not
accessible from this test package, add a brief comment pointing to the source
file that defines the enum/bit positions and define local constants with the
exact names to avoid fragile magic numbers). Ensure you update the lines that
currently reference optFlag to use the named constants so future enum reorders
won’t silently break.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 4e175012-63ee-42b5-ad26-198c1f1bb4d9

📥 Commits

Reviewing files that changed from the base of the PR and between 8b0672c and 6efbef7.

📒 Files selected for processing (3)
  • pkg/planner/core/stats_test.go
  • pkg/sessionctx/variable/varsutil_test.go
  • pkg/statistics/handle/handletest/handle_test.go

@qw4990

qw4990 commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

/retest

@tiprow

tiprow Bot commented Jun 8, 2026

Copy link
Copy Markdown

@qw4990: PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test.

Details

In response to this:

/retest

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

@qw4990

qw4990 commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

/ok-to-test

@ti-chi-bot ti-chi-bot Bot added the ok-to-test Indicates a PR is ready to be tested. label Jun 8, 2026
@qw4990

qw4990 commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

/retest

1 similar comment
@qw4990

qw4990 commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

/retest

@qw4990

qw4990 commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

/retest

@qw4990

qw4990 commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

/test unit-test

@tiprow

tiprow Bot commented Jun 8, 2026

Copy link
Copy Markdown

@qw4990: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test fast_test_tiprow_for_release

Use /test all to run all jobs.

Details

In response to this:

/test unit-test

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

@ti-chi-bot ti-chi-bot Bot added cherry-pick-approved Cherry pick PR approved by release team. and removed do-not-merge/cherry-pick-not-approved labels Jun 8, 2026
@qw4990 qw4990 changed the title planner: index pruning using existing infra (#64999) planner: index pruning using existing infra (#64999) | tidb-test=pr/2760 Jun 8, 2026
@qw4990

qw4990 commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

/test mysql-test

@tiprow

tiprow Bot commented Jun 8, 2026

Copy link
Copy Markdown

@qw4990: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test fast_test_tiprow_for_release

Use /test all to run all jobs.

Details

In response to this:

/test mysql-test

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

@0xPoe 0xPoe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

AskedColumnGroup [][]*expression.Column is never used.

@ti-chi-bot ti-chi-bot Bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jun 8, 2026
@0xPoe

0xPoe commented Jun 8, 2026

Copy link
Copy Markdown
Member

/hold

Feel free to unhold once we address #69009 (review), or we could ignore it.

@ti-chi-bot ti-chi-bot Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 8, 2026
@qw4990

qw4990 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

/unhold

@ti-chi-bot ti-chi-bot Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 9, 2026
@qw4990

qw4990 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

/retest

1 similar comment
@qw4990

qw4990 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

/retest

@ti-chi-bot

ti-chi-bot Bot commented Jun 9, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 0xPoe, terry1purcell

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot Bot added approved lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jun 9, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jun 9, 2026

Copy link
Copy Markdown

[LGTM Timeline notifier]

Timeline:

  • 2026-06-08 16:18:47.02621618 +0000 UTC m=+804028.096533570: ☑️ agreed by 0xPoe.
  • 2026-06-09 02:51:40.317161968 +0000 UTC m=+842001.387479428: ☑️ agreed by terry1purcell.

@ti-chi-bot
ti-chi-bot Bot merged commit bd0431a into pingcap:release-8.5 Jun 9, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved cherry-pick-approved Cherry pick PR approved by release team. lgtm ok-to-test Indicates a PR is ready to be tested. release-note-none Denotes a PR that doesn't merit a release note. sig/planner SIG: Planner size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants