*: use user keyspace's collation mode for DXF tasks#69677
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR threads ChangesUseNewCollate Plumbing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DDLExecutor
participant DDLReorgMeta
participant backfillDistExecutor
participant copr
participant tables
DDLExecutor->>DDLReorgMeta: set UseNewCollate
backfillDistExecutor->>DDLReorgMeta: read UseNewCollate default
backfillDistExecutor->>copr: NewCopContext(..., useNewCollate)
copr->>tables: ColumnInfos2ColumnsAndNamesWithCollate(...)
backfillDistExecutor->>tables: TableFromMetaWithCollate(...)
tables-->>backfillDistExecutor: collate-aware table
sequenceDiagram
participant LoadDataController
participant Plan
participant tables
participant expression
LoadDataController->>Plan: GetUseNewCollateOrDefault(...)
LoadDataController->>tables: TableFromMetaWithCollate(...)
LoadDataController->>expression: BuildSimpleExpr(..., WithUseNewCollate(...))
tables-->>LoadDataController: collate-aware table
expression-->>LoadDataController: generated-column expression
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/ddl/index.go (1)
2700-2716: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDerive
useNewCollatefromcopCtxinsidewriteChunk.
writeChunkalready hasc := copCtx.GetBase(), which now carriesUseNewCollate. Taking a separate boolean makes it possible for callers to encode handles/restored data with a different collation than the cop context used for expression metadata.Proposed direction
func writeChunk( ctx context.Context, writers []ingest.Writer, indexes []table.Index, indexConditionCheckers []func(row chunk.Row) (bool, error), copCtx copr.CopContext, loc *time.Location, errCtx errctx.Context, writeStmtBufs *variable.WriteStmtBufs, copChunk *chunk.Chunk, tblInfo *model.TableInfo, - useNewCollate bool, ) (rowCnt int, bytes int, err error) { iter := chunk.NewIterator4Chunk(copChunk) c := copCtx.GetBase() + useNewCollate := c.UseNewCollateAlso applies to: 2736-2779
🤖 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 2700 - 2716, `writeChunk` should not accept a separate `useNewCollate` flag; derive the collation mode from `copCtx.GetBase()` inside `writeChunk` so encoding stays consistent with the cop context’s expression metadata. Update `writeChunk` and its call sites to use the `UseNewCollate` value from `copCtx` (through the existing `c := copCtx.GetBase()` path) and remove the extra boolean parameter to prevent callers from passing mismatched collation state.pkg/ddl/backfilling_operators.go (1)
114-121: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse the persisted reorg collation for index encoding too.
tbl.UseNewCollate()comes from the live table object, whileNewReorgCopContextuses the reorg snapshot; if those differ on a resumed/background job, scan metadata and index KV encoding can drift apart. ReusecopCtx.GetBase().UseNewCollatehere and at the other callsite.🤖 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 114 - 121, The index encoding path is still using the live table collation via tbl.UseNewCollate(), which can diverge from the reorg snapshot used by NewReorgCopContext on resumed jobs. Update the index creation logic in backfilling_operators.go to reuse copCtx.GetBase().UseNewCollate for tables.NewIndexWithCollate, and apply the same change at the other callsite so scan metadata and KV encoding stay consistent.pkg/ddl/export_test.go (1)
77-86: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the CopContext's captured collation instead of re-querying the process default.
copCtxalready carries the collation it was built with (CopContextBase.UseNewCollate, set byNewCopContext/NewReorgCopContext), obtained here viac := copCtx.GetBase(). Callingcollate.NewCollationEnabled()at line 84 ignores that captured value and re-reads the worker's current global setting instead. This defeats the purpose of this exact PR stack (avoiding worker-global lookups in favor of a captured/task-level value) and means this test helper cannot correctly exercise scenarios where the CopContext's collation differs from the process default.🐛 Proposed fix
- handle, err := ddl.BuildHandle(collate.NewCollationEnabled(), handleData, c.TableInfo, c.PrimaryKeyInfo, time.Local, errctx.StrictNoWarningContext) + handle, err := ddl.BuildHandle(c.UseNewCollate, handleData, c.TableInfo, c.PrimaryKeyInfo, time.Local, errctx.StrictNoWarningContext)If
collatebecomes unused elsewhere in this file, remove the now-unnecessary import at line 36.🤖 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 77 - 86, `ConvertRowToHandleAndIndexDatum` is ignoring the `CopContext`’s captured collation and re-reading the process default via `collate.NewCollationEnabled()`. Use the collation stored on `c := copCtx.GetBase()` when calling `ddl.BuildHandle` so this helper follows the task-level setting from `NewCopContext`/`NewReorgCopContext`. If `collate` is no longer needed in `export_test.go`, remove that import as well.
🧹 Nitpick comments (1)
pkg/ddl/copr/copr_ctx_test.go (1)
109-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
useNewCollate=trueconstructor path.This only adapts the existing test with
false; a regression that drops the new flag inNewCopContextBasewould still pass. Add a small true-case assertion thatbase.UseNewCollatematches the argument.🤖 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_test.go` around lines 109 - 120, The current test only exercises the NewCopContextSingleIndex path with useNewCollate set to false, so it won’t catch regressions in the true path. Update the copr_ctx_test coverage around NewCopContextSingleIndex and GetBase by adding a small assertion for the useNewCollate=true constructor case and verify base.UseNewCollate matches the argument, alongside the existing TableInfo check.
🤖 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/backfilling_operators.go`:
- Around line 917-918: The writeChunk call is reading collation mode from w.tbl
again instead of using the fixed collation captured in w.copCtx. Update the call
site in backfilling_operators.go so writeChunk receives
w.copCtx.GetBase().UseNewCollate rather than w.tbl.UseNewCollate, keeping row
decoding, handle construction, and index writes aligned with the same snapshot.
Use the existing writeChunk invocation and w.copCtx reference as the location to
make this change.
---
Outside diff comments:
In `@pkg/ddl/backfilling_operators.go`:
- Around line 114-121: The index encoding path is still using the live table
collation via tbl.UseNewCollate(), which can diverge from the reorg snapshot
used by NewReorgCopContext on resumed jobs. Update the index creation logic in
backfilling_operators.go to reuse copCtx.GetBase().UseNewCollate for
tables.NewIndexWithCollate, and apply the same change at the other callsite so
scan metadata and KV encoding stay consistent.
In `@pkg/ddl/export_test.go`:
- Around line 77-86: `ConvertRowToHandleAndIndexDatum` is ignoring the
`CopContext`’s captured collation and re-reading the process default via
`collate.NewCollationEnabled()`. Use the collation stored on `c :=
copCtx.GetBase()` when calling `ddl.BuildHandle` so this helper follows the
task-level setting from `NewCopContext`/`NewReorgCopContext`. If `collate` is no
longer needed in `export_test.go`, remove that import as well.
In `@pkg/ddl/index.go`:
- Around line 2700-2716: `writeChunk` should not accept a separate
`useNewCollate` flag; derive the collation mode from `copCtx.GetBase()` inside
`writeChunk` so encoding stays consistent with the cop context’s expression
metadata. Update `writeChunk` and its call sites to use the `UseNewCollate`
value from `copCtx` (through the existing `c := copCtx.GetBase()` path) and
remove the extra boolean parameter to prevent callers from passing mismatched
collation state.
---
Nitpick comments:
In `@pkg/ddl/copr/copr_ctx_test.go`:
- Around line 109-120: The current test only exercises the
NewCopContextSingleIndex path with useNewCollate set to false, so it won’t catch
regressions in the true path. Update the copr_ctx_test coverage around
NewCopContextSingleIndex and GetBase by adding a small assertion for the
useNewCollate=true constructor case and verify base.UseNewCollate matches the
argument, alongside the existing TableInfo check.
🪄 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: e82e626e-b969-4548-a71f-cd1cb089b9a0
📒 Files selected for processing (32)
lightning/pkg/errormanager/errormanager.gopkg/ddl/backfilling_dist_executor.gopkg/ddl/backfilling_dist_scheduler.gopkg/ddl/backfilling_operators.gopkg/ddl/backfilling_txn_executor.gopkg/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/job_scheduler.gopkg/ddl/reorg.gopkg/dxf/importinto/conflictedkv/handler.gopkg/dxf/importinto/planner.gopkg/dxf/importinto/task_executor.gopkg/executor/admin.gopkg/executor/batch_checker.gopkg/executor/importer/import.gopkg/executor/importer/sampler.gopkg/executor/importer/table_import.gopkg/expression/expression.gopkg/infoschema/tables.gopkg/lightning/backend/kv/base.gopkg/lightning/backend/kv/kv2sql.gopkg/lightning/backend/kv/sql2kv.gopkg/meta/model/reorg.gopkg/planner/core/expression_rewriter.gopkg/table/table.gopkg/table/tables/mutation_checker_test.gopkg/table/tables/partition.gopkg/table/tables/tables.go
💤 Files with no reviewable changes (1)
- pkg/ddl/job_scheduler.go
Co-authored-by: D3Hunter <jujj603@gmail.com>
|
/retest |
|
/retest |
ingress-bot
left a comment
There was a problem hiding this comment.
This review was generated by AI and should be verified by a human reviewer.
Manual follow-up is recommended before merge.
Summary
- Total findings: 8
- Inline comments: 3
- Summary-only findings (no inline anchor): 5
Findings (highest risk first)
⚠️ [Major] (3)
- CopContextBase.UseNewCollate is a dead field that looks authoritative but is never read (pkg/ddl/copr/copr_ctx.go:49, pkg/ddl/copr/copr_ctx.go:150, pkg/ddl/backfilling_operators.go:918)
- Mixed-version rolling upgrade can produce inconsistent collation encoding for cross-keyspace add-index/import (pkg/ddl/backfilling_dist_scheduler.go:205, pkg/executor/importer/table_import.go:94, pkg/meta/model/reorg.go:98, pkg/executor/importer/import.go:338)
- Rolling upgrade: in-flight reorg/import jobs with nil UseNewCollate fall back to the executor process default, risking split collation encoding (pkg/ddl/backfilling_dist_scheduler.go:206, pkg/meta/model/reorg.go:98, pkg/executor/importer/import.go:338, pkg/executor/importer/table_import.go:94)
🟡 [Minor] (3)
- Error message still names the old
tables.TableFromMetafunction after the call site switched toTableFromMetaWithCollate(pkg/executor/importer/table_import.go:98, pkg/executor/importer/sampler.go:341) - Optional-collation-flag field/getter/setter duplicated verbatim across two packages (pkg/executor/importer/import.go:343, pkg/executor/importer/import.go:360, pkg/executor/importer/import.go:369, pkg/meta/model/reorg.go:103, pkg/meta/model/reorg.go:162, pkg/meta/model/reorg.go:171)
- New useNewCollate flag is positioned inconsistently across sibling function signatures (pkg/table/tables/tables.go:173, pkg/table/tables/tables.go:248, pkg/ddl/index_cop.go:259, pkg/ddl/index_cop.go:148, pkg/ddl/index.go:2700, pkg/ddl/copr/copr_ctx.go:78, pkg/ddl/reorg.go:850)
ℹ️ [Info] (1)
NewCopContextBasedoc comment wasn't updated to mention the new collation parameter, unlike its three sibling constructors (pkg/ddl/copr/copr_ctx.go:76)
🧹 [Nit] (1)
- Identical admin-check-and-verify test helper duplicated across two realtikvtest packages (tests/realtikvtest/addindextest1/cross_ks_test.go:331, tests/realtikvtest/importintotest3/cross_ks_test.go:608)
Unanchored findings
⚠️ [Major] (3)
- CopContextBase.UseNewCollate is a dead field that looks authoritative but is never read
- Request: Either make
writeChunk/getRestoreData/related call sites consumecopCtx.GetBase().UseNewCollateas the single source of truth, or drop the field fromCopContextBaseand its assignment inNewCopContextBasesince it currently has no consumer.
- Request: Either make
- Mixed-version rolling upgrade can produce inconsistent collation encoding for cross-keyspace add-index/import
- Request: Confirm whether DXF prevents pre-PR executors from running these subtasks during a rolling upgrade; if it does not, gate the differing-collation encoding until full upgrade (or document that cross-keyspace add-index/import-into on such keyspaces must not run mid-upgrade) and add a mixed-version validation for the encoding path.
- Rolling upgrade: in-flight reorg/import jobs with nil UseNewCollate fall back to the executor process default, risking split collation encoding
- Request: Confirm whether DXF prevents pre-PR executors/in-flight jobs from resuming these subtasks during a rolling upgrade; if not, gate or reject the differing-collation encoding until the job's collation mode is known, and add a mixed-version/old-metadata test that resumes a nil-
UseNewCollatejob and asserts index consistency.
- Request: Confirm whether DXF prevents pre-PR executors/in-flight jobs from resuming these subtasks during a rolling upgrade; if not, gate or reject the differing-collation encoding until the job's collation mode is known, and add a mixed-version/old-metadata test that resumes a nil-
🟡 [Minor] (2)
- Error message still names the old
tables.TableFromMetafunction after the call site switched toTableFromMetaWithCollate- Request: Update both error strings to name
tables.TableFromMetaWithCollate(or drop the function name and just say "failed to build table from meta") so the message matches the code that actually runs.
- Request: Update both error strings to name
- Optional-collation-flag field/getter/setter duplicated verbatim across two packages
- Request: Extract the pointer-bool-with-default pattern into one small shared helper type (for example in a common metadata/compat util package) and have both
PlanandDDLReorgMetaembed or delegate to it, so the fallback semantics only exist in one place.
- Request: Extract the pointer-bool-with-default pattern into one small shared helper type (for example in a common metadata/compat util package) and have both
| t.recordPrefix = tablecodec.GenTableRecordPrefix(physicalTableID) | ||
| t.indexPrefix = tablecodec.GenTableIndexPrefix(physicalTableID) | ||
| t.encoder = codec.NewEncoder(collate.NewCollationEnabled()) | ||
| func newTableCommon(tblInfo *model.TableInfo, physicalTableID int64, cols []*table.Column, allocs autoid.Allocators, constraints []*table.Constraint, useNewCollate bool) TableCommon { |
There was a problem hiding this comment.
🟡 [Minor] New useNewCollate flag is positioned inconsistently across sibling function signatures
Impact
This PR threads a new useNewCollate bool flag through a large number of signatures, but its position in the parameter list flips depending on the function: TableFromMetaWithCollate and BuildHandle/getRestoreData put it first, while newTableCommon (called directly by TableFromMetaWithCollate two hunks later in the same file) and writeChunk/NewCopContextBase put it last.
With no single convention to copy, the next contributor adding a related flag (or wiring a new call site) has to re-derive the ordering for each function instead of pattern-matching a sibling, which is exactly the kind of drift risk that surfaces as a misplaced argument in a future refactor of these encoding-path helpers.
Scope
pkg/table/tables/tables.go:173—TableFromMetaWithCollatepkg/table/tables/tables.go:248—newTableCommonpkg/ddl/index_cop.go:259—BuildHandlepkg/ddl/index_cop.go:148—getRestoreDatapkg/ddl/index.go:2700—writeChunkpkg/ddl/copr/copr_ctx.go:78—NewCopContextBasepkg/ddl/reorg.go:850—buildCommonHandleFromChunkRow
Evidence
TableFromMetaWithCollate(useNewCollate bool, allocs autoid.Allocators, tblInfo *model.TableInfo) puts the flag first and immediately calls newTableCommon(tblInfo, physicalTableID, cols, allocs, constraints, useNewCollate), which puts the same flag last; the same split repeats between BuildHandle/getRestoreData (flag first) and writeChunk/NewCopContextBase (flag last).
Change request
Pick one convention for this new flag (trailing is the more common Go idiom here, matching writeChunk/NewCopContextBase) and apply it uniformly across TableFromMetaWithCollate, BuildHandle, getRestoreData, and buildCommonHandleFromChunkRow.
| tblInfo *model.TableInfo, | ||
| idxCols []*model.IndexColumn, | ||
| requestSource string, | ||
| useNewCollate bool, |
There was a problem hiding this comment.
ℹ️ [Info] NewCopContextBase doc comment wasn't updated to mention the new collation parameter, unlike its three sibling constructors
NewCopContext, NewCopContextSingleIndex, and NewCopContextMultiIndex all got their doc comments updated to say "creates a ... with a fixed collation mode", but NewCopContextBase — the function that actually receives and stores useNewCollate into the struct — kept its original one-line doc with no mention of the new parameter.
| } | ||
| } | ||
|
|
||
| func checkImportTableAndIndexes(tk *testkit.TestKit, tableName string, indexes []string, expectedCount string) { |
There was a problem hiding this comment.
🧹 [Nit] Identical admin-check-and-verify test helper duplicated across two realtikvtest packages
checkTableAndIndexes (addindextest1) and checkImportTableAndIndexes (importintotest3), both newly added by this PR, run the exact same three-step verification (admin check, row count, per-index force-index count) with only the function name differing.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, GMHDBJD, windtalker 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 |
|
/test idc-jenkins-ci-tidb/check_dev_2 |
|
@joechenrh: The specified target(s) for The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions 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. |
|
/test check-dev2 |
|
/retest |
|
/cherry-pick release-nextgen-202603 |
|
@D3Hunter: new pull request created to branch DetailsIn response to this:
Instructions 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. |
What problem does this PR solve?
Issue Number: ref #69563
Problem Summary:
In next-gen deployments, DXF workers may run in a keyspace whose
mysql.tidb.new_collation_enableddiffers from the submitting user keyspace. If ADD INDEX or IMPORT INTO encodes user-table data with the worker-global setting, generated keys or restored row data can become inconsistent with the user table.What changed and how does it work?
Fix scope in this PR:
Supported:
ADD INDEXandIMPORT INTOkey/data encoding for non-partitioned tables.LOWER(),UPPER(),CONCAT(), andSUBSTR()in generated columns, expression indexes, and IMPORT INTO column assignments.Out of scope / not validated in this PR:
=,<=>,!=,<,<=,>,>=,IN,LIKE/ILIKE/REGEXP,IF/CASEbranches using those predicates,STRCMP(),FIELD(),LOCATE()/INSTR(),GREATEST()/LEAST(), andWEIGHT_STRING().Check List
Tests
Manual test setup:
mysql.tidb.new_collation_enabled = False.mysql.tidb.new_collation_enabled = True.ADMIN CHECK TABLE, forced index reads vs primary reads, and post-load/post-backfill DML (INSERT,UPDATE,DELETE) followed by anotherADMIN CHECK TABLEand forced index reads.IMPORT INTO:
ADMIN CHECK TABLEfails.id1values.idx_fk_prefixreportsindex-count:3 != record-count:0.index-count:1 != record-count:0.ADMIN CHECK TABLEreports data inconsistency; forced reads through all four generated-column indexes reportindex-count:3 != record-count:0.ADMIN CHECK TABLEreports data inconsistency; forced reads through all four assignment indexes reportindex-count:3 != record-count:0.IMPORT INTO SQL details
Clustered VARCHAR PK + secondary VARCHAR index
Schema:
Import mapping:
IMPORT INTO trigger_varchar_pk_varchar_idx(@1,id,fk);Clustered VARCHAR PK + secondary INT index
Schema:
Import mapping:
IMPORT INTO trigger_varchar_pk_int_idx(fk,id,@3);Composite clustered PK with VARCHAR part + secondary INT index
Schema:
Import mapping:
Composite clustered INT PK + secondary VARCHAR index
Schema:
Import mapping:
Composite CHAR PK + secondary VARCHAR index
Schema:
Import mapping:
Clustered VARCHAR PK + prefix secondary VARCHAR index
Schema:
Import mapping:
IMPORT INTO trigger_varchar_pk_prefix_varchar_idx(@1,id,fk);Clustered VARCHAR PK + secondary VARCHAR index + extra payload
Schema:
Import mapping:
IMPORT INTO trigger_record_ok_index_bad_extra_payload(@1,id,fk);Generated columns with string transformations
Schema:
Import mapping:
IMPORT INTO t_import_generated(@1,id,raw);Assignment expressions with string transformations
Schema:
Import mapping / SET:
Controls without mismatched string collation in encoded keys
Cases:
ADD INDEX:
ADMIN CHECK TABLEor secondary-index reads show table/index inconsistency.ADMIN CHECK TABLEor secondary-index reads show table/index inconsistency.ADMIN CHECK TABLEreports data inconsistency; forced reads through all four generated-column indexes report table/index count mismatch.ADMIN CHECK TABLEreports data inconsistency; forced reads through all four expression indexes reportindex-count:3 != record-count:0.ADD INDEX SQL details
Clustered VARCHAR PK + new secondary VARCHAR index
Schema / DML:
ADD INDEX DDL:
Composite clustered PK with VARCHAR part + new secondary INT index
Schema / DML:
ADD INDEX DDL:
Generated columns with string transformations
Schema / DML:
ADD INDEX DDL:
Expression indexes with string transformations
Schema / DML:
ADD INDEX DDL:
Side effects
Documentation
Release note