Repository navigation
Conversation
ti-chi-bot
Bot
added
release-note-none
|
Hi @joechenrh. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. 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. |
Codecov Report❌ Patch coverage is Please upload reports for the commit d414ef7 to get more accurate results. Additional details and impacted files@@ Coverage Diff @@
## master #60650 +/- ##
================================================
- Coverage 77.7732% 73.1749% -4.5983%
================================================
Files 2016 1828 -188
Lines 552852 498374 -54478
================================================
- Hits 429971 364685 -65286
+ Misses 121139 111727 -9412
- Partials 1742 21962 +20220
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
1 of 2 tasks
Signed-off-by: Ruihao Chen <joechenrh@gmail.com>
Signed-off-by: Ruihao Chen <joechenrh@gmail.com>
Signed-off-by: Ruihao Chen <joechenrh@gmail.com>
Contributor
Author
|
/retest |
|
@joechenrh: Cannot trigger testing until a trusted user reviews the PR and leaves an 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. |
Reorganize the spec so type handling (EvalString-based conversion) is part of the function design, not an afterthought. Key changes: - Math section includes non-array cols with concrete example - Function semantics explicitly specify EvalString path - Type safety table shows current vs new approach - Migration includes removing indexSupportFastCheck type restriction Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- NULL JSON: function returns CRC32(MD5(prefix)) matching index-side CONCAT_WS behavior - Dedup ordering: moved after cast (matching index dedup semantics) - Pseudocode: use expression.BuildCastFunction instead of non-existent Datum.EvalString - Target type: documented as function class metadata - COUNT(*) asymmetry: documented as benign - handleChecksum: noted behavioral change from mvCheckBuilder Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use Datum.ConvertTo(sc, fieldType) instead of expression framework's BuildCastFunction. Simpler implementation, correctness verified by per-type tests rather than code-path analysis alone. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
9 tasks: parser → expression → planner → unit tests → mvXorCheckBuilder → swap builders → per-type admin tests → delete old code → full verification Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move per-entry CRC32(MD5(CONCAT_WS(...))) computation into the subquery as an aliased column, then BIT_XOR it in the outer GROUP BY query. Previously the CRC referenced raw column names in the outer scope where they weren't accessible. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nd related dead code Remove the old CRC32-sum-based MV index check approach now that mvXorCheckBuilder (BIT_XOR + JSON_ARRAY_XOR_CRC32) is active. Deleted: - mvCheckBuilder struct and all its methods from check_table_index.go - JSONCRC32Mod const, jsonSumCRC32FunctionClass, builtinJSONSumCRC32Sig (scalar + vec eval), and BuildJSONSumCrc32FunctionWithCheck from pkg/expression - case *ast.JSONSumCrc32Expr handler from expression_rewriter.go - SafeToShareAcrossSession entry for builtinJSONSumCRC32Sig from the generated thread-safety file convertJSON2String is retained as it is still used by builtinJSONArrayXorCRC32Sig. Parser-layer JSONSumCrc32 definitions are left for a separate cleanup pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove all code replaced by JSON_ARRAY_XOR_CRC32 and mvXorCheckBuilder: - mvCheckBuilder struct and methods - jsonSumCRC32FunctionClass, builtinJSONSumCRC32Sig - BuildJSONSumCrc32FunctionWithCheck - JSONCRC32Mod constant - TestJSONSumCrc32 test - expression_rewriter case for JSONSumCrc32Expr Retained: convertJSON2String (still used by JSON_ARRAY_XOR_CRC32), parser-level definitions (separate module, cleaned up later). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…remnants Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
joechenrh
force-pushed
the
test_mv_index
branch
from
March 19, 2026 08:18
337a8f4 to
3cc177a
Compare
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request implements a builder-pattern abstraction for fast admin table/index validation, replacing MV-specific branching with index-type-agnostic execution. It refactors the checksum aggregation strategy from Changes
Sequence DiagramsequenceDiagram
participant Client as Test/Client
participant Exec as FastCheckTableExec
participant Pool as WorkerPool
participant Builder as indexCheckBuilder
participant DB as Database<br/>(Query Execution)
participant Report as consistency.Reporter
Client->>Exec: Next()
Exec->>Exec: Create worker pool + select builder<br/>(normalCheckBuilder or mvXorCheckBuilder)
Exec->>Pool: Start() pool
loop For each partition task
Pool->>Builder: handleChecksum()<br/>(compute checksum columns)
Builder-->>Pool: column expr
Pool->>DB: Execute buildChecksumQuery()<br/>(table/index BIT_XOR phase)
DB-->>Pool: grouped checksum results
Pool->>DB: Compare table vs index checksums
alt Checksum mismatch
Pool->>Builder: buildCheckRowQuery()<br/>(detail row query)
Builder-->>Pool: SQL with record detail
Pool->>DB: Execute detail query
DB-->>Pool: recordWithChecksum slice
Pool->>Report: Report mismatch<br/>via consistency.Reporter
Report-->>Client: Error details
end
end
Exec->>Pool: Release() pool
Exec-->>Client: OperatorErr() result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
|
@joechenrh: 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. |
joechenrh
marked this pull request as draft
March 19, 2026 08:22
ti-chi-bot
Bot
added
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/parser/parser.y (1)
8456-8478:⚠️ Potential issue | 🟠 MajorCapture
parser.explicitCharsetbefore parsing the prefix expression.Line 8468 snapshots a parser-global flag only after the trailing prefix expression has already reduced. The
JSONArrayXorCrc32production parsesExpressionat position $8 afterCastType "ARRAY", but the action code runs after $8 is fully parsed. Any nestedCAST/CONVERT/JSON_ARRAY_XOR_CRC32inside that prefix will resetparser.explicitCharsetto false before the outerJSONArrayXorCrc32Expr.ExplicitCharSetcaptures it. For example,JSON_ARRAY_XOR_CRC32(a AS CHAR CHARACTER SET latin1 ARRAY, CAST(b AS CHAR))will lose the outer explicit charset flag because the innerCASTresets it.Restructure to capture the flag immediately after
CastType "ARRAY"reduces, before the prefixExpressionis parsed—use a helper nonterminal or equivalent to snapshot the flag state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/parser/parser.y` around lines 8456 - 8478, The parser currently reads parser.explicitCharset only after the prefix Expression ($8) is parsed in the jsonArrayXorCrc32 production, which allows nested casts to clobber the flag; fix by snapshotting the flag immediately after CastType "ARRAY" reduces (before parsing the prefix Expression). Create a small helper nonterminal (e.g., ExplicitCharsetMarker or SnapshotExplicitCharset) right after CastType "ARRAY" that reduces immediately and stores parser.explicitCharset into a temporary value, then use that snapshot when building the JSONArrayXorCrc32Expr.ExplicitCharSet in the action for jsonArrayXorCrc32 so nested CAST/CONVERT/JSON_ARRAY_XOR_CRC32 cannot reset the captured flag. Ensure the production references the snapshot symbol (instead of reading parser.explicitCharset later) and clear parser.explicitCharset only where existing logic expects.pkg/executor/builder.go (1)
577-617:⚠️ Potential issue | 🔴 CriticalDon't drop the partial-index recovery path.
buildIdxColsConcatHandleColsnow expands to all table columns only when a generated column is present, andcontainsGenedColOrPartialIndexis populated from the same boolean. That removes partial indexes from the special recovery path entirely. A partial index without generated columns still needs predicate columns loaded and the executor flag set, otherwiseadmin recover indexcan rebuild entries for rows that should stay excluded.Also applies to: 632-643
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/builder.go` around lines 577 - 617, The function buildIdxColsConcatHandleCols uses the hasGenedCol boolean to decide to expand to all table columns, which inadvertently drops the partial-index recovery path; change the condition so it expands columns when a generated column is present OR the index is partial (use indexInfo's partial-index indicator instead of only hasGenedCol), e.g. treat (hasGenedCol || indexInfo.IsPartial) when deciding to set columns = tblInfo.Columns, and make the same change where the duplicate logic appears (the block referenced by containsGenedColOrPartialIndex); keep the rest of the logic for common/PK handle and the ExtraHandle column intact.
🧹 Nitpick comments (1)
pkg/planner/core/logical_plan_builder.go (1)
4353-4356: Fix comment typo and make intent explicitThe comment has a typo (“craete”) and can be tightened to clearly state this is an internal/admin-only metadata rewrite for planner compatibility.
📝 Suggested comment update
-// buildTableForAdminCheckSQL make a copy of table info if necessary. -// Since we want to utilize MVIndex to read the data in FAST ADMIN CHECK, to prevent from adding -// many hacky code in the planner module, we craete a new table info with all MVIndex-related meta -// removed. So optimizer will process it like a normal table. +// buildTableForAdminCheckSQL clones table metadata for FAST ADMIN CHECK when MV indexes exist. +// It strips MV-index-specific metadata so optimizer path-building can treat the table like a +// normal table in this internal admin-check flow.As per coding guidelines, comments SHOULD explain non-obvious intent, constraints, invariants, concurrency guarantees, SQL/compatibility contracts, or important performance trade-offs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/logical_plan_builder.go` around lines 4353 - 4356, Fix the comment above buildTableForAdminCheckSQL: correct the typo “craete” to “create” and tighten the wording to explicitly state that this function performs an internal/admin-only rewrite of table metadata by removing MVIndex-related metadata so the optimizer treats the resulting table as a normal table (no external behavior change). Keep the comment concise and focused on intent and rationale for the metadata rewrite to aid future maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/plans/2026-03-18-mv-index-fast-check-refactor.md`:
- Line 132: The docs contain author-local absolute paths like the command "Run:
`cd /mnt/data/joechenrh/pingcap/tidb && go build ./pkg/executor/...`" which are
not copy-pasteable; update these to use repository-root relative commands or an
explicit placeholder (e.g., replace the full path with either "cd <repo_root> &&
go build ./pkg/executor/..." or simply "go build ./pkg/executor/..." when run
from the repo root). Apply the same change to the other occurrences called out
(around the snippets at 223, 326, and 383-408) so all examples use repo-root
relative paths or <dir> placeholders instead of the hard-coded /mnt/data/...
author checkout.
In `@docs/superpowers/plans/2026-03-19-mv-index-fast-check-phase-c.md`:
- Around line 160-186: The code currently deduplicates array elements by the
uint32 CRC value (seen map keyed by crc) which can collapse distinct elements
that collide and cause diverging checksums; change deduplication to key on the
normalized element bytes (not the CRC): after you obtain the normalized
string/typed datum via convertJSONToTargetDatum and datum.ConvertTo
(strDatum.GetString) produce the canonical encoded index-key bytes (or the
normalized string) and use that as the seen map key (e.g., seen
map[string]struct{} keyed by the encoded bytes/str), then still compute the
CRC32(MD5(...)) for XOR into result; update references in the loop (seen,
concat/prefix/separator, md5.Sum, crc32.ChecksumIEEE) so dedup checks use the
encoded/normalized bytes before deciding to xor the crc.
In `@pkg/executor/builder.go`:
- Around line 3197-3198: The log/message incorrectly describes the adjusted
sample-rate formula: change the explanatory string in the branch that computes
sampleRate = math.Min(1, 150000/approxiCount) so it matches the actual formula
(use "150000" not "15000"); update the sprintf/return message that references
approxiCount and sampleRate (the same expression currently returning
fmt.Sprintf(... "15000/%v" ...)) to say "150000/%v" so the displayed note
matches the computed value.
- Around line 3224-3235: The code builds a local normalized cols slice when
hasPkHist(task.HandleCols) or when HandleCols is non-int, but AnalyzeColumnsExec
(and any ColReq/AnalyzeColumnsExec.colsInfo) still references the old
task.ColsInfo, leaving executor metadata out of sync; update the code that
creates AnalyzeColumnsExec/ColReq so that after computing cols (the local
variable), you assign task.ColsInfo = cols and pass cols into
AnalyzeColumnsExec.colsInfo (or set AnalyzeColumnsExec.colsInfo = cols) instead
of using the stale task.ColsInfo; do the same fix in the analogous block around
the AnalyzeColumnsExec creation at the other location (lines ~3261-3274) and
ensure references to task.TblInfo.Columns, hasPkHist, and task.HandleCols remain
consistent.
- Around line 513-543: The fast-check path erroneously includes partial indexes;
update indexSupportFastCheck to detect and return false for any index that has a
partial predicate (i.e., skip indexes with a partial filter) so they are
excluded from the fast admin check, and ensure buildCheckTable continues to rely
on indexSupportFastCheck when deciding to create FastCheckTableExec; add the
partial-index check by testing the index metadata (e.g., a
partial-expression/partial-filter flag or field on model.IndexInfo) and return
false if present.
In `@pkg/executor/check_table_index.go`:
- Around line 662-697: The initSessCtx function borrows a sys session via
w.e.BaseExecutor.GetSysSession() (se) but never releases it on error paths or
after use; update initSessCtx to always call se.Release() (or the appropriate
return-to-pool method) when returning early on GetSQLExecutor().ExecuteInternal
failures and in the returned cleanup closure after restoring session vars;
additionally treat failures to set tidb_snapshot as actionable by returning an
error instead of only logging so callers run against the intended snapshot.
Ensure references to se, GetSysSession, ExecuteInternal and the returned cleanup
closure are updated accordingly so the session is returned to the pool on every
path (including the snapshot set failure and any other early returns).
- Around line 980-994: The function queryToRow leaks the internal result set
when sqlexec.DrainRecordSet returns an error because rs.Close() is only called
on the successful path; update queryToRow to always close rs regardless of
DrainRecordSet outcome (use defer or explicit close in the error path) after
obtaining rs from se.GetSQLExecutor().ExecuteInternal, ensuring any error from
rs.Close() is logged via logutil.BgLogger().Warn; reference: function
queryToRow, variables rs and qCtx, and call sites sqlexec.DrainRecordSet(...)
and rs.Close().
- Around line 491-502: The code lowercases tblCol.GeneratedExprString into
generatedExpr which mutates string literals (e.g., JSON paths) and causes
mismatches; preserve the original casing by stopping the strings.ToLower call
and operate on the original GeneratedExprString when extracting/rewriting the
cast expression (used in ExtractCastArrayExpr, generatedExpr, inner,
tableArrayExpr, and tableFilterCol). Update the logic that trims "cast" and ")"
to work on the original-cased string (not a lowercased copy) and ensure any
comparisons that require case-insensitivity use explicit case-insensitive checks
rather than altering the expression text.
In `@pkg/executor/test/admintest/admin_test.go`:
- Line 1582: Tests mutate package-level tunables like
executor.CheckTableFastBucketSize (and other globals altered at the noted
locations); save the current value(s) at the start of the test, set the desired
test value(s), and register a t.Cleanup closure to restore the original value(s)
(use the same symbol names: executor.CheckTableFastBucketSize and any other
mutated executor.* globals referenced) so later tests remain order-independent
and deterministic.
- Around line 1059-1060: The code currently discards errors returned by
tables.NewIndex when assigning tk.uniqueIndex and tk.plainIndex; change the call
sites (the assignments that call tables.NewIndex for tbl.Meta().Indices[0] and
[1]) to capture the error returns immediately, check for non-nil errors, and
fail the test or return (e.g., t.Fatalf or require.NoError) with a clear message
referencing the index and table ID; do the same for the other similar
occurrences noted (around lines 1606 and 2370) so any constructor failure
surfaces immediately instead of causing later nil derefs.
In `@pkg/expression/builtin_json.go`:
- Around line 330-350: The deduplication must use the normalized cast string,
not the 32-bit CRC: change the seen map from map[uint32]struct{} to
map[string]struct{} (initialized with val.GetElemCount()), check presence with
seen[str] (where str is the normalized cast result from f(fakeSctx,
val.ArrayGetElem(i), ft)), only compute md5/hex/crc and XOR result with crc when
the string was not already seen, and store seen[str] = struct{}{}; apply the
same exact change in pkg/expression/builtin_json_vec.go so both scalar and
vector paths deduplicate on the casted string rather than the crc.
In `@pkg/planner/core/logical_plan_builder.go`:
- Around line 4586-4593: The admin-check MV-metadata rewrite
(buildTableForAdminCheckSQL) is applied before
tryLockMDLAndUpdateSchemaIfNecessary which can return a new table and drop the
MV flag; instead, either call buildTableForAdminCheckSQL after
tryLockMDLAndUpdateSchemaIfNecessary or detect when
tryLockMDLAndUpdateSchemaIfNecessary replaces tbl and reapply the MV-metadata
rewrite to the new table; update the logic around kv.GetInternalSourceType(ctx)
== kv.InternalTxnAdmin, tbl, buildTableForAdminCheckSQL and
tryLockMDLAndUpdateSchemaIfNecessary (using b.ctx, dbName, b.is) so the MV-flag
persists on the final tbl used for planning.
In `@pkg/server/tests/commontest/tidb_test.go`:
- Around line 3483-3492: The fixed 10ms sleep is racy; replace it with a polling
loop that repeatedly queries "show processlist" via dbt.MustQuery and counts
rows (using rs.Next()) until the killed session disappears (rsCnt == 1) or a
reasonable timeout elapses (e.g., a few seconds), then assert with
require.Equal; update the block inside ts.RunTests to poll with short sleeps
between retries rather than a single time.Sleep(10 * time.Millisecond) so the
test is deterministic on slow CI.
- Line 1180: The calls to topsql.SetupTopSQLForTest in tidb_test.go are
incorrect and must use the exported helper name topsql.SetupTopProfilingForTest;
update both occurrences (the call at the current spot and the one around line
1245) to call topsql.SetupTopProfilingForTest so the tests compile against
pkg/util/topsql/topsql.go.
In `@tests/integrationtest/t/expression/multi_valued_index.test`:
- Around line 527-545: Add an explicit "set tidb_enable_fast_table_check = 1;"
before the SQL block testing multi-valued index fast admin check so the tests
exercise the fast-path (mvXorCheckBuilder) deterministically; modify the
TestFastAdminCheck SQL snippet (the block creating int_table, combined_table,
multi_index_table and running admin check table statements) to include that SET
statement at the top so the fast table check is enabled for these cases.
---
Outside diff comments:
In `@pkg/executor/builder.go`:
- Around line 577-617: The function buildIdxColsConcatHandleCols uses the
hasGenedCol boolean to decide to expand to all table columns, which
inadvertently drops the partial-index recovery path; change the condition so it
expands columns when a generated column is present OR the index is partial (use
indexInfo's partial-index indicator instead of only hasGenedCol), e.g. treat
(hasGenedCol || indexInfo.IsPartial) when deciding to set columns =
tblInfo.Columns, and make the same change where the duplicate logic appears (the
block referenced by containsGenedColOrPartialIndex); keep the rest of the logic
for common/PK handle and the ExtraHandle column intact.
In `@pkg/parser/parser.y`:
- Around line 8456-8478: The parser currently reads parser.explicitCharset only
after the prefix Expression ($8) is parsed in the jsonArrayXorCrc32 production,
which allows nested casts to clobber the flag; fix by snapshotting the flag
immediately after CastType "ARRAY" reduces (before parsing the prefix
Expression). Create a small helper nonterminal (e.g., ExplicitCharsetMarker or
SnapshotExplicitCharset) right after CastType "ARRAY" that reduces immediately
and stores parser.explicitCharset into a temporary value, then use that snapshot
when building the JSONArrayXorCrc32Expr.ExplicitCharSet in the action for
jsonArrayXorCrc32 so nested CAST/CONVERT/JSON_ARRAY_XOR_CRC32 cannot reset the
captured flag. Ensure the production references the snapshot symbol (instead of
reading parser.explicitCharset later) and clear parser.explicitCharset only
where existing logic expects.
---
Nitpick comments:
In `@pkg/planner/core/logical_plan_builder.go`:
- Around line 4353-4356: Fix the comment above buildTableForAdminCheckSQL:
correct the typo “craete” to “create” and tighten the wording to explicitly
state that this function performs an internal/admin-only rewrite of table
metadata by removing MVIndex-related metadata so the optimizer treats the
resulting table as a normal table (no external behavior change). Keep the
comment concise and focused on intent and rationale for the metadata rewrite to
aid future maintainers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bcc84259-4089-4a04-bdd7-26e22b7daf2a
📒 Files selected for processing (29)
docs/superpowers/plans/2026-03-18-mv-index-fast-check-refactor.mddocs/superpowers/plans/2026-03-19-mv-index-fast-check-phase-c.mddocs/superpowers/specs/2026-03-18-mv-index-fast-check-phase-c-design.mddocs/superpowers/specs/2026-03-18-mv-index-fast-check-refactor-design.mdpkg/executor/BUILD.bazelpkg/executor/builder.gopkg/executor/check_table_index.gopkg/executor/select.gopkg/executor/test/admintest/BUILD.bazelpkg/executor/test/admintest/admin_test.gopkg/expression/bench_test.gopkg/expression/builtin_json.gopkg/expression/builtin_json_test.gopkg/expression/builtin_json_vec.gopkg/expression/builtin_json_vec_test.gopkg/expression/builtin_threadsafe_generated.gopkg/parser/ast/functions.gopkg/parser/misc.gopkg/parser/parser.gopkg/parser/parser.ypkg/parser/parser_test.gopkg/planner/core/expression_rewriter.gopkg/planner/core/logical_plan_builder.gopkg/server/tests/commontest/BUILD.bazelpkg/server/tests/commontest/tidb_test.gotests/integrationtest/r/expression/json.resulttests/integrationtest/r/expression/multi_valued_index.resulttests/integrationtest/t/expression/json.testtests/integrationtest/t/expression/multi_valued_index.test
💤 Files with no reviewable changes (8)
- pkg/executor/BUILD.bazel
- pkg/server/tests/commontest/BUILD.bazel
- tests/integrationtest/r/expression/json.result
- pkg/expression/builtin_threadsafe_generated.go
- pkg/executor/select.go
- pkg/expression/builtin_json_vec_test.go
- pkg/parser/parser_test.go
- pkg/expression/builtin_json_test.go
|
|
||
| - [ ] **Step 3: Verify compilation** | ||
|
|
||
| Run: `cd /mnt/data/joechenrh/pingcap/tidb && go build ./pkg/executor/...` |
There was a problem hiding this comment.
Use repo-root commands instead of an author-local checkout path.
Hard-coding /mnt/data/joechenrh/pingcap/tidb makes these steps non-copy-pasteable for anyone else. Keep the commands relative to the repository root, or use an explicit placeholder like <dir>.
As per coding guidelines, "Commands in docs SHOULD be copy-pasteable from repository root unless explicitly scoped; use explicit placeholders such as <package_name>, , and
."Also applies to: 223-223, 326-326, 383-408
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-03-18-mv-index-fast-check-refactor.md` at line
132, The docs contain author-local absolute paths like the command "Run: `cd
/mnt/data/joechenrh/pingcap/tidb && go build ./pkg/executor/...`" which are not
copy-pasteable; update these to use repository-root relative commands or an
explicit placeholder (e.g., replace the full path with either "cd <repo_root> &&
go build ./pkg/executor/..." or simply "go build ./pkg/executor/..." when run
from the repo root). Apply the same change to the other occurrences called out
(around the snippets at 223, 326, and 383-408) so all examples use repo-root
relative paths or <dir> placeholders instead of the hard-coded /mnt/data/...
author checkout.
Contributor
Author
There was a problem hiding this comment.
These are internal planning docs under docs/superpowers/ — they're not part of TiDB's user-facing documentation and exist only as context for the AI agent working session. They don't need to be copy-pasteable for other contributors.
There was a problem hiding this comment.
Rate Limit Exceeded
@joechenrh have exceeded the limit for the number of chat messages per hour. Please wait 0 minutes and 33 seconds before sending another message.
Contributor
Author
There was a problem hiding this comment.
These are internal planning docs under docs/superpowers/ — they're not part of TiDB's user-facing documentation and exist only as context for the AI agent working session. They don't need to be copy-pasteable for other contributors.
There was a problem hiding this comment.
@joechenrh, understood — thanks for the context! docs/superpowers/ files are internal AI agent planning artifacts and not part of TiDB's contributor-facing documentation, so the copy-pasteable command guideline doesn't apply to them. I'll remember this for future reviews.
🐰 noted and filed away!
✏️ Learnings added
Learnt from: joechenrh
Repo: pingcap/tidb PR: 60650
File: docs/superpowers/plans/2026-03-18-mv-index-fast-check-refactor.md:132-132
Timestamp: 2026-03-19T15:38:30.553Z
Learning: In pingcap/tidb, files under `docs/superpowers/` (e.g., `docs/superpowers/plans/`, `docs/superpowers/specs/`) are internal AI agent working session artifacts (planning docs, design specs for agentic workers). They are NOT part of TiDB's user-facing or contributor-facing documentation. The copy-pasteable command guideline (commands must be runnable from repo root or use explicit placeholders) does NOT apply to files under `docs/superpowers/`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: joechenrh
Repo: pingcap/tidb PR: 60650
File: pkg/planner/core/logical_plan_builder.go:0-0
Timestamp: 2026-03-19T15:22:12.351Z
Learning: Repo: pingcap/tidb
File: pkg/planner/core/logical_plan_builder.go (buildDataSource)
Learning: For internal admin transactions (kv.InternalTxnAdmin), apply buildTableForAdminCheckSQL after tryLockMDLAndUpdateSchemaIfNecessary(...) so the MV-index metadata rewrite is based on the refreshed schema and persists on the final table object used for planning.
Learnt from: AilinKid
Repo: pingcap/tidb PR: 66156
File: pkg/planner/core/task.go:249-254
Timestamp: 2026-02-27T06:57:20.797Z
Learning: In `pkg/planner/core/task.go` within `attach2Task4PhysicalHashJoin`, when propagating `IndexJoinInfo` from child tasks to the parent hash join task: by design, only one side (either lTask or rTask) should carry `IndexJoinInfo`. The code checks both sides and assigns whichever is present, but in practice only one child will have it set in valid index join embedding scenarios.
Learnt from: CR
Repo: pingcap/tidb PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-17T04:39:16.731Z
Learning: Applies to docs/**/*.md : Commands in docs SHOULD be copy-pasteable from repository root unless explicitly scoped; use explicit placeholders such as `<package_name>`, `<TestName>`, and `<dir>`
Learnt from: CR
Repo: pingcap/tidb PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-12T11:56:25.042Z
Learning: Applies to **/*.md : Commands in docs SHOULD be copy-pasteable from repository root unless explicitly scoped.
Learnt from: terry1purcell
Repo: pingcap/tidb PR: 66695
File: pkg/planner/cardinality/BUILD.bazel:62-66
Timestamp: 2026-03-13T01:48:14.212Z
Learning: In pingcap/tidb cherry-pick PRs, unresolved merge conflicts (e.g., conflict markers in BUILD.bazel or other files) are expected artifacts of the cherry-pick process and are already tracked by the ti-chi-bot hold mechanism. Do NOT flag merge conflicts as review issues — they are not a reason to block or comment on the code review.
Learnt from: CR
Repo: pingcap/tidb PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-17T04:39:16.731Z
Learning: Treat `docs/agents/ddl/*` as hypotheses until verified in code/tests; if implementation and documentation differ, MUST update the docs to match reality and call it out in the PR/issue
Learnt from: disksing
Repo: pingcap/tidb PR: 67030
File: DEPS.bzl:7743-7749
Timestamp: 2026-03-16T07:24:31.541Z
Learning: Repo: pingcap/tidb — Policy: DEPS.bzl is generated by `make bazel_prepare`; avoid hand-editing generated mirror URLs in feature PRs. Any mirror list adjustments should be made in the Bazel dependency generation flow, not directly in DEPS.bzl.
Learnt from: CR
Repo: pingcap/tidb PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-17T04:39:16.731Z
Learning: Follow `.agents/skills/tidb-test-guidelines` for placement, naming, `shard_count` guidance, planner-specific casetest rules, and related testdata conventions
Learnt from: CR
Repo: pingcap/tidb PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-12T17:03:05.283Z
Learning: Applies to pkg/ddl/** : If implementation and `docs/agents/ddl/*` differ, MUST update the docs to match reality and call it out in the PR/issue. Do not defer.
Learnt from: CR
Repo: pingcap/tidb PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-12T17:35:01.255Z
Learning: If `AGENTS.md` or docs under `docs/agents/` changed, follow the checklist in `docs/agents/agents-review-guide.md` before finishing.
Learnt from: CR
Repo: pingcap/tidb PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-12T17:03:05.283Z
Learning: Applies to **/*.go : Because TiDB is a complex system, code SHOULD remain maintainable for future readers with basic TiDB familiarity, including readers who are not experts in the specific subsystem/feature.
Learnt from: cryo-zd
Repo: pingcap/tidb PR: 65532
File: pkg/planner/core/preprocess.go:1906-1910
Timestamp: 2026-03-12T08:03:56.097Z
Learning: In pingcap/tidb (PR `#65532`), `buildSelect` in `pkg/planner/core/logical_plan_builder.go` derives the privilege-check DB for `FOR UPDATE OF` lock targets using `getLowerDB(tName.Schema, ...)`. For alias or unqualified lock targets whose `TableName.Schema` is empty after preprocessing, this falls back to `currentDB` and authorizes against the wrong database. The correct fix is to use `tNameW.DBInfo.Name.L` (with a nil guard) directly, since `tNameW` is already fully resolved by `checkLockClauseTables` in preprocess.go. This is consistent with how privilege checks for INSERT/DELETE use `tnW.DBInfo.Name.L` elsewhere in the codebase.
Learnt from: CR
Repo: pingcap/tidb PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-16T13:18:37.190Z
Learning: Follow `docs/agents/notes-guide.md` for notes formatting and conventions.
Learnt from: CR
Repo: pingcap/tidb PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-27T04:11:22.671Z
Learning: Applies to AGENTS.md : Document agent responsibilities, capabilities, and interactions in AGENTS.md
Learnt from: CR
Repo: pingcap/tidb PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-12T12:48:57.065Z
Learning: Applies to pkg/executor/** : For SQL behavior changes in executor, perform targeted unit test plus relevant integration test.
Learnt from: joechenrh
Repo: pingcap/tidb PR: 66878
File: tests/realtikvtest/importintotest/import_into_test.go:253-267
Timestamp: 2026-03-11T06:26:14.065Z
Learning: In pingcap/tidb, PR `#66878` (cherry-pick of `#58401` to release-7.5) also bundles the follow-up nil-check fix from PR `#63146`. `TestOnUpdateColumn` in `tests/realtikvtest/importintotest/import_into_test.go` was cherry-picked from PR `#63146` and tests that `IMPORT INTO` on a table with an `ON UPDATE CURRENT_TIMESTAMP` column does NOT panic due to a nil `onDup` map in `resolveGeneratedColumns`. It is NOT intended to cover the transitive generated-column / index-inconsistency fix from `#58401/`#58400.
- Restore builder.go to master's version (6365 lines) which was
incorrectly kept at old version (6034 lines) during merge resolution.
Re-apply only our MV-index changes: indexSupportFastCheck function
and buildCheckTable modification to use it.
- Restore select.go to master's version (CheckTableFastBucketSize
stays as atomic.Int64 in select.go; update check_table_index.go to
use .Load() and .Store() consistently).
- Restore pkg/server/tests/commontest/{tidb_test.go,BUILD.bazel} to
master; these files had no MV-index changes.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…index tests - Restore admin_test.go to master's version (2560 lines) to recover require.NoError checks after tables.NewIndex calls and unrelated tests that were lost during merge resolution. - Re-add our MV-index specific additions: - TestExtractCastArrayExpr: tests castArrayRegexp extraction logic - TestAdminCheckMVIndex: tests fast admin check correctness for MV indexes - TestAdminCheckMVIndexAllTypes: tests all supported CAST ARRAY types - Fix TestAdminCheckMVIndex: add require.NoError after tables.NewIndex call Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fix CRC dedup bug in JSON_ARRAY_XOR_CRC32: use normalized cast result string (str) as dedup key instead of crc32 hash, avoiding false mismatches from hash collisions. Fix applied to both scalar and vectorized implementations. - Remove strings.ToLower on generated column expressions: lowercasing could corrupt case-sensitive JSON path string literals (e.g. $.myField). TiDB already normalizes SQL keywords to lowercase when storing generated expressions, so the ToLower was both redundant for keywords and harmful for string literals. - Fix GetSysSession leak in initSessCtx: add ReleaseSysSession call to the cleanup closure so the system session is always returned to the pool. - Fix result-set leak in queryToRow: use defer rs.Close() so the record set is closed even when DrainRecordSet returns an error. - Add t.Cleanup in TestAdminCheckMVIndex to restore CheckTableFastBucketSize and LookupCheckThreshold after the test. - Move buildTableForAdminCheckSQL call to after tryLockMDLAndUpdateSchemaIfNecessary so the table copy is based on the latest schema version. - Exclude partial indexes from fast admin check: a partial index with a WHERE condition would produce false mismatches because the checksum query scans all rows regardless of the condition. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Set tidb_enable_fast_table_check = 1 before the TestFastAdminCheck block so the fast path (mvXorCheckBuilder) is exercised deterministically even if the session default changes in the future. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hawkingrei 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 |
Contributor
|
/ok-to-test |
# Conflicts: # pkg/parser/parser.go
2 of 13 tasks
This branch has not been deployed
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.
What problem does this PR solve?
Issue Number: close #60649
Problem Summary:
As title, this PR add support for fast multi-value index check.
The rationale can be found in the linked issue, let's just take a simple schema as example here:
To check the data-consistency for index
mvi, we need to read the below columns from table side and index side respectivelySELECT i1, i2, j FROM t USE INDEX()SELECT i1, i2, _V$_mvi_0 FROM t USE INDEX(mvi)Here
_V$_mvi_0is the hidden column generated by multi-valued index, and we use this column to scan the whole index.After retrieving the data, we can calculate the checksum and detect the inconsistent record.
What changed and how does it work?
The changes in this PR can be divided into three parts:
Support for Fast Admin Checks on MV Indexes:
This PR enhances support for multi-valued (MV) indexes in FAST ADMIN CHECK TABLE with some restricted column types.
We implement separate logic to generate checksum SQL for MV indexes. Unlike normal indexes that use
BIT_XORto aggregate per-row checksums, we simply useSUMfor MV indexes. It's necessary because calculating per-rowchecksum for multi-valued index is impractical.
Optimizer Adaptations:
Previously, the usage of multi-valued index is limited to index merge. To support reading data from MV index via SQL, we add some new logic:
IsMVIndexfor index andHiddenfor index columns) in table meta. So optimizer can treat this table as normal table.Adjustments to the
JSON_SUM_CRC32Function:To align index-side and table-side calculations, we modified the internal implementation of
JSON_SUM_CRC32:MODto calculated crc32 value to prevent overflowCheck 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
JSON_ARRAY_XOR_CRC32()function for JSON array CRC32 computation with XOR aggregation.Improvements
ADMIN CHECK TABLEoperations.Deprecations
JSON_SUM_CRC32()withJSON_ARRAY_XOR_CRC32().