planner: keep pruned index estimation ranges valid#69747
Conversation
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>
|
[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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPlanner 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. ChangesIndex estimation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
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. 🔧 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" 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 |
Codecov Report❌ Patch coverage is 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
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
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.UnionRangesto 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.
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>
|
/test pull-error-log-review |
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.
pruneEstimateRangetruncates 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:LowExclude/HighExcludeflags. 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](froma = 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.a = 5 AND id IN (11, 22)prunes[5 11,5 11], [5 22,5 22]to[5,5], [5,5]and counts thea = 5rows twice.What changed and how does it work?
In
pruneEstimateRange:ranger.UnionRangesso ranges that collapse to the same or overlapping prefixes are counted once.Execution ranges (
path.Ranges) are untouched; only the copy passed toGetRowCountByIndexRangeschanges. With the fix, all three query shapes above estimate the row count of thea = 5prefix (10 rows in the regression test), instead of 12.50/12.50/20.00 before.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