planner: fix partial index pruning due to shared Range pointer mutation#70002
planner: fix partial index pruning due to shared Range pointer mutation#70002yy782 wants to merge 2 commits into
Conversation
…ointer UnionRanges builds sortRange objects holding pointers to the original Range. When merging adjacent or overlapping ranges, it mutates the pointed Range directly, corrupting the caller's input because the pointer is shared across all sortRange items. This causes CheckPartialIndexes to incorrectly determine that partial index conditions are implied by query conditions, leading to wrong index pruning. Ref pingcap#69779
…p#69779) Verify that a >= 0 does NOT imply a < 3 for partial index pruning, which is the regression case from the shared pointer bug in UnionRanges.
ti-chi-bot
Bot
added
release-note-none
|
[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 |
ti-chi-bot
Bot
added
contribution
|
Hi @yy782. Thanks for your PR. I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthrough
ChangesRange metadata and partial-index validation
Estimated code review effort: 3 (Moderate) | ~20 minutes 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 |
|
/ok-to-test |
ti-chi-bot
Bot
added
ok-to-test
There was a problem hiding this comment.
⚠️ Not ready to approve
The UnionRanges merge path still assigns HighVal by reference (slice aliasing), which can reintroduce shared-state behavior and undermine the intended isolation fix.
Pull request overview
This PR addresses a planner correctness bug where partial indexes could be incorrectly accepted/pruned due to UnionRanges mutating shared *Range inputs, leading to wrong implication checks and potentially incorrect query results (ref #69779).
Changes:
- Prevents
UnionRangesfrom mutating caller-owned*Rangeinstances by cloning the active merge range. - Adds an integration test case that reproduces and guards against the “partial index chosen when predicate doesn’t imply index predicate” wrong-result scenario.
- Updates the corresponding integration test result file.
File summaries
| File | Description |
|---|---|
| pkg/util/ranger/ranger.go | Clones the working range in UnionRanges to avoid mutating shared *Range pointers during merges. |
| tests/integrationtest/t/planner/core/casetest/index/partialindex.test | Adds a regression query case ensuring a >= 0 does not incorrectly allow using a partial index defined with a < 3. |
| tests/integrationtest/r/planner/core/casetest/index/partialindex.result | Records expected results for the new regression case (ensuring full rowset is returned). |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Low
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| lastRange.encodedStart = objects[i].encodedStart | ||
| lastRange.encodedEnd = objects[i].encodedEnd | ||
| lastRange.originalValue = objects[i].originalValue.Clone() |
There was a problem hiding this comment.
After Clone(), lastRange.originalValue is already an independent Range object. The HighVal assignment on line 694 only changes where this cloned object's HighVal field points — it does not mutate the caller's original Range in any way.
While the cloned HighVal and objects[i].HighVal do share the same underlying array, the returned ranges' HighVal is only ever read (for range endpoint comparison), never mutated in-place by downstream consumers. So this slice aliasing is safe in practice.
I'm leaning toward keeping the current approach — it avoids an unnecessary copy and the semantics are correct for this use case. That said, if you feel a deep copy here adds more confidence, I'm open to it.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #70002 +/- ##
================================================
- Coverage 76.3239% 73.9288% -2.3951%
================================================
Files 2041 2058 +17
Lines 559772 579166 +19394
================================================
+ Hits 427240 428171 +931
- Misses 131631 150634 +19003
+ Partials 901 361 -540
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest |
What problem does this PR solve?
Issue Number: ref #69779
Problem Summary:
What changed and how does it work?
When query condition is a < 3 and partial index condition is a >= 0, CheckPartialIndexes wrongly prunes the index. The root cause is UnionRanges mutating shared Range pointers, corrupting the caller's input. The fix deep copies the original value when initializing lastRange to isolate merge side effects.See issue #69779 for details.
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
Bug Fixes
Tests