ddl: extract add-index KV generation helper#69706
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note-none
📝 WalkthroughWalkthroughAdds ChangesDDL index backfill KV generation
Estimated code review effort: 3 (Moderate) | ~25 minutes Multi-value index helper extraction
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested labels: 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69706 +/- ##
================================================
- Coverage 76.3227% 75.8452% -0.4775%
================================================
Files 2041 2078 +37
Lines 560399 582317 +21918
================================================
+ Hits 427712 441660 +13948
- Misses 131786 138096 +6310
- Partials 901 2561 +1660
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/table/tables/mutation_checker_test.go (1)
403-440: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the other branches of
TryGetHandleRestoredData.This test only exercises the "column needs restored data, no matching secondary-index column" path. The function has two other notable branches that remain untested: the
SetNullpath (whenNeedRestoredDataWithCollateis false for a PK column) and the bin-collation tail-space-count conversion path (ConvertDatumToTailSpaceCount), plus the truncation path whereidxactually shares a column withpkIdx(exercisingmaxIndexLen). Given this helper now has a wider blast radius (called directly from DDL index KV generation, not just through the wrapper), broader coverage would help catch regressions.🤖 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 403 - 440, Expand TestTryGetHandleRestoredData to cover the remaining TryGetHandleRestoredData branches: add cases where NeedRestoredDataWithCollate is false so the SetNull path is exercised, where a PK column uses bin collation so ConvertDatumToTailSpaceCount is covered, and where the secondary index shares a column with the PK index so the maxIndexLen truncation path runs. Use TryGetHandleRestoredData, FindPrimaryIndex, and the existing tableInfo/index setup as anchors when adding the new assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/ddl/index.go`:
- Around line 2742-2748: The handling in indexKVHandleForPhysicalTable should
always rebuild the kv.PartitionHandle for V1 global indexes using the current
physicalID instead of trusting an existing PartitionHandle. Update the logic in
indexKVHandleForPhysicalTable so that when the input handle is already a
kv.PartitionHandle, you unwrap its inner handle and create a new
kv.NewPartitionHandle with physicalID, ensuring reused or decoded handles cannot
preserve a stale partition ID.
In `@pkg/table/tables/tables.go`:
- Around line 1787-1815: TryGetHandleRestoredData is mutating the shared
restored-handle buffer that generateIndexKVsForRow reuses across indexes, so
later indexes in the same row can see truncated or null-marked values from
earlier ones. Fix this by avoiding in-place modification of the input slice:
either clone handleDts at the start of TryGetHandleRestoredData before calling
TryTruncateRestoredData and ConvertDatumToTailSpaceCount, or copy the buffer at
the generateIndexKVsForRow call site so each index works on its own
restored-handle data. Keep the behavior of needHandleRestoredData and the
returned filtered slice unchanged.
---
Nitpick comments:
In `@pkg/table/tables/mutation_checker_test.go`:
- Around line 403-440: Expand TestTryGetHandleRestoredData to cover the
remaining TryGetHandleRestoredData branches: add cases where
NeedRestoredDataWithCollate is false so the SetNull path is exercised, where a
PK column uses bin collation so ConvertDatumToTailSpaceCount is covered, and
where the secondary index shares a column with the PK index so the maxIndexLen
truncation path runs. Use TryGetHandleRestoredData, FindPrimaryIndex, and the
existing tableInfo/index setup as anchors when adding the new assertions.
🪄 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: 5cc96f35-39d5-49ec-879f-79f2094e0328
📒 Files selected for processing (8)
pkg/ddl/backfilling_operators.gopkg/ddl/index.gopkg/ddl/index_cop.gopkg/table/tables/BUILD.bazelpkg/table/tables/index.gopkg/table/tables/index_test.gopkg/table/tables/mutation_checker_test.gopkg/table/tables/tables.go
💤 Files with no reviewable changes (1)
- pkg/ddl/index_cop.go
ti-chi-bot
Bot
removed
do-not-merge/needs-linked-issue
do-not-merge/needs-triage-completed
labels
|
/retest-required |
1 similar comment
|
/retest-required |
expxiaoli
force-pushed
the
tikv_precheck_PR_helper
branch
from
0d2840b to
ded1ed2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/executor/admin.go (1)
407-414: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winHoist invariant lookups out of the per-row loop.
tblInfo,e.table.UseNewCollate(), andtables.FindPrimaryIndex(tblInfo)don't change across rows/chunks withinfetchRecoverRows, yet they're recomputed on every row iteration.FindPrimaryIndexscans the table's indices, so this is unnecessary repeated work in what can be a large batch-processing loop.♻️ Proposed refactor: compute once outside the loop
func (e *RecoverIndexExec) fetchRecoverRows(ctx context.Context, srcResult distsql.SelectResult, result *backfillResult) ([]recoverRows, error) { e.recoverRows = e.recoverRows[:0] idxValLen := len(e.index.Meta().Columns) result.scanRowCount = 0 + tblInfo := e.table.Meta() + useNewCollate := e.table.UseNewCollate() + pkIdx := tables.FindPrimaryIndex(tblInfo) for { ... e.idxValsBufs[result.scanRowCount] = idxVals - tblInfo := e.table.Meta() rsData := tables.TryGetHandleRestoredData( - e.table.UseNewCollate(), - tblInfo, - tables.FindPrimaryIndex(tblInfo), + useNewCollate, + tblInfo, + pkIdx, plannercore.GetCommonHandleDatum(e.handleCols, row), e.index.Meta(), )🤖 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/admin.go` around lines 407 - 414, Hoist the invariant table metadata lookups out of the per-row loop in fetchRecoverRows: tblInfo, e.table.UseNewCollate(), and tables.FindPrimaryIndex(tblInfo) are constant across iterations and should be computed once before processing rows/chunks. Use the existing fetchRecoverRows flow and the tblInfo / tables.TryGetHandleRestoredData call site to move those values into local variables outside the loop, then reuse them for each row.
🤖 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/executor/admin.go`:
- Around line 407-414: Hoist the invariant table metadata lookups out of the
per-row loop in fetchRecoverRows: tblInfo, e.table.UseNewCollate(), and
tables.FindPrimaryIndex(tblInfo) are constant across iterations and should be
computed once before processing rows/chunks. Use the existing fetchRecoverRows
flow and the tblInfo / tables.TryGetHandleRestoredData call site to move those
values into local variables outside the loop, then reuse them for each row.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5e197e82-7aca-4567-b149-c9fb5c70d3cc
📒 Files selected for processing (10)
pkg/ddl/backfilling_operators.gopkg/ddl/index.gopkg/ddl/index_cop.gopkg/ddl/reorg_util_test.gopkg/executor/admin.gopkg/table/tables/BUILD.bazelpkg/table/tables/index.gopkg/table/tables/index_test.gopkg/table/tables/mutation_checker_test.gopkg/table/tables/tables.go
💤 Files with no reviewable changes (1)
- pkg/ddl/index_cop.go
✅ Files skipped from review due to trivial changes (1)
- pkg/table/tables/BUILD.bazel
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/ddl/reorg_util_test.go
- pkg/table/tables/index_test.go
- pkg/table/tables/index.go
- pkg/ddl/index.go
- pkg/ddl/backfilling_operators.go
- pkg/table/tables/mutation_checker_test.go
- pkg/table/tables/tables.go
|
/retest-required |
| } | ||
|
|
||
| // TryGetHandleRestoredData returns restored common-handle data if needed. | ||
| func TryGetHandleRestoredData( |
There was a problem hiding this comment.
why ddl space precheck feature need to change this part? why not directly check the encoded KV size
There was a problem hiding this comment.
Directly checking encoded KV size will miss restored handle data for common handle and new collation indexes. Reusing the add-index KV helper keeps precheck and real backfill using the same encoding path.
There was a problem hiding this comment.
kv size already include them
There was a problem hiding this comment.
I check the following split PR, TryGetHandleRestoredData is not required for the KV helper extraction. I will remove this helper to keep this PR clean.
ti-chi-bot
Bot
removed
the
size/XL
|
[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 |
ti-chi-bot
Bot
added
the
size/L
2 participants
What problem does this PR solve?
Issue Number: ref #68354
Problem Summary:
This PR is split from #68490.
The add-index backfill path already has logic for generating index KVs, including multi-valued indexes, global indexes, partial indexes, and common-handle restored data. Future prediction/precheck logic needs to reuse the same behavior instead of reimplementing another index KV generation path.
What changed and how does it work?
This PR extracts helper logic for add-index KV generation with minimal behavior change:
generateIndexKVsForRowso follow-up TiKV precheck PRs can generate sampled index KVs through the same path as real add-index backfill.EncodeRawIndexKeyValuesso follow-up TiKV precheck PRs can encode raw index key suffixes in the same value order as index KV generation.Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
New Features
Bug Fixes
Tests