ddl,tables: only write the index when it meets partial index condition (#62762)#68833
Open
ti-chi-bot wants to merge 1 commit into
Open
ddl,tables: only write the index when it meets partial index condition (#62762)#68833ti-chi-bot wants to merge 1 commit into
ti-chi-bot wants to merge 1 commit into
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThis PR implements comprehensive support for partial indexes (indexes with WHERE conditions) in TiDB. It adds metadata models for condition expressions, per-row condition evaluation during DML/DDL, integrates conditions into the backfill pipeline, enforces validation constraints (fast-reorg requirement, column modification restrictions, no partitioned tables), and updates admin check table logic to handle partial index conditions. The implementation includes extensive test coverage for creation, DML operations, DDL constraints, and verification. ChangesPartial Index Support Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
|
@ti-chi-bot: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/executor/builder.go (1)
510-545:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winResolve merge conflict markers and fix FastCheckTable session-var access in
buildCheckTable
pkg/executor/builder.go(buildCheckTable, ~lines 510-545) still contains<<<<<<< HEAD/=======/>>>>>>> ...conflict markers → hard compile failure.- The same hunk calls
b.ctx.GetSessionVars().FastCheckTable, butexecutorBuilder.ctxis acontext.Context; session vars are onb.sctx(sessionctx.Context). Replace withb.sctx.GetSessionVars().FastCheckTable.🤖 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/executor/builder.go` around lines 510 - 545, The diff contains unresolved merge conflict markers and an incorrect session-var access in buildCheckTable: remove the conflict markers and unify the logic (prefer a single boolean name, e.g., canUseFastCheck) so the loop over v.IndexInfos tests idx.MVIndex or idx.IsColumnarIndex() and column Length != types.UnspecifiedLength to set canUseFastCheck = false and break appropriately; also replace b.ctx.GetSessionVars().FastCheckTable with b.sctx.GetSessionVars().FastCheckTable so the code uses the sessionctx.Context field.pkg/meta/model/index.go (1)
109-171:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winResolve the
IndexInfomerge conflict before merging.This block still has conflict markers and duplicate struct content, which makes the package invalid Go and blocks compilation.
🤖 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 109 - 171, The file contains leftover Git conflict markers and duplicated IndexInfo definitions; remove the conflict markers and keep a single, consistent IndexInfo struct (prefer the newer definition with ast.CIStr, Tp as ast.IndexType, and added fields InvertedInfo/FullTextInfo/ConditionExprString/AffectColumn), then keep the Hash64 and Equals methods below the struct (ensure they reference IndexInfo.ID). Also update imports/types as needed (replace model.CIStr/model.IndexType with ast.CIStr/ast.IndexType and ensure base.Hasher is imported) so the package compiles.
🟠 Major comments (3)
pkg/ddl/index_presplit.go-186-200 (1)
186-200:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle partitioned global indexes the same way in the bound-split path.
getSplitIdxKeysFromBoundcurrently fans out by partition ID for every partitioned table, but global index keys live undertblInfo.ID. That meansSPLIT ... BETWEEN ...will generate boundaries in the wrong keyspace for partitioned global indexes and miss the actual global-index range.Suggested fix
func getSplitIdxKeysFromBound( sctx sessionctx.Context, tblInfo *model.TableInfo, idxInfo *model.IndexInfo, lower, upper []types.Datum, splitNum int, ) (keys [][]byte, err error) { pi := tblInfo.GetPartitionInfo() - if pi == nil { + if pi == nil || idxInfo.Global { keys = make([][]byte, 0, splitNum) return getSplitIdxPhysicalKeysFromBound( sctx, tblInfo, idxInfo, tblInfo.ID, lower, upper, splitNum, keys) } keys = make([][]byte, 0, splitNum*len(pi.Definitions)) for _, p := range pi.Definitions { keys, err = getSplitIdxPhysicalKeysFromBound( sctx, tblInfo, idxInfo, p.ID, lower, upper, splitNum, keys) if err != nil { return nil, err } } return keys, nil }🤖 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_presplit.go` around lines 186 - 200, The current fan-out over pi.Definitions uses each partition ID for getSplitIdxPhysicalKeysFromBound which breaks global indexes (their keys live under tblInfo.ID); update the branch in the partition-handling block (where pi := tblInfo.GetPartitionInfo() and the loop over pi.Definitions) to check the index type (e.g., idxInfo.IsGlobal or idxInfo.Global) and if the index is global, call getSplitIdxPhysicalKeysFromBound once with tblInfo.ID (not p.ID) so global-index boundaries are generated in the correct keyspace; otherwise keep the per-partition calls as-is.pkg/ddl/index_cop.go-85-92 (1)
85-92:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPotential nil pointer dereference:
kvReqfields accessed before error check.If
Build()returns an error,kvReqmay be nil, and accessingkvReq.RequestSourceon lines 87-89 will panic.🐛 Proposed fix: move error check before accessing kvReq
kvReq, err := builder. Build() + if err != nil { + return nil, conditionPushed, err + } kvReq.RequestSource.RequestSourceInternal = true kvReq.RequestSource.RequestSourceType = getDDLRequestSource(model.ActionAddIndex) kvReq.RequestSource.ExplicitRequestSourceType = kvutil.ExplicitTypeDDL - if err != nil { - return nil, conditionPushed, err - }🤖 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_cop.go` around lines 85 - 92, Move the error check for builder.Build() before any use of kvReq to avoid nil dereference: call kvReq, err := builder.Build(), then immediately if err != nil { return nil, conditionPushed, err } before touching kvReq.RequestSource; after the nil-check set kvReq.RequestSource.RequestSourceInternal, kvReq.RequestSource.RequestSourceType = getDDLRequestSource(model.ActionAddIndex) and kvReq.RequestSource.ExplicitRequestSourceType = kvutil.ExplicitTypeDDL. Ensure you reference the same local symbols (kvReq, builder.Build(), getDDLRequestSource, model.ActionAddIndex, kvutil.ExplicitTypeDDL, conditionPushed) when moving the error handling.pkg/table/tables/index_test.go-703-717 (1)
703-717:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlways disable the failpoint on failure paths.
If
tc.ddlor any assertion in the callback fails,testfailpoint.Disable(...)is skipped and the enabled failpoint can leak into later iterations/tests. Please isolate each case in a helper/subtest anddeferthe disable immediately afterEnableCall.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/table/tables/index_test.go` around lines 703 - 717, The failpoint enabled via testfailpoint.EnableCall("github.com/pingcap/tidb/pkg/ddl/afterWaitSchemaSynced", ...) may not be disabled if tc.ddl or assertions in the callback fail; to fix, wrap each case in a subtest/helper (t.Run) or immediately defer the disable right after calling testfailpoint.EnableCall so the cleanup always runs: call testfailpoint.EnableCall(...), then immediately defer testfailpoint.Disable(t, "github.com/pingcap/tidb/pkg/ddl/afterWaitSchemaSynced") before executing tk.MustExec(tc.ddl) and the assertions inside the callback to guarantee the failpoint is disabled on all exit paths.
🟡 Minor comments (2)
pkg/executor/test/admintest/admin_test.go-242-242 (1)
242-242:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDuplicate assertion.
Same as above - this
require.NoError(t, err)duplicates line 241.Proposed fix
indexOpr, err := tables.NewIndex(tblInfo.ID, tblInfo, cpIdx) require.NoError(t, err) - require.NoError(t, err) txn, err := store.Begin()🤖 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/executor/test/admintest/admin_test.go` at line 242, The test contains a duplicate assertion calling require.NoError(t, err) twice; remove the redundant duplicate so only a single require.NoError(t, err) remains for the same error check. Locate the duplicate use of require.NoError in the admin test (search for require.NoError and the variable err in the failing test function) and delete the extra line, leaving the original assertion intact.pkg/executor/test/admintest/admin_test.go-99-99 (1)
99-99:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDuplicate assertion.
This
require.NoError(t, err)is redundant since the same check is already performed on line 98.Proposed fix
indexOpr, err := tables.NewIndex(tblInfo.ID, tblInfo, idxInfo) require.NoError(t, err) - require.NoError(t, err) txn, err := store.Begin()🤖 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/executor/test/admintest/admin_test.go` at line 99, Remove the duplicate require.NoError(t, err) in the test in pkg/executor/test/admintest/admin_test.go: keep the existing NoError check that appears earlier and delete the second, redundant require.NoError call so the test asserts the error only once (then run tests to confirm).
🧹 Nitpick comments (1)
br/tests/br_partial_index/run.sh (1)
40-40: 💤 Low valueQuote shell variables to prevent word splitting.
The static analysis hints are valid. Variables
$PD_ADDRand$DBshould be double-quoted to prevent word splitting and globbing issues.Suggested fix
-run_br --pd $PD_ADDR backup db -s "local://$TEST_DIR/$DB" --db $DB +run_br --pd "$PD_ADDR" backup db -s "local://$TEST_DIR/$DB" --db "$DB"-run_br restore db --db $DB -s "local://$TEST_DIR/$DB" --pd $PD_ADDR +run_br restore db --db "$DB" -s "local://$TEST_DIR/$DB" --pd "$PD_ADDR"Also applies to: 47-47
🤖 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 `@br/tests/br_partial_index/run.sh` at line 40, The run_br invocation in run.sh uses unquoted shell variables which can cause word-splitting/globbing; update the command(s) (the `run_br --pd $PD_ADDR backup db -s "local://$TEST_DIR/$DB" --db $DB` lines) to double-quote the variables (e.g., "$PD_ADDR" and "$DB") in both occurrences (lines shown and the second occurrence) so all references to PD_ADDR and DB are quoted consistently.
🤖 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 `@br/tests/run_group_br_tests.sh`:
- Around line 30-36: Remove the unresolved Git conflict markers and produce a
single coherent assignment for the associative array keys ["G07"] and ["G08"]:
delete the lines containing <<<<<<< HEAD, ======= and >>>>>>> and merge the two
conflicting values so the final assignments include the unioned test names
(ensure ["G07"] contains the intended entries and ["G08"] contains the intended
entries including br_partial_index or br_pitr_table_filter as required by the
branch change); update the array entries for ["G07"] and ["G08"] so they are
valid shell string assignments with no conflict markers remaining.
In `@pkg/ddl/backfilling_operators.go`:
- Around line 283-292: The file contains unresolved git conflict markers; remove
all conflict markers and merge the two variants so the cherry-picked
partial-index changes are preserved: add the new fields (ctx, tableScanRowCount,
conditionPushed) to the IndexRecordChunk struct, keep or add the newDistSQLCtx
helper, add the indexConditionCheckers field and the initIndexConditionCheckers
method, and incorporate the conditional checker logic into WriteChunk; also
reconcile the alternate implementations in scanRecords,
WriteExternalStoreOperator/IndexIngestOperator worker factory code paths,
HandleTask, and Close/init flows so there are no leftover <<<<>>>/==== markers
and the code compiles with the partial-index behavior intact (use function/type
names IndexRecordChunk, scanRecords, WriteExternalStoreOperator,
IndexIngestOperator, HandleTask, Close, initIndexConditionCheckers, WriteChunk,
newDistSQLCtx, indexConditionCheckers to locate and merge the correct blocks).
In `@pkg/ddl/copr/copr_ctx.go`:
- Around line 397-423: GetSchemaAndNames currently declares colName as ast.CIStr
but the file doesn't import the ast package; fix the compile error by changing
the colName declaration to use model.CIStr (or alternatively add the missing
import for github.com/pingcap/tidb/pkg/parser/ast). Update the local variable
declaration in CopContextBase.GetSchemaAndNames (the colName variable used to
populate types.FieldName) so it references model.CIStr, ensuring consistency
with the project types and allowing the file to compile.
In `@pkg/ddl/executor.go`:
- Around line 5011-5023: Remove the leftover git conflict markers and choose the
intended code block: delete the "<<<<<<< HEAD" and ">>>>>>> ..." markers and
keep the added logic that declares conditionString, calls
CheckAndBuildIndexConditionString(tblInfo, indexOption.Condition), handles the
returned error, and checks len(conditionString) > 0 with
job.ReorgMeta.IsFastReorg to return dbterror.ErrUnsupportedAddPartialIndex when
appropriate; also remove any duplicate helper/function definitions introduced by
the conflict (ensure only one CheckAndBuildIndexConditionString implementation
remains) so the file builds cleanly.
In `@pkg/ddl/export_test.go`:
- Around line 55-59: Resolve the unresolved merge conflict in the
NewTableScanOperator call: remove the conflict markers (<<<<<<<, =======,
>>>>>>>) and pick the correct argument set for NewTableScanOperator (either the
existing wctx with nil metadata or the cherry‑picked opCtx plus
&model.DDLReorgMeta{} and &execute.TestCollector{}); if you choose the
cherry‑picked form, add/import model and execute.TestCollector and ensure opCtx
is declared/constructed in this test; if you keep the HEAD form, restore the
original call using wctx and nils and remove references to undefined opCtx and
execute.TestCollector so the file compiles.
In `@pkg/ddl/index_merge_tmp.go`:
- Around line 159-164: The file contains unresolved merge markers around the
constructor return—remove the conflict markers (<<<<<<<, =======, >>>>>>>) and
keep the intended return that initializes buffers: ensure the struct literal
includes buffers: newTempIdxBuffers(bfCtx.batchCnt) and the trailing ", nil"
return so the constructor/function (the initializer that references
newTempIdxBuffers and bfCtx.batchCnt) compiles cleanly; delete the other
duplicated fragment and markers so only the correct return path remains.
In `@pkg/ddl/index.go`:
- Around line 2585-2588: In writeChunk, when index.Meta().HasCondition() &&
indexConditionCheckers != nil triggers an error from
indexConditionCheckers[i](row), change the return to return the error with a nil
kv.Handle (not 0) to match other error paths; locate the block around
indexConditionCheckers and replace the incorrect "return 0, 0,
errors.Trace(err)" with a return that supplies 0 for the int, nil for the
kv.Handle, and the traced error so all signatures and other error paths remain
consistent.
- Around line 422-435: Resolve the leftover conflict markers in pkg/ddl/index.go
by merging both sides: keep the call to setGlobalIndexVersion(tblInfo, idxInfo)
and also wire up partial-index fields by invoking
CheckAndBuildIndexConditionString(tblInfo, indexOption.Condition) and assign
idxInfo.ConditionExprString and idxInfo.AffectColumn via buildAffectColumn; in
fetchRowColVals restore the global-index V1 behavior that wraps handles with
kv.NewPartitionHandle(...) and preserve the partial-index fast-reorg guard so
both behaviors coexist; and in writeChunk (which returns (int, kv.Handle,
error)) replace any literal numeric zero return for the kv.Handle (e.g., return
0, 0, errors.Trace(err)) with nil (return 0, nil, errors.Trace(err)) or the
correct kv.Handle value. Ensure you remove all conflict markers (<<<<<<<,
=======, >>>>>>>) and keep the referenced symbols: setGlobalIndexVersion,
CheckAndBuildIndexConditionString, buildAffectColumn,
idxInfo.ConditionExprString, idxInfo.AffectColumn, fetchRowColVals,
kv.NewPartitionHandle, writeChunk, and kv.Handle.
In `@pkg/ddl/integration_test.go`:
- Around line 56-144: The file contains unresolved Git conflict markers around
TestPartialIndex (remove the lines starting with <<<<<<<, =======, and >>>>>>>
and keep the intended TestPartialIndex implementation) and missing imports used
by that test; add imports for fmt and github.com/pingcap/tidb/pkg/util/dbterror
to the import block so fmt.Sprintf and dbterror.ErrUnsupportedAddPartialIndex
used in TestPartialIndex compile; ensure the cleaned-up TestPartialIndex
(function name TestPartialIndex) remains intact and all conflict markers are
removed.
In `@pkg/errno/errcode.go`:
- Around line 1167-1175: The file contains unresolved git conflict markers
(<<<<<<< HEAD / ======= / >>>>>>>) around the errno constant block; remove the
conflict markers and reconcile the two versions so the constants compile
cleanly—ensure ErrEngineAttributeInvalidFormat, ErrStorageClassInvalidSpec,
ErrModifyColumnReferencedByPartialCondition, and
ErrCheckPartialIndexWithoutFastCheck are present only once and any duplicate or
outdated entries from the HEAD side are removed; after editing, run `go build`
to verify there are no duplicate identifier or syntax errors.
In `@pkg/errno/errname.go`:
- Around line 1159-1167: The file contains unresolved merge conflict markers
(<<<<<<< HEAD, =======, >>>>>>>) which must be removed and the
duplicate/conflicting entries reconciled; locate the conflicting block around
the error definitions such as ErrQueryExecStopped,
ErrEngineAttributeInvalidFormat, ErrStorageClassInvalidSpec,
ErrModifyColumnReferencedByPartialCondition, and
ErrCheckPartialIndexWithoutFastCheck and decide which variant to keep, then
delete the conflict markers and any unwanted duplicate lines so the final code
compiles cleanly.
In `@pkg/executor/check_table_index.go`:
- Around line 25-30: There are unresolved git conflict markers in
check_table_index.go; remove all markers and restore the intended code from the
cherry-picked commit so the file compiles. Specifically: clean up the import
block around the symbols (ensure the final import list includes the packages
used by the file such as failpoint/operator/errno as required by the
cherry-picked change), resolve the helper function definitions in the block
around the previous lines 359-394 (keep the correct helper implementations from
the cherry-picked commit), fix the SQL query generation section around the
previous lines 511-521 so the intended query-building code is retained, and
restore the meetError handling block around the previous lines 586-606 to the
cherry-picked version; remove all <<<<<<<, =======, >>>>>>> markers and run `go
build` to verify no missing or duplicate symbols remain.
In `@pkg/executor/test/admintest/admin_test.go`:
- Around line 2171-2216: The file contains unresolved Git conflict markers
(<<<<<<< HEAD, =======, >>>>>>>) that break compilation; remove the conflict
markers and keep the intended new test function TestAdminCheckGeneratedColumns
(including its body that sets up store/domain, creates table t, corrupts an
index via tables.NewIndex/Delete/Create, and runs ADMIN CHECK TABLE) so the test
compiles and runs; ensure you only retain the finalized test code and not the
marker lines, run `go test` or the repo's test suite to confirm build success
and no duplicate definitions.
In `@pkg/meta/model/BUILD.bazel`:
- Around line 21-25: Remove the Git merge conflict markers from the deps array
in pkg/meta/model/BUILD.bazel: delete the lines containing <<<<<<< HEAD,
=======, and >>>>>>> ..., and ensure the deps list contains the intended entries
"//pkg/config/kerneltype" and "//pkg/parser" (inside the deps block) so the
BUILD file is valid and parsable.
In `@pkg/meta/model/job_args.go`:
- Around line 1360-1392: The file contains unresolved Git conflict markers
(<<<<<<<, =======, >>>>>>>) around the IndexArg additions; remove these markers
and properly merge the two sides so the final code includes the SplitOpt
*IndexArgSplitOpt and ConditionString fields in the IndexArg struct, retains the
GetColumnarIndexType method, and defines the IndexArgSplitOpt type (Lower,
Upper, Num, ValueLists) with correct braces and JSON tags; ensure there are no
leftover conflict markers and the resulting IndexArg, GetColumnarIndexType, and
IndexArgSplitOpt declarations compile cleanly.
In `@pkg/planner/core/logical_plan_builder.go`:
- Around line 5465-5481: Remove the merge-conflict markers and implement the
intended skipPruning flow: compute skipPruning := tblInfo.GetPartitionInfo() !=
nil || hasFK || nonPruned == nil, iterate tblInfo.Indices and if any
idx.ConditionExprString is non-empty set skipPruning = true and break, then if
skipPruning call buildSingleTableColPosInfoForDelete(tbl, cols2PosInfo) (instead
of the old call with prunedColCnt) otherwise proceed with the normal pruning
path; ensure you remove the <<<<<<<, =======, >>>>>>> markers and reference
tblInfo, hasFK, nonPruned, tbl, cols2PosInfo, tblInfo.Indices and
buildSingleTableColPosInfoForDelete correctly.
In `@pkg/server/tests/commontest/tidb_test.go`:
- Around line 2287-2297: There are unresolved Git conflict markers (<<<<<<<,
=======, >>>>>>>) inside the SQL test entries in tidb_test.go; remove the
conflict markers and keep the intended final SQL strings (the ones with the
AGG_TO_COP() hint and adjusted whitespace) so each test entry is a single valid
string literal (e.g., the entries containing "/*+
read_from_storage(tikv[`stmtstats`.`t`]), AGG_TO_COP() */ ..." and "/*+
AGG_TO_COP() */ ...") and ensure the slice of test cases no longer contains any
merge markers so the file compiles.
In `@pkg/table/tables/BUILD.bazel`:
- Around line 32-36: The file pkg/table/tables/BUILD.bazel contains unresolved
git conflict markers (e.g., <<<<<<< HEAD, =======, >>>>>>> 8c2781681a4) around
the dependency entries and must be cleaned up: open the BUILD.bazel, remove the
conflict markers and choose the correct dependency set (ensure the entries like
"//pkg/sessionctx/stmtctx" and "//pkg/sessionctx/vardef" are either kept or
removed according to intended change), make the file a single valid BUILD rule
without any marker lines, and run a quick build or bazel query to verify the
file is syntactically correct; repeat the same resolution for the second
conflict block corresponding to lines 84-88.
In `@pkg/util/dbterror/ddl_terror.go`:
- Around line 83-89: The file contains unresolved git merge conflict markers
around the DDL error declarations; remove the conflict markers (<<<<<<< HEAD,
=======, >>>>>>>) and keep the intended declarations for
ErrUnsupportedAddPartialIndex and ErrModifyColumnReferencedByPartialCondition so
they are valid Go code; ensure the two constants are declared using
ClassDDL.NewStdErr / ClassDDL.NewStd as in the diff and that imports
(parser_mysql, mysql) remain correct so the file compiles.
In `@tests/realtikvtest/addindextest3/operator_test.go`:
- Around line 424-430: Resolve the merge conflict markers and make the
TestTuneWorkerPoolSize code consistent by choosing the distributed task context
variant: replace the conflicted block with a single clean initialization using
ddl.NewDistTaskOperatorCtx to get (opCtx, cancel), call
ddl.NewTableScanOperator(opCtx, sessPool, copCtx, nil, 2, 0,
&model.DDLReorgMeta{}, nil, &execute.TestCollector{}), remove wctx and
wctx.Cancel() usages and instead call cancel() where the context should be
released, and ensure execute.TestCollector is imported/available.
- Around line 118-122: Resolve the git conflict in the TestBackfillOperators
call to NewTableScanOperator: replace the conflict markers and choose the
correct constructor signature used in this test file (use the existing context
variables like wctx, sessPool, copCtx, srcChkPool) and remove or properly
import/define any new identifiers from the cherry-pick (opCtx,
model.DDLReorgMeta, execute.TestCollector) if you intend to use the new
signature; specifically, either restore the original call
NewTableScanOperator(wctx, sessPool, copCtx, srcChkPool, 3, 0, nil, nil) or
update the file to declare/import opCtx, model.DDLReorgMeta{}, and
execute.TestCollector and pass them consistently to NewTableScanOperator so
there are no unresolved merge markers or undefined symbols.
---
Outside diff comments:
In `@pkg/executor/builder.go`:
- Around line 510-545: The diff contains unresolved merge conflict markers and
an incorrect session-var access in buildCheckTable: remove the conflict markers
and unify the logic (prefer a single boolean name, e.g., canUseFastCheck) so the
loop over v.IndexInfos tests idx.MVIndex or idx.IsColumnarIndex() and column
Length != types.UnspecifiedLength to set canUseFastCheck = false and break
appropriately; also replace b.ctx.GetSessionVars().FastCheckTable with
b.sctx.GetSessionVars().FastCheckTable so the code uses the sessionctx.Context
field.
In `@pkg/meta/model/index.go`:
- Around line 109-171: The file contains leftover Git conflict markers and
duplicated IndexInfo definitions; remove the conflict markers and keep a single,
consistent IndexInfo struct (prefer the newer definition with ast.CIStr, Tp as
ast.IndexType, and added fields
InvertedInfo/FullTextInfo/ConditionExprString/AffectColumn), then keep the
Hash64 and Equals methods below the struct (ensure they reference IndexInfo.ID).
Also update imports/types as needed (replace model.CIStr/model.IndexType with
ast.CIStr/ast.IndexType and ensure base.Hasher is imported) so the package
compiles.
---
Major comments:
In `@pkg/ddl/index_cop.go`:
- Around line 85-92: Move the error check for builder.Build() before any use of
kvReq to avoid nil dereference: call kvReq, err := builder.Build(), then
immediately if err != nil { return nil, conditionPushed, err } before touching
kvReq.RequestSource; after the nil-check set
kvReq.RequestSource.RequestSourceInternal, kvReq.RequestSource.RequestSourceType
= getDDLRequestSource(model.ActionAddIndex) and
kvReq.RequestSource.ExplicitRequestSourceType = kvutil.ExplicitTypeDDL. Ensure
you reference the same local symbols (kvReq, builder.Build(),
getDDLRequestSource, model.ActionAddIndex, kvutil.ExplicitTypeDDL,
conditionPushed) when moving the error handling.
In `@pkg/ddl/index_presplit.go`:
- Around line 186-200: The current fan-out over pi.Definitions uses each
partition ID for getSplitIdxPhysicalKeysFromBound which breaks global indexes
(their keys live under tblInfo.ID); update the branch in the partition-handling
block (where pi := tblInfo.GetPartitionInfo() and the loop over pi.Definitions)
to check the index type (e.g., idxInfo.IsGlobal or idxInfo.Global) and if the
index is global, call getSplitIdxPhysicalKeysFromBound once with tblInfo.ID (not
p.ID) so global-index boundaries are generated in the correct keyspace;
otherwise keep the per-partition calls as-is.
In `@pkg/table/tables/index_test.go`:
- Around line 703-717: The failpoint enabled via
testfailpoint.EnableCall("github.com/pingcap/tidb/pkg/ddl/afterWaitSchemaSynced",
...) may not be disabled if tc.ddl or assertions in the callback fail; to fix,
wrap each case in a subtest/helper (t.Run) or immediately defer the disable
right after calling testfailpoint.EnableCall so the cleanup always runs: call
testfailpoint.EnableCall(...), then immediately defer testfailpoint.Disable(t,
"github.com/pingcap/tidb/pkg/ddl/afterWaitSchemaSynced") before executing
tk.MustExec(tc.ddl) and the assertions inside the callback to guarantee the
failpoint is disabled on all exit paths.
---
Minor comments:
In `@pkg/executor/test/admintest/admin_test.go`:
- Line 242: The test contains a duplicate assertion calling require.NoError(t,
err) twice; remove the redundant duplicate so only a single require.NoError(t,
err) remains for the same error check. Locate the duplicate use of
require.NoError in the admin test (search for require.NoError and the variable
err in the failing test function) and delete the extra line, leaving the
original assertion intact.
- Line 99: Remove the duplicate require.NoError(t, err) in the test in
pkg/executor/test/admintest/admin_test.go: keep the existing NoError check that
appears earlier and delete the second, redundant require.NoError call so the
test asserts the error only once (then run tests to confirm).
---
Nitpick comments:
In `@br/tests/br_partial_index/run.sh`:
- Line 40: The run_br invocation in run.sh uses unquoted shell variables which
can cause word-splitting/globbing; update the command(s) (the `run_br --pd
$PD_ADDR backup db -s "local://$TEST_DIR/$DB" --db $DB` lines) to double-quote
the variables (e.g., "$PD_ADDR" and "$DB") in both occurrences (lines shown and
the second occurrence) so all references to PD_ADDR and DB are quoted
consistently.
🪄 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: 87d21f53-1a94-4998-bb47-4c904a56aa8f
📒 Files selected for processing (52)
br/tests/br_partial_index/run.shbr/tests/run_group_br_tests.sherrors.tomlpkg/ddl/backfilling_operators.gopkg/ddl/backfilling_test.gopkg/ddl/backfilling_txn_executor.gopkg/ddl/bench_test.gopkg/ddl/column.gopkg/ddl/copr/BUILD.bazelpkg/ddl/copr/copr_ctx.gopkg/ddl/copr/copr_ctx_test.gopkg/ddl/executor.gopkg/ddl/export_test.gopkg/ddl/index.gopkg/ddl/index_cop.gopkg/ddl/index_cop_test.gopkg/ddl/index_merge_tmp.gopkg/ddl/index_presplit.gopkg/ddl/integration_test.gopkg/ddl/modify_column.gopkg/ddl/partition.gopkg/errno/errcode.gopkg/errno/errname.gopkg/executor/builder.gopkg/executor/check_table_index.gopkg/executor/distsql_test.gopkg/executor/split.gopkg/executor/split_test.gopkg/executor/test/admintest/admin_test.gopkg/executor/test/writetest/write_test.gopkg/infoschema/perfschema/tables.gopkg/meta/model/BUILD.bazelpkg/meta/model/index.gopkg/meta/model/job_args.gopkg/planner/core/expression_codec_fn.gopkg/planner/core/logical_plan_builder.gopkg/server/tests/commontest/tidb_test.gopkg/table/index.gopkg/table/tables/BUILD.bazelpkg/table/tables/index.gopkg/table/tables/index_test.gopkg/table/tables/tables.gopkg/table/tables/testutil/BUILD.bazelpkg/table/tables/testutil/indexcheck.gopkg/util/dbterror/ddl_terror.gotests/integrationtest/r/ddl/integration.resulttests/integrationtest/r/executor/admin.resulttests/integrationtest/t/ddl/integration.testtests/integrationtest/t/executor/admin.testtests/realtikvtest/addindextest3/operator_test.gotests/realtikvtest/addindextest4/BUILD.bazeltests/realtikvtest/addindextest4/partial_index_test.go
💤 Files with no reviewable changes (3)
- pkg/ddl/copr/BUILD.bazel
- pkg/ddl/copr/copr_ctx_test.go
- pkg/ddl/backfilling_test.go
Comment on lines
+30
to
+36
| <<<<<<< HEAD | ||
| ["G07"]='br_pitr br_restore_physical br_blocklist' | ||
| ["G08"]='br_tikv_outage2 br_ttl br_views_and_sequences br_z_gc_safepoint br_autorandom br_file_corruption br_tiflash_conflict br_pitr_table_filter' | ||
| ======= | ||
| ["G07"]='br_pitr br_restore_physical' | ||
| ["G08"]='br_tikv_outage2 br_ttl br_views_and_sequences br_z_gc_safepoint br_autorandom br_file_corruption br_tiflash_conflict br_pitr_table_filter br_partial_index' | ||
| >>>>>>> 8c2781681a4 (ddl,tables: only write the index when it meets partial index condition (#62762)) |
There was a problem hiding this comment.
Unresolved merge conflict markers must be removed.
The merge conflict markers (<<<<<<< HEAD, =======, >>>>>>>) are still present in the shell script. This will cause syntax errors when the script is executed.
🔧 Proposed fix
-<<<<<<< HEAD
- ["G07"]='br_pitr br_restore_physical br_blocklist'
- ["G08"]='br_tikv_outage2 br_ttl br_views_and_sequences br_z_gc_safepoint br_autorandom br_file_corruption br_tiflash_conflict br_pitr_table_filter'
-=======
["G07"]='br_pitr br_restore_physical'
["G08"]='br_tikv_outage2 br_ttl br_views_and_sequences br_z_gc_safepoint br_autorandom br_file_corruption br_tiflash_conflict br_pitr_table_filter br_partial_index'
->>>>>>> 8c2781681a4 (ddl,tables: only write the index when it meets partial index condition (`#62762`))📝 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
| <<<<<<< HEAD | |
| ["G07"]='br_pitr br_restore_physical br_blocklist' | |
| ["G08"]='br_tikv_outage2 br_ttl br_views_and_sequences br_z_gc_safepoint br_autorandom br_file_corruption br_tiflash_conflict br_pitr_table_filter' | |
| ======= | |
| ["G07"]='br_pitr br_restore_physical' | |
| ["G08"]='br_tikv_outage2 br_ttl br_views_and_sequences br_z_gc_safepoint br_autorandom br_file_corruption br_tiflash_conflict br_pitr_table_filter br_partial_index' | |
| >>>>>>> 8c2781681a4 (ddl,tables: only write the index when it meets partial index condition (#62762)) | |
| ["G07"]='br_pitr br_restore_physical' | |
| ["G08"]='br_tikv_outage2 br_ttl br_views_and_sequences br_z_gc_safepoint br_autorandom br_file_corruption br_tiflash_conflict br_pitr_table_filter br_partial_index' |
🧰 Tools
🪛 Shellcheck (0.11.0)
[error] 30-30: Fix any mentioned problems and try again.
(SC1072)
🤖 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 `@br/tests/run_group_br_tests.sh` around lines 30 - 36, Remove the unresolved
Git conflict markers and produce a single coherent assignment for the
associative array keys ["G07"] and ["G08"]: delete the lines containing <<<<<<<
HEAD, ======= and >>>>>>> and merge the two conflicting values so the final
assignments include the unioned test names (ensure ["G07"] contains the intended
entries and ["G08"] contains the intended entries including br_partial_index or
br_pitr_table_filter as required by the branch change); update the array entries
for ["G07"] and ["G08"] so they are valid shell string assignments with no
conflict markers remaining.
Comment on lines
+283
to
+292
| <<<<<<< HEAD | ||
| ======= | ||
| ctx *OperatorCtx | ||
| // tableScanRowCount is the number of rows scanned by the corresponding TableScanTask. | ||
| // If the index is a partial index, the number of rows in the Chunk may be less than tableScanRowCount. | ||
| tableScanRowCount int64 | ||
| // conditionPushed records whether the index condition has been pushed down. If it's true, the ingest worker | ||
| // can skip running the checker in TiDB side. | ||
| conditionPushed bool | ||
| >>>>>>> 8c2781681a4 (ddl,tables: only write the index when it meets partial index condition (#62762)) |
There was a problem hiding this comment.
Critical: File contains multiple unresolved merge conflict markers.
This file has approximately 12 unresolved git merge conflict blocks scattered throughout, including in:
IndexRecordChunkstruct definition (lines 283-292)scanRecordsfunction (lines 544-557, 567-582, 599-608)WriteExternalStoreOperatorworker factory (lines 695-730)IndexIngestOperatorworker factory (lines 792-822)HandleTaskmethod (lines 903-935)Close/initIndexConditionCheckersmethods (lines 958-981)WriteChunkmethod (lines 1012-1023)
The code will not compile until all conflicts are resolved. The cherry-pick version introduces:
- New fields in
IndexRecordChunk(ctx,tableScanRowCount,conditionPushed) - New
newDistSQLCtxhelper method indexConditionCheckersfield andinitIndexConditionCheckersmethod- Conditional checker logic in
WriteChunk
Please resolve these merge conflicts to proceed with the partial index implementation.
🤖 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/backfilling_operators.go` around lines 283 - 292, The file contains
unresolved git conflict markers; remove all conflict markers and merge the two
variants so the cherry-picked partial-index changes are preserved: add the new
fields (ctx, tableScanRowCount, conditionPushed) to the IndexRecordChunk struct,
keep or add the newDistSQLCtx helper, add the indexConditionCheckers field and
the initIndexConditionCheckers method, and incorporate the conditional checker
logic into WriteChunk; also reconcile the alternate implementations in
scanRecords, WriteExternalStoreOperator/IndexIngestOperator worker factory code
paths, HandleTask, and Close/init flows so there are no leftover <<<<>>>/====
markers and the code compiles with the partial-index behavior intact (use
function/type names IndexRecordChunk, scanRecords, WriteExternalStoreOperator,
IndexIngestOperator, HandleTask, Close, initIndexConditionCheckers, WriteChunk,
newDistSQLCtx, indexConditionCheckers to locate and merge the correct blocks).
Comment on lines
+397
to
+423
| // GetSchemaAndNames returns the schema and nameslice returned from the internal cop request. | ||
| func (c *CopContextBase) GetSchemaAndNames() (*expression.Schema, types.NameSlice) { | ||
| exprColumns := make([]*expression.Column, 0, len(c.ExprColumnInfos)) | ||
| names := types.NameSlice{} | ||
| for i, col := range c.ExprColumnInfos { | ||
| newCol := col.Clone().(*expression.Column) | ||
| newCol.Index = i | ||
| exprColumns = append(exprColumns, newCol) | ||
|
|
||
| // Specially handle the extra handle column. | ||
| // We cannot get the name of extra handle column from tableInfo. | ||
| var colName ast.CIStr | ||
| if col.ID == model.ExtraHandleID { | ||
| colName = model.ExtraHandleName | ||
| } else { | ||
| colName = c.TableInfo.Columns[col.Index].Name | ||
| } | ||
|
|
||
| names = append(names, &types.FieldName{ | ||
| TblName: c.TableInfo.Name, | ||
| ColName: colName, | ||
| }) | ||
| } | ||
| schema := expression.NewSchema(exprColumns...) | ||
|
|
||
| return schema, names | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
sed -n '17,27p;397,423p' pkg/ddl/copr/copr_ctx.goRepository: pingcap/tidb
Length of output: 1358
Fix CopContextBase.GetSchemaAndNames compile error (pkg/ddl/copr/copr_ctx.go).
GetSchemaAndNames uses ast.CIStr but pkg/ddl/copr/copr_ctx.go doesn’t import github.com/pingcap/tidb/pkg/parser/ast; change colName to model.CIStr (or add the missing ast import) so the file compiles.
🤖 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/copr/copr_ctx.go` around lines 397 - 423, GetSchemaAndNames currently
declares colName as ast.CIStr but the file doesn't import the ast package; fix
the compile error by changing the colName declaration to use model.CIStr (or
alternatively add the missing import for
github.com/pingcap/tidb/pkg/parser/ast). Update the local variable declaration
in CopContextBase.GetSchemaAndNames (the colName variable used to populate
types.FieldName) so it references model.CIStr, ensuring consistency with the
project types and allowing the file to compile.
Comment on lines
+5011
to
+5023
| <<<<<<< HEAD | ||
| ======= | ||
| var conditionString string | ||
| if indexOption != nil { | ||
| conditionString, err = CheckAndBuildIndexConditionString(tblInfo, indexOption.Condition) | ||
| if err != nil { | ||
| return errors.Trace(err) | ||
| } | ||
| if len(conditionString) > 0 && !job.ReorgMeta.IsFastReorg { | ||
| return dbterror.ErrUnsupportedAddPartialIndex.GenWithStackByArgs("add partial index without fast reorg is not supported") | ||
| } | ||
| } | ||
| >>>>>>> 8c2781681a4 (ddl,tables: only write the index when it meets partial index condition (#62762)) |
There was a problem hiding this comment.
Resolve the leftover cherry-pick conflict markers.
This file still contains unresolved <<<<<<< / >>>>>>> blocks. It will not build as-is, and the lower block also leaves duplicate helper definitions in the same file.
Also applies to: 7178-7216
🤖 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 5011 - 5023, Remove the leftover git
conflict markers and choose the intended code block: delete the "<<<<<<< HEAD"
and ">>>>>>> ..." markers and keep the added logic that declares
conditionString, calls CheckAndBuildIndexConditionString(tblInfo,
indexOption.Condition), handles the returned error, and checks
len(conditionString) > 0 with job.ReorgMeta.IsFastReorg to return
dbterror.ErrUnsupportedAddPartialIndex when appropriate; also remove any
duplicate helper/function definitions introduced by the conflict (ensure only
one CheckAndBuildIndexConditionString implementation remains) so the file builds
cleanly.
Comment on lines
+55
to
+59
| <<<<<<< HEAD | ||
| scanOp := ddl.NewTableScanOperator(wctx, sessPool, copCtx, srcChkPool, 1, 0, nil, nil) | ||
| ======= | ||
| scanOp := ddl.NewTableScanOperator(opCtx, sessPool, copCtx, srcChkPool, 1, 0, &model.DDLReorgMeta{}, nil, &execute.TestCollector{}) | ||
| >>>>>>> 8c2781681a4 (ddl,tables: only write the index when it meets partial index condition (#62762)) |
There was a problem hiding this comment.
Critical: Unresolved merge conflict markers must be resolved.
This file contains unresolved git merge conflict markers (<<<<<<<, =======, >>>>>>>) that will cause a compilation failure. The conflict is between two versions of the NewTableScanOperator call:
- HEAD: Uses
wctxcontext andnilfor metadata arguments - Cherry-pick: Uses
opCtx(undefined),&model.DDLReorgMeta{}, and&execute.TestCollector{}(undefined import)
Additionally, the cherry-pick version references opCtx and execute.TestCollector which are not declared or imported in this file.
🤖 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/export_test.go` around lines 55 - 59, Resolve the unresolved merge
conflict in the NewTableScanOperator call: remove the conflict markers (<<<<<<<,
=======, >>>>>>>) and pick the correct argument set for NewTableScanOperator
(either the existing wctx with nil metadata or the cherry‑picked opCtx plus
&model.DDLReorgMeta{} and &execute.TestCollector{}); if you choose the
cherry‑picked form, add/import model and execute.TestCollector and ensure opCtx
is declared/constructed in this test; if you keep the HEAD form, restore the
original call using wctx and nils and remove references to undefined opCtx and
execute.TestCollector so the file compiles.
Comment on lines
+2287
to
+2297
| <<<<<<< HEAD | ||
| {"select /*+ read_from_storage(tikv[`stmtstats`.`t`]) */ bit_xor(crc32(md5(concat_ws(0x2, `_tidb_rowid`, `a`)))), ((cast(crc32(md5(concat_ws(0x2, `_tidb_rowid`))) as signed) - 0) div 1 % 1024), count(*) from `stmtstats`.`t` use index() where 0 = 0 group by ((cast(crc32(md5(concat_ws(0x2, `_tidb_rowid`))) as signed) - 0) div 1 % 1024)", []tikvrpc.CmdType{tikvrpc.CmdCop}}, | ||
| {"select bit_xor(crc32(md5(concat_ws(0x2, `_tidb_rowid`, `a`)))), ((cast(crc32(md5(concat_ws(0x2, `_tidb_rowid`))) as signed) - 0) div 1 % 1024), count(*) from `stmtstats`.`t` use index(`idx`) where 0 = 0 group by ((cast(crc32(md5(concat_ws(0x2, `_tidb_rowid`))) as signed) - 0) div 1 % 1024)", []tikvrpc.CmdType{tikvrpc.CmdCop}}, | ||
| {"select /*+ read_from_storage(tikv[`stmtstats`.`t`]) */ bit_xor(crc32(md5(concat_ws(0x2, `_tidb_rowid`, `a`)))), ((cast(crc32(md5(concat_ws(0x2, `_tidb_rowid`))) as signed) - 0) div 1 % 1024), count(*) from `stmtstats`.`t` use index() where 0 = 0 group by ((cast(crc32(md5(concat_ws(0x2, `_tidb_rowid`))) as signed) - 0) div 1 % 1024)", []tikvrpc.CmdType{tikvrpc.CmdCop}}, | ||
| {"select bit_xor(crc32(md5(concat_ws(0x2, `_tidb_rowid`, `a`)))), ((cast(crc32(md5(concat_ws(0x2, `_tidb_rowid`))) as signed) - 0) div 1 % 1024), count(*) from `stmtstats`.`t` use index(`idx`) where 0 = 0 group by ((cast(crc32(md5(concat_ws(0x2, `_tidb_rowid`))) as signed) - 0) div 1 % 1024)", []tikvrpc.CmdType{tikvrpc.CmdCop}}, | ||
| ======= | ||
| {"select /*+ read_from_storage(tikv[`stmtstats`.`t`]), AGG_TO_COP() */ bit_xor(crc32(md5(concat_ws(0x2, `_tidb_rowid`, `a`)))), ((cast(crc32(md5(concat_ws(0x2, `_tidb_rowid`))) as signed) - 0) div 1 % 1024), count(*) from `stmtstats`.`t` use index() where (0 = 0) group by ((cast(crc32(md5(concat_ws(0x2, `_tidb_rowid`))) as signed) - 0) div 1 % 1024)", []tikvrpc.CmdType{tikvrpc.CmdCop}}, | ||
| {"select /*+ AGG_TO_COP() */ bit_xor(crc32(md5(concat_ws(0x2, `_tidb_rowid`, `a`)))), ((cast(crc32(md5(concat_ws(0x2, `_tidb_rowid`))) as signed) - 0) div 1 % 1024), count(*) from `stmtstats`.`t` use index(`idx`) where (0 = 0) group by ((cast(crc32(md5(concat_ws(0x2, `_tidb_rowid`))) as signed) - 0) div 1 % 1024)", []tikvrpc.CmdType{tikvrpc.CmdCop}}, | ||
| {"select /*+ read_from_storage(tikv[`stmtstats`.`t`]), AGG_TO_COP() */ bit_xor(crc32(md5(concat_ws(0x2, `_tidb_rowid`, `a`)))), ((cast(crc32(md5(concat_ws(0x2, `_tidb_rowid`))) as signed) - 0) div 1 % 1024), count(*) from `stmtstats`.`t` use index() where (0 = 0) group by ((cast(crc32(md5(concat_ws(0x2, `_tidb_rowid`))) as signed) - 0) div 1 % 1024)", []tikvrpc.CmdType{tikvrpc.CmdCop}}, | ||
| {"select /*+ AGG_TO_COP() */ bit_xor(crc32(md5(concat_ws(0x2, `_tidb_rowid`, `a`)))), ((cast(crc32(md5(concat_ws(0x2, `_tidb_rowid`))) as signed) - 0) div 1 % 1024), count(*) from `stmtstats`.`t` use index(`idx`) where (0 = 0) group by ((cast(crc32(md5(concat_ws(0x2, `_tidb_rowid`))) as signed) - 0) div 1 % 1024)", []tikvrpc.CmdType{tikvrpc.CmdCop}}, | ||
| >>>>>>> 8c2781681a4 (ddl,tables: only write the index when it meets partial index condition (#62762)) |
There was a problem hiding this comment.
Critical: Unresolved merge conflict markers must be removed.
The test case contains Git merge conflict markers (<<<<<<< HEAD, =======, >>>>>>> 8c2781681a4) that will prevent compilation. These markers must be resolved before this PR can be merged.
Once resolved, the updated SQL strings appear to add the AGG_TO_COP() optimizer hint and adjust whitespace formatting around the WHERE clause, which seems reasonable for testing partial index support with different query patterns.
🤖 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/server/tests/commontest/tidb_test.go` around lines 2287 - 2297, There are
unresolved Git conflict markers (<<<<<<<, =======, >>>>>>>) inside the SQL test
entries in tidb_test.go; remove the conflict markers and keep the intended final
SQL strings (the ones with the AGG_TO_COP() hint and adjusted whitespace) so
each test entry is a single valid string literal (e.g., the entries containing
"/*+ read_from_storage(tikv[`stmtstats`.`t`]), AGG_TO_COP() */ ..." and "/*+
AGG_TO_COP() */ ...") and ensure the slice of test cases no longer contains any
merge markers so the file compiles.
Comment on lines
+32
to
+36
| <<<<<<< HEAD | ||
| ======= | ||
| "//pkg/sessionctx/stmtctx", | ||
| "//pkg/sessionctx/vardef", | ||
| >>>>>>> 8c2781681a4 (ddl,tables: only write the index when it meets partial index condition (#62762)) |
There was a problem hiding this comment.
Resolve merge conflict markers before merging.
This file contains unresolved git merge conflict markers at lines 32-36 and 84-88. These must be removed and the conflicts properly resolved before the PR can be merged, as they will break the build.
Also applies to: 84-88
🤖 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/table/tables/BUILD.bazel` around lines 32 - 36, The file
pkg/table/tables/BUILD.bazel contains unresolved git conflict markers (e.g.,
<<<<<<< HEAD, =======, >>>>>>> 8c2781681a4) around the dependency entries and
must be cleaned up: open the BUILD.bazel, remove the conflict markers and choose
the correct dependency set (ensure the entries like "//pkg/sessionctx/stmtctx"
and "//pkg/sessionctx/vardef" are either kept or removed according to intended
change), make the file a single valid BUILD rule without any marker lines, and
run a quick build or bazel query to verify the file is syntactically correct;
repeat the same resolution for the second conflict block corresponding to lines
84-88.
Comment on lines
+83
to
+89
| <<<<<<< HEAD | ||
| ======= | ||
| // ErrUnsupportedAddPartialIndex the partial index condition is not supported | ||
| ErrUnsupportedAddPartialIndex = ClassDDL.NewStdErr(mysql.ErrUnsupportedDDLOperation, parser_mysql.Message(fmt.Sprintf(mysql.MySQLErrName[mysql.ErrUnsupportedDDLOperation].Raw, "add partial index: %s"), nil)) | ||
| // ErrModifyColumnReferencedByPartialCondition is used when a column is referenced by a partial index condition. | ||
| ErrModifyColumnReferencedByPartialCondition = ClassDDL.NewStd(mysql.ErrModifyColumnReferencedByPartialCondition) | ||
| >>>>>>> 8c2781681a4 (ddl,tables: only write the index when it meets partial index condition (#62762)) |
There was a problem hiding this comment.
Unresolved merge conflict markers must be removed.
The merge conflict markers (<<<<<<< HEAD, =======, >>>>>>>) are still present in the code. This will cause compilation failure and must be resolved before merging.
🔧 Proposed fix
-<<<<<<< HEAD
-=======
// ErrUnsupportedAddPartialIndex the partial index condition is not supported
ErrUnsupportedAddPartialIndex = ClassDDL.NewStdErr(mysql.ErrUnsupportedDDLOperation, parser_mysql.Message(fmt.Sprintf(mysql.MySQLErrName[mysql.ErrUnsupportedDDLOperation].Raw, "add partial index: %s"), nil))
// ErrModifyColumnReferencedByPartialCondition is used when a column is referenced by a partial index condition.
ErrModifyColumnReferencedByPartialCondition = ClassDDL.NewStd(mysql.ErrModifyColumnReferencedByPartialCondition)
->>>>>>> 8c2781681a4 (ddl,tables: only write the index when it meets partial index condition (`#62762`))📝 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
| <<<<<<< HEAD | |
| ======= | |
| // ErrUnsupportedAddPartialIndex the partial index condition is not supported | |
| ErrUnsupportedAddPartialIndex = ClassDDL.NewStdErr(mysql.ErrUnsupportedDDLOperation, parser_mysql.Message(fmt.Sprintf(mysql.MySQLErrName[mysql.ErrUnsupportedDDLOperation].Raw, "add partial index: %s"), nil)) | |
| // ErrModifyColumnReferencedByPartialCondition is used when a column is referenced by a partial index condition. | |
| ErrModifyColumnReferencedByPartialCondition = ClassDDL.NewStd(mysql.ErrModifyColumnReferencedByPartialCondition) | |
| >>>>>>> 8c2781681a4 (ddl,tables: only write the index when it meets partial index condition (#62762)) | |
| // ErrUnsupportedAddPartialIndex the partial index condition is not supported | |
| ErrUnsupportedAddPartialIndex = ClassDDL.NewStdErr(mysql.ErrUnsupportedDDLOperation, parser_mysql.Message(fmt.Sprintf(mysql.MySQLErrName[mysql.ErrUnsupportedDDLOperation].Raw, "add partial index: %s"), nil)) | |
| // ErrModifyColumnReferencedByPartialCondition is used when a column is referenced by a partial index condition. | |
| ErrModifyColumnReferencedByPartialCondition = ClassDDL.NewStd(mysql.ErrModifyColumnReferencedByPartialCondition) |
🤖 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/util/dbterror/ddl_terror.go` around lines 83 - 89, The file contains
unresolved git merge conflict markers around the DDL error declarations; remove
the conflict markers (<<<<<<< HEAD, =======, >>>>>>>) and keep the intended
declarations for ErrUnsupportedAddPartialIndex and
ErrModifyColumnReferencedByPartialCondition so they are valid Go code; ensure
the two constants are declared using ClassDDL.NewStdErr / ClassDDL.NewStd as in
the diff and that imports (parser_mysql, mysql) remain correct so the file
compiles.
Comment on lines
+118
to
+122
| <<<<<<< HEAD | ||
| scanOp := ddl.NewTableScanOperator(wctx, sessPool, copCtx, srcChkPool, 3, 0, nil, nil) | ||
| ======= | ||
| scanOp := ddl.NewTableScanOperator(opCtx, sessPool, copCtx, srcChkPool, 3, 0, &model.DDLReorgMeta{}, nil, &execute.TestCollector{}) | ||
| >>>>>>> 8c2781681a4 (ddl,tables: only write the index when it meets partial index condition (#62762)) |
There was a problem hiding this comment.
Critical: Unresolved merge conflict markers must be resolved.
This test file contains unresolved git merge conflict markers. The conflict is in the NewTableScanOperator constructor call within TestBackfillOperators:
- HEAD: Uses
wctxwithnilmetadata - Cherry-pick: Uses
opCtx(undefined),&model.DDLReorgMeta{}, and&execute.TestCollector{}(undefined)
The cherry-pick version references identifiers that are not declared or imported in this file.
🤖 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/realtikvtest/addindextest3/operator_test.go` around lines 118 - 122,
Resolve the git conflict in the TestBackfillOperators call to
NewTableScanOperator: replace the conflict markers and choose the correct
constructor signature used in this test file (use the existing context variables
like wctx, sessPool, copCtx, srcChkPool) and remove or properly import/define
any new identifiers from the cherry-pick (opCtx, model.DDLReorgMeta,
execute.TestCollector) if you intend to use the new signature; specifically,
either restore the original call NewTableScanOperator(wctx, sessPool, copCtx,
srcChkPool, 3, 0, nil, nil) or update the file to declare/import opCtx,
model.DDLReorgMeta{}, and execute.TestCollector and pass them consistently to
NewTableScanOperator so there are no unresolved merge markers or undefined
symbols.
Comment on lines
+424
to
+430
| <<<<<<< HEAD | ||
| wctx := workerpool.NewContext(ctx) | ||
| scanOp := ddl.NewTableScanOperator(wctx, sessPool, copCtx, nil, 2, 0, nil, nil) | ||
| ======= | ||
| opCtx, cancel := ddl.NewDistTaskOperatorCtx(ctx) | ||
| scanOp := ddl.NewTableScanOperator(opCtx, sessPool, copCtx, nil, 2, 0, &model.DDLReorgMeta{}, nil, &execute.TestCollector{}) | ||
| >>>>>>> 8c2781681a4 (ddl,tables: only write the index when it meets partial index condition (#62762)) |
There was a problem hiding this comment.
Critical: Second unresolved merge conflict in TestTuneWorkerPoolSize.
Same issue as above - unresolved merge conflict markers with undefined identifiers (opCtx, execute.TestCollector). Line 439 also references wctx.Cancel() which may be incorrect depending on which side of the conflict is chosen.
🤖 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/realtikvtest/addindextest3/operator_test.go` around lines 424 - 430,
Resolve the merge conflict markers and make the TestTuneWorkerPoolSize code
consistent by choosing the distributed task context variant: replace the
conflicted block with a single clean initialization using
ddl.NewDistTaskOperatorCtx to get (opCtx, cancel), call
ddl.NewTableScanOperator(opCtx, sessPool, copCtx, nil, 2, 0,
&model.DDLReorgMeta{}, nil, &execute.TestCollector{}), remove wctx and
wctx.Cancel() usages and instead call cancel() where the context should be
released, and ensure execute.TestCollector is imported/available.
|
@ti-chi-bot: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
ti-chi-bot
Bot
added
cherry-pick-approved
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
2 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 #62762
This is the second PR for partial index
#63448
#62759
-> #62762
What problem does this PR solve?
Issue Number: close #62761
Problem Summary:
What changed and how does it work?
tables.indexto parse/store the expression ofpartialConditionExpression.MeetPartialConditionto check whether it'll meet the condition.admin show ddl jobfrom the added count to the scanned count.admin check tablefor partial index.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
Release Notes
New Features
Improvements
admin check tablebehavior for partial indexes, requiringtidb_enable_fast_table_checkto be enabled.Limitations