planner: tighten full-range fast path for index estimation#68474
planner: tighten full-range fast path for index estimation#68474terry1purcell wants to merge 3 commits into
Conversation
Two fixes raised in the release-8.5 cherry-pick review (PR pingcap#66695) that also apply on master: 1. Move canSkipIndexEstimation ahead of IndexStatsIsInvalid so the full-range fast path short-circuits before IndexStatsIsInvalid queues an unnecessary async histogram load when the index stats are not fully loaded. Guarded by idx != nil so the pseudo fallback is preserved. 2. Reject ranges with LowExclude or HighExclude in isFullRangeIncludingNulls. A range like (NULL, +inf) drops the NULL endpoint and shrinks the result, so it must not trigger the fast path. Tighten TestCanSkipIndexEstimation to actually exercise these cases: model 10 NULL rows in the index histogram (NullCount=10) so the not-null estimate is strictly below RealtimeCount, switch the assertion to require.Less, and add a case covering (NULL, +inf) with an exclusive lower bound. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@terry1purcell I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
|
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)
📝 WalkthroughWalkthroughMoves the index-estimation fast-path earlier to return scaled realtime counts for inclusive full-range non-MV/non-partial indexes, tightens full-range NULL-boundary checks to reject exclusive bounds, and updates tests and EXPLAIN expectations to match the new short-circuiting behavior. ChangesIndex Cardinality Estimation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Hi @terry1purcell. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with 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. |
…eorder LoadNeededHistograms no longer loads stats for non-MV indexes like ia that were only touched as candidate access paths — the full-range fast path now short-circuits before IndexStatsIsInvalid queues them for async load. Update the section-3.3 expected plans to keep ia in the partial-stats list, and clarify the section comment. Co-Authored-By: Claude Opus 4.7 (1M context) <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 |
|
/ok-to-test |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #68474 +/- ##
================================================
- Coverage 77.2806% 75.6766% -1.6040%
================================================
Files 2010 2008 -2
Lines 555350 563899 +8549
================================================
- Hits 429178 426740 -2438
- Misses 125252 137039 +11787
+ Partials 920 120 -800
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
… fast path The previous test set the index to FullLoad, which made IndexStatsIsInvalid a no-op for the async-load queue — so the test would pass even if canSkipIndexEstimation were removed or if the fast-path reorder were reverted. Switch the mock index to NewStatsAllEvictedStatus so IndexStatsIsInvalid would queue it for async load if it were ever reached, and assert that AsyncLoadHistogramNeededItems does not contain the index after the full-range call. This now fails if the fast path moves back below IndexStatsIsInvalid. Switch sctx to the testkit's real session via GetPlanCtx() since recordUsedItemStatsStatus needs a domain to resolve the table when the stats are not FullLoad. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two fixes raised in the release-8.5 cherry-pick review (PR #66695) that also apply on master:
Move canSkipIndexEstimation ahead of IndexStatsIsInvalid so the full-range fast path short-circuits before IndexStatsIsInvalid queues an unnecessary async histogram load when the index stats are not fully loaded. Guarded by idx != nil so the pseudo fallback is preserved.
Reject ranges with LowExclude or HighExclude in isFullRangeIncludingNulls. A range like (NULL, +inf) drops the NULL endpoint and shrinks the result, so it must not trigger the fast path.
Tighten TestCanSkipIndexEstimation to actually exercise these cases: model 10 NULL rows in the index histogram (NullCount=10) so the not-null estimate is strictly below RealtimeCount, switch the assertion to require.Less, and add a case covering (NULL, +inf) with an exclusive lower bound.
What problem does this PR solve?
Issue Number: ref #64791
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
Bug Fixes
Tests