Repository navigation
planner: optimize for full range (#66304) | tidb-test=pr/2743 - #66695
Merged
ti-chi-bot[bot] merged 12 commits intoJun 2, 2026
Merged
ti-chi-bot[bot] merged 12 commits into
ti-chi-bot[bot] merged 12 commits into
Conversation
ti-chi-bot
added
do-not-merge/hold
1 of 13 tasks
Member
Author
|
@terry1purcell This PR has conflicts, I have hold it. |
|
@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide. 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 ti-community-infra/tichi repository. |
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR refactors TiDB's query planner to optimize cardinality estimation and path selection. It introduces early-return logic for index range estimation when covering full NULL-inclusive ranges, restructures candidate path comparison with new state fields, adjusts index column extraction logic, and optimizes table path statistics computation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/planner/cardinality/BUILD.bazel (1)
49-66:⚠️ Potential issue | 🟠 MajorAdd
bazel_prepareoutput to this PR for Bazel-file changes.Since this PR changes a
BUILD.bazelfile, include the generated Bazel metadata updates produced bymake bazel_prepare.As per coding guidelines: "MUST run
make bazel_prepareand include resulting Bazel metadata changes in the PR when adding/moving/renaming/removing Go files, changing Bazel files, updating Bazel test targets, or changinggo.mod/go.sum".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/cardinality/BUILD.bazel` around lines 49 - 66, This change touches Bazel BUILD metadata (the cardiniality_test target in BUILD.bazel with a modified shard_count), so run make bazel_prepare locally to regenerate Bazel metadata and include the resulting changes from that command in the PR; commit the generated Bazel outputs (the files created/updated by bazel_prepare) alongside this BUILD.bazel edit so the repository's Bazel state remains consistent with the change to the cardiniality_test target.pkg/planner/cardinality/row_count_index.go (1)
41-42:⚠️ Potential issue | 🔴 CriticalFix signature mismatch: some callers pass extra argument that function doesn't accept
Three test call sites in pkg/planner/cardinality/selectivity_test.go (lines 343, 351, 358) are passing a 5th
nilargument, butGetRowCountByIndexRangesis defined to accept only 4 parameters. Update the function signature to accept the extra parameter, or remove the extra argument from these call sites to align all callers with the function definition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/cardinality/row_count_index.go` around lines 41 - 42, The function signature for GetRowCountByIndexRanges currently accepts four parameters but some callers pass a fifth nil; update the signature to accept an extra parameter (e.g., extra interface{} or opts interface{}) so callers that pass nil compile, e.g., change func GetRowCountByIndexRanges(sctx planctx.PlanContext, coll *statistics.HistColl, idxID int64, indexRanges []*ranger.Range) to include a final extra parameter and leave the new argument unused inside the function (or use it if needed), ensuring all existing call sites that pass nil continue to work; update any references to GetRowCountByIndexRanges accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/planner/cardinality/BUILD.bazel`:
- Around line 62-66: Resolve the unresolved Git conflict in the BUILD file by
removing the conflict markers (<<<<<<<, =======, >>>>>>>) and leaving a single
valid shard_count assignment for the cardinality target (choose either
shard_count = 34 or shard_count = 45 based on intended change for the
planner/cardinality logic and branch context, ensuring only one shard_count line
remains), then run make bazel_prepare and include the generated Bazel metadata
changes in the PR so the BUILD change and metadata stay in sync.
In `@pkg/planner/cardinality/row_count_index.go`:
- Around line 64-83: The diff contains unresolved merge conflict markers and
mixed implementations: one branch returns a float64 count while the other uses
statistics.RowEstimate, causing compile errors; remove the conflict markers and
unify to a single implementation that uses hasColumnStats(sctx, coll, idxCols)
&& !ranger.HasFullRange(indexRanges, false) to call
getPseudoRowCountWithPartialStats(sctx, coll, indexRanges,
float64(coll.RealtimeCount), idxCols) and assign result =
statistics.RowEstimate{Est: count, MinEst: count, MaxEst: maxCount}, otherwise
compute colsLen (use idx.Info.Unique and len(idx.Info.Columns)), call
getPseudoRowCountByIndexRanges(sc.TypeCtx(), indexRanges,
float64(coll.RealtimeCount), colsLen) and wrap the returned count with
statistics.DefaultRowEst(count); ensure variables count, maxCount, err and
result have consistent types, preserve sc.EnableOptimizerCETrace logging by
calling ceTraceRange(sctx, coll.PhysicalID, colNames, indexRanges, "Index
Stats-Pseudo", uint64(result.Est)) or equivalent using result.Est, and remove
all conflict markers so the file compiles.
In `@pkg/planner/cardinality/selectivity_test.go`:
- Around line 306-360: The test fails to compile because it uses ast.NewCIStr
(undefined here) and assumes a different cardinality API shape; change the
TableByName call to use pmodel.NewCIStr (replace ast.NewCIStr with
pmodel.NewCIStr) so the CIStr import matches the rest of the file, and update
the three calls to cardinality.GetRowCountByIndexRanges and their result usages
to match the current cardinality implementation (adjust the function parameters
if it doesn't accept a nil for the last arg and access the actual result field
name returned by that function instead of .Est if the real struct uses a
different field).
In `@pkg/planner/core/find_best_task.go`:
- Around line 682-693: Remove the leftover merge-conflict markers and reconcile
the candidatePath struct/field model so it uses the new fields instead of the
old isMatchProp boolean; specifically replace the old isMatchProp bool with
matchPropResult property.PhysicalPropMatchResult and include the new fields
partialOrderMatchResult property.PartialOrderMatchResult, isFullRange bool and
eqOrInCount int while keeping indexJoinCols int, then update all usages of
candidatePath that referenced isMatchProp or the old model (including logic
around TopN/partial order, full-range checks and equal-predicate counting) to
use matchPropResult, partialOrderMatchResult, isFullRange and eqOrInCount
accordingly and remove any remaining conflict markers at the noted locations so
the file compiles.
In `@pkg/planner/core/stats.go`:
- Around line 335-357: The deriveTablePathStats function contains unresolved
merge conflict markers and an undeclared identifier lenAccessConds; remove the
conflict block and unify the intended logic: replace the conflict with the
optimized branch that checks if lenAccessConds==0 (ensure lenAccessConds is
defined/available in scope) and if ds.Table.GetPartitionedTable()==nil then set
path.CountAfterAccess = float64(ds.StatisticTable.RealtimeCount), otherwise call
cardinality.GetRowCountByColumnRanges (capture statistics.RowEstimate and set
path.CountAfterAccess = countEst.Est); then, if !isIm invoke
adjustCountAfterAccess(ds, path) to apply the lower-bound logic. Ensure you
remove references to the old GetRowCountByIntColumnRanges or reconcile its usage
with GetRowCountByColumnRanges and fix imports/types so the function compiles.
- Around line 351-352: The call to GetRowCountByColumnRanges uses the old
5-arg/RowEstimate API; update it to the current 4-arg signature that returns a
float64: call cardinality.GetRowCountByColumnRanges(ds.SCtx(),
&ds.StatisticTable.HistColl, pkCol.ID, path.Ranges), capture the returned
float64 (not a RowEstimate) and assign it directly to path.CountAfterAccess (or
convert as needed), and remove the extra boolean argument and RowEstimate
handling.
---
Outside diff comments:
In `@pkg/planner/cardinality/BUILD.bazel`:
- Around line 49-66: This change touches Bazel BUILD metadata (the
cardiniality_test target in BUILD.bazel with a modified shard_count), so run
make bazel_prepare locally to regenerate Bazel metadata and include the
resulting changes from that command in the PR; commit the generated Bazel
outputs (the files created/updated by bazel_prepare) alongside this BUILD.bazel
edit so the repository's Bazel state remains consistent with the change to the
cardiniality_test target.
In `@pkg/planner/cardinality/row_count_index.go`:
- Around line 41-42: The function signature for GetRowCountByIndexRanges
currently accepts four parameters but some callers pass a fifth nil; update the
signature to accept an extra parameter (e.g., extra interface{} or opts
interface{}) so callers that pass nil compile, e.g., change func
GetRowCountByIndexRanges(sctx planctx.PlanContext, coll *statistics.HistColl,
idxID int64, indexRanges []*ranger.Range) to include a final extra parameter and
leave the new argument unused inside the function (or use it if needed),
ensuring all existing call sites that pass nil continue to work; update any
references to GetRowCountByIndexRanges accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ebd6c3d9-5dca-4927-8375-f5f4d8879658
📒 Files selected for processing (5)
pkg/planner/cardinality/BUILD.bazelpkg/planner/cardinality/row_count_index.gopkg/planner/cardinality/selectivity_test.gopkg/planner/core/find_best_task.gopkg/planner/core/stats.go
ti-chi-bot
Bot
added
cherry-pick-approved
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/planner/core/find_best_task.go (1)
1178-1193:⚠️ Potential issue | 🟠 MajorUse unsigned-aware full-range detection for index-join candidates.
At Line 1191,
ranger.HasFullRange(path.Ranges, false)hardcodes signed-handle semantics. For unsigned PK-handle ranges (e.g.,[0,+inf)), this can cacheisFullRangeincorrectly and bias later prefer-range pruning decisions.💡 Proposed fix
-func getIndexCandidateForIndexJoin(sctx planctx.PlanContext, path *util.AccessPath, indexJoinCols int) *candidatePath { +func getIndexCandidateForIndexJoin(sctx planctx.PlanContext, tableInfo *model.TableInfo, path *util.AccessPath, indexJoinCols int) *candidatePath { candidate := &candidatePath{path: path, indexJoinCols: indexJoinCols} // cherry-pick from the master: false instead of property.PropNotMatched in PR#63958 candidate.isMatchProp = false candidate.accessCondsColMap = util.ExtractCol2Len(sctx.GetExprCtx().GetEvalCtx(), path.AccessConds, path.IdxCols, path.IdxColLens) candidate.indexCondsColMap = util.ExtractCol2Len(sctx.GetExprCtx().GetEvalCtx(), append(path.AccessConds, path.IndexFilters...), path.FullIdxCols, path.FullIdxColLens) @@ - candidate.isFullRange = ranger.HasFullRange(path.Ranges, false) + candidate.isFullRange = path.IsFullScanRange(tableInfo) candidate.eqOrInCount = candidate.equalPredicateCount() return candidate }// update call site(s) accordingly: candidate := getIndexCandidateForIndexJoin(sctx, ds.TableInfo, path, indexJoinCols)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/find_best_task.go` around lines 1178 - 1193, The call in getIndexCandidateForIndexJoin sets candidate.isFullRange using ranger.HasFullRange(path.Ranges, false) which forces signed semantics and misdetects unsigned PK-handle ranges; change the code to use unsigned-aware full-range detection by passing the correct unsigned flag (derived from the table/column metadata) into HasFullRange (update getIndexCandidateForIndexJoin signature if needed to accept the table info or an isUnsigned boolean), and ensure candidate.isFullRange is computed via ranger.HasFullRange(path.Ranges, isUnsigned) so index-join candidates use correct full-range semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/planner/core/find_best_task.go`:
- Around line 1178-1193: The call in getIndexCandidateForIndexJoin sets
candidate.isFullRange using ranger.HasFullRange(path.Ranges, false) which forces
signed semantics and misdetects unsigned PK-handle ranges; change the code to
use unsigned-aware full-range detection by passing the correct unsigned flag
(derived from the table/column metadata) into HasFullRange (update
getIndexCandidateForIndexJoin signature if needed to accept the table info or an
isUnsigned boolean), and ensure candidate.isFullRange is computed via
ranger.HasFullRange(path.Ranges, isUnsigned) so index-join candidates use
correct full-range semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 427a91ac-a7fa-4c63-9fb3-e1367cb228c8
📒 Files selected for processing (6)
.cursor/commands/new-question.mdpkg/planner/cardinality/row_count_index.gopkg/planner/cardinality/selectivity_test.gopkg/planner/core/find_best_task.gopkg/planner/core/stats.gopkg/planner/util/path.go
ti-chi-bot
Bot
added
size/XXL
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/planner/core/logical_plans_test.go`:
- Around line 1564-1575: visitInfoArray.Less currently compares fields
independently which can make Less(i,j) and Less(j,i) both true; change it to a
strict lexicographic comparator that compares privilege first, if equal compare
db, if equal compare table, if equal compare column, and only return true when
the first non-equal field is less, otherwise return false — update the
visitInfoArray.Less method to perform sequential equality checks (privilege,
then db, then table, then column) and return false at the end to satisfy
sort.Interface strict weak ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5e817103-d76f-43d6-bb08-7d04cf063ad7
📒 Files selected for processing (2)
pkg/planner/core/find_best_task.gopkg/planner/core/logical_plans_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/planner/core/logical_plans_test.go (1)
2940-2957:⚠️ Potential issue | 🟡 MinorMissing resource cleanup in test loop.
Same issue as
testColumnPrivilegeVisitInfo- theMockContext()creates a domain with a stats handle that should be closed after each iteration to prevent resource leaks.🔧 Proposed fix
checkVisitInfo(t, builder.visitInfo, tt.ans, tt.sql) fp := TryFastPlan(s.ctx, nodeW) require.NotNil(t, fp, "expect '%s' to get point get plan", tt.sql) checkVisitInfo(t, pointGetVisitInfo4Test, tt.ans, tt.sql) + + domain.GetDomain(sctx).StatsHandle().Close() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/logical_plans_test.go` around lines 2940 - 2957, The test loop creates a MockContext() for each iteration but never closes the domain/stats handle it creates, causing resource leaks; after you finish using sctx (after TryFastPlan and checks) call the appropriate cleanup on the domain created for that context (e.g. domain.GetDomain(sctx).Close() or the equivalent StatsHandle.Close() for your environment) to release resources; ensure this cleanup runs for every iteration (no defer outside loop) and reference MockContext() and domain.GetDomain(sctx) in the fix so the newly created domain/stats handle is properly closed each loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/planner/core/logical_plans_test.go`:
- Around line 2806-2822: The test loop created a domain via MockContext() but
never closes its stats handle; call domain.GetDomain(sctx).StatsHandle().Close()
at the end of each iteration (after TryFastPlan/require.Nil and before the loop
continues) to mirror testNormalVisitInfo’s cleanup and avoid leaking resources;
locate the MockContext() usage in this test and add the StatsHandle().Close()
call for the domain returned by domain.GetDomain(sctx).
---
Outside diff comments:
In `@pkg/planner/core/logical_plans_test.go`:
- Around line 2940-2957: The test loop creates a MockContext() for each
iteration but never closes the domain/stats handle it creates, causing resource
leaks; after you finish using sctx (after TryFastPlan and checks) call the
appropriate cleanup on the domain created for that context (e.g.
domain.GetDomain(sctx).Close() or the equivalent StatsHandle.Close() for your
environment) to release resources; ensure this cleanup runs for every iteration
(no defer outside loop) and reference MockContext() and domain.GetDomain(sctx)
in the fix so the newly created domain/stats handle is properly closed each
loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 35c5233c-a70f-4f49-9a45-baa38d5ded55
📒 Files selected for processing (1)
pkg/planner/core/logical_plans_test.go
terry1purcell
force-pushed
the
cherry-pick-66304-to-release-8.5
branch
from
March 14, 2026 00:35
f02d756 to
4579d4a
Compare
ti-chi-bot
Bot
added
size/XL
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/planner/core/find_best_task.go (2)
848-865: Refresh the comment to match the new semantics.
compareEqOrInnow compares the cachedcandidatePath.eqOrInCount, which comes fromequalPredicateCount(), not the rawpath.EqOrInCondCount. The current doc still reads like it is the raw path field.As per coding guidelines, comments SHOULD explain non-obvious intent, constraints, invariants, concurrency guarantees, SQL/compatibility contracts, or important performance trade-offs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/find_best_task.go` around lines 848 - 865, Update the top comment for compareEqOrIn to state that the function compares the cached candidatePath.eqOrInCount (populated via equalPredicateCount()) rather than the raw path.EqOrInCondCount; explain the invariant that partial index paths short-circuit comparison (len(path.PartialIndexPaths)>0 returns tie) and clarify the intent that a larger cached eqOrInCount wins (returns 1 for lhs, -1 for rhs, 0 for tie), so readers know the comparison uses the precomputed/evaluated equal-predicate count and not the underlying path field.
1159-1164: Consider centralizing the cached candidate-state initialization.These constructors now all have to stay in lockstep on
isMatchProp,isFullRange, andeqOrInCount. A small shared initializer would reduce drift the next time skyline-pruning state grows again.As per coding guidelines, code SHOULD remain maintainable for future readers with basic TiDB familiarity and SHOULD be self-documenting through clear naming and structure.
Also applies to: 1168-1174, 1178-1192, 1196-1204, 1208-1212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/find_best_task.go` around lines 1159 - 1164, Extract the repeated candidate-state setup into a single helper (e.g., newCandidatePath or initCandidatePath) and call it from each constructor (including getTableCandidate and the other similar blocks around candidate creation). The helper should accept the candidate (or ds, path, prop) and set isMatchProp via isMatchProp(ds,path,prop), populate accessCondsColMap with util.ExtractCol2Len(ds.SCtx().GetExprCtx().GetEvalCtx(), path.AccessConds, nil, nil), set isFullRange via path.IsFullScanRange(ds.TableInfo), and compute eqOrInCount by calling equalPredicateCount() on the candidate; replace the duplicated lines in each constructor with a single call to this helper so all fields are initialized in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/planner/core/find_best_task.go`:
- Around line 848-865: Update the top comment for compareEqOrIn to state that
the function compares the cached candidatePath.eqOrInCount (populated via
equalPredicateCount()) rather than the raw path.EqOrInCondCount; explain the
invariant that partial index paths short-circuit comparison
(len(path.PartialIndexPaths)>0 returns tie) and clarify the intent that a larger
cached eqOrInCount wins (returns 1 for lhs, -1 for rhs, 0 for tie), so readers
know the comparison uses the precomputed/evaluated equal-predicate count and not
the underlying path field.
- Around line 1159-1164: Extract the repeated candidate-state setup into a
single helper (e.g., newCandidatePath or initCandidatePath) and call it from
each constructor (including getTableCandidate and the other similar blocks
around candidate creation). The helper should accept the candidate (or ds, path,
prop) and set isMatchProp via isMatchProp(ds,path,prop), populate
accessCondsColMap with util.ExtractCol2Len(ds.SCtx().GetExprCtx().GetEvalCtx(),
path.AccessConds, nil, nil), set isFullRange via
path.IsFullScanRange(ds.TableInfo), and compute eqOrInCount by calling
equalPredicateCount() on the candidate; replace the duplicated lines in each
constructor with a single call to this helper so all fields are initialized in
one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 419c3cc9-4dab-4834-ad84-1318b4c706ac
📒 Files selected for processing (5)
pkg/planner/cardinality/row_count_index.gopkg/planner/cardinality/selectivity_test.gopkg/planner/core/find_best_task.gopkg/planner/core/stats.gopkg/planner/util/path.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/planner/cardinality/row_count_index.go
- pkg/planner/util/path.go
- pkg/planner/cardinality/selectivity_test.go
ti-chi-bot
Bot
added
size/L
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #66695 +/- ##
================================================
Coverage ? 55.6143%
================================================
Files ? 1824
Lines ? 660364
Branches ? 0
================================================
Hits ? 367257
Misses ? 266205
Partials ? 26902
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
terry1purcell
changed the title
|
/retest |
Contributor
|
/retest-required |
The helper is called from both table and index candidate factories, so the previous "table scan" wording was misleading. Reword to cover access paths generally and note why tableInfo is needed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… 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>
TestCanSkipIndexEstimation imports pkg/statistics/asyncload to assert the fast path does not queue the evicted index for async load. Add the missing strict dep so nogo / bazel_build don't fail. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
/retest-required |
ti-chi-bot
Bot
added
cherry-pick-approved
qw4990
approved these changes
Jun 2, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, qw4990, terry1purcell, wjhuang2016 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Contributor
|
/unhold |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
ti-chi-bot
Bot
added
do-not-merge/cherry-pick-not-approved
cherry-pick-approved
This was referenced Jun 3, 2026
Merged
ti-chi-bot
Bot
added
do-not-merge/cherry-pick-not-approved
cherry-pick-approved
This was referenced Jun 10, 2026
1 of 13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is an automated cherry-pick of #66304
What problem does this PR solve?
Issue Number: ref #64791 ref #63856 ref #63235
Problem Summary:
Removed link to tidb-test PR | tidb-test=pr/2708
What changed and how does it work?
This PR provides 2 main optimizations:
There are 3 issues referenced here that this PR improves upon:
#64791 - relates to async loading of statistics. This PR will avoid estimation for columns that exist in the index, but not in the query - meaning that estimation logic will build a full range. Such columns may not have had their statistics sync loaded (since they aren't interesting to the query).
#63856 - relates to long planning times with large number of indexes. Reducing overhead in skylinePruning and skipping over estimation for uninteresting indexes will improve planning time.
#63235 - customer reported that the fullrange check in skylinePruning was expensive. Reducing the number of times that this is set in skylinePruning should reduce that overhead. However, it is now included in regular index estimation which can incur cost - which should be negligible or irrelevant if estimation is skipped for any indexes.
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
Improvements
Tests