ddl,parser: modify schema to store the partial condition (#62759)#68831
Merged
ti-chi-bot[bot] merged 3 commits intoJun 4, 2026
Merged
Conversation
ti-chi-bot
added
do-not-merge/hold
13 tasks
Member
Author
|
@YangKeao 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. |
|
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)
📝 WalkthroughWalkthroughValidate and normalize partial-index WHERE predicates, persist the normalized condition string in index metadata and job args, surface predicate text in information_schema and SHOW CREATE TABLE, reject PRIMARY KEYs with partial conditions, and add integration and TiKV tests. ChangesPartial Index Condition Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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)
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)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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 (5)
tests/integrationtest/r/executor/show.result (1)
1144-1240:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUnresolved merge conflict markers must be removed.
This file contains Git merge conflict markers (
<<<<<<< HEAD,=======,>>>>>>>) that will cause the test to fail. The conflict between the HEAD branch (lines 1145-1215 showing column/index privilege tests) and the cherry-picked changes (lines 1217-1240 showing partial index SHOW CREATE TABLE tests) must be resolved.Resolve the conflict by deciding whether to:
- Keep both test sections (if they don't conflict semantically), or
- Keep only the partial index tests (if the other tests are unrelated to this PR)
Then remove all conflict markers.
🤖 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/executor/show.result` around lines 1144 - 1240, The file contains unresolved Git conflict markers (<<<<<<< HEAD, =======, >>>>>>>) splitting tests for "show columns"/"show index" (e.g., show columns from test_show_columns_db.t, show index from test_show_index_db.t) and partial-index SHOW CREATE TABLE cases (e.g., show create table t); remove the conflict markers and resolve by either merging both test sections into a single cohesive test file (keeping the column/index privilege tests and the partial-index/show create table tests) or by selecting the intended section (prefer the partial-index SHOW CREATE TABLE block if this PR is about partial index handling), then ensure the final file has no conflict markers and the SQL statements (create table, show create table, show columns, show index, set names) remain in the correct order and syntactically valid.tests/integrationtest/r/explain_easy.result (1)
200-270:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUnresolved merge conflict markers will cause test failures.
This result file contains git merge conflict markers (
<<<<<<< HEAD,=======,>>>>>>> 4865e394b32) that were not resolved. The integration test harness will compare actual query output against this malformed expected output, causing all affected assertions to fail.Resolve the conflict by keeping the version with the
PREDICATEcolumn (lines 236-269) and removing lines 200-235 along with all conflict markers.🤖 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/explain_easy.result` around lines 200 - 270, The file contains unresolved git conflict markers (<<<<<<< HEAD, =======, >>>>>>> 4865e394b32) and two competing blocks; remove the conflict markers and the older HEAD block (the version without the PREDICATE column) and keep the version that includes the PREDICATE column/header and the plan_tree outputs; ensure only the final resolved content remains (no conflict markers or duplicate blocks) so tests compare against the expected output with PREDICATE present.tests/integrationtest/t/executor/show.test (1)
437-524:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUnresolved merge conflict markers will break test execution.
This test file contains unresolved git merge conflict markers. The test runner will fail to parse the file since
<<<<<<< HEAD,=======, and>>>>>>> ...are not valid SQL syntax.Resolve by keeping both test sections (
TestShowColumns/TestShowIndexfrom HEAD andTestShowCreateTableWithPartialIndexfrom the incoming branch) and removing all conflict markers.🤖 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/t/executor/show.test` around lines 437 - 524, The file contains unresolved Git conflict markers (<<<<<<< HEAD, =======, >>>>>>> 4865e394b32) splitting two test blocks; remove the conflict markers and merge the content so both test sections remain: retain the TestShowColumns and TestShowIndex blocks from HEAD and the TestShowCreateTableWithPartialIndex block from the incoming branch, ensuring the SQL is contiguous and syntactically valid (no leftover markers).pkg/meta/model/index.go (1)
108-170:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winResolve the unresolved merge conflict in
pkg/meta/model/index.go(compilation currently fails)
pkg/meta/model/index.gocontains merge conflict markers (<<<<<<< HEAD/=======/>>>>>>> ...) at lines ~108–169—this must be resolved.- The post-cherry-pick code includes
func (index *IndexInfo) Hash64(h base.Hasher)(line ~151); ensure thebasepackage is actually imported sobase.Hasherbuilds with the final chosenIndexInfodefinition.🤖 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/meta/model/index.go` around lines 108 - 170, The file has unresolved merge conflict markers around the IndexInfo struct and trailing methods; remove the conflict markers and keep the intended unified definition (the version that includes ast.CIStr, ast.IndexType, InvertedIndexInfo, FullTextIndexInfo and ConditionExprString) and the Hash64/Equals methods (symbols: IndexInfo, Hash64, Equals, InvertedIndexInfo, FullTextIndexInfo, ConditionExprString). After resolving the conflict, update imports to include the base package so base.Hasher is available for Hash64 and ensure any renamed types (ast.CIStr/ast.IndexType) are imported or adjusted consistently; run build to confirm the file compiles.pkg/ddl/executor.go (1)
4978-4997:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winResolve the cherry-pick conflict and finish the new
IndexArgwiring.Lines 4993-4997 still contain merge markers, so this file will not build. After resolving them, keep the
ConditionStringassignment intact—pkg/ddl/index.go:1036-1040depends on it to persist the partial-index predicate—and either restore the missingsplitOptinitialization or drop that field from this hunk.🤖 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/ddl/executor.go` around lines 4978 - 4997, The file contains unresolved merge markers in the ModifyIndexArgs IndexArg construction—remove the conflict markers and finish wiring so IndexArg includes ConditionString (set from CheckAndBuildIndexConditionString) and either include SplitOpt (splitOpt) or remove the SplitOpt field consistently; ensure you keep ConditionString intact because pkg/ddl/index.go relies on it, and if you restore SplitOpt also initialize it where other IndexArg fields are set.
🧹 Nitpick comments (2)
pkg/ddl/integration_test.go (2)
134-151: ⚖️ Poor tradeoffDeep nesting in ALTER TABLE validation loop.
The five-level nesting reduces readability. Consider extracting a helper function similar to the CREATE TABLE test segment.
🤖 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/ddl/integration_test.go` around lines 134 - 151, Extract the nested ALTER TABLE validation into a helper to mirror the CREATE TABLE test pattern: create a function (e.g., testPartialIndexValidation or validateAddIndexPartial) that accepts the testkit instance and the two data structures differentTypeLiterals and differentColumnTypes and performs the inner loops that create table t with a given columnType, run the ALTER TABLE ... add index idx_b(b) where a = <literal>, assert success with tk.MustExec when i == j and assert the specific error with tk.MustGetDBError(..., dbterror.ErrUnsupportedAddPartialIndex) otherwise, and drop the table; replace the five-level nested loops in the test with calls to this helper to improve readability while keeping the same assertions (reference symbols: differentTypeLiterals, differentColumnTypes, tk.MustExec, tk.MustGetDBError, dbterror.ErrUnsupportedAddPartialIndex).
106-110: ⚖️ Poor tradeoffHigh cyclomatic complexity in nested loops.
The four-level nesting (column types → literals → CREATE TABLE tests) creates high cyclomatic complexity. While acceptable for comprehensive test coverage, consider extracting the innermost logic into a helper function to improve readability.
♻️ Optional refactoring to reduce nesting
+func testColumnTypeLiteral(tk *testkit.TestKit, columnType, literal string, shouldSucceed bool) { + tk.MustExec("drop table if exists t;") + sql := fmt.Sprintf("create table t (a %s, b int, key(b) where a = %s);", columnType, literal) + if shouldSucceed { + tk.MustExec(sql) + tk.MustExec("drop table t;") + } else { + tk.MustGetDBError(sql, dbterror.ErrUnsupportedAddPartialIndex) + } +} + for i, columnTypes := range differentColumnTypes { for j, literals := range differentTypeLiterals { - checkColumnTypes(columnTypes, literals, i == j) + for _, columnType := range columnTypes { + for _, literal := range literals { + testColumnTypeLiteral(tk, columnType, literal, i == j) + } + } } }🤖 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/ddl/integration_test.go` around lines 106 - 110, The nested loops over differentColumnTypes and differentTypeLiterals in the test create high cyclomatic complexity; extract the innermost test logic currently called via checkColumnTypes(columnTypes, literals, i == j) into a helper function (e.g., new helper runColumnTypeCheck(columnTypes, literals, expectMatch)) and replace the inner body to call that helper from the loops, moving CREATE TABLE setup/assertions into the helper to flatten nesting and improve readability while keeping the existing differentColumnTypes, differentTypeLiterals, and checkColumnTypes semantics.
🤖 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/ddl/create_table.go`:
- Around line 1380-1390: Remove the leftover conflict markers and restore the
merged logic: first, keep the new partial-index guard that checks constr.Option
!= nil && constr.Option.Condition != nil and returns
dbterror.ErrUnsupportedAddPartialIndex, then call the existing helper
isSingleIntPKFromCol (not the undefined isSingleIntPK) to compute isSingleIntPK
using the same arguments (constr, lastCol). Ensure there are no <<<<<<<,
=======, >>>>>>> markers left and that the final hunk uses
isSingleIntPKFromCol(constr, lastCol).
In `@pkg/ddl/index.go`:
- Around line 61-67: Resolve the merge conflict markers in pkg/ddl/index.go by
removing the conflict lines (<<<<<<<, =======, >>>>>>>) and restoring both
imports so that pmodel is imported as pmodel
("github.com/pingcap/tidb/pkg/parser/model") and format is imported
("github.com/pingcap/tidb/pkg/parser/format"); ensure the import block includes
both pmodel and format alongside the existing mysql and opcode imports so
references to pmodel and format elsewhere in the file compile.
- Around line 425-434: checkAndBuildIndexInfo currently calls
CheckAndBuildIndexConditionString and sets idxInfo.ConditionExprString even when
isPK==true, allowing a partial predicate on ADD PRIMARY KEY; modify
checkAndBuildIndexInfo (the path that calls BuildIndexInfo with isPK) to detect
isPK and reject any indexOption.Condition (or non-empty conditionString) by
returning an error instead of calling CheckAndBuildIndexConditionString or
assigning idxInfo.ConditionExprString; ensure the guard triggers before setting
idxInfo.ConditionExprString so partial primary keys cannot be persisted.
In `@pkg/executor/show.go`:
- Around line 1283-1295: Remove the leftover merge conflict markers (<<<<<<<
HEAD, =======, >>>>>>>) in show.go and restore the intended conditional block
that writes index options; specifically ensure the code around
idxInfo.InvertedInfo, idxInfo.FullTextInfo (using ParserType.SQLName()), and
idxInfo.ConditionExprString uses fmt.Fprintf(buf, ...) as in the incoming change
so the three if-blocks remain and the conflict markers are deleted (look for
symbols idxInfo, InvertedInfo, FullTextInfo, ConditionExprString, buf and
fmt.Fprintf to locate and fix the section).
In `@pkg/meta/model/job_args.go`:
- Around line 1360-1392: Remove the leftover git conflict markers and resolve
the IndexArg struct: add the missing fields referenced by GetColumnarIndexType
(declare ColumnarIndexType ColumnarIndexType and IsColumnar bool) and also add
the new fields SplitOpt *IndexArgSplitOpt and ConditionString string; ensure
IndexArgSplitOpt type is present (Lower/Upper/Num/ValueLists) and then keep
GetColumnarIndexType as-is so it can read a.ColumnarIndexType and a.IsColumnar
without compile errors.
In `@tests/realtikvtest/addindextest4/BUILD.bazel`:
- Around line 13-16: Remove the Git merge conflict markers (<<<<<<< HEAD,
=======, >>>>>>> ...) in the BUILD.bazel fragment and keep the shard_count = 6
attribute intact; simply delete the conflict lines and ensure the file contains
the clean stanza with shard_count = 6 so the test parallelism change
(partial_index_test.go inclusion) is preserved.
---
Outside diff comments:
In `@pkg/ddl/executor.go`:
- Around line 4978-4997: The file contains unresolved merge markers in the
ModifyIndexArgs IndexArg construction—remove the conflict markers and finish
wiring so IndexArg includes ConditionString (set from
CheckAndBuildIndexConditionString) and either include SplitOpt (splitOpt) or
remove the SplitOpt field consistently; ensure you keep ConditionString intact
because pkg/ddl/index.go relies on it, and if you restore SplitOpt also
initialize it where other IndexArg fields are set.
In `@pkg/meta/model/index.go`:
- Around line 108-170: The file has unresolved merge conflict markers around the
IndexInfo struct and trailing methods; remove the conflict markers and keep the
intended unified definition (the version that includes ast.CIStr, ast.IndexType,
InvertedIndexInfo, FullTextIndexInfo and ConditionExprString) and the
Hash64/Equals methods (symbols: IndexInfo, Hash64, Equals, InvertedIndexInfo,
FullTextIndexInfo, ConditionExprString). After resolving the conflict, update
imports to include the base package so base.Hasher is available for Hash64 and
ensure any renamed types (ast.CIStr/ast.IndexType) are imported or adjusted
consistently; run build to confirm the file compiles.
In `@tests/integrationtest/r/executor/show.result`:
- Around line 1144-1240: The file contains unresolved Git conflict markers
(<<<<<<< HEAD, =======, >>>>>>>) splitting tests for "show columns"/"show index"
(e.g., show columns from test_show_columns_db.t, show index from
test_show_index_db.t) and partial-index SHOW CREATE TABLE cases (e.g., show
create table t); remove the conflict markers and resolve by either merging both
test sections into a single cohesive test file (keeping the column/index
privilege tests and the partial-index/show create table tests) or by selecting
the intended section (prefer the partial-index SHOW CREATE TABLE block if this
PR is about partial index handling), then ensure the final file has no conflict
markers and the SQL statements (create table, show create table, show columns,
show index, set names) remain in the correct order and syntactically valid.
In `@tests/integrationtest/r/explain_easy.result`:
- Around line 200-270: The file contains unresolved git conflict markers
(<<<<<<< HEAD, =======, >>>>>>> 4865e394b32) and two competing blocks; remove
the conflict markers and the older HEAD block (the version without the PREDICATE
column) and keep the version that includes the PREDICATE column/header and the
plan_tree outputs; ensure only the final resolved content remains (no conflict
markers or duplicate blocks) so tests compare against the expected output with
PREDICATE present.
In `@tests/integrationtest/t/executor/show.test`:
- Around line 437-524: The file contains unresolved Git conflict markers
(<<<<<<< HEAD, =======, >>>>>>> 4865e394b32) splitting two test blocks; remove
the conflict markers and merge the content so both test sections remain: retain
the TestShowColumns and TestShowIndex blocks from HEAD and the
TestShowCreateTableWithPartialIndex block from the incoming branch, ensuring the
SQL is contiguous and syntactically valid (no leftover markers).
---
Nitpick comments:
In `@pkg/ddl/integration_test.go`:
- Around line 134-151: Extract the nested ALTER TABLE validation into a helper
to mirror the CREATE TABLE test pattern: create a function (e.g.,
testPartialIndexValidation or validateAddIndexPartial) that accepts the testkit
instance and the two data structures differentTypeLiterals and
differentColumnTypes and performs the inner loops that create table t with a
given columnType, run the ALTER TABLE ... add index idx_b(b) where a =
<literal>, assert success with tk.MustExec when i == j and assert the specific
error with tk.MustGetDBError(..., dbterror.ErrUnsupportedAddPartialIndex)
otherwise, and drop the table; replace the five-level nested loops in the test
with calls to this helper to improve readability while keeping the same
assertions (reference symbols: differentTypeLiterals, differentColumnTypes,
tk.MustExec, tk.MustGetDBError, dbterror.ErrUnsupportedAddPartialIndex).
- Around line 106-110: The nested loops over differentColumnTypes and
differentTypeLiterals in the test create high cyclomatic complexity; extract the
innermost test logic currently called via checkColumnTypes(columnTypes,
literals, i == j) into a helper function (e.g., new helper
runColumnTypeCheck(columnTypes, literals, expectMatch)) and replace the inner
body to call that helper from the loops, moving CREATE TABLE setup/assertions
into the helper to flatten nesting and improve readability while keeping the
existing differentColumnTypes, differentTypeLiterals, and checkColumnTypes
semantics.
🪄 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: 2c3c5a06-3ded-417e-b53c-9adce06d0dc0
📒 Files selected for processing (17)
pkg/ddl/create_table.gopkg/ddl/executor.gopkg/ddl/index.gopkg/ddl/integration_test.gopkg/executor/infoschema_reader.gopkg/executor/show.gopkg/infoschema/tables.gopkg/meta/model/index.gopkg/meta/model/job_args.gopkg/util/dbterror/ddl_terror.gotests/integrationtest/r/executor/infoschema_reader.resulttests/integrationtest/r/executor/show.resulttests/integrationtest/r/explain_easy.resulttests/integrationtest/t/executor/infoschema_reader.testtests/integrationtest/t/executor/show.testtests/realtikvtest/addindextest4/BUILD.bazeltests/realtikvtest/addindextest4/partial_index_test.go
Comment on lines
+425
to
+434
| <<<<<<< HEAD | ||
| setGlobalIndexVersion(tblInfo, idxInfo) | ||
| ======= | ||
|
|
||
| conditionString, err := CheckAndBuildIndexConditionString(tblInfo, indexOption.Condition) | ||
| if err != nil { | ||
| return nil, errors.Trace(err) | ||
| } | ||
| idxInfo.ConditionExprString = conditionString | ||
| >>>>>>> 4865e394b32 (ddl,parser: modify schema to store the partial condition (#62759)) |
There was a problem hiding this comment.
Reject partial predicates on ADD PRIMARY KEY in this path too.
checkAndBuildIndexInfo reaches BuildIndexInfo with isPK=true, so this hunk currently allows ALTER TABLE ... ADD PRIMARY KEY ... WHERE ... to persist a partial primary key and bypass the guard added in create_table.go.
Suggested guard after resolving the hunk
idxInfo.Global = indexOption.Global
+ setGlobalIndexVersion(tblInfo, idxInfo)
conditionString, err := CheckAndBuildIndexConditionString(tblInfo, indexOption.Condition)
if err != nil {
return nil, errors.Trace(err)
}
+ if isPrimary && len(conditionString) > 0 {
+ return nil, dbterror.ErrUnsupportedAddPartialIndex.GenWithStackByArgs("creating a primary key with partial index is not supported")
+ }
idxInfo.ConditionExprString = conditionString🤖 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/ddl/index.go` around lines 425 - 434, checkAndBuildIndexInfo currently
calls CheckAndBuildIndexConditionString and sets idxInfo.ConditionExprString
even when isPK==true, allowing a partial predicate on ADD PRIMARY KEY; modify
checkAndBuildIndexInfo (the path that calls BuildIndexInfo with isPK) to detect
isPK and reject any indexOption.Condition (or non-empty conditionString) by
returning an error instead of calling CheckAndBuildIndexConditionString or
assigning idxInfo.ConditionExprString; ensure the guard triggers before setting
idxInfo.ConditionExprString so partial primary keys cannot be persisted.
Comment on lines
1360
to
1392
| <<<<<<< HEAD | ||
| ======= | ||
|
|
||
| // Only used for job args v2. | ||
| SplitOpt *IndexArgSplitOpt `json:"split_opt,omitempty"` | ||
|
|
||
| // ConditionString is used to store the partial index condition string for the index. | ||
| ConditionString string `json:"partial_condition_str,omitempty"` | ||
| } | ||
|
|
||
| // GetColumnarIndexType gets the real columnar index type in a backward compatibility way. | ||
| func (a *IndexArg) GetColumnarIndexType() ColumnarIndexType { | ||
| // For compatibility, if columnar index type is not set, and it's a columnar index, it's a vector index. | ||
|
|
||
| // If the columnar index type is NA and it's not a columnar index, it's a general index. | ||
| if a.ColumnarIndexType == ColumnarIndexTypeNA && !a.IsColumnar { | ||
| return ColumnarIndexTypeNA | ||
| } | ||
| // If the columnar index type is NA and it's a columnar index, it's a vector index. | ||
| if a.ColumnarIndexType == ColumnarIndexTypeNA && a.IsColumnar { | ||
| return ColumnarIndexTypeVector | ||
| } | ||
| return a.ColumnarIndexType | ||
| } | ||
|
|
||
| // IndexArgSplitOpt is a field of IndexArg used by index presplit. | ||
| type IndexArgSplitOpt struct { | ||
| Lower []string `json:"lower,omitempty"` | ||
| Upper []string `json:"upper,omitempty"` | ||
| Num int64 `json:"num,omitempty"` | ||
| ValueLists [][]string `json:"value_lists,omitempty"` | ||
| >>>>>>> 4865e394b32 (ddl,parser: modify schema to store the partial condition (#62759)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for merge conflict markers
echo "=== Checking for merge conflict markers ==="
rg -n '<<<<<<|======|>>>>>>' pkg/meta/model/job_args.go
# Check if ColumnarIndexType and IsColumnar are defined in IndexArg
echo -e "\n=== Searching for ColumnarIndexType field definition ==="
rg -n 'ColumnarIndexType\s+' pkg/meta/model/job_args.go
echo -e "\n=== Searching for IsColumnar field definition ==="
rg -n 'IsColumnar\s+' pkg/meta/model/job_args.go
# Check the full IndexArg struct definition
echo -e "\n=== IndexArg struct definition ==="
ast-grep --pattern $'type IndexArg struct {
$$$
}'Repository: pingcap/tidb
Length of output: 3043
Fix merge conflict markers and missing fields referenced by GetColumnarIndexType in pkg/meta/model/job_args.go.
pkg/meta/model/job_args.gostill contains unresolved merge conflict markers insideIndexArg(lines 1360-1361 and 1391), which will break compilation.(*IndexArg).GetColumnarIndexType()referencesa.ColumnarIndexTypeanda.IsColumnar, but those fields are not declared onIndexArg(onlyIsVectorand other fields are present), so the code won’t compile—this looks like an incomplete cherry-pick.
🤖 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/meta/model/job_args.go` around lines 1360 - 1392, Remove the leftover git
conflict markers and resolve the IndexArg struct: add the missing fields
referenced by GetColumnarIndexType (declare ColumnarIndexType ColumnarIndexType
and IsColumnar bool) and also add the new fields SplitOpt *IndexArgSplitOpt and
ConditionString string; ensure IndexArgSplitOpt type is present
(Lower/Upper/Num/ValueLists) and then keep GetColumnarIndexType as-is so it can
read a.ColumnarIndexType and a.IsColumnar without compile errors.
ti-chi-bot
Bot
added
cherry-pick-approved
close pingcap#62758 (cherry picked from commit e90bfe7)
YangKeao
force-pushed
the
cherry-pick-62759-to-release-8.5
branch
from
f4880bd to
d8da466
Compare
Member
|
/unhold |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/ddl/index.go (1)
424-428:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject partial predicates on
ADD PRIMARY KEYin both paths.
pkg/ddl/create_table.go:1381-1386already rejects partial primary keys, but this file still letsALTER TABLE ... ADD PRIMARY KEY ... WHERE ...persist one: Lines 424-428 keep the predicate when the AST is present, and Lines 1030-1033 restorearg.ConditionStringduring owner replay afterIndexOption.Conditionhas been stripped from job args. That leaves the alter path with metadata the create path explicitly forbids.Suggested guard
conditionString, err := CheckAndBuildIndexConditionString(tblInfo, indexOption.Condition) if err != nil { return nil, errors.Trace(err) } + if isPrimary && len(conditionString) > 0 { + return nil, dbterror.ErrUnsupportedAddPartialIndex.GenWithStackByArgs("creating a primary key with partial index is not supported") + } idxInfo.ConditionExprString = conditionString- if len(arg.ConditionString) > 0 { + if isPK && len(arg.ConditionString) > 0 { + job.State = model.JobStateCancelled + return ver, dbterror.ErrUnsupportedAddPartialIndex.GenWithStackByArgs("creating a primary key with partial index is not supported") + } + if len(arg.ConditionString) > 0 { indexInfo.ConditionExprString = arg.ConditionString }Also applies to: 1030-1033
🤖 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/ddl/index.go` around lines 424 - 428, The code currently allows a partial index predicate to be attached to a PRIMARY KEY via CheckAndBuildIndexConditionString and during owner replay; modify both places to reject or strip predicates when the index is a primary key: in the block that calls CheckAndBuildIndexConditionString (where idxInfo or indexOption is available) add a guard that if the index isPrimary (e.g., idxInfo.Primary or indexOption.Primary as applicable) then return an error (or clear the condition) instead of setting idxInfo.ConditionExprString; likewise in the owner-replay path where arg.ConditionString is restored (the code that copies arg.ConditionString back into IndexInfo), do not restore or persist a ConditionString when IndexInfo.Primary is true—clear it or return an error so partial predicates cannot be attached to a primary key during replay.
🤖 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/ddl/index.go`:
- Around line 3808-3814: The partial-index predicate check in
checkIndexCondition (pkg/ddl/index.go) wrongly assumes the constant side is an
ast.ValueExpr; modify the logic to detect and unwrap an ast.UnaryOperationExpr
whose Operator is '+' or '-' and whose Operand is an ast.ValueExpr before doing
the type assertion, so signed numeric literals (e.g., -1, +1) are treated as
ValueExprs; update the predicate validation to use the unwrapped value for type
checks and error messages. Also add a regression test in
tests/realtikvtest/addindextest4/partial_index_test.go that creates partial
indexes with predicates like "WHERE col1 > -100" and "WHERE col1 <= +1" to
ensure the unwrapping works.
---
Duplicate comments:
In `@pkg/ddl/index.go`:
- Around line 424-428: The code currently allows a partial index predicate to be
attached to a PRIMARY KEY via CheckAndBuildIndexConditionString and during owner
replay; modify both places to reject or strip predicates when the index is a
primary key: in the block that calls CheckAndBuildIndexConditionString (where
idxInfo or indexOption is available) add a guard that if the index isPrimary
(e.g., idxInfo.Primary or indexOption.Primary as applicable) then return an
error (or clear the condition) instead of setting idxInfo.ConditionExprString;
likewise in the owner-replay path where arg.ConditionString is restored (the
code that copies arg.ConditionString back into IndexInfo), do not restore or
persist a ConditionString when IndexInfo.Primary is true—clear it or return an
error so partial predicates cannot be attached to a primary key during replay.
🪄 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: 84fd872f-d3e2-4c5c-8f8b-b7b59d428a31
📒 Files selected for processing (18)
pkg/ddl/create_table.gopkg/ddl/executor.gopkg/ddl/index.gopkg/ddl/integration_test.gopkg/executor/infoschema_reader.gopkg/executor/show.gopkg/infoschema/tables.gopkg/meta/model/index.gopkg/meta/model/job_args.gopkg/util/dbterror/ddl_terror.gotests/integrationtest/r/executor/infoschema_reader.resulttests/integrationtest/r/executor/show.resulttests/integrationtest/r/explain_easy.resulttests/integrationtest/t/executor/infoschema_reader.testtests/integrationtest/t/executor/show.testtests/realtikvtest/addindextest4/BUILD.bazeltests/realtikvtest/addindextest4/integration_test.gotests/realtikvtest/addindextest4/partial_index_test.go
✅ Files skipped from review due to trivial changes (2)
- tests/integrationtest/r/explain_easy.result
- tests/integrationtest/r/executor/infoschema_reader.result
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/util/dbterror/ddl_terror.go
- tests/integrationtest/t/executor/infoschema_reader.test
- pkg/infoschema/tables.go
- pkg/ddl/create_table.go
- pkg/ddl/integration_test.go
- pkg/executor/infoschema_reader.go
Comment on lines
+3808
to
+3814
| // The another side must be a literal value, and it must have the same type as the column. | ||
| constantExpr, ok := anotherSide.(ast.ValueExpr) | ||
| if !ok { | ||
| return dbterror.ErrUnsupportedAddPartialIndex.GenWithStackByArgs( | ||
| "partial index condition must include a literal value on the other side of the binary operation") | ||
| } | ||
| // Reference `types.DefaultTypeForValue`, they are all possible types for literal values. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact code region
sed -n '3780,3845p' pkg/ddl/index.go | cat -n
# Find all occurrences of the specific error message to locate full predicate parsing logic
rg -n "partial index condition must include a literal value" -S pkg/ddl/index.go
# Search nearby for any unary +/- unwrapping or similar helpers
rg -n "UnaryOperationExpr|unwrap|signed|DefaultTypeForValue" -S pkg/ddl/index.go
# Broader search: where partial index conditions are parsed/validated with literal requirements
rg -n "partial index condition" -S pkg/ddl pkg/parser pkg/executor pkg/planner
# Look for partial-index tests involving negative literals
rg -n "partial index" -S . | head -n 50
rg -n "<\\s*-1|<=\\s*\\+?1|>\\s*-1|>=\\s*\\+?1|\\(-1\\)|\\+1" -S . | head -n 50Repository: pingcap/tidb
Length of output: 16114
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '3680,3925p' pkg/ddl/index.go | cat -n
# Narrow search for unary operation handling within ddl/index.go
rg -n "UnaryOperationExpr" pkg/ddl/index.go
rg -n "ValueExpr" pkg/ddl/index.go | head -n 50
rg -n "partial index condition must include a literal value on the other side of the binary operation" -S .
# Look for partial index tests and any usage with negative literals
rg -n "add partial index|partial index" tests -S | head -n 200
# Specifically check for negative numeric literals in partial index related tests/DDL statements
rg -n "partial index[^\\n]*-[0-9]|-[0-9][^\\n]*partial index" -S tests | head -n 200
# Search for helper unwrapping/evaluating constants/unary ops near partial index logic
rg -n "unwrap.*unary|Unwrap.*Unary|Signed|Unary|constant.*fold|Eval|GetExpr.*Value|AsValueExpr|ValueExpr\\(" -S pkg/ddl/index.go pkg/ddl | head -n 200Repository: pingcap/tidb
Length of output: 11410
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '3680,3925p' pkg/ddl/index.go | cat -n
rg -n "UnaryOperationExpr" pkg/ddl/index.go
rg -n "ValueExpr" pkg/ddl/index.go | head -n 50
rg -n "partial index condition must include a literal value on the other side of the binary operation" -S .
rg -n "add partial index|partial index" tests -S | head -n 200
rg -n "partial index[^\n]*-[0-9]|-[0-9][^\n]*partial index" -S tests | head -n 200
rg -n "unwrap.*unary|Unwrap.*Unary|Signed|Unary|constant.*fold|Eval|AsValueExpr|ValueExpr\\(" -S pkg/ddl/index.go pkg/ddl | head -n 200Repository: pingcap/tidb
Length of output: 11410
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Look for any partial-index DDL tests/fixtures (exact phrase + variants)
rg -n "partial index|add partial index|ADD PARTIAL INDEX|partial_index" -S . | head -n 200
# 2) Look for parser/AST tests involving negative numeric literals and unary expressions
rg -n "UnaryOperationExpr" -S pkg/parser | head -n 200
rg -n "col\\s*[<>!=]=?\\s*-1|\\b-1\\b.*(parse|ast|expr)" -S pkg/parser pkg/executor pkg/planner | head -n 200
rg -n "(-[0-9]+)" -S pkg/parser/test | head -n 200 || true
# 3) If parser exposes node types in tests, search for "GetInt64" or ValueExpr construction around negative literals
rg -n "ValueExpr|NewValueExpr|driver\\.ValueExpr|ast\\.ValueExpr" -S pkg/parser | head -n 200Repository: pingcap/tidb
Length of output: 50442
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find callers of CheckAndBuildIndexConditionString / checkIndexCondition
rg -n "CheckAndBuildIndexConditionString\\(" -S pkg/ddl | head -n 50
rg -n "checkIndexCondition\\(" -S pkg/ddl | head -n 50
# 2) Inspect the referenced partial index test file if it exists
ls -la tests/realtikvtest/addindextest4 || true
if [ -f tests/realtikvtest/addindextest4/partial_index_test.go ]; then
sed -n '1,200p' tests/realtikvtest/addindextest4/partial_index_test.go | cat -n
rg -n "partial index|add partial index|WHERE" tests/realtikvtest/addindextest4/partial_index_test.go
rg -n "-[0-9]+|\\+\\s*[0-9]+" tests/realtikvtest/addindextest4/partial_index_test.go || true
fi
# 3) Inspect surrounding code around the call site in pkg/ddl/index.go
# (Use the line of checkIndexCondition definition we already saw.)
sed -n '3600,3725p' pkg/ddl/index.go | cat -nRepository: pingcap/tidb
Length of output: 6755
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for negative/positive literals in the partial index test file
rg -n -- "-[0-9]+|\\+\\s*[0-9]+" tests/realtikvtest/addindextest4/partial_index_test.go || true
rg -n -- "where .*-[0-9]+|where .*\\+\\s*[0-9]+" tests/realtikvtest/addindextest4/partial_index_test.go || true
# Inspect surrounding code in pkg/ddl/index.go to see how indexOption.Condition is parsed/constructed
sed -n '390,520p' pkg/ddl/index.go | cat -n
sed -n '420,470p' pkg/ddl/index.go | cat -n
# Also inspect executor side call site if it does any expression preprocessing
sed -n '4920,5040p' pkg/ddl/executor.go | cat -nRepository: pingcap/tidb
Length of output: 13135
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check entire partial index test file for negative literals
rg -n -- "-[0-9]+|\\+\\s*[0-9]+" tests/realtikvtest/addindextest4/partial_index_test.go || true
rg -n -- "where .*-[0-9]+|where .*\\+\\s*[0-9]+" tests/realtikvtest/addindextest4/partial_index_test.go || true
# Find other uses of indexOption.Condition within pkg/ddl
rg -n "indexOption\\.Condition" -S pkg/ddl | head -n 200
# Search for constant folding / expression simplification helpers near partial index condition building
rg -n "Fold|ConstantFold|Simplif|Simplify|normalize|unwrap|UnaryOperationExpr" -S pkg/ddl/index.go pkg/ddl | head -n 200
# Parser-side: find existing tests that assert unary minus structure around ValueExpr
rg -n "opcode\\.Minus|opcode\\.Plus" -S pkg/parser/parser_test.go pkg/parser/ast -S | head -n 200
rg -n -- "\"-1\"|\\b-1\\b" -S pkg/parser/parser_test.go | head -n 200Repository: pingcap/tidb
Length of output: 5921
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check entire partial index test file for negative literals
rg -n -- "-[0-9]+|\\+\\s*[0-9]+" tests/realtikvtest/addindextest4/partial_index_test.go || true
rg -n -- "where .*-[0-9]+|where .*\\+\\s*[0-9]+" tests/realtikvtest/addindextest4/partial_index_test.go || true
# Find other uses of indexOption.Condition within pkg/ddl
rg -n "indexOption\\.Condition" -S pkg/ddl | head -n 200
# Search for constant folding / expression simplification helpers near partial index condition building
rg -n "Fold|ConstantFold|Simplif|Simplify|normalize|unwrap|UnaryOperationExpr" -S pkg/ddl/index.go pkg/ddl | head -n 200
# Parser-side: find existing tests that assert unary +/- structure around ValueExpr
rg -n "opcode\\.Minus|opcode\\.Plus" -S pkg/parser/parser_test.go pkg/parser/ast -S | head -n 200
rg -n -- "\"-1\"|\\b-1\\b" -S pkg/parser/parser_test.go | head -n 200Repository: pingcap/tidb
Length of output: 5921
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Negative literal usage in partial index test
rg -n -- "-[0-9]+|\\+\\s*[0-9]+" tests/realtikvtest/addindextest4/partial_index_test.go || true
# Show any '-' occurrences near WHERE clauses (broader)
rg -n -- "where .*-[0-9]+|where .*\\+\\s*[0-9]+" tests/realtikvtest/addindextest4/partial_index_test.go || true
# Inspect parser grammar around unary +/- rules
sed -n '4130,4205p' pkg/parser/parser.y | cat -n
# Parser tests for -1 or unary operation parsing
rg -n -- "\\b-1\\b" pkg/parser/parser_test.go | head -n 50
rg -n -- "UnaryOperationExpr" pkg/parser/parser_test.go | head -n 50
rg -n -- "opcode\\.Minus|opcode\\.Plus" pkg/parser/parser_test.go | head -n 50
# Search for expr simplification/constant folding in ddl before checkIndexCondition
rg -n "Simplif|ConstantFold|Fold|Reduce|Unwrap|normalize|Eval" -S pkg/ddl/index.go pkg/ddl | head -n 200Repository: pingcap/tidb
Length of output: 19073
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Negative literal usage in partial index test
rg -n -- "-[0-9]+|\\+\\s*[0-9]+" tests/realtikvtest/addindextest4/partial_index_test.go || true
# Show any '-' occurrences near WHERE clauses (broader)
rg -n -- "where .*-[0-9]+|where .*\\+\\s*[0-9]+" tests/realtikvtest/addindextest4/partial_index_test.go || true
# Inspect parser grammar around unary +/- rules
sed -n '4130,4205p' pkg/parser/parser.y | cat -n
# Parser tests for -1 or unary operation parsing
rg -n -- "\\b-1\\b" pkg/parser/parser_test.go | head -n 50
rg -n -- "UnaryOperationExpr" pkg/parser/parser_test.go | head -n 50
rg -n -- "opcode\\.Minus|opcode\\.Plus" pkg/parser/parser_test.go | head -n 50
# Search for expr simplification/constant folding in ddl before checkIndexCondition
rg -n -- "Simplif|ConstantFold|Fold|Reduce|Unwrap|normalize|Eval" pkg/ddl/index.go pkg/ddl | head -n 200Repository: pingcap/tidb
Length of output: 19073
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search within partial index test file for any negative literals or unary +/- patterns
rg -n -- "-[0-9]+|\\+\\s*[0-9]+" tests/realtikvtest/addindextest4/partial_index_test.go || true
# Broader search for negative literals in partial index conditions
rg -n -- "partial index|where .*-[0-9]+|where .*\\+\\s*[0-9]+" tests -S | head -n 100Repository: pingcap/tidb
Length of output: 13361
Unwrap signed numeric literals for partial index predicate validation
pkg/ddl/index.go’s checkIndexCondition only accepts the “constant” side of col <op> const as ast.ValueExpr (anotherSide.(ast.ValueExpr)), but the parser represents signed numerics as *ast.UnaryOperationExpr wrapping a ValueExpr (SignedLiteral: '-' NumLiteral / '+' NumLiteral in pkg/parser/parser.y). This rejects predicates like col1 > -1 / col1 <= +1 when defining a partial index. Unwrap unary +/- to the inner ValueExpr before the type assertion, and add a regression test in tests/realtikvtest/addindextest4/partial_index_test.go (e.g. where col1 > -100 and/or where col1 <= +1).
🤖 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/ddl/index.go` around lines 3808 - 3814, The partial-index predicate check
in checkIndexCondition (pkg/ddl/index.go) wrongly assumes the constant side is
an ast.ValueExpr; modify the logic to detect and unwrap an
ast.UnaryOperationExpr whose Operator is '+' or '-' and whose Operand is an
ast.ValueExpr before doing the type assertion, so signed numeric literals (e.g.,
-1, +1) are treated as ValueExprs; update the predicate validation to use the
unwrapped value for type checks and error messages. Also add a regression test
in tests/realtikvtest/addindextest4/partial_index_test.go that creates partial
indexes with predicates like "WHERE col1 > -100" and "WHERE col1 <= +1" to
ensure the unwrapping works.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #68831 +/- ##
================================================
Coverage ? 55.7968%
================================================
Files ? 1826
Lines ? 658987
Branches ? 0
================================================
Hits ? 367694
Misses ? 264149
Partials ? 27144
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Member
|
/retest |
wjhuang2016
approved these changes
Jun 4, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wjhuang2016, 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 |
ti-chi-bot
Bot
added
approved
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
Member
|
/retest |
Member
|
/retest |
3 similar comments
Member
|
/retest |
Member
|
/retest |
Member
|
/retest |
This was referenced Jun 5, 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
3 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 #62759
What problem does this PR solve?
Issue Number: close #62758
#63448
-> #62759
#62762
What changed and how does it work?
IndexOption.predicateto thetidb_indexestable to show the index condition.ShowExecto show the index condition.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
Information Schema
Show/Create
Validation
Tests