Skip to content

planner: keep pruned index estimation ranges valid#69747

Open
terry1purcell wants to merge 3 commits into
pingcap:masterfrom
terry1purcell:pruneestimaterange
Open

planner: keep pruned index estimation ranges valid#69747
terry1purcell wants to merge 3 commits into
pingcap:masterfrom
terry1purcell:pruneestimaterange

Conversation

@terry1purcell

@terry1purcell terry1purcell commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: close #69746

Problem Summary:

For a non-unique secondary index, the planner appends the int handle column to the execution ranges. pruneEstimateRange truncates those ranges back to the declared index columns before row-count estimation (index statistics only cover the declared columns), but the truncation is wrong in two ways:

  1. It keeps the original LowExclude/HighExclude flags. A bound that lost its handle values must become inclusive because truncation widens it to the whole prefix; keeping the flag collapses (5 10, 5 +inf] (from a = 5 AND id > 10) into the empty range (5, 5], which estimates ~0 rows. The result no longer scales with the actual selectivity of the prefix.
  2. It estimates each pruned range independently, so multiple handle points under one prefix double count: a = 5 AND id IN (11, 22) prunes [5 11,5 11], [5 22,5 22] to [5,5], [5,5] and counts the a = 5 rows twice.

What changed and how does it work?

In pruneEstimateRange:

  • A bound whose values were truncated becomes inclusive, since dropping the trailing handle dimensions widens it to the whole prefix.
  • The pruned ranges are merged with ranger.UnionRanges so ranges that collapse to the same or overlapping prefixes are counted once.

Execution ranges (path.Ranges) are untouched; only the copy passed to GetRowCountByIndexRanges changes. With the fix, all three query shapes above estimate the row count of the a = 5 prefix (10 rows in the regression test), instead of 12.50/12.50/20.00 before.

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.

Fix the issue that the row count estimation of a non-unique secondary index was too low or double counted when predicates on an integer primary key extended the index scan range.

Summary by CodeRabbit

  • Bug Fixes
    • Improved row-count estimation for non-unique index scans when handle-range dimensions are appended and later truncated.
    • Fixed exclusive bound truncation to avoid collapsing ranges into empty results.
    • Enhanced merging of equivalent/overlapping prefix ranges to prevent double-counted estimates.
  • Tests
    • Added a planner test covering truncated handle-range behavior, exclusive-to-inclusive bound handling, and merged range estimation.

pruneEstimateRange truncates ranges built over the index columns plus
the appended handle columns down to the declared index columns so they
align with index statistics. The truncation kept the original exclusion
flags and estimated every pruned range independently, which breaks in
two ways once a handle predicate extends the range:

- An exclusive bound whose values were dropped collapses the range to
  empty: a = 5 AND id > 10 builds (5 10, 5 +inf], which pruned to
  (5, 5] and estimated ~0 rows regardless of the selectivity of a = 5.
- Multiple handle points under one prefix double count: a = 5 AND
  id IN (11, 22) pruned to [5,5], [5,5] and counted the a = 5 rows
  twice.

Make a truncated bound inclusive, since dropping trailing dimensions
widens it to the whole prefix, and merge the pruned ranges with
ranger.UnionRanges so overlapping prefixes are counted once.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@ti-chi-bot ti-chi-bot Bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/planner SIG: Planner labels Jul 9, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jul 9, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign hawkingrei for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found 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 the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 9, 2026
@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: c84d061a-2ff7-4f0d-b908-719929bd110a

📥 Commits

Reviewing files that changed from the base of the PR and between a5a04bd and aa6f7bd.

📒 Files selected for processing (1)
  • pkg/planner/core/stats.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • pkg/planner/core/stats.go

📝 Walkthrough

Walkthrough

Planner index-range estimation now adjusts truncated handle bounds and merges duplicate prefixes before cardinality estimation. A regression test validates exclusive handle predicates and multiple handle points on the same secondary-index prefix.

Changes

Index estimation

Layer / File(s) Summary
Context-aware range pruning
pkg/planner/core/stats.go, pkg/planner/core/BUILD.bazel
pruneEstimateRange uses ranger context, clears exclusions when truncation removes bound values, unions equivalent prefixes, and supplies the merged ranges for cardinality estimation.
Truncated handle range regression test
pkg/planner/cardinality/selectivity_test.go
Adds TestIndexRangeEstimationWithTruncatedHandleRange, covering exclusive handle bounds and multiple handle points under one index prefix.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related issues

  • #69748: Addresses related selectivity behavior for handle-extended non-unique index ranges.
  • #69744: Concerns handle-column suffixes in non-unique index ranges, although it modifies a different planner path.

Possibly related PRs

  • pingcap/tidb#66695: Also changes index row-count estimation behavior for index range scans.

Suggested reviewers: AilinKid, wjhuang2016, windtalker

Poem

I’m a rabbit counting ranges neat,
Merging hops that once would meet.
Bounds turn kind, no gaps remain,
Prefixes count just once again.
The planner nibbles stats with cheer!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately summarizes the main fix to pruned index estimation ranges.
Description check ✅ Passed The description matches the template with issue number, problem summary, implementation details, tests, and release note.
Linked Issues check ✅ Passed The code fixes the exact #69746 bugs by making truncated bounds inclusive and merging pruned ranges before estimation.
Out of Scope Changes check ✅ Passed The added test and build dependency support the range-estimation fix and do not appear unrelated to the issue.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

🔧 golangci-lint (2.12.2)

level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: context deadline exceeded"
level=error msg="Timeout exceeded: try increasing it by passing --timeout option"


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.

@codecov

codecov Bot commented Jul 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 70.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.2844%. Comparing base (52f7a7a) to head (aa6f7bd).
⚠️ Report is 5 commits behind head on master.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #69747        +/-   ##
================================================
- Coverage   76.3195%   74.2844%   -2.0351%     
================================================
  Files          2041       2054        +13     
  Lines        560153     580131     +19978     
================================================
+ Hits         427506     430947      +3441     
- Misses       131746     148529     +16783     
+ Partials        901        655       -246     
Flag Coverage Δ
integration 40.8986% <70.0000%> (+1.1933%) ⬆️

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

Components Coverage Δ
dumpling 60.4471% <ø> (ø)
parser ∅ <ø> (∅)
br 47.4060% <ø> (-15.3154%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI 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.

Pull request overview

This PR fixes row-count estimation for non-unique secondary indexes whose execution ranges are extended with appended integer-handle columns, ensuring pruned estimation ranges remain valid (inclusive bounds after truncation) and avoiding double-counting by merging collapsed prefixes.

Changes:

  • Adjust pruned estimation-range bounds to become inclusive when truncation drops appended handle dimensions.
  • Merge pruned estimation ranges via ranger.UnionRanges to avoid double-counting overlapping/collapsed prefixes.
  • Add a regression test covering exclusive-bound truncation and prefix-range merging behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
pkg/planner/core/stats.go Updates pruneEstimateRange to fix exclusivity after truncation and to union pruned ranges before stats estimation.
pkg/planner/core/BUILD.bazel Adds the //pkg/util/ranger/context dependency required by the new UnionRanges call-site.
pkg/planner/cardinality/selectivity_test.go Adds a regression test validating estimation correctness for truncated handle ranges.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkg/planner/core/stats.go
terry1purcell and others added 2 commits July 10, 2026 14:37
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The autofix commit replaced the direct return of ranger.UnionRanges with
an equivalent explicit error check, but the added lines were not gofmt
formatted, failing the nogo (gofmt/gci) build check. The original code
already propagates the error, so revert to it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@terry1purcell

Copy link
Copy Markdown
Contributor Author

/test pull-error-log-review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/planner SIG: Planner size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

planner: index estimation ranges pruned to declared columns become empty or double count when handle predicates extend the range

2 participants