*: use user keyspace's collation mode for DXF tasks (#69677)#69702
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
component/import
do-not-merge/hold
|
@D3Hunter 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 (4)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThreads a fixed ChangesCollation mode plumbing
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: context deadline exceeded" 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/ddl/backfilling_dist_scheduler.go (1)
209-231: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winUnresolved git merge-conflict markers left in source (compile-breaking).
Lines 209, 211, and 224 contain literal conflict markers. Beyond the invalid syntax, naively resolving by keeping both sides would leave a duplicate
tbldeclaration (line 219 vs. line 228) and an unassignedtblInforeference inside the collate-resolution branch, since only the HEAD side (line 210) callsgetTblInfo.🔧 Suggested conflict resolution
-<<<<<<< HEAD tblInfo, err := getTblInfo(ctx, store, job) -======= + if err != nil { + return nil, nil, err + } // we don't touch table data during add-index, a fake Allocators is enough. defaultUseNewCollate := collate.NewCollationEnabled() failpoint.Inject("overrideDefaultUseNewCollateForBackfillStep", func(val failpoint.Value) { defaultUseNewCollate = val.(bool) }) useNewCollate := job.ReorgMeta.GetUseNewCollateOrDefault(defaultUseNewCollate) failpoint.InjectCall("afterResolveUserTableNewCollateForBackfillStep", job, defaultUseNewCollate, useNewCollate) tbl, err := tables.TableFromMetaWithCollate( useNewCollate, autoid.NewAllocators(tblInfo.SepAutoInc()), tblInfo, ) ->>>>>>> 09d214004a2 (*: use user keyspace's collation mode for DXF tasks (`#69677`)) if err != nil { return nil, nil, err } - tbl, err := getTable(d.ddlCtx.getAutoIDRequirement(), job.SchemaID, tblInfo) - if err != nil { - return nil, nil, err - } return store, tbl, 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/backfilling_dist_scheduler.go` around lines 209 - 231, Resolve the merge conflict in backfilling_dist_scheduler.go by removing all literal conflict markers and choosing a single coherent flow in the backfill setup around the getTblInfo/getTable/TableFromMetaWithCollate logic. Keep the initial tblInfo fetch from getTblInfo, then use that same tblInfo when resolving collation and constructing the table so there is only one tbl declaration and no uninitialized tblInfo reference; make sure the final code path compiles cleanly with one err check sequence.
🧹 Nitpick comments (1)
pkg/executor/importer/sampler.go (1)
338-342: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate table-construction logic with
table_import.go'snewEncodingTable.This block (allocator +
tables.TableFromMetaWithCollate+ annotated error) duplicatesnewEncodingTableintable_import.go, same package. Consider generalizingnewEncodingTableto accept atable.Table(instead of*LoadDataController) so both call sites share one implementation — reduces risk of the collation flag being wired inconsistently in the future.♻️ Proposed consolidation
-func newEncodingTable(e *LoadDataController) (table.Table, error) { - idAlloc := kv.NewPanickingAllocators(e.Table.Meta().SepAutoInc()) - tbl, err := tables.TableFromMetaWithCollate(e.Table.UseNewCollate(), idAlloc, e.Table.Meta()) - if err != nil { - return nil, errors.Annotatef(err, "failed to tables.TableFromMeta %s", e.Table.Meta().Name) - } - return tbl, nil -} +func newEncodingTable(srcTbl table.Table) (table.Table, error) { + idAlloc := kv.NewPanickingAllocators(srcTbl.Meta().SepAutoInc()) + tbl, err := tables.TableFromMetaWithCollate(srcTbl.UseNewCollate(), idAlloc, srcTbl.Meta()) + if err != nil { + return nil, errors.Annotatef(err, "failed to tables.TableFromMeta %s", srcTbl.Meta().Name) + } + return tbl, nil +}Then call
newEncodingTable(s.table)insampleOneFileandnewEncodingTable(e.Table)intable_import.go.🤖 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/importer/sampler.go` around lines 338 - 342, The table-construction block in sampleOneFile duplicates the logic already implemented by newEncodingTable, including allocator setup, TableFromMetaWithCollate, and error annotation. Refactor newEncodingTable in table_import.go to accept a table.Table instead of *LoadDataController, then update both sampleOneFile and the existing import path to call the shared helper so the collation flag and error handling stay consistent in one place.
🤖 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/export_test.go`:
- Line 36: The test helper using ConvertRowToHandleAndIndexDatum is deriving its
handle encoder from the process-global collation setting instead of the
passed-in copCtx. Update the helper to use the collation mode from copCtx (or
the table snapshot context it represents) when creating the encoder, and remove
any reliance on collate.NewCollationEnabled() so cross-keyspace tests reflect
the context under test. Also update the second related usage noted in the diff
to keep both helper paths consistent.
In `@pkg/executor/importer/BUILD.bazel`:
- Around line 120-124: Remove the unresolved merge-conflict markers from the
BUILD.bazel entry and keep only the intended shard_count value in the importer
target. Clean up the conflict block around the shard_count setting so
pkg/executor/importer/BUILD.bazel contains valid Bazel syntax with no literal
<<<<<<<, =======, or >>>>>>> markers.
In `@pkg/infoschema/BUILD.bazel`:
- Around line 52-56: Resolve the merge conflict in the BUILD.bazel deps list by
removing the literal conflict markers and keeping the intended dependency
entries. Update the infoschema BUILD target so the deps section is valid Bazel
syntax again, using the surrounding target definition and the package dependency
names in this block to locate and clean up the conflict.
---
Outside diff comments:
In `@pkg/ddl/backfilling_dist_scheduler.go`:
- Around line 209-231: Resolve the merge conflict in
backfilling_dist_scheduler.go by removing all literal conflict markers and
choosing a single coherent flow in the backfill setup around the
getTblInfo/getTable/TableFromMetaWithCollate logic. Keep the initial tblInfo
fetch from getTblInfo, then use that same tblInfo when resolving collation and
constructing the table so there is only one tbl declaration and no uninitialized
tblInfo reference; make sure the final code path compiles cleanly with one err
check sequence.
---
Nitpick comments:
In `@pkg/executor/importer/sampler.go`:
- Around line 338-342: The table-construction block in sampleOneFile duplicates
the logic already implemented by newEncodingTable, including allocator setup,
TableFromMetaWithCollate, and error annotation. Refactor newEncodingTable in
table_import.go to accept a table.Table instead of *LoadDataController, then
update both sampleOneFile and the existing import path to call the shared helper
so the collation flag and error handling stay consistent in one place.
🪄 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: 49d0c520-27a3-4c2f-af23-41e161b4fd24
📒 Files selected for processing (45)
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/BUILD.bazelpkg/dxf/importinto/conflictedkv/handler.gopkg/dxf/importinto/planner.gopkg/dxf/importinto/task_executor.gopkg/executor/admin.gopkg/executor/batch_checker.gopkg/executor/importer/BUILD.bazelpkg/executor/importer/chunk_process_testkit_test.gopkg/executor/importer/import.gopkg/executor/importer/import_test.gopkg/executor/importer/sampler.gopkg/executor/importer/table_import.gopkg/expression/expression.gopkg/infoschema/BUILD.bazelpkg/infoschema/tables.gopkg/lightning/backend/kv/base.gopkg/lightning/backend/kv/kv2sql.gopkg/lightning/backend/kv/sql2kv.gopkg/meta/model/job_test.gopkg/meta/model/reorg.gopkg/table/table.gopkg/table/tables/BUILD.bazelpkg/table/tables/mutation_checker_test.gopkg/table/tables/partition.gopkg/table/tables/tables.gopkg/table/tables/tables_test.gotests/realtikvtest/addindextest1/BUILD.bazeltests/realtikvtest/addindextest1/cross_ks_test.gotests/realtikvtest/configs/next-gen/pd.tomltests/realtikvtest/importintotest3/BUILD.bazeltests/realtikvtest/importintotest3/cross_ks_test.gotests/realtikvtest/testkit.go
💤 Files with no reviewable changes (1)
- pkg/ddl/job_scheduler.go
| "github.com/pingcap/tidb/pkg/table" | ||
| "github.com/pingcap/tidb/pkg/types" | ||
| "github.com/pingcap/tidb/pkg/util/chunk" | ||
| "github.com/pingcap/tidb/pkg/util/collate" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the cop context’s collation mode in this test helper.
ConvertRowToHandleAndIndexDatum receives copCtx, so deriving the handle encoder from collate.NewCollationEnabled() can make cross-keyspace tests use the process-global mode instead of the context/table snapshot being tested.
Proposed fix
- "github.com/pingcap/tidb/pkg/util/collate"
@@
- 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)Also applies to: 84-84
🤖 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` at line 36, The test helper using
ConvertRowToHandleAndIndexDatum is deriving its handle encoder from the
process-global collation setting instead of the passed-in copCtx. Update the
helper to use the collation mode from copCtx (or the table snapshot context it
represents) when creating the encoder, and remove any reliance on
collate.NewCollationEnabled() so cross-keyspace tests reflect the context under
test. Also update the second related usage noted in the diff to keep both helper
paths consistent.
| <<<<<<< HEAD | ||
| shard_count = 40, | ||
| ======= | ||
| shard_count = 42, | ||
| >>>>>>> 09d214004a2 (*: use user keyspace's collation mode for DXF tasks (#69677)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Unresolved merge-conflict markers break the BUILD file.
Lines 120-124 contain literal <<<<<<< HEAD / ======= / >>>>>>> conflict markers left in the file, which is invalid Bazel syntax and will fail parsing/build. This matches the PR comment noting the cherry-pick has merge conflicts and is on hold.
🔧 Proposed fix
-<<<<<<< HEAD
- shard_count = 40,
-=======
- shard_count = 42,
->>>>>>> 09d214004a2 (*: use user keyspace's collation mode for DXF tasks (`#69677`))
+ shard_count = 42,📝 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.
| <<<<<<< HEAD | |
| shard_count = 40, | |
| ======= | |
| shard_count = 42, | |
| >>>>>>> 09d214004a2 (*: use user keyspace's collation mode for DXF tasks (#69677)) | |
| shard_count = 42, |
🤖 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/importer/BUILD.bazel` around lines 120 - 124, Remove the
unresolved merge-conflict markers from the BUILD.bazel entry and keep only the
intended shard_count value in the importer target. Clean up the conflict block
around the shard_count setting so pkg/executor/importer/BUILD.bazel contains
valid Bazel syntax with no literal <<<<<<<, =======, or >>>>>>> markers.
| <<<<<<< HEAD | ||
| ======= | ||
| "//pkg/util/chunk", | ||
| "//pkg/util/collate", | ||
| >>>>>>> 09d214004a2 (*: use user keyspace's collation mode for DXF tasks (#69677)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Unresolved merge-conflict markers break the BUILD file.
Lines 52-56 contain literal <<<<<<< HEAD / ======= / >>>>>>> conflict markers, making this an invalid Bazel deps list. Same root cause as in pkg/executor/importer/BUILD.bazel — the cherry-pick conflicts noted in the PR thread need to be resolved before this can build.
🔧 Proposed fix
-<<<<<<< HEAD
-=======
- "//pkg/util/chunk",
- "//pkg/util/collate",
->>>>>>> 09d214004a2 (*: use user keyspace's collation mode for DXF tasks (`#69677`))
+ "//pkg/util/chunk",
+ "//pkg/util/collate",📝 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.
| <<<<<<< HEAD | |
| ======= | |
| "//pkg/util/chunk", | |
| "//pkg/util/collate", | |
| >>>>>>> 09d214004a2 (*: use user keyspace's collation mode for DXF tasks (#69677)) | |
| "//pkg/util/chunk", | |
| "//pkg/util/collate", |
🤖 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/infoschema/BUILD.bazel` around lines 52 - 56, Resolve the merge conflict
in the BUILD.bazel deps list by removing the literal conflict markers and
keeping the intended dependency entries. Update the infoschema BUILD target so
the deps section is valid Bazel syntax again, using the surrounding target
definition and the package dependency names in this block to locate and clean up
the conflict.
…ix/cherry-pick-69702-merge
|
Cherry-pick conflicts appear resolved; removing the |
ti-chi-bot
removed
the
do-not-merge/hold
|
/hold Let'me test again with this branch. |
ti-chi-bot
Bot
added
the
do-not-merge/hold
|
/test unit-test |
|
@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. |
|
/retest |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-nextgen-202603 #69702 +/- ##
===========================================================
Coverage ? 76.1669%
===========================================================
Files ? 1936
Lines ? 540677
Branches ? 0
===========================================================
Hits ? 411817
Misses ? 128860
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/unhold |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, GMHDBJD, joechenrh, 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 |
ti-chi-bot
Bot
merged commit f55c185
into
pingcap:release-nextgen-202603
5 participants
This is an automated cherry-pick of #69677
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
Summary by CodeRabbit
New Features
Bug Fixes
Tests