planner: support basic usage of partial index (#65051) - #68832
Merged
ti-chi-bot[bot] merged 8 commits intoJun 11, 2026
Merged
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
component/statistics
do-not-merge/hold
13 tasks
Member
Author
|
@winoros 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:
📝 WalkthroughWalkthroughAdds a new partidx constraint library, marks/prunes conditional index access paths, propagates a NotAlwaysValid flag into physical index scans and plan-cache checks, wires failpoint hooks for pruning tests, and adds unit/integration tests and fixtures. ChangesPartial Index Planning Support
Sequence Diagram(s)sequenceDiagram
participant DataSource
participant CheckPartialIndexes
participant CheckPartialIndexByFilters
participant partidx
participant Planner
participant PhysicalIndexScan
DataSource->>CheckPartialIndexes: PossibleAccessPaths, PushedDownConds
CheckPartialIndexes->>CheckPartialIndexByFilters: each partial-index path
CheckPartialIndexByFilters->>partidx: CheckConstraints / AlwaysMeetConstraints
partidx-->>CheckPartialIndexByFilters: valid, notAlwaysValid
CheckPartialIndexByFilters-->>CheckPartialIndexes: valid, notAlwaysValid
CheckPartialIndexes-->>Planner: updated PossibleAccessPaths (pruned/marked)
Planner->>PhysicalIndexScan: construct scan from path
PhysicalIndexScan->>PhysicalIndexScan: set NotAlwaysValid from path flag
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 15
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/casetest/index/index_test.go (1)
23-37:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix merge-conflict import block so pkg/planner/core/casetest/index/index_test.go builds
- Remove unresolved
<<<<<<</>>>>>>>markers in the import section (lines ~23-37); the file currently won’t compile.- Keep
github.com/pingcap/tidb/pkg/parser/modelsincemodel.NewCIStris still used (line ~295).- If both
github.com/pingcap/tidb/pkg/planner/utilandgithub.com/pingcap/tidb/pkg/utilare imported, one must be aliased because both packages declarepackage util.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/planner/core/casetest/index/index_test.go` around lines 23 - 37, Remove the merge markers and fix the import list so the file compiles: delete the <<<<<<< / ======= / >>>>>>> lines, keep github.com/pingcap/tidb/pkg/parser/model (because model.NewCIStr is used), and retain any needed imports from the other side (e.g., github.com/pingcap/tidb/pkg/parser/ast, github.com/pingcap/tidb/pkg/planner/util, github.com/pingcap/tidb/pkg/session/sessmgr) but avoid the package name collision with github.com/pingcap/tidb/pkg/util by aliasing one of them (for example alias github.com/pingcap/tidb/pkg/planner/util as plannerutil) and update any uses accordingly (referencing model.NewCIStr and any plannerutil symbols if renamed).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/planner/core/casetest/index/BUILD.bazel`:
- Around line 12-37: The BUILD.bazel hunk contains unresolved git conflict
markers (<<<<<<<, =======, >>>>>>>) around the shard_count and deps block;
remove the conflict markers and reconcile the two versions so the final block
sets shard_count = 11 and includes the merged deps list (keep "//pkg/domain",
"//pkg/domain/infosync", add "//pkg/parser/ast", "//pkg/planner/util",
"//pkg/session/sessmgr", and retain "//pkg/store/mockstore", "//pkg/testkit",
"//pkg/testkit/testdata", "//pkg/testkit/testfailpoint",
"//pkg/testkit/testmain", "//pkg/testkit/testsetup" and
"`@com_github_pingcap_failpoint//`:failpoint"), making sure commas and bracket
syntax are correct and there are no leftover conflict lines in the deps array.
In `@pkg/planner/core/casetest/index/index_test.go`:
- Around line 403-445: The test enables the failpoint named by fpName
("github.com/pingcap/tidb/pkg/planner/core/rule/InjectCheckForIndexPrune")
multiple times using failpoint.EnableCall but only calls failpoint.Disable at
the end, risking leakage if an assertion fails; immediately after each
failpoint.EnableCall (or right after the first EnableCall) register a guaranteed
cleanup via t.Cleanup(func(){ require.NoError(t, failpoint.Disable(fpName)) })
or pair each scenario's EnableCall with its own t.Cleanup to always disable the
failpoint; refer to fpName, failpoint.EnableCall, failpoint.Disable and
t.Cleanup to locate and fix the spots.
In `@pkg/planner/core/exhaust_physical_plans.go`:
- Around line 799-957: The file contains unresolved git conflict markers
(<<<<<<<, =======, >>>>>>>) around the implementations of
buildDataSource2IndexScanByIndexJoinProp,
buildDataSource2TableScanByIndexJoinProp and completeIndexJoinFeedBackInfo;
remove the conflict markers and reconcile the two variants so the final code
contains a single correct implementation for each of those functions (preserve
the intended logic such as indexValid, getBestIndexJoinPathResultByProp usage,
table-path handling, ranges/keyOff2IdxOff flow, and the call to
completeIndexJoinFeedBackInfo), ensure the function signatures and referenced
symbols (buildDataSource2IndexScanByIndexJoinProp,
buildDataSource2TableScanByIndexJoinProp, completeIndexJoinFeedBackInfo,
getBestIndexJoinPathResultByProp, constructDS2IndexScanTask,
constructDS2TableScanTask) remain consistent, and run build/tests to verify no
remaining conflict markers or compile errors.
In `@pkg/planner/core/indexmerge_path.go`:
- Around line 138-152: The file contains unresolved Git conflict markers inside
the function generateNormalIndexPartialPaths4DNF: you must remove the conflict
markers and reconcile the two variants into one consistent function signature
and body; decide whether the function should handle multiple dnfItems (dnfItems
[]expression.Expression, candidatePaths []*util.AccessPath, returning
([]*util.AccessPath, bool, []bool)) or the single-item variant (item
expression.Expression, candidatePath *util.AccessPath, returning
(*util.AccessPath, bool)), then update the function name/signature and its
internal logic accordingly (including the partial index rejection using
candidatePath.Index.HasCondition() if keeping the single-item form), remove all
<<<<<<<, =======, >>>>>>> markers, and ensure callers of
generateNormalIndexPartialPaths4DNF are adjusted to the chosen signature so the
file compiles.
In `@pkg/planner/core/operator/logicalop/logical_datasource.go`:
- Around line 32-36: Remove the leftover merge-conflict markers in
logical_datasource.go and restore both imports so the file imports
"github.com/pingcap/tidb/pkg/planner/core/constraint" and
"github.com/pingcap/tidb/pkg/planner/core/partidx" (used by PredicatePushDown ->
constraint.DeleteTrueExprs and CheckPartialIndexes -> partidx.CheckConstraints /
partidx.AlwaysMeetConstraints); then fix the DataSource struct so assignments in
CheckPartialIndexes compile by adding an AllPossibleAccessPaths field (matching
the type used where PossibleAccessPaths is defined) or alternatively update
CheckPartialIndexes to assign into the existing PossibleAccessPaths
consistently—prefer adding AllPossibleAccessPaths to DataSource to preserve
current logic.
- Around line 752-763: DataSource currently uses ds.AllPossibleAccessPaths but
the DataSource struct only defines PossibleAccessPaths; add a new field
AllPossibleAccessPaths []*util.AccessPath to the DataSource struct (same
visibility and type as PossibleAccessPaths) and ensure every place that
initializes or updates PossibleAccessPaths also sets/updates
AllPossibleAccessPaths (e.g., constructor/initializer and any methods that
modify access paths), so the slices.DeleteFunc calls in logical_datasource.go
and the iteration in stats.go use a populated AllPossibleAccessPaths;
alternatively, if you prefer the other approach, replace all references to
AllPossibleAccessPaths (in logical_datasource.go and pkg/planner/core/stats.go)
with PossibleAccessPaths consistently across the change set.
In `@pkg/planner/core/operator/physicalop/physical_index_scan.go`:
- Around line 482-499: Partition pruning currently drops the global-index
partition filter when all returned partition IDs are in pInfo.IDsInDDLToIgnore()
causing len(args)==1 and an early return of conditions; instead, detect this
case after building ignoreMap/args (using idxArr, pInfo.Definitions and
pInfo.IDsInDDLToIgnore()) and do not return the unmodified conditions—preserve
the global-index partition filter by adding a predicate that yields no matches
(e.g., a false/empty-partition predicate) or otherwise ensure the scan remains
restricted, so that an empty effective partition set does not become an
unfiltered global-index scan.
In `@pkg/planner/core/partidx/check_constraint.go`:
- Around line 181-220: The checkIsNullRejected function fails to recognize the
"IS NOT NULL" pattern because it never handles the unary NOT wrapper (e.g.,
NOT(ISNULL(a))), causing PartIdxCondNotAlwaysValid to flip incorrectly; update
checkIsNullRejected to detect a unary-not scalar function (ast.UnaryNot / NOT)
whose single argument is an IsNull scalar function on the target column and
treat that as not rejected (return false); ensure you reference
checkIsNullRejected and the ScalarFunction handling so the new branch checks for
filter.FuncName == ast.UnaryNot (or equivalent NOT token), extracts the inner
arg, confirms it's an IsNull on targetCol (using col.Equal with
sctx.GetExprCtx().GetEvalCtx()) and returns false.
In `@pkg/planner/core/plan_cacheable_checker.go`:
- Around line 585-609: The file contains unresolved git merge markers (<<<<<<<,
=======, >>>>>>>) around the plan-type switch; remove those markers and keep the
intended merged branch code: use x.PartialPlansRaw -> subPlans, handle
physicalop.PhysicalIndexReader (append x.IndexPlan),
physicalop.PhysicalIndexLookUpReader (append x.IndexPlan), and the checks on
physicalop.PhysicalIndexScan (use x.IsFullScan(), x.Index.HasCondition(),
x.NotAlwaysValid) and physicalop.PhysicalTableScan (use x.IsFullScan()); ensure
duplicate or old symbols like PartialPlans/PhysicalIndexScan casing are not left
behind so the file compiles.
In `@pkg/planner/core/planbuilder.go`:
- Around line 1454-1470: Remove the leftover merge conflict markers and
normalize the logic: keep the boolean flag allUndeterminedPath, iterate over the
available slice and call availablePath.IsUndetermined() (stop and set
allUndeterminedPath = false if any path is determined), and if
allUndeterminedPath append tablePath to available; delete any stray variables or
markers like allMVIIndexPath and the conflict tokens (<<<<<<<, =======, >>>>>>>)
so the path-selection branch in planbuilder.go compiles and behaves
unambiguously.
In `@pkg/planner/core/rule/rule_prune_indexes.go`:
- Around line 202-210: The second safety block in buildFinalResult that checks
"if len(result) == len(tablePaths)+len(mvIndexPaths) && len(preferredIndexes) ==
0 { return paths }" restores pruned zero-score regular indexes; remove that
condition (or its early return) so buildFinalResult does not undo pruning when
only mandatory table/MV paths remain — keep the existing first safety check "if
len(result) == 0 { return paths }" and delete the additional fallback
referencing result, tablePaths, mvIndexPaths, preferredIndexes, and paths.
In `@pkg/planner/core/stats.go`:
- Around line 138-145: Remove the leftover git conflict markers and restore the
intended logic: call ds.CheckPartialIndexes() and iterate over
ds.AllPossibleAccessPaths (not ds.PossibleAccessPaths). Replace the conflict
block (the <<<<<<</=======/>>>>>>> lines) with the two statements in the merged
change so partial-index initialization runs and you fill index paths for all
paths using ds.CheckPartialIndexes() followed by for _, path := range
ds.AllPossibleAccessPaths { ... } in the function (refer to symbols
ds.CheckPartialIndexes, ds.AllPossibleAccessPaths, and ds.PossibleAccessPaths to
locate the hunk).
In `@pkg/planner/util/path.go`:
- Around line 426-468: The file contains unresolved merge markers around the
AccessPath methods; remove the conflict markers so only a single implementation
remains (keep the IsFullScanRange, IsUndetermined and IsIndexJoinUnapplicable
methods as shown), and add the missing import for mysql so the call to
mysql.HasUnsignedFlag(...) in AccessPath.IsFullScanRange compiles; update the
import block to include "github.com/pingcap/tidb/parser/mysql" (or the correct
local module path used in the repo) and ensure there are no leftover <<<<<<<,
=======, >>>>>>> tokens in pkg/planner/util/path.go.
- Around line 431-441: The file uses mysql.HasUnsignedFlag inside
AccessPath.IsFullScanRange but the package is not imported; add an import for
"github.com/pingcap/tidb/pkg/parser/mysql" to the import block in
pkg/planner/util/path.go so mysql.HasUnsignedFlag(pkColInfo.GetFlag()) resolves;
locate the AccessPath.IsFullScanRange function and ensure the mysql package is
referenced by that import name.
In `@tests/integrationtest/r/planner/core/casetest/index/partialindex.result`:
- Around line 43-50: The fixture is asserting a Point_Get on a partial unique
index (Point_Get ... index:idx3(c)) but the planner change excludes partial
indexes from point-get plans; update the test expectation by regenerating the
integration test result for this case (partialindex) so it no longer asserts
Point_Get for queries involving the partial index (look for the Selection block
and the Point_Get operator info and replace the expected plan with the actual
plan produced after the merge-conflict resolution, or run the test generator to
refresh partialindex.result).
---
Outside diff comments:
In `@pkg/planner/core/casetest/index/index_test.go`:
- Around line 23-37: Remove the merge markers and fix the import list so the
file compiles: delete the <<<<<<< / ======= / >>>>>>> lines, keep
github.com/pingcap/tidb/pkg/parser/model (because model.NewCIStr is used), and
retain any needed imports from the other side (e.g.,
github.com/pingcap/tidb/pkg/parser/ast,
github.com/pingcap/tidb/pkg/planner/util,
github.com/pingcap/tidb/pkg/session/sessmgr) but avoid the package name
collision with github.com/pingcap/tidb/pkg/util by aliasing one of them (for
example alias github.com/pingcap/tidb/pkg/planner/util as plannerutil) and
update any uses accordingly (referencing model.NewCIStr and any plannerutil
symbols if renamed).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 03747b56-f2ff-4053-8862-8ac6b691a80d
📒 Files selected for processing (19)
pkg/expression/util.gopkg/planner/core/casetest/index/BUILD.bazelpkg/planner/core/casetest/index/index_test.gopkg/planner/core/exhaust_physical_plans.gopkg/planner/core/indexmerge_path.gopkg/planner/core/operator/logicalop/BUILD.bazelpkg/planner/core/operator/logicalop/logical_datasource.gopkg/planner/core/operator/physicalop/physical_index_scan.gopkg/planner/core/partidx/BUILD.bazelpkg/planner/core/partidx/check_constraint.gopkg/planner/core/plan_cacheable_checker.gopkg/planner/core/planbuilder.gopkg/planner/core/point_get_plan.gopkg/planner/core/rule/rule_prune_indexes.gopkg/planner/core/stats.gopkg/planner/util/path.gopkg/util/ranger/types.gotests/integrationtest/r/planner/core/casetest/index/partialindex.resulttests/integrationtest/t/planner/core/casetest/index/partialindex.test
Comment on lines
+43
to
+50
| explain format='brief' select * from t where c = 10 and b > 30; | ||
| id estRows task access object operator info | ||
| Selection 1.00 root gt(planner__core__casetest__index__partialindex.t.b, 30) | ||
| └─Point_Get 1.00 root table:t, index:idx3(c) | ||
| explain format='brief' select * from t where c = 10 and b > 40; | ||
| id estRows task access object operator info | ||
| Selection 1.00 root gt(planner__core__casetest__index__partialindex.t.b, 40) | ||
| └─Point_Get 1.00 root table:t, index:idx3(c) |
There was a problem hiding this comment.
The expected Point_Get here looks backwards for a partial index.
These two cases currently lock in Point_Get ... index:idx3(c) on a partial unique index. The planner changes in this PR stack explicitly exclude partial indexes from point-get plans, so this fixture should be regenerated after the merge-conflict resolution instead of asserting Point_Get here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integrationtest/r/planner/core/casetest/index/partialindex.result`
around lines 43 - 50, The fixture is asserting a Point_Get on a partial unique
index (Point_Get ... index:idx3(c)) but the planner change excludes partial
indexes from point-get plans; update the test expectation by regenerating the
integration test result for this case (partialindex) so it no longer asserts
Point_Get for queries involving the partial index (look for the Selection block
and the Point_Get operator info and replace the expected plan with the actual
plan produced after the merge-conflict resolution, or run the test generator to
refresh partialindex.result).
ti-chi-bot
Bot
added
cherry-pick-approved
Member
|
/unhold |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/planner/core/operator/logicalop/logical_datasource.go`:
- Around line 760-763: Currently ParseSimpleExpr failures are converted into
valid=false, silently demoting partial indexes; instead modify
CheckPartialIndexByFilters() (and its caller CheckPartialIndexes()) to propagate
parse errors rather than hiding them: change their signatures to return an error
(e.g., (bool,bool,error)), detect the ParseSimpleExpr error from
ds.SCtx()/index.ConditionExprString and return it with contextual information
(index name/table and the offending ConditionExprString), and update all callers
to bubble the error up or fail the planner so partial-index metadata parse
failures are reported rather than treated as predicate mismatches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: efbdab7f-422a-4b6f-9a0e-9d069d9b49d2
📒 Files selected for processing (15)
pkg/expression/util.gopkg/planner/core/casetest/index/BUILD.bazelpkg/planner/core/casetest/index/index_test.gopkg/planner/core/exhaust_physical_plans.gopkg/planner/core/find_best_task.gopkg/planner/core/indexmerge_path.gopkg/planner/core/operator/logicalop/BUILD.bazelpkg/planner/core/operator/logicalop/logical_datasource.gopkg/planner/core/partidx/check_constraint.gopkg/planner/core/physical_plans.gopkg/planner/core/plan_cacheable_checker.gopkg/planner/core/planbuilder.gopkg/planner/core/point_get_plan.gopkg/planner/core/stats.gopkg/planner/util/path.go
💤 Files with no reviewable changes (3)
- pkg/planner/core/operator/logicalop/BUILD.bazel
- pkg/planner/core/stats.go
- pkg/planner/util/path.go
✅ Files skipped from review due to trivial changes (2)
- pkg/planner/core/find_best_task.go
- pkg/planner/core/casetest/index/BUILD.bazel
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/expression/util.go
- pkg/planner/core/planbuilder.go
- pkg/planner/core/point_get_plan.go
- pkg/planner/core/partidx/check_constraint.go
Comment on lines
+760
to
+763
| expr, err := expression.ParseSimpleExpr(ds.SCtx().GetExprCtx(), index.ConditionExprString, expression.WithInputSchemaAndNames(ds.schema, columnNames, ds.TableInfo)) | ||
| if err != nil { | ||
| return false, false | ||
| } |
There was a problem hiding this comment.
Don't silently demote partial indexes on predicate parse failures.
Line 760 turns any ParseSimpleExpr failure into valid=false, so corrupted or unsupported partial-index metadata becomes indistinguishable from an ordinary predicate mismatch. That silently changes access-path selection and can make hinted partial indexes disappear without any actionable signal. Please plumb this error out of CheckPartialIndexByFilters() / CheckPartialIndexes() so the planner can fail with context instead of silently falling back.
As per coding guidelines, "Keep error handling actionable and contextual; avoid silently swallowing errors."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/planner/core/operator/logicalop/logical_datasource.go` around lines 760 -
763, Currently ParseSimpleExpr failures are converted into valid=false, silently
demoting partial indexes; instead modify CheckPartialIndexByFilters() (and its
caller CheckPartialIndexes()) to propagate parse errors rather than hiding them:
change their signatures to return an error (e.g., (bool,bool,error)), detect the
ParseSimpleExpr error from ds.SCtx()/index.ConditionExprString and return it
with contextual information (index name/table and the offending
ConditionExprString), and update all callers to bubble the error up or fail the
planner so partial-index metadata parse failures are reported rather than
treated as predicate mismatches.
Source: Coding guidelines
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #68832 +/- ##
================================================
Coverage ? 55.6487%
================================================
Files ? 1845
Lines ? 665115
Branches ? 0
================================================
Hits ? 370128
Misses ? 267373
Partials ? 27614
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
winoros
force-pushed
the
cherry-pick-65051-to-release-8.5
branch
from
June 6, 2026 16:00
1611813 to
baaaa0b
Compare
Member
|
/retest |
Member
|
/unhold |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/planner/core/casetest/index/index_test.go (1)
396-434:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuarantee failpoint teardown with
t.Cleanup(Line 396 onward).
failpoint.Disableonly runs at Line 433. If anyrequire/query fails earlier, the failpoint can leak and make later tests flaky.Suggested minimal fix
fpName := "github.com/pingcap/tidb/pkg/planner/core/rule/InjectCheckForIndexPrune" + t.Cleanup(func() { + require.NoError(t, failpoint.Disable(fpName)) + }) require.NoError(t, failpoint.EnableCall(fpName, func(paths []*plannerutil.AccessPath) { for _, path := range paths { if path != nil && path.Index != nil && path.Index.ConditionExprString != "" { require.True(t, false, "Partial index should be pruned") } } })) @@ - require.NoError(t, failpoint.Disable(fpName)) }) }As per coding guidelines, “Unit tests in a package that uses failpoints: MUST enable failpoints before tests and disable afterward.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/planner/core/casetest/index/index_test.go` around lines 396 - 434, The test enables a failpoint via fpName using failpoint.EnableCall but only disables it with failpoint.Disable at the end, risking leak if an earlier require/query fails; wrap the enable/disable with t.Cleanup to guarantee teardown: after each failpoint.EnableCall(fpName, ...) call register a t.Cleanup(func(){ require.NoError(t, failpoint.Disable(fpName)) }) (or register once immediately after the first successful EnableCall) so the named failpoint is always disabled even on test failures; update code around fpName, failpoint.EnableCall and failpoint.Disable to use t.Cleanup.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/planner/core/rule/rule_prune_indexes.go`:
- Around line 254-260: The loop uses ds.TableInfo.Columns[col.Offset] without
safety checks and can panic; add the same nil/bounds guards used in the
fallback: verify ds.TableInfo and ds.TableInfo.Columns are non-nil, check col is
non-nil (if pointer) and that col.Offset is within
0..len(ds.TableInfo.Columns)-1 before indexing, then look up the column ID and
test req.interestingColIDs; only return score when all checks pass. Ensure you
update the code around path.Index, path.Index.ConditionExprString, and
path.Index.AffectColumn to include these guards.
---
Duplicate comments:
In `@pkg/planner/core/casetest/index/index_test.go`:
- Around line 396-434: The test enables a failpoint via fpName using
failpoint.EnableCall but only disables it with failpoint.Disable at the end,
risking leak if an earlier require/query fails; wrap the enable/disable with
t.Cleanup to guarantee teardown: after each failpoint.EnableCall(fpName, ...)
call register a t.Cleanup(func(){ require.NoError(t, failpoint.Disable(fpName))
}) (or register once immediately after the first successful EnableCall) so the
named failpoint is always disabled even on test failures; update code around
fpName, failpoint.EnableCall and failpoint.Disable to use t.Cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 590c5450-52bb-4a04-bacb-3297290514ff
📒 Files selected for processing (11)
pkg/planner/core/casetest/index/BUILD.bazelpkg/planner/core/casetest/index/index_test.gopkg/planner/core/exhaust_physical_plans.gopkg/planner/core/find_best_task.gopkg/planner/core/operator/logicalop/logical_datasource.gopkg/planner/core/physical_plans.gopkg/planner/core/plan_cacheable_checker.gopkg/planner/core/rule/BUILD.bazelpkg/planner/core/rule/rule_prune_indexes.gopkg/planner/core/stats.gopkg/planner/util/path.go
✅ Files skipped from review due to trivial changes (2)
- pkg/planner/core/rule/BUILD.bazel
- pkg/planner/core/find_best_task.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/planner/core/stats.go
- pkg/planner/core/plan_cacheable_checker.go
- pkg/planner/util/path.go
- pkg/planner/core/exhaust_physical_plans.go
- pkg/planner/core/operator/logicalop/logical_datasource.go
Comment on lines
+254
to
+260
| if path.Index != nil && path.Index.ConditionExprString != "" { | ||
| for _, col := range path.Index.AffectColumn { | ||
| if _, found := req.interestingColIDs[ds.TableInfo.Columns[col.Offset].ID]; !found { | ||
| return score | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing nil and bounds checks can panic.
This block accesses ds.TableInfo.Columns[col.Offset] without validating that ds.TableInfo is non-nil, that col is non-nil, or that col.Offset is within bounds. The fallback path at lines 287-293 has proper bounds checking—this block should match that pattern.
🛡️ Proposed fix adding safety checks
if path.Index != nil && path.Index.ConditionExprString != "" {
+ if ds.TableInfo == nil || ds.TableInfo.Columns == nil {
+ return score
+ }
for _, col := range path.Index.AffectColumn {
- if _, found := req.interestingColIDs[ds.TableInfo.Columns[col.Offset].ID]; !found {
+ if col == nil || col.Offset < 0 || col.Offset >= len(ds.TableInfo.Columns) {
+ return score
+ }
+ colInfo := ds.TableInfo.Columns[col.Offset]
+ if colInfo == nil {
+ return score
+ }
+ if _, found := req.interestingColIDs[colInfo.ID]; !found {
return score
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if path.Index != nil && path.Index.ConditionExprString != "" { | |
| for _, col := range path.Index.AffectColumn { | |
| if _, found := req.interestingColIDs[ds.TableInfo.Columns[col.Offset].ID]; !found { | |
| return score | |
| } | |
| } | |
| } | |
| if path.Index != nil && path.Index.ConditionExprString != "" { | |
| if ds.TableInfo == nil || ds.TableInfo.Columns == nil { | |
| return score | |
| } | |
| for _, col := range path.Index.AffectColumn { | |
| if col == nil || col.Offset < 0 || col.Offset >= len(ds.TableInfo.Columns) { | |
| return score | |
| } | |
| colInfo := ds.TableInfo.Columns[col.Offset] | |
| if colInfo == nil { | |
| return score | |
| } | |
| if _, found := req.interestingColIDs[colInfo.ID]; !found { | |
| return score | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/planner/core/rule/rule_prune_indexes.go` around lines 254 - 260, The loop
uses ds.TableInfo.Columns[col.Offset] without safety checks and can panic; add
the same nil/bounds guards used in the fallback: verify ds.TableInfo and
ds.TableInfo.Columns are non-nil, check col is non-nil (if pointer) and that
col.Offset is within 0..len(ds.TableInfo.Columns)-1 before indexing, then look
up the column ID and test req.interestingColIDs; only return score when all
checks pass. Ensure you update the code around path.Index,
path.Index.ConditionExprString, and path.Index.AffectColumn to include these
guards.
…051-to-release-8.5 # Conflicts: # pkg/planner/core/stats.go
winoros
force-pushed
the
cherry-pick-65051-to-release-8.5
branch
from
June 9, 2026 08:37
fd1474a to
9c49283
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/planner/core/rule/rule_prune_indexes.go (1)
261-267:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd nil and bounds checks to prevent panic.
This block accesses
ds.TableInfo.Columns[col.Offset].IDwithout validating thatds.TableInfo,ds.TableInfo.Columns, orcolare non-nil, or thatcol.Offsetis within bounds. The fallback path at lines 290-310 includes these safety checks; this block should match that pattern to avoid runtime panics when encountering partial indexes with unexpected metadata.🛡️ Proposed fix adding safety guards
if path.Index != nil && path.Index.ConditionExprString != "" { + if ds.TableInfo == nil || ds.TableInfo.Columns == nil { + return score + } for _, col := range path.Index.AffectColumn { + if col == nil || col.Offset < 0 || col.Offset >= len(ds.TableInfo.Columns) { + return score + } + colInfo := ds.TableInfo.Columns[col.Offset] + if colInfo == nil { + return score + } - if _, found := req.interestingColIDs[ds.TableInfo.Columns[col.Offset].ID]; !found { + if _, found := req.interestingColIDs[colInfo.ID]; !found { return score } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/planner/core/rule/rule_prune_indexes.go` around lines 261 - 267, The current loop over path.Index.AffectColumn accesses ds.TableInfo.Columns[col.Offset].ID without safety checks; update the block that checks path.Index != nil && path.Index.ConditionExprString != "" to first verify ds != nil, ds.TableInfo != nil, ds.TableInfo.Columns != nil, and that col is non-nil and 0 <= col.Offset < len(ds.TableInfo.Columns) before reading .ID, mirroring the null/bounds guards used in the fallback branch (the same pattern around the loop over path.Index.AffectColumn); only perform the req.interestingColIDs lookup and potentially return score when all these guards pass for each col.
🧹 Nitpick comments (1)
pkg/planner/core/rule/rule_prune_indexes.go (1)
208-210: Clarify/limit the index-prune fallback atrule_prune_indexes.go(lines 208–210)
preferredIndexesis only populated for indexes withIsSingleScanoridxScore.interestingCount > 0; so whenpreferredIndexesis empty andbuildFinalResultreturns onlytablePaths + mvIndexPaths(i.e., noindexMergeIndexPaths), this fallback returns the originalpaths, effectively disabling pruning and re-adding all regular 0-score indexes.- This matches the existing test expectation in
TestPruneIndexesByWhereAndOrder/no_interesting_columns(it keeps all access paths when there are no interesting columns), but it can also trigger when interesting columns exist yet no regular index covers them.- If the intent is “don’t prune only when there truly are no interesting columns,” gate the fallback on that condition (e.g.,
len(req.interestingColIDs)==0) rather than onpreferredIndexes==0.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/planner/core/rule/rule_prune_indexes.go` around lines 208 - 210, The fallback in rule_prune_indexes.go currently returns the original paths when buildFinalResult yields only tablePaths+mvIndexPaths and preferredIndexes is empty, which can incorrectly disable pruning when interesting columns exist but no preferred regular indexes were found; change the guard so the fallback triggers only when there are truly no interesting columns (check len(req.interestingColIDs) == 0) rather than len(preferredIndexes) == 0, i.e., keep the early return condition based on the absence of interestingColIDs while still allowing pruning when interesting columns exist but preferredIndexes is empty; update the conditional that references result, tablePaths, mvIndexPaths, preferredIndexes (and mentions indexMergeIndexPaths) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@pkg/planner/core/rule/rule_prune_indexes.go`:
- Around line 261-267: The current loop over path.Index.AffectColumn accesses
ds.TableInfo.Columns[col.Offset].ID without safety checks; update the block that
checks path.Index != nil && path.Index.ConditionExprString != "" to first verify
ds != nil, ds.TableInfo != nil, ds.TableInfo.Columns != nil, and that col is
non-nil and 0 <= col.Offset < len(ds.TableInfo.Columns) before reading .ID,
mirroring the null/bounds guards used in the fallback branch (the same pattern
around the loop over path.Index.AffectColumn); only perform the
req.interestingColIDs lookup and potentially return score when all these guards
pass for each col.
---
Nitpick comments:
In `@pkg/planner/core/rule/rule_prune_indexes.go`:
- Around line 208-210: The fallback in rule_prune_indexes.go currently returns
the original paths when buildFinalResult yields only tablePaths+mvIndexPaths and
preferredIndexes is empty, which can incorrectly disable pruning when
interesting columns exist but no preferred regular indexes were found; change
the guard so the fallback triggers only when there are truly no interesting
columns (check len(req.interestingColIDs) == 0) rather than
len(preferredIndexes) == 0, i.e., keep the early return condition based on the
absence of interestingColIDs while still allowing pruning when interesting
columns exist but preferredIndexes is empty; update the conditional that
references result, tablePaths, mvIndexPaths, preferredIndexes (and mentions
indexMergeIndexPaths) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6b5848c3-486c-44b2-b706-556aa14c9e84
📒 Files selected for processing (21)
pkg/expression/util.gopkg/planner/core/casetest/index/BUILD.bazelpkg/planner/core/casetest/index/index_test.gopkg/planner/core/exhaust_physical_plans.gopkg/planner/core/find_best_task.gopkg/planner/core/indexmerge_path.gopkg/planner/core/operator/logicalop/BUILD.bazelpkg/planner/core/operator/logicalop/logical_datasource.gopkg/planner/core/partidx/BUILD.bazelpkg/planner/core/partidx/check_constraint.gopkg/planner/core/physical_plans.gopkg/planner/core/plan_cacheable_checker.gopkg/planner/core/planbuilder.gopkg/planner/core/point_get_plan.gopkg/planner/core/rule/BUILD.bazelpkg/planner/core/rule/rule_prune_indexes.gopkg/planner/core/stats.gopkg/planner/util/path.gopkg/util/ranger/types.gotests/integrationtest/r/planner/core/casetest/index/partialindex.resulttests/integrationtest/t/planner/core/casetest/index/partialindex.test
✅ Files skipped from review due to trivial changes (3)
- pkg/planner/core/operator/logicalop/BUILD.bazel
- tests/integrationtest/r/planner/core/casetest/index/partialindex.result
- pkg/expression/util.go
🚧 Files skipped from review as they are similar to previous changes (17)
- pkg/planner/core/rule/BUILD.bazel
- pkg/planner/core/stats.go
- pkg/planner/core/physical_plans.go
- pkg/planner/core/partidx/BUILD.bazel
- pkg/planner/core/casetest/index/BUILD.bazel
- pkg/planner/core/find_best_task.go
- tests/integrationtest/t/planner/core/casetest/index/partialindex.test
- pkg/planner/core/indexmerge_path.go
- pkg/planner/core/operator/logicalop/logical_datasource.go
- pkg/planner/util/path.go
- pkg/planner/core/plan_cacheable_checker.go
- pkg/util/ranger/types.go
- pkg/planner/core/point_get_plan.go
- pkg/planner/core/partidx/check_constraint.go
- pkg/planner/core/planbuilder.go
- pkg/planner/core/exhaust_physical_plans.go
- pkg/planner/core/casetest/index/index_test.go
winoros
approved these changes
Jun 9, 2026
…051-to-release-8.5
winoros
force-pushed
the
cherry-pick-65051-to-release-8.5
branch
from
June 10, 2026 04:20
84ab621 to
b62f363
Compare
YangKeao
approved these changes
Jun 10, 2026
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
winoros
force-pushed
the
cherry-pick-65051-to-release-8.5
branch
from
June 10, 2026 06:21
b62f363 to
59a06c5
Compare
Member
|
/retest |
Contributor
|
/lgtm |
Contributor
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lance6716, windtalker, winoros, YangKeao 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 |
Member
|
/retest |
…051-to-release-8.5 # Conflicts: # pkg/planner/core/casetest/rule/BUILD.bazel
Member
|
/retest |
1 similar comment
Member
|
/retest |
This was referenced Jul 2, 2026
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
5 participants
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 #65051
What problem does this PR solve?
Issue Number: close #64344
Problem Summary:
What changed and how does it work?
This pr adds the basic support for the partial index.
Please feel free to review the unit test for its usage.
We are using the existing extracting range logic to support the partial index's flexibility: we can use the partial index defined as
idx(b) where b > 2when the given SQL iswhere b > 3.For the complex functions like
idx(b) where sin(b) > 0.1, we check whether there's an exact one in the WHERE clause.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
New Features
Tests