codec, table: make new collation setting explicit in encoding#69566
Open
D3Hunter wants to merge 7 commits into
Open
codec, table: make new collation setting explicit in encoding#69566D3Hunter wants to merge 7 commits into
D3Hunter wants to merge 7 commits into
Conversation
use local new_collate settting remove dependency change change change change change
ti-chi-bot
Bot
added
release-note-none
|
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 collation-aware encoder state through row/index encoding and restored-data handling, updates table wiring and session bootstrap to store or load that state, and adjusts dependent callers, tests, and build targets. ChangesCollation-aware encoder plumbing
Estimated code review effort: 4 (Complex) | ~75 minutes Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
D3Hunter
commented
Jul 1, 2026
| } | ||
|
|
||
| // ColumnNeedRestoredData checks whether a single index column needs restored data. | ||
| func ColumnNeedRestoredData(idxCol *IndexColumn, colInfos []*ColumnInfo) bool { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
pkg/planner/core/expression_rewriter.go (1)
261-286: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReused pooled rewriter doesn't refresh
useNewCollate.New rewriters get
useNewCollatefromcollate.NewCollationEnabled()at creation (line 265), but the reuse branch (lines 271-285) resetsasScalar,preprocess, fold counters, stacks,ctx,err, andplanCtxfields — yet never touchesuseNewCollate. A pooled rewriter therefore keeps whatever collation-enabled value was true the first time that pool slot was allocated, even if the global setting changes afterward (e.g. in tests togglingcollate.SetNewCollationEnabledForTest, or across builder reuse). All three call sites that now gate oner.useNewCollate(SetCollationExprhandling,castCollationForIn,patternLikeOrIlikeToExpression) would silently use a stale flag instead of the previous behavior of reading the live global on every check.🐛 Proposed fix
rewriter = b.rewriterPool[b.rewriterCounter-1] rewriter.asScalar = false rewriter.preprocess = nil rewriter.disableFoldCounter = 0 rewriter.tryFoldCounter = 0 rewriter.ctxStack = rewriter.ctxStack[:0] rewriter.ctxNameStk = rewriter.ctxNameStk[:0] rewriter.ctx = ctx rewriter.err = nil + rewriter.useNewCollate = collate.NewCollationEnabled() rewriter.planCtx.plan = p🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/planner/core/expression_rewriter.go` around lines 261 - 286, The pooled expressionRewriter reuse path is leaving useNewCollate stale after the first allocation. Update the reuse branch in expressionRewriterBuilder.getRewriter to refresh useNewCollate from collate.NewCollationEnabled() just like the constructor path, so SetCollationExpr, castCollationForIn, and patternLikeOrIlikeToExpression always observe the current setting.pkg/tablecodec/tablecodec.go (1)
983-989: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPass the collation mode into
DecodeIndexKVEx
DecodeIndexKVExstill hard-codescollate.NewCollationEnabled()for clustered-index v1, whilepkg/executor/mem_reader.go:206has no way to supply the table’s collation mode. That keeps this decode path tied to global state and can mis-decode restored data when the encode mode differs.🤖 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/tablecodec/tablecodec.go` around lines 983 - 989, DecodeIndexKVEx currently hard-codes collate.NewCollationEnabled() for clustered-index v1, which ties decoding to global state. Update DecodeIndexKVEx and its call chain to accept the table’s collation mode as an argument, and pass that value through to decodeIndexKvForClusteredIndexVersion1 instead of constructing a new collation locally. Also update the caller in the mem reader path so it forwards the correct mode when decoding restored data.
🤖 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/table/tblctx/buffers.go`:
- Around line 26-27: EncodeBinlogRowData is re-reading
collate.NewCollationEnabled instead of using the caller’s encoder, which can
make binlog row bytes differ from WriteMemBufferEncoded. Update the
tblctx/buffers.go flow so WriteMemBufferEncoded passes its encoder/new-collation
state into EncodeBinlogRowData, and have EncodeBinlogRowData use that provided
encoder consistently rather than consulting global collation state. Keep the
encoding path explicit by threading the same encoder through the related helpers
and call sites.
---
Outside diff comments:
In `@pkg/planner/core/expression_rewriter.go`:
- Around line 261-286: The pooled expressionRewriter reuse path is leaving
useNewCollate stale after the first allocation. Update the reuse branch in
expressionRewriterBuilder.getRewriter to refresh useNewCollate from
collate.NewCollationEnabled() just like the constructor path, so
SetCollationExpr, castCollationForIn, and patternLikeOrIlikeToExpression always
observe the current setting.
In `@pkg/tablecodec/tablecodec.go`:
- Around line 983-989: DecodeIndexKVEx currently hard-codes
collate.NewCollationEnabled() for clustered-index v1, which ties decoding to
global state. Update DecodeIndexKVEx and its call chain to accept the table’s
collation mode as an argument, and pass that value through to
decodeIndexKvForClusteredIndexVersion1 instead of constructing a new collation
locally. Also update the caller in the mem reader path so it forwards the
correct mode when decoding restored data.
🪄 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: 0528e36f-41d7-421a-aed8-a935c543c9e0
📒 Files selected for processing (36)
pkg/ddl/column.gopkg/ddl/index.gopkg/executor/admin.gopkg/executor/test/executor/BUILD.bazelpkg/executor/test/executor/executor_test.gopkg/executor/write.gopkg/expression/expression.gopkg/meta/model/table.gopkg/planner/core/expression_rewriter.gopkg/server/handler/tests/BUILD.bazelpkg/server/handler/tests/http_handler_test.gopkg/store/mockstore/BUILD.bazelpkg/store/mockstore/cluster_test.gopkg/store/mockstore/unistore/cophandler/cop_handler_test.gopkg/table/tables/index.gopkg/table/tables/mutation_checker.gopkg/table/tables/mutation_checker_test.gopkg/table/tables/partition.gopkg/table/tables/tables.gopkg/table/tables/testutil/BUILD.bazelpkg/table/tables/testutil/indexcheck.gopkg/table/tblctx/BUILD.bazelpkg/table/tblctx/buffers.gopkg/table/tblctx/buffers_test.gopkg/tablecodec/tablecodec.gopkg/tablecodec/tablecodec_test.gopkg/types/etc.gopkg/util/codec/codec.gopkg/util/codec/codec_test.gopkg/util/codec/collation_test.gopkg/util/rowDecoder/BUILD.bazelpkg/util/rowDecoder/decoder_test.gopkg/util/rowcodec/bench_test.gopkg/util/rowcodec/rowcodec_test.gotests/realtikvtest/addindextest2/BUILD.bazeltests/realtikvtest/addindextest2/global_sort_test.go
💤 Files with no reviewable changes (1)
- pkg/meta/model/table.go
Comment on lines
+26
to
+27
| "github.com/pingcap/tidb/pkg/util/codec" | ||
| "github.com/pingcap/tidb/pkg/util/collate" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep binlog row encoding on the caller’s encoder.
EncodeBinlogRowData re-reads collate.NewCollationEnabled() instead of using the same encoder as WriteMemBufferEncoded, so binlog old-row bytes can diverge from table row encoding when the caller’s explicit collation mode differs from the current global. Pass the encoder in here as well. Based on PR objectives, this refactor is meant to make the new-collation setting explicit in encoding paths.
Proposed fix
"github.com/pingcap/tidb/pkg/util/codec"
- "github.com/pingcap/tidb/pkg/util/collate"
@@
-func (b *EncodeRowBuffer) EncodeBinlogRowData(loc *time.Location, ec errctx.Context) ([]byte, error) {
- enc := codec.NewEncoder(collate.NewCollationEnabled())
+func (b *EncodeRowBuffer) EncodeBinlogRowData(enc codec.Encoder, loc *time.Location, ec errctx.Context) ([]byte, error) {
value, err := tablecodec.EncodeOldRow(enc, loc, b.row, b.colIDs, nil, nil)Also applies to: 90-92
🤖 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/tblctx/buffers.go` around lines 26 - 27, EncodeBinlogRowData is
re-reading collate.NewCollationEnabled instead of using the caller’s encoder,
which can make binlog row bytes differ from WriteMemBufferEncoded. Update the
tblctx/buffers.go flow so WriteMemBufferEncoded passes its encoder/new-collation
state into EncodeBinlogRowData, and have EncodeBinlogRowData use that provided
encoder consistently rather than consulting global collation state. Keep the
encoding path explicit by threading the same encoder through the related helpers
and call sites.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69566 +/- ##
================================================
- Coverage 76.3268% 75.8775% -0.4493%
================================================
Files 2041 2078 +37
Lines 561003 583167 +22164
================================================
+ Hits 428196 442493 +14297
- Misses 131906 138302 +6396
- Partials 901 2372 +1471
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
D3Hunter
commented
Jul 1, 2026
| for _, idxCol := range idxCols { | ||
| if model.ColumnNeedRestoredData(idxCol, colInfos) { | ||
| col := colInfos[idxCol.Offset] | ||
| if types.NeedRestoredDataWithCollate(model.GetIdxChangingFieldType(idxCol, col), useNewCollate) { |
|
🔍 Starting code review for this PR... |
ingress-bot
reviewed
Jul 1, 2026
ingress-bot
left a comment
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: 9
- Inline comments: 9
- Summary-only findings (no inline anchor): 0
Findings (highest risk first)
⚠️ [Major] (2)
- Index/table encoder captures new-collation flag at construction, defeating the documented post-init sync.Once invariant (pkg/table/tables/index.go:196, pkg/table/tables/index.go:99, pkg/table/tables/index.go:186, pkg/table/tables/tables.go:252, pkg/table/tables/tables.go:795)
- No test pins encode and decode to the same collation source across the new snapshot-vs-live seam (pkg/util/codec/collation_test.go:47, pkg/table/tables/tables.go:252, pkg/tablecodec/tablecodec.go:997, pkg/table/tables/mutation_checker.go:257, pkg/table/tables/mutation_checker_test.go:253)
🟡 [Minor] (4)
- EncodeRowBuffer.EncodeBinlogRowData bypasses the injected encoder and reads the global collation state (pkg/table/tblctx/buffers.go:91, pkg/table/tblctx/buffers.go:53)
- DecodeIndexKVEx was not given a WithCollate variant; its v1 dispatch still reads the global collation state (pkg/tablecodec/tablecodec.go:988, pkg/tablecodec/tablecodec.go:1001)
DecodeIndexKVWithCollatedoc comment is mechanism narration — it does not explain the invariant this function exists to satisfy (pkg/tablecodec/tablecodec.go:1001)- Method forms of EncodeKey/EncodeValue/HashCode carry identical doc comments as their package-level counterparts, hiding the fixed-vs-dynamic collation distinction (pkg/util/codec/codec.go:325, pkg/util/codec/codec.go:338, pkg/util/codec/codec.go:1912)
🧹 [Nit] (3)
WithUseNewCollatedoc comment is a tautology — it adds no intent over the parameter name (pkg/expression/expression.go:106)- initTableCommonWithIndices remains a free function after initTableCommon was promoted to a method (pkg/table/tables/tables.go:311)
NeedRestoredDataWithCollatedoc comment is pure mechanism narration with no intent explanation (pkg/types/etc.go:144)
ingress-bot
reviewed
Jul 1, 2026
ingress-bot
left a comment
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: 13
- Inline comments: 12
- Summary-only findings (no inline anchor): 0
Findings (highest risk first)
🚨 [Blocker] (1)
- Collation flag frozen at table/index construction defeats the documented sync.Once deferral and can drift persisted index bytes (pkg/table/tables/index.go:196, pkg/table/tables/index.go:85, pkg/table/tables/tables.go:252, pkg/session/session.go:4408)
⚠️ [Major] (1)
- New per-Encoder collation tests never exercise the frozen-flag vs live-global divergence, which is the only behavior the refactor actually changes (pkg/util/codec/collation_test.go:47, pkg/util/codec/codec.go:233, pkg/util/collate/collate.go:141, pkg/table/tables/mutation_checker_test.go:253)
🟡 [Minor] (7)
- Stale
initNeedRestoreDatacomment contradicts new encoder-based collation capture (pkg/table/tables/index.go:196, pkg/table/tables/index.go:63) - EncodeBinlogRowData creates a fresh encoder instead of accepting one as a parameter (pkg/table/tblctx/buffers.go:91, pkg/table/tblctx/buffers.go:55)
- DecodeIndexKVEx version-1 path not migrated to explicit collation parameter (pkg/tablecodec/tablecodec.go:988, pkg/executor/mem_reader.go:206)
GenIndexKeydoc comment not updated to describe the newenc codec.Encoderparameter (pkg/tablecodec/tablecodec.go:1234)- GenIndexKey and GenIndexValuePortal have asymmetric first-parameter types, forcing callers to unwrap the encoder at every call site (pkg/tablecodec/tablecodec.go:1235, pkg/tablecodec/tablecodec.go:1641, pkg/table/tables/index.go:196, pkg/table/tables/mutation_checker_test.go:496)
(encoder Encoder) EncodeKeyand(encoder Encoder) EncodeValuedoc comments are verbatim copies of package-level functions, hiding the fixed-collation behavioral distinction (pkg/util/codec/codec.go:325, pkg/util/codec/codec.go:338)(encoder Encoder) HashCodeis placed onEncoderbutuseNewCollatehas no effect on its output (pkg/util/codec/codec.go:1912, pkg/util/codec/collation_test.go:1346)
🧹 [Nit] (4)
encoderfield onTableCommonlacks a comment explaining the fixed-at-init collation invariant (pkg/table/tables/tables.go:84)DecodeIndexKVWithCollatedoc comment is mechanical and omits when to prefer it overDecodeIndexKV(pkg/tablecodec/tablecodec.go:1001)TryGetCommonPkColumnRestoredIdscomment not updated to document the newuseNewCollateparameter (pkg/tablecodec/tablecodec.go:1650)NeedRestoredDataWithCollatecomment restates the signature without explaining when to prefer it overNeedRestoredData(pkg/types/etc.go:144)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/session/global_init.go`:
- Around line 59-65: The temporary bootstrap session created in
createSessionWithOpt should be marked as restricted before it reads mysql.tidb.
Update the global init flow around sess so it preserves the same contract as the
bootstrap path in session.go by setting InRestrictedSQL on the session before
calling sess.getTableValue for tidbSystemTZ, then keep the existing error
handling unchanged.
🪄 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: 98675bef-9e44-49e6-9b52-290b8249ab00
📒 Files selected for processing (3)
pkg/session/BUILD.bazelpkg/session/global_init.gopkg/session/session.go
Comment on lines
+59
to
+65
| sess, err := createSessionWithOpt(store, dom, dom.GetSchemaValidator(), dom.InfoCache(), nil) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // get system tz from mysql.tidb | ||
| tz, err := sess.getTableValue(ctx, mysql.TiDBTable, tidbSystemTZ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Mark the temporary bootstrap session as restricted before reading mysql.tidb.
Line 59 creates a new internal session, but the old inline load ran after bootstrap sessions were marked InRestrictedSQL in pkg/session/session.go Lines 4393-4395. Preserve that contract before Line 65 reads system-table values.
Proposed fix
sess, err := createSessionWithOpt(store, dom, dom.GetSchemaValidator(), dom.InfoCache(), nil)
if err != nil {
return err
}
+ sess.GetSessionVars().InRestrictedSQL = true
// get system tz from mysql.tidb
tz, err := sess.getTableValue(ctx, mysql.TiDBTable, tidbSystemTZ)📝 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
| sess, err := createSessionWithOpt(store, dom, dom.GetSchemaValidator(), dom.InfoCache(), nil) | |
| if err != nil { | |
| return err | |
| } | |
| // get system tz from mysql.tidb | |
| tz, err := sess.getTableValue(ctx, mysql.TiDBTable, tidbSystemTZ) | |
| sess, err := createSessionWithOpt(store, dom, dom.GetSchemaValidator(), dom.InfoCache(), nil) | |
| if err != nil { | |
| return err | |
| } | |
| sess.GetSessionVars().InRestrictedSQL = true | |
| // get system tz from mysql.tidb | |
| tz, err := sess.getTableValue(ctx, mysql.TiDBTable, tidbSystemTZ) |
🤖 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/session/global_init.go` around lines 59 - 65, The temporary bootstrap
session created in createSessionWithOpt should be marked as restricted before it
reads mysql.tidb. Update the global init flow around sess so it preserves the
same contract as the bootstrap path in session.go by setting InRestrictedSQL on
the session before calling sess.getTableValue for tidbSystemTZ, then keep the
existing error handling unchanged.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/table/tables/mutation_checker_test.go (1)
261-268: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate stale test-dimension comment.
The comment above (lines 178-186) enumerates the test's dimensions (location, unique/non-unique, clustered index, string collation) but doesn't mention the newly added
useNewCollateouter loop. Worth a one-line addition for future readers.🤖 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/mutation_checker_test.go` around lines 261 - 268, Update the stale test-dimension comment in mutation_checker_test to include the new useNewCollate dimension. Keep the comment near the test setup that iterates over collate.NewCollationEnabled/SetNewCollationEnabledForTest and mention that the matrix now covers location, unique/non-unique, clustered index, string collation, and the new collation toggle so the comment matches the nested loops.
🤖 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.
Nitpick comments:
In `@pkg/table/tables/mutation_checker_test.go`:
- Around line 261-268: Update the stale test-dimension comment in
mutation_checker_test to include the new useNewCollate dimension. Keep the
comment near the test setup that iterates over
collate.NewCollationEnabled/SetNewCollationEnabledForTest and mention that the
matrix now covers location, unique/non-unique, clustered index, string
collation, and the new collation toggle so the comment matches the nested loops.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: acda1002-9fc6-454c-af1e-34887615ea43
📒 Files selected for processing (5)
pkg/expression/expression.gopkg/table/tables/mutation_checker_test.gopkg/table/tables/tables.gopkg/types/etc.gopkg/util/codec/codec.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/expression/expression.go
- pkg/types/etc.go
- pkg/util/codec/codec.go
- pkg/table/tables/tables.go
Contributor
Author
|
/retest |
wjhuang2016
approved these changes
Jul 2, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: wjhuang2016 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 |
[LGTM Timeline notifier]Timeline:
|
joechenrh
reviewed
Jul 2, 2026
joechenrh
reviewed
Jul 2, 2026
D3Hunter
commented
Jul 4, 2026
D3Hunter
left a comment
D3Hunter
left a comment
Contributor
Author
There was a problem hiding this comment.
Summary
- Total findings: 8
- Inline comments: 8
- Summary-only findings (no inline anchor): 0
Findings (highest risk first)
⚠️ [Major] (4)
- Raw row reads can use a different common-handle restore rule than writes (
pkg/table/tables/tables.go:1542, pkg/table/tables/tables.go:1099, and pkg/table/tables/tables.go:1126) - UnionScan pending-index decode ignores the captured index collation mode (
pkg/executor/mem_reader.go:206 and pkg/tablecodec/tablecodec.go:988) - Pessimistic unchanged unique-key locks can be encoded with the wrong collation mode (
pkg/executor/write.go:413) - DDL index backfill restored data can mix global and captured collation state (
pkg/ddl/index.go:2481 and pkg/ddl/index.go:2735)
🟡 [Minor] (4)
- Encoder method docs overstate collation effects (
pkg/util/codec/codec.go:338 and pkg/util/codec/codec.go:1913) - New public collation-mode names use non-canonical
Collatewording (pkg/expression/expression.go:67, pkg/expression/expression.go:106, pkg/util/codec/codec.go:77, and pkg/tablecodec/tablecodec.go:1001) - Explicit-collation index constructor leaks a package-local abstraction (
pkg/table/tables/index.go:89 and pkg/table/tables/tables.go:270) - needRestoredData comment still describes the old global-collation dependency (
pkg/table/tables/index.go:63 and pkg/table/tables/index.go:196)
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
4 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.
What problem does this PR solve?
Issue Number: ref #69563
Problem Summary:
Some lower-level codec and tablecodec helper paths read the global new-collation setting directly when deciding how to encode values or whether restored data is needed. That makes deep utility behavior depend on process-wide state and makes the new-collation global harder to eliminate incrementally.
This PR is the first part of the refactor. It removes the implicit global dependency from the encoding paths touched here by threading an explicit collation setting through the relevant table, tablecodec, and codec helpers. Follow-up work should continue eliminating the remaining global variable usage.
in the first phase, we we will only refactor this part to make sure nextgen addindex/import-into can work in the case in the issue
What changed and how does it work?
codec.Encoder, which stores the new-collation setting used byEncodeKey,EncodeValue, andHashCode.codec.Encoderthrough row and index encoding helpers intablecodec,table/tables, andtable/tblctx.NeedRestoredDataWithCollatewhen the collation setting is already known.Check List
Tests
Unit tests:
./tools/check/failpoint-go-test.sh pkg/util/codec -run TestEncoderNewCollationEnabled -count=1./tools/check/failpoint-go-test.sh pkg/tablecodec -run 'Test(RowCodec|DecodeColumnValue|TimeCodec|CutRow|UniqueGlobalIndexKeyWithNullValues)$' -count=1./tools/check/failpoint-go-test.sh pkg/table/tables -run 'Test(CheckRowInsertionConsistency|CheckIndexKeysAndCheckHandleConsistency)$' -count=1pushd pkg/table/tblctx >/dev/null && go test -run 'Test(EncodeRow|EncodeBufferReserve)$' -tags=intest,deadlock -count=1 && popd >/dev/nullManual validation:
make bazel_preparegit diff --check master...HEADmake lintSide effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Bug Fixes
Chores