*: support descending-order indexes (#2519)#68049
Draft
takaidohigasi wants to merge 25 commits intopingcap:masterfrom
Draft
*: support descending-order indexes (#2519)#68049takaidohigasi wants to merge 25 commits intopingcap:masterfrom
takaidohigasi wants to merge 25 commits intopingcap:masterfrom
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
do-not-merge/needs-tests-checked
do-not-merge/work-in-progress
|
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:
📝 WalkthroughWalkthroughAdds persisted per-column DESC index metadata with versioning and gating, DESC-aware encoding/decoding and KV-range generation, planner scan-direction matching for mixed ASC/DESC, TiKV-version preflight for persisting DESC, servability checks that fail queries against unsupported index versions, and extensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Parser
participant DDLExecutor
participant PD
participant Storage
Client->>Parser: CREATE INDEX ... DESC
Parser->>DDLExecutor: submit DDL job
DDLExecutor->>DDLExecutor: check vardef.EnableDescendingIndex
alt Gate enabled
DDLExecutor->>PD: GetStores (bounded timeout)
PD-->>DDLExecutor: stores with versions
DDLExecutor->>DDLExecutor: parse/validate TiKV store semver >= MinTiKVVersionForDescIndex
alt All compatible
DDLExecutor->>Storage: Persist IndexInfo (Version=1, IndexColumn.Desc=true)
Storage-->>DDLExecutor: OK
DDLExecutor-->>Client: Success
else Incompatible
DDLExecutor-->>Client: Error (upgrade TiKV)
end
else Gate disabled
DDLExecutor->>Storage: Persist IndexInfo (Desc cleared, Version=0)
Storage-->>DDLExecutor: OK
DDLExecutor-->>Client: Success
end
sequenceDiagram
participant Client
participant Planner
participant PropMatcher
participant Encoder
participant TiKV
Client->>Planner: SELECT ... ORDER BY (mixed ASC/DESC)
Planner->>Planner: checkAllIndicesServable (IndexInfo.IsServable)
alt Unservable index
Planner-->>Client: Error (index version too new)
else Servable
Planner->>PropMatcher: matchProperty
PropMatcher-->>Planner: AccessPath + MatchedScanDesc (per-column decisions)
Planner->>Encoder: Build KV ranges (pass desc[] flags)
Encoder->>Encoder: EncodeIndexKeyWithDesc (invert bytes for DESC cols)
Encoder-->>TiKV: KV ranges
TiKV-->>Planner: Ordered rows
Planner-->>Client: Results
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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)
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 |
|
[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
contribution
|
Hi @takaidohigasi. Thanks for your PR. I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
|
Hi @takaidohigasi. 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. |
ti-chi-bot
Bot
added
release-note
takaidohigasi
force-pushed
the
feature/desc-index
branch
from
e35d2ed to
b1504e5
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This was referenced Apr 25, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/executor/admin.go (1)
821-829:⚠️ Potential issue | 🟠 MajorResume-key bootstrap can skip DESC index rows on first batch
After Line 821 switched to DESC-aware encoding, the initial sentinel key from
init()is no longer guaranteed to sort before all real index keys. UsingPrefixNext()at Line 829 on that sentinel can start too far ahead, soADMIN CLEANUP INDEXmay miss dangling entries for descending indexes.🔧 Suggested fix
diff --git a/pkg/executor/admin.go b/pkg/executor/admin.go @@ -import ( - "context" - "math" +import ( + "context" @@ - keyRanges.FirstPartitionRange()[0].StartKey = kv.Key(e.lastIdxKey).PrefixNext() + if len(e.lastIdxKey) > 0 { + keyRanges.FirstPartitionRange()[0].StartKey = kv.Key(e.lastIdxKey).PrefixNext() + } @@ func (e *CleanupIndexExec) init() error { e.idxChunk = chunk.New(e.getIdxColTypes(), e.InitCap(), e.MaxChunkSize()) e.idxValues = kv.NewHandleMap() e.batchKeys = make([]kv.Key, 0, e.batchSize) e.idxValsBufs = make([][]types.Datum, e.batchSize) - sc := e.Ctx().GetSessionVars().StmtCtx - idxKey, _, err := e.index.GenIndexKey(sc.ErrCtx(), sc.TimeZone(), []types.Datum{{}}, kv.IntHandle(math.MinInt64), nil) - if err != nil { - return err - } - e.lastIdxKey = idxKey + // First scan should start from full-range lower bound. + // Resume key is populated after scanning real rows. + e.lastIdxKey = nil return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/admin.go` around lines 821 - 829, The resume sentinel advancement using PrefixNext() can skip DESC-encoded index rows; update the code that sets keyRanges.FirstPartitionRange()[0].StartKey (currently using kv.Key(e.lastIdxKey).PrefixNext()) to not call PrefixNext and instead use the raw sentinel key so it sorts at-or-before real index entries (i.e., set StartKey = kv.Key(e.lastIdxKey) or otherwise compute a resume start key that is encoding-aware for DESC columns). Modify the assignment in admin.go (the block using distsql.IndexRangesToKVRangesWithDesc, keyRanges.SetToNonPartitioned, and keyRanges.FirstPartitionRange()) to remove PrefixNext and ensure the resume key logic respects DESC index encoding.
🧹 Nitpick comments (2)
pkg/executor/executor_pkg_test.go (1)
67-67: Add one DESC-path unit case forbuildKvRangesForIndexJoin.All updated call sites in this file pass
desc=nil, so this test file currently validates only the ASC-compatible path. A focused case with at least onedesc=trueflag would better guard the new descending-index range-build path.Also applies to: 97-97, 119-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/executor_pkg_test.go` at line 67, Current tests only exercise the ascending path because all calls to buildKvRangesForIndexJoin pass desc=nil; add at least one unit test invocation that supplies a non-nil desc slice with at least one true to exercise the descending-index path. Locate the test cases calling buildKvRangesForIndexJoin in executor_pkg_test.go (the calls shown around the existing lines) and add a parallel case that reuses the same ctx, joinKeyRows, indexRanges, keyOff2IdxOff but passes desc like []bool{false, true} (or matching index column count) and asserts the returned kvRanges and err behave as expected for DESC. Ensure you update all similar call sites in that file so one covers DESC semantics while keeping existing ASC cases intact.pkg/ddl/index.go (1)
444-449: Version fence wiring is correct; minor future-proofing note.Gating
idxInfo.Version = 1onHasDescColumn()correctly couples the metadata version to the feature actually being persisted (sinceDescis already filtered throughdescEnabledinbuildIndexColumns), so v0 binaries cannot resurface unknown-format indexes via theIsServablefence. Good.Optional: if a future change introduces another reason to bump
IndexInfo.Version(e.g., a new on-disk format), an unconditional= 1here will silently downgrade it. A defensiveif idxInfo.Version < 1 { idxInfo.Version = 1 }(or a small helper) would make it safe under composition. Not blocking — there is no other writer today.♻️ Optional defensive write
- if idxInfo.HasDescColumn() { - idxInfo.Version = 1 - } + if idxInfo.HasDescColumn() && idxInfo.Version < 1 { + idxInfo.Version = 1 + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ddl/index.go` around lines 444 - 449, The current code unconditionally sets idxInfo.Version = 1 when idxInfo.HasDescColumn() is true; change this to only raise the version if it's lower than 1 to avoid silently downgrading future higher versions — i.e., replace the assignment with a conditional check such as if idxInfo.Version < 1 { idxInfo.Version = 1 } within the same HasDescColumn() branch (refer to idxInfo.Version, idxInfo.HasDescColumn(), and buildIndexColumns to locate the logic).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/ddl/executor.go`:
- Around line 1059-1069: The DESC-index TiKV compatibility check currently lives
only in CreateTable (iterating tbInfo.Indices and calling
checkTiKVSupportsDescIndex), which lets CreateTableWithInfo and
BatchCreateTableWithInfo bypass the gate; move the check into the shared
createTableWithInfoJob path so every code path that persists a table runs it: in
createTableWithInfoJob, iterate over the passed tbInfo.Indices and call
checkTiKVSupportsDescIndex(ctx) (returning the error if present) before
persisting the table; remove the duplicate loop from CreateTable to avoid
redundant checks.
- Around line 5028-5038: The code currently skips stores with empty or
unparsable s.Version (using semver.NewVersion and logging via
logutil.DDLLogger().Warn), which lets CREATE INDEX proceed unsafely; change the
logic in the DESC-index support check so that an empty s.Version or a parseErr
from semver.NewVersion is treated as a blocking error (do not continue), e.g.,
return a descriptive error (or set the overall check to fail) with context
including store ID and version and the parseErr, instead of log-and-continue;
ensure the error surfaces up from the function that performs the DESC-index
support check so index creation is prevented when any store version is unknown
or unparsable.
In `@pkg/distsql/request_builder.go`:
- Around line 705-723: The wrapper helpers IndexRangesToKVRanges,
IndexRangesToKVRangesWithDesc, and IndexRangesToKVRangesWithInterruptSignal
currently pass desc=nil which causes DESC indexes to be encoded as ascending;
update these wrappers so when desc==nil they fetch the real per-column desc
flags from the index metadata (model.IndexInfo) for the given tid/idxID and pass
that slice into IndexRangesToKVRangesWithDescAndInterruptSignal (or
alternatively change the wrappers to return an error/compile-time safeguard to
prevent use without real metadata); also update internal call sites like
appendRanges() and BuildTableRanges() to stop calling the nil-desc helpers (or
ensure they propagate IndexInfo-derived desc) so DESC indexes produce correct
byte ranges.
In `@pkg/executor/builder.go`:
- Around line 4137-4138: The assumption that common-handle PK columns are always
ascending is unsafe when EnableDescendingIndex may set IndexColumn.Desc; either
explicitly reject DESC on clustered PKs or implement DESC-aware encoding for
common-handle paths. Fix options: (A) add validation in CreatePrimaryKey (or
buildIndexColumns) to error if IndexColumn.Desc is true for a clustered primary
key when the table uses common handles (check EnableDescendingIndex and TiKV
support) and update the comment at the buildKvRangesForIndexJoin callsites; or
(B) implement DESC handling by updating buildKvRangesForIndexJoin and the
distsql.CommonHandleRangesToKVRanges code to encode/decode common-handle ranges
with per-column DESC bits, and then adjust callers (including places noted in
find_best_task.go) and the comment to reflect proper DESC support. Ensure the
chosen fix updates the comment at the referenced buildKvRangesForIndexJoin
callsites to accurately state whether DESC is unsupported or now handled.
In `@pkg/planner/core/find_best_task.go`:
- Around line 1175-1186: The code assumes appended CommonHandle PK suffix is
always ascending by initializing thisIdxDesc to false when colIdx >=
len(path.Index.Columns); instead, determine the direction from the table's
primary/index metadata: when colIdx >= len(path.Index.Columns) look up the
corresponding primary key column's Desc flag from the primary index (e.g., via
the plan/path's primary index or table primary key columns) and set thisIdxDesc
accordingly so thisScanDesc is computed from the actual PK column direction
rather than hard-coded ASC; keep the existing matchedScanDesc/matchedScanDescSet
logic and return property.PropNotMatched as before when directions conflict.
In `@pkg/planner/core/planbuilder.go`:
- Around line 1347-1353: The servability check on the snapshot index
(index.IsServable() / index.UnservableErr(tblName.O)) is being run too early for
RC/forUpdateRead planning and must be performed after latest-index
reconciliation; update the logic in planbuilder.go so you first call the
reconciliation routine (e.g., invoke GetLatestIndexInfo() / the latest-index
reconciliation path used in forUpdateRead) to refresh or resolve the index, then
run index.IsServable() and return index.UnservableErr(...) only after that
reconciliation; make the same change for the other occurrence later in the file
(the block around the 1361–1370 equivalent).
In `@pkg/planner/util/path.go`:
- Around line 135-147: AccessPath.Clone currently omits the new MatchedScanDesc
field so cloned AccessPath instances lose the reverse-scan flag; update
AccessPath.Clone to copy MatchedScanDesc from the receiver into the new clone
(same as other metadata fields) so that GetOriginalPhysicalIndexScan sees the
correct value and preserves IndexScan.Desc across cloning (e.g., in index-merge
and plan rebuild paths); after the change add a regression test that plans the
same statement twice (or via plan cache/apply) for an index that requires a
reverse scan and asserts the resulting IndexScan.Desc remains true.
In `@pkg/table/tables/mutation_checker.go`:
- Around line 275-285: The code unconditionally bitwise-complements v for DESC
columns even when v was reconstructed by DecodeIndexKV (restored data),
corrupting restored DESC values; change the logic so the ^0xFF undo only runs
when the bytes actually came from the key (i.e., not when NeedRestoredData(...)
is true). Concretely, around the existing block that checks
indexInfo.Columns[i].Desc and modifies v, guard the complement with a check
using NeedRestoredData(indexInfo, i) (or propagate a "fromKey" flag from
DecodeIndexKV) so that v is only complemented for key-resident bytes before
calling DecodeColumnValue.
In `@pkg/tablecodec/tablecodec_test.go`:
- Line 440: Remove the unused local constant prefixLen in tablecodec_test.go
(the declaration "const prefixLen = 11") or alternatively use it where intended;
specifically delete the prefixLen declaration so the Go test package compiles
(or reference prefixLen in the surrounding test logic such as in functions/Test
helpers that compute the prefix length) and ensure no other code expects this
identifier.
In `@pkg/util/codec/codec.go`:
- Around line 1623-1650: The DESC decode branch currently allocates a new tmp
slice for every variable-length column (see the cases for ^bytesFlag,
^compactBytesFlag, ^decimalFlag) and Decoder.DecodeOne clones descending columns
into inv and peek() clones the remaining key, causing per-row heap churn; change
this to reuse a scratch buffer (e.g., a []byte scratch on Decoder or a small
sync.Pool) and/or add flag-specific fast paths that avoid full copying: modify
Decoder.DecodeOne to reuse the decoder's scratch when preparing descending
complements instead of allocating inv/tmp, and add overloads or helper variants
for peekBytes/peekCompactBytes/types.DecimalPeak that accept a scratch buffer to
write the complemented bytes in-place (or operate on complemented views) so the
complementing work does not allocate per call.
---
Outside diff comments:
In `@pkg/executor/admin.go`:
- Around line 821-829: The resume sentinel advancement using PrefixNext() can
skip DESC-encoded index rows; update the code that sets
keyRanges.FirstPartitionRange()[0].StartKey (currently using
kv.Key(e.lastIdxKey).PrefixNext()) to not call PrefixNext and instead use the
raw sentinel key so it sorts at-or-before real index entries (i.e., set StartKey
= kv.Key(e.lastIdxKey) or otherwise compute a resume start key that is
encoding-aware for DESC columns). Modify the assignment in admin.go (the block
using distsql.IndexRangesToKVRangesWithDesc, keyRanges.SetToNonPartitioned, and
keyRanges.FirstPartitionRange()) to remove PrefixNext and ensure the resume key
logic respects DESC index encoding.
---
Nitpick comments:
In `@pkg/ddl/index.go`:
- Around line 444-449: The current code unconditionally sets idxInfo.Version = 1
when idxInfo.HasDescColumn() is true; change this to only raise the version if
it's lower than 1 to avoid silently downgrading future higher versions — i.e.,
replace the assignment with a conditional check such as if idxInfo.Version < 1 {
idxInfo.Version = 1 } within the same HasDescColumn() branch (refer to
idxInfo.Version, idxInfo.HasDescColumn(), and buildIndexColumns to locate the
logic).
In `@pkg/executor/executor_pkg_test.go`:
- Line 67: Current tests only exercise the ascending path because all calls to
buildKvRangesForIndexJoin pass desc=nil; add at least one unit test invocation
that supplies a non-nil desc slice with at least one true to exercise the
descending-index path. Locate the test cases calling buildKvRangesForIndexJoin
in executor_pkg_test.go (the calls shown around the existing lines) and add a
parallel case that reuses the same ctx, joinKeyRows, indexRanges, keyOff2IdxOff
but passes desc like []bool{false, true} (or matching index column count) and
asserts the returned kvRanges and err behave as expected for DESC. Ensure you
update all similar call sites in that file so one covers DESC semantics while
keeping existing ASC cases intact.
🪄 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: e0598d5b-06f5-452d-91a4-9f39acc9e790
📒 Files selected for processing (25)
pkg/ddl/db_integration_test.gopkg/ddl/desc_index_version_check_test.gopkg/ddl/executor.gopkg/ddl/index.gopkg/distsql/request_builder.gopkg/executor/admin.gopkg/executor/builder.gopkg/executor/distsql.gopkg/executor/executor_pkg_test.gopkg/executor/index_merge_reader.gopkg/executor/infoschema_reader.gopkg/executor/show.gopkg/meta/model/index.gopkg/meta/model/index_test.gopkg/planner/core/find_best_task.gopkg/planner/core/operator/physicalop/physical_index_scan.gopkg/planner/core/planbuilder.gopkg/planner/util/path.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/sysvar.gopkg/table/tables/mutation_checker.gopkg/tablecodec/tablecodec.gopkg/tablecodec/tablecodec_test.gopkg/util/codec/codec.gopkg/util/codec/codec_test.go
…cap#2519) Add IndexColumn.Desc, IndexInfo.Version, and a tidb_enable_descending_index sysvar (default OFF) so the DESC keyword in CREATE INDEX round-trips through metadata, SHOW CREATE TABLE, and INFORMATION_SCHEMA.STATISTICS. Encoding and planner behavior are unchanged in this phase; queries continue to use reverse coprocessor scans for ORDER BY DESC, so results stay correct regardless of the gate. IndexInfo.Version bumps to 1 whenever any column is DESC. An explicit IsServable() check lets future TiDB versions refuse to serve queries against indexes whose metadata format they do not understand, once physical encoding lands in a later phase. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
pingcap#2519) matchProperty now records the uniform stored direction of the matched index columns on path.MatchedIdxDesc and rejects paths whose matched columns mix directions (a single forward/reverse cop scan cannot satisfy them). The scan-direction setters for PhysicalIndexScan and the index variant of BatchPointGet XOR the required order with that direction, so a DESC-stored index satisfies ORDER BY DESC with a forward scan and satisfies ORDER BY ASC with a reverse scan. No behavior change when tidb_enable_descending_index is off (the default) because no column ever has Desc=true in that state. Correctness of the new forward-scan path depends on the physical-encoding work in a later phase; until then the feature gate should remain off in production. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
…pingcap#2519) Add per-column descending-order encoding helpers that bitwise-complement the memcomparable bytes of DESC columns so a forward byte-order scan yields the user-declared direction. The helpers reuse the existing encode()/DecodeOne() dispatch by complementing around them, keeping the new surface area small and supporting every type the ascending path supports (int, uint, float, bytes/string, null, decimal, duration, time). This commit adds only the codec primitives — no callers are wired yet, so behaviour is unchanged. Subsequent phases thread per-column Desc flags through tablecodec (index key writes) and distsql (range bounds). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
…3b of pingcap#2519) GenIndexKey now takes the IndexColumn.Desc flag into account when writing index keys: columns with Desc=true have their memcomparable bytes bitwise-complemented so a forward RocksDB iterator yields the declared direction. All-ascending indexes (the overwhelmingly common case) stay on the original EncodeKey fast path, producing byte-identical output to before — crucial for reading legacy data unchanged. Read-side range-bound encoding in distsql.EncodeIndexKey still produces ascending-encoded bounds, so enabling tidb_enable_descending_index and writing to a DESC index with this commit alone will hide the rows from subsequent queries. The read-side bound construction, including the low↔high swap needed for DESC-differentiating columns, lands in phase 3c. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
… 3c of pingcap#2519) Thread per-column descending-order flags from IndexInfo into the byte-range builders used by IndexReader, IndexLookUp, IndexMerge, admin check, and the index-join inner builder. The encoder complements the stored bytes and, if the resulting byte low/high pair is inverted (which happens when a differentiating column is DESC), swaps them and their exclude flags so the range handed to TiKV stays well-ordered. Supporting pieces: * codec.peek() learns the descending flag bytes so codec.CutOne and every walker layered on it (DecodeIndexHandle, CutIndexKeyNew, ...) advances past DESC-encoded columns without schema context. * mutation_checker's consistency check un-complements a column's bytes before handing them to DecodeColumnValue, and uses DecodeKeyHead instead of DecodeIndexKey where only the index id is needed. * indexColDescFlags() materialises an IndexInfo.Columns[i].Desc slice on demand, returning nil for all-ascending indexes so callers stay on the original allocation-free fast path. INSERT against a DESC index now passes through end-to-end; SELECT served through the unistore/TiKV coprocessor emulation still fails because that layer has yet to learn DESC decoding. The coordinated TiKV work is phase 3d — at which point the integration test can be extended to cover SELECT. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
…ngcap#2519) Teach Decoder.DecodeOne to recognise the bitwise-complemented leading flag of a descending-order column and transparently invert the column's bytes before dispatching through the ascending switch. The unistore coprocessor emulator calls into this decoder when streaming index columns back to TiDB, so this change is the last piece needed to exercise INSERT + SELECT against a DESC index end-to-end through MockStore. Auto-detection (rather than a threaded per-column desc flag) avoids touching the tipb protobuf for now: the flag byte itself disambiguates ascending from descending, and the sole theoretical collision (^floatFlag == maxFlag == 0xFA) never occurs on stored rows because maxFlag is exclusively a range-sentinel marker that this decoder never receives. The real TiKV coprocessor (Rust) still needs the same treatment for production use; that lands in a follow-up commit to pingcap/tikv. The DDL integration test now covers point lookups (both present and missing keys), a forward-ordered DESC scan that satisfies ORDER BY DESC without a reverse flag, and the reverse-scan path used to satisfy ORDER BY ASC on a DESC-stored index. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
The fence was added to IndexInfo's metadata in phase 1 but no caller
invoked it, leaving an old TiDB binary that read schema written by a
newer one free to silently drop the unknown Version field and serve
queries against an index it doesn't understand. Wire the check in:
* getPossibleAccessPaths returns an error when an enumerated index
has Version > maxKnownIndexVersion. SELECT/UPDATE/DELETE planning
runs through here, so they fail fast with a clear message.
* buildInsert calls a new checkAllIndicesServable helper. INSERT
VALUES never enumerates access paths so it needs an explicit guard
before the executor touches any index-maintenance path.
The error message names the index, the table, the version mismatch,
and the remediation (upgrade TiDB or DROP INDEX) so an operator can
act without grepping source.
Tests cover both the model-level helper and an end-to-end forge: an
in-memory IndexInfo with Version=99 must be rejected by SELECT and
INSERT alike.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
When CREATE INDEX or CREATE TABLE would persist a column with Desc=true, walk the cluster's PD-reported store list and refuse to proceed if any non-TiFlash, non-tombstone TiKV store is below MinTiKVVersionForDescIndex. This is the third defence in the rolling-upgrade plan from phase 1: the sysvar gate prevents accidental enablement, IndexInfo.Version protects an older TiDB from serving an unfamiliar index, and now this version gate prevents writing a DESC-encoded index against TiKV that cannot decode it. The DDL paths covered: * CreateIndex / createIndex: secondary index DDL. * CreatePrimaryKey: standalone PK DDL. * CreateTable: KEY (col DESC) inside CREATE TABLE statements. Mock stores (which never satisfy tikv.Storage) skip the gate so the existing MockStore-based integration tests continue to exercise DESC indexes via the unistore auto-detection path. The pure version-comparison core (checkStoresMeetDescIndexMinVersion) is factored into its own helper and unit-tested across seven scenarios: old/new TiKV mix, TiFlash exclusion, tombstone handling, missing or malformed version strings, and a malformed minVersion constant. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
Drop the AllSameOrder gate at the start of matchProperty and replace the
per-column "is the index direction uniform?" check with a per-column
"does forward or reverse scan satisfy this column?" check. Concretely,
for each (sortItem, idxCol) pair we compute
thisScanDesc = sortItem.Desc != idxCol.Desc
and require all matched pairs to agree. matchProperty now records the
unified decision on path.MatchedScanDesc (renamed from MatchedIdxDesc),
so the consumer can set IndexScan.Desc directly without the previous
XOR-with-the-query-direction step.
This unlocks the MySQL 8.0 flagship case: INDEX(a ASC, b DESC) now
satisfies "ORDER BY a ASC, b DESC" with a forward cop scan, and
"ORDER BY a DESC, b ASC" with a reverse cop scan. Uniform requests
against an all-ascending index continue to work because the per-column
check collapses to a single boolean (all matched pairs share the same
sortItem.Desc, all share idxCol.Desc=false, so thisScanDesc is uniform).
Tests:
* TestDescendingIndexScanDirection: explicitly asserts that
INDEX(a, b DESC) ORDER BY a, b DESC keeps order without a Sort
and is NOT marked as a reverse scan; the inverted ORDER BY uses
the reverse-scan path; "ORDER BY a, b" still falls back to Sort.
* TestMixedDirectionCompositeIndex: end-to-end INSERT + SELECT,
asserting actual row order for both forward and reverse cases.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/executor/admin.go (1)
858-870:⚠️ Potential issue | 🟠 MajorADMIN CLEANUP INDEX silently fails on DESC-leading secondary indexes due to incorrect range initialization.
When the index has a DESC leading column,
init()seedse.lastIdxKeywithGenIndexKey([NULL], MinInt64), which applies DESC-aware encoding: NULL complements to0xFFand sorts at the end of the index keyspace, not the start. The cleanup loop then overwritesStartKeywithlastIdxKey.PrefixNext()(line 829), stepping past the entire index range on the first iteration. The scan returns no results, and no dangling entries are found or deleted—making the operation silently ineffective.
IndexRangesToKVRangesWithDesccorrectly computes the full range given the DESC flags, but line 829 defeats it by overwriting the start boundary. Either seedlastIdxKeywith the actual lower-bound key (e.g., the index prefix itself soPrefixNextlands on the first physical key), or on the first iteration use theStartKeyfrom the range and only overwrite withlastIdxKey.PrefixNext()on subsequent iterations.Existing tests do not cover
ADMIN CLEANUP INDEXon DESC indexes, which is why this went undetected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/admin.go` around lines 858 - 870, The cleanup loop seeds e.lastIdxKey in CleanupIndexExec.init using GenIndexKey with a NULL and MinInt64 which yields a DESC-aware upper-most key; change the initialization so the scanner starts at the true lower bound: either (preferred) set e.lastIdxKey to the index prefix lower-bound (i.e., the raw index prefix key for the index columns) so that calling PrefixNext advances to the first physical key, or modify the cleanup loop that uses StartKey/lastIdxKey (the code that sets StartKey = lastIdxKey.PrefixNext()) to use the computed range.StartKey from IndexRangesToKVRangesWithDesc on the first iteration and only switch to lastIdxKey.PrefixNext() on subsequent iterations; update CleanupIndexExec.init (and the loop that reads lastIdxKey/StartKey) accordingly to ensure DESC-leading columns do not cause the start to skip the entire range.
🧹 Nitpick comments (6)
pkg/ddl/create_table.go (1)
1340-1351: Use a structureddbterrorcode instead oferrors.Errorffor the DESC-on-clustered-PK rejection.Other DDL rejections in this file (e.g.,
dbterror.ErrPKIndexCantBeInvisible,dbterror.ErrUnsupportedClusteredSecondaryKey,dbterror.ErrGeneralUnsupportedDDL.GenWithStackByArgs(...)) flow throughdbterror, which carries a MySQL-compatible error code and terror class. A bareerrors.Errorfhere means clients thatterror.IsCode-match (or look at MySQL error code) won't recognize this case, and the warning surface in tests/SHOW WARNINGS will look inconsistent.♻️ Suggested refactor
- for _, key := range constr.Keys { - if key.Desc { - return nil, errors.Errorf( - "DESC is not supported on the columns of a clustered PRIMARY KEY; either drop the DESC keyword or declare the primary key as NONCLUSTERED") - } - } + for _, key := range constr.Keys { + if key.Desc { + return nil, dbterror.ErrGeneralUnsupportedDDL.GenWithStackByArgs( + "DESC on the columns of a clustered PRIMARY KEY; either drop the DESC keyword or declare the primary key as NONCLUSTERED") + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ddl/create_table.go` around lines 1340 - 1351, The rejection for DESC on clustered PRIMARY KEY currently returns a plain errors.Errorf which lacks a MySQL-compatible dbterror code; replace that call inside the loop over constr.Keys (the block checking key.Desc) with a dbterror-based error—e.g. use dbterror.ErrGeneralUnsupportedDDL.GenWithStackByArgs("DESC is not supported on the columns of a clustered PRIMARY KEY; either drop the DESC keyword or declare the primary key as NONCLUSTERED") or, if a more specific dbterror constant exists, use that constant's GenWithStackByArgs variant so the error carries the proper code/class and integrates with terror.IsCode and SHOW WARNINGS.pkg/executor/builder.go (1)
4137-4138: Make the ASC-only common-handle invariant point to the DDL guard.These
desc=nilcall sites are correct, but the comment would be easier to trust if it named the upstream contract explicitly: clustered PRIMARY KEY columns are rejected withDESCat DDL time, so common-handle range building stays ASC-only.✏️ Suggested comment tweak
- // Common handle PK is always ascending (no per-column DESC), so desc=nil. + // Clustered PRIMARY KEY columns cannot be DESC (rejected at DDL time), + // so common-handle lookups always use ascending encoding and pass desc=nil.Based on learnings, clustered PRIMARY KEY
DESCis explicitly rejected inpkg/ddl/create_table.goandpkg/ddl/index.go.Also applies to: 4146-4146, 5115-5116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/builder.go` around lines 4137 - 4138, Update the inline comment that currently states "Common handle PK is always ascending (no per-column DESC), so desc=nil." to explicitly reference the upstream DDL contract that enforces this: mention that clustered PRIMARY KEY columns with DESC are rejected at DDL time (see pkg/ddl/create_table.go and pkg/ddl/index.go), and indicate why the call to buildKvRangesForIndexJoin(..., desc=nil, ...) is therefore safe; update the other similar sites (around the buildKvRangesForIndexJoin call at the noted locations and lines ~4146 and ~5115) to the same explicit DDL-guard wording so readers can trust the ASC-only invariant.pkg/distsql/request_builder.go (1)
859-928: DESC encoding + swap logic looks correct, with one assumption worth restating in code.Walked through the swap/exclude/PrefixNext interaction for single-column DESC,
(asc, desc), and(desc, asc)composite cases — the half-open byte ranges produced match the semantic ranges in every case I tried. The doc comment calls out the right limitation:This one-shot swap is correct for single-column DESC ranges and for composite ranges whose differentiating column is DESC.
This is correct under the ranger invariant that within a single
*ranger.Range, all prefix columns are equality (LowVal[i] == HighVal[i]) and at most the trailing column has a non-equality bound. If two columns ever differ inLowVal/HighValof the sameRange, the byte-vs-semantic ordering can diverge in mixed-direction indexes and the one-shot swap would be insufficient. That invariant holds in current TiDB ranger output, but it's a quiet contract — consider adding a one-line note ("relies on ranger producing one-differing-column ranges") so a future planner change doesn't silently break this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/distsql/request_builder.go` around lines 859 - 928, The doc comment for EncodeIndexKeyWithDesc should explicitly state it relies on the ranger.Range invariant that all prefix columns are equal (LowVal[i] == HighVal[i]) and at most one trailing column can differ; update the comment above EncodeIndexKeyWithDesc to add a single sentence referencing this contract (e.g., "Relies on ranger producing ranges where only one column may differ between LowVal and HighVal") and mention ranger.Range/LowVal/HighVal so future readers know the swap logic assumes that invariant.pkg/meta/model/index_test.go (1)
94-100: Comment doesn't quite describe what's tested.The comment promises "unknown fields from a newer TiDB must decode cleanly on an older binary," but the test below just round-trips the known
Descfield via the same struct on the same binary — it doesn't simulate decoding JSON with truly unknown extra fields. Either retitle the comment to match what's tested (round-trip ofDescsurvives marshal/unmarshal, including viaClone), or extend the test to actually unmarshal a JSON blob containing an unrecognized key alongside"desc":trueand assert no error.📝 Comment-only clarification
- // Unknown fields from a newer TiDB must decode cleanly on an older binary. + // Round-trip: a marshalled DESC column unmarshals back with Desc=true, + // and Clone preserves the flag. var decoded IndexColumn require.NoError(t, json.Unmarshal(descJSON, &decoded)) require.True(t, decoded.Desc)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/meta/model/index_test.go` around lines 94 - 100, The current comment is misleading because the test only round-trips the known Desc field; change the test to actually simulate unknown fields by creating a JSON blob that contains "desc": true plus an extra unknown key (e.g., a new_field) and then json.Unmarshal that blob into the IndexColumn (using descJSON or a new variable like jsonWithExtra) and assert require.NoError(t, ...) and require.True(t, decoded.Desc); also keep the existing assertion that descCol.Clone().Desc is preserved, and update the comment to describe that the test verifies unmarshalling JSON with unknown fields into IndexColumn succeeds and that Clone preserves Desc.pkg/util/codec/codec.go (2)
345-369:EncodeKeyWithDescdefeatsencode's batchpreReallocby encoding one datum per call.
encode()opens withpreRealloc(b, vals, comparable1)to sizebfor the entire batch. Calling it per element (encode(loc, b, v[i:i+1], true)inside the loop) reduces that to a per-datum capacity check and may incur extrareallocBytescalls when encoding a wide composite key.A simple alternative: encode the full batch once, then walk it with
peekto find each column's byte range and complement only the DESC ones in place. This keeps a single capacity calculation and avoids per-element overhead. Not a correctness issue and not on the per-row decode hot path, so leaving as a recommended follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/util/codec/codec.go` around lines 345 - 369, EncodeKeyWithDesc currently calls encode per-datum which defeats encode's batch preRealloc; instead, call encode once for the full v slice (encode(loc, b, v, true)), then iterate through the encoded bytes using peek to locate each datum's byte range and apply the bytewise complement only for columns marked DESC in desc; keep the same comparable flag and behavior for types that disallow DESC (the existing check for types.KindMysqlJSON and types.KindVectorFloat32), and ensure you preserve the original start/stop boundaries discovered by peek so the in-place complement operates on the correct ranges.
406-425:CutOneWithDesccan callpeek(b)directly — no complement copy needed.Now that
peek()understands the DESC-complemented flags (lines 1631-1658) for both fixed- and variable-length types, cutting a DESC column doesn't need any scratch at all:peek(b)on the original DESC bytes already returns the encoded length. The current implementation allocateslen(b)bytes (the entire remaining key, not just this column) and complements them only to fall back into the ASC peek arm. The comment on lines 413–415 ("take the minimum slice needed") also doesn't match themake([]byte, len(b))that follows.This mirrors what
Decoder.DecodeOnealready does (peek first, then complement onlylengthbytes intodescBuf).♻️ Proposed simplification
func CutOneWithDesc(b []byte, desc bool) (data []byte, remain []byte, err error) { if !desc { return CutOne(b) } if len(b) < 1 { return nil, nil, errors.New("invalid encoded key") } - // peek() in cmpl-space gives the encoded length. We only need one byte of - // scratch (the flag) plus up to the encoded length for variable-length - // types, so take the minimum slice needed. - cp := make([]byte, len(b)) - for i := range b { - cp[i] = b[i] ^ 0xFF - } - l, err := peek(cp) + // peek understands DESC-complemented flags directly (see the ^*Flag arms + // in peek), so no scratch buffer is needed to cut past a DESC column. + l, err := peek(b) if err != nil { return nil, nil, errors.Trace(err) } return b[:l], b[l:], nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/util/codec/codec.go` around lines 406 - 425, CutOneWithDesc currently allocates and complements an entire copy of b before calling peek; instead call peek directly on the original DESC bytes (i.e. replace the cp allocation/complement loop with l, err := peek(b)) because peek already understands DESC-complemented flags for fixed and variable types, then keep the existing error handling and return b[:l], b[l:], nil; this mirrors Decoder.DecodeOne which only complements into a small descBuf when needed and avoids the unnecessary full-buffer allocation in CutOneWithDesc.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/ddl/executor.go`:
- Around line 1110-1123: The TiKV-descending-index compatibility loop (iterating
tbInfo.Indices, calling idx.HasDescColumn() and checkTiKVSupportsDescIndex(ctx))
runs before the "IF NOT EXISTS" / OnExistIgnore fast-path, causing CREATE TABLE
IF NOT EXISTS to fail on old/unhealthy TiKV even when the table already exists;
move or delay this loop/check so it executes only after you determine the
OnExistIgnore short-circuit won't return (i.e., after the existing-table fast
path in the CreateTable/CreateTableWithInfo/BatchCreateTableWithInfo flow),
ensuring that when OnExistIgnore is true and the table is present you return
early without calling checkTiKVSupportsDescIndex.
- Around line 5009-5017: Replace the unbounded context used in
pdCli.GetAllStores (currently calling GetAllStores(context.Background())) with a
cancellable/timeout-bound context so PD lookups can't hang indefinitely; create
a context with a short timeout (or derive from the caller/session context) and
pass that to pdCli.GetAllStores, ensure you cancel the context after use, and
keep the subsequent call to checkStoresMeetDescIndexMinVersion(stores,
MinTiKVVersionForDescIndex, intest.InTest) unchanged.
In `@pkg/ddl/index.go`:
- Around line 136-140: The current check unconditionally rejects descending
order for columnar indexes (if ip.Desc && columnarIndexType !=
model.ColumnarIndexTypeNA) which breaks the compatibility path when descending
indexes are globally disabled; update the condition to also require the
descending-index feature flag (e.g., descEnabled) so that the rejection only
happens when DESC is enabled: change the guard to something like if descEnabled
&& ip.Desc && columnarIndexType != model.ColumnarIndexTypeNA and keep the
existing error construction via
dbterror.ErrUnsupportedAddColumnarIndex.FastGenByArgs(columnarIndexType.SQLName())
so VECTOR/INVERTED index clauses remain no-ops when descEnabled is false.
- Line 121: The code currently recomputes descEnabled using
vardef.EnableDescendingIndex.Load() inside buildIndexColumns (and the similar
block at lines ~184-191), causing the DESC/ASC decision to depend on global
state at execution time; instead use the persisted decision stored in the DDL
job/index metadata (e.g., the IndexPart or IndexInfo field like ip.Desc or the
flag set by checkAndBuildIndexInfo) so the create-time choice is honored.
Replace references to vardef.EnableDescendingIndex.Load() in buildIndexColumns
and the other block with the already-stored DESC boolean passed into or read
from the IndexPart/IndexInfo, and if necessary change buildIndexColumns'
signature or its call sites (including checkAndBuildIndexInfo) to accept that
persisted desc flag rather than re-reading the global gate.
In `@pkg/executor/index_merge_reader.go`:
- Around line 194-195: In Open(), don't ignore the error returned by
distsql.IndexRangesToKVRangesWithDesc: capture its error return, and if non-nil
return it (with contextual message referencing the table/index and the range)
instead of proceeding to assign e.partitionKeyRanges; update the call site
around IndexRangesToKVRangesWithDesc (and references to indexColDescFlags,
e.ranges, and e.partitionKeyRanges) to check the error and propagate it up from
Open() so range-encoding failures are surfaced.
In `@pkg/planner/core/planbuilder.go`:
- Around line 4103-4110: The current servability check (checkAllIndicesServable)
is only applied for INSERT/REPLACE but must also be enforced for LOAD DATA and
IMPORT INTO because buildLoadData and buildImportInto can mutate secondary
indexes without using getPossibleAccessPaths; add calls to
checkAllIndicesServable(tableInfo) at the start of buildLoadData and
buildImportInto (or factor it into the shared planner entry used by those flows)
and return the error if non-nil so older TiDB binaries will refuse operations
that would write indexes with unsupported metadata.
---
Outside diff comments:
In `@pkg/executor/admin.go`:
- Around line 858-870: The cleanup loop seeds e.lastIdxKey in
CleanupIndexExec.init using GenIndexKey with a NULL and MinInt64 which yields a
DESC-aware upper-most key; change the initialization so the scanner starts at
the true lower bound: either (preferred) set e.lastIdxKey to the index prefix
lower-bound (i.e., the raw index prefix key for the index columns) so that
calling PrefixNext advances to the first physical key, or modify the cleanup
loop that uses StartKey/lastIdxKey (the code that sets StartKey =
lastIdxKey.PrefixNext()) to use the computed range.StartKey from
IndexRangesToKVRangesWithDesc on the first iteration and only switch to
lastIdxKey.PrefixNext() on subsequent iterations; update CleanupIndexExec.init
(and the loop that reads lastIdxKey/StartKey) accordingly to ensure DESC-leading
columns do not cause the start to skip the entire range.
---
Nitpick comments:
In `@pkg/ddl/create_table.go`:
- Around line 1340-1351: The rejection for DESC on clustered PRIMARY KEY
currently returns a plain errors.Errorf which lacks a MySQL-compatible dbterror
code; replace that call inside the loop over constr.Keys (the block checking
key.Desc) with a dbterror-based error—e.g. use
dbterror.ErrGeneralUnsupportedDDL.GenWithStackByArgs("DESC is not supported on
the columns of a clustered PRIMARY KEY; either drop the DESC keyword or declare
the primary key as NONCLUSTERED") or, if a more specific dbterror constant
exists, use that constant's GenWithStackByArgs variant so the error carries the
proper code/class and integrates with terror.IsCode and SHOW WARNINGS.
In `@pkg/distsql/request_builder.go`:
- Around line 859-928: The doc comment for EncodeIndexKeyWithDesc should
explicitly state it relies on the ranger.Range invariant that all prefix columns
are equal (LowVal[i] == HighVal[i]) and at most one trailing column can differ;
update the comment above EncodeIndexKeyWithDesc to add a single sentence
referencing this contract (e.g., "Relies on ranger producing ranges where only
one column may differ between LowVal and HighVal") and mention
ranger.Range/LowVal/HighVal so future readers know the swap logic assumes that
invariant.
In `@pkg/executor/builder.go`:
- Around line 4137-4138: Update the inline comment that currently states "Common
handle PK is always ascending (no per-column DESC), so desc=nil." to explicitly
reference the upstream DDL contract that enforces this: mention that clustered
PRIMARY KEY columns with DESC are rejected at DDL time (see
pkg/ddl/create_table.go and pkg/ddl/index.go), and indicate why the call to
buildKvRangesForIndexJoin(..., desc=nil, ...) is therefore safe; update the
other similar sites (around the buildKvRangesForIndexJoin call at the noted
locations and lines ~4146 and ~5115) to the same explicit DDL-guard wording so
readers can trust the ASC-only invariant.
In `@pkg/meta/model/index_test.go`:
- Around line 94-100: The current comment is misleading because the test only
round-trips the known Desc field; change the test to actually simulate unknown
fields by creating a JSON blob that contains "desc": true plus an extra unknown
key (e.g., a new_field) and then json.Unmarshal that blob into the IndexColumn
(using descJSON or a new variable like jsonWithExtra) and assert
require.NoError(t, ...) and require.True(t, decoded.Desc); also keep the
existing assertion that descCol.Clone().Desc is preserved, and update the
comment to describe that the test verifies unmarshalling JSON with unknown
fields into IndexColumn succeeds and that Clone preserves Desc.
In `@pkg/util/codec/codec.go`:
- Around line 345-369: EncodeKeyWithDesc currently calls encode per-datum which
defeats encode's batch preRealloc; instead, call encode once for the full v
slice (encode(loc, b, v, true)), then iterate through the encoded bytes using
peek to locate each datum's byte range and apply the bytewise complement only
for columns marked DESC in desc; keep the same comparable flag and behavior for
types that disallow DESC (the existing check for types.KindMysqlJSON and
types.KindVectorFloat32), and ensure you preserve the original start/stop
boundaries discovered by peek so the in-place complement operates on the correct
ranges.
- Around line 406-425: CutOneWithDesc currently allocates and complements an
entire copy of b before calling peek; instead call peek directly on the original
DESC bytes (i.e. replace the cp allocation/complement loop with l, err :=
peek(b)) because peek already understands DESC-complemented flags for fixed and
variable types, then keep the existing error handling and return b[:l], b[l:],
nil; this mirrors Decoder.DecodeOne which only complements into a small descBuf
when needed and avoids the unnecessary full-buffer allocation in CutOneWithDesc.
🪄 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: 164a269f-29fd-41ea-b927-5543085e2bda
📒 Files selected for processing (27)
pkg/ddl/create_table.gopkg/ddl/db_integration_test.gopkg/ddl/desc_index_version_check_test.gopkg/ddl/executor.gopkg/ddl/index.gopkg/distsql/request_builder.gopkg/executor/admin.gopkg/executor/builder.gopkg/executor/distsql.gopkg/executor/executor_pkg_test.gopkg/executor/index_merge_reader.gopkg/executor/infoschema_reader.gopkg/executor/show.gopkg/meta/model/index.gopkg/meta/model/index_test.gopkg/planner/core/find_best_task.gopkg/planner/core/operator/physicalop/physical_index_scan.gopkg/planner/core/planbuilder.gopkg/planner/util/path.gopkg/planner/util/path_test.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/sysvar.gopkg/table/tables/mutation_checker.gopkg/tablecodec/tablecodec.gopkg/tablecodec/tablecodec_test.gopkg/util/codec/codec.gopkg/util/codec/codec_test.go
✅ Files skipped from review due to trivial changes (2)
- pkg/util/codec/codec_test.go
- pkg/ddl/db_integration_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/executor/show.go
- pkg/table/tables/mutation_checker.go
- pkg/ddl/desc_index_version_check_test.go
- pkg/executor/executor_pkg_test.go
…Reader (pingcap#2519) Addresses CodeRabbit round-2 feedback on the global-index branch in IndexMergeReaderExecutor.Open: the previous code discarded the error and could surface a downstream nil-keyrange panic. Return the error directly so callers see a clear failure. Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
…p#2519) Addresses CodeRabbit round-2 feedback: bulk-load paths write every secondary index of the target table just like INSERT does, so they must run through the same servability fence. Without this, an unservable IndexInfo (e.g. version produced by a future TiDB and loaded back into an older one) would be silently mis-maintained at load time. Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
…cap#2519) Addresses CodeRabbit round-2 feedback. Previously buildIndexColumns re-read the global atomic at job-execution time, so a `SET GLOBAL tidb_enable_descending_index = OFF` between statement submission and DDL owner replay could silently flip the persisted schema layout. Move the gate into a new ApplyDescGateToIndexParts helper that walks the AST IndexPartSpecification slice and zeroes out every Desc flag when the feature is disabled. Call it at every CREATE-time chokepoint (CreateTable / CreateIndex / CreatePrimaryKey / createColumnarIndex) before the DDL job is enqueued. The decision is now fully baked into the AST, independent of any later toggling. As a side-effect this also restores the historical "DESC silently dropped" behaviour for columnar (vector / inverted / fulltext) indexes when the gate is off — the columnar-DESC reject inside buildIndexColumns only fires when the user explicitly opted in. Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
…ngcap#2519) Addresses CodeRabbit round-2 feedback in createTableWithInfoJob. The descending-index TiKV-version gate previously ran before the OnExistIgnore branch, so a no-op \`CREATE TABLE IF NOT EXISTS\` against an already-existing table would still hit PD. Move the preflight loop after the existence check so the no-op path stays purely local. Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
Addresses CodeRabbit round-2 feedback. checkTiKVSupportsDescIndex previously called pdCli.GetAllStores(context.Background()), which could hang DDL indefinitely if PD is unreachable. Wrap the call with a 5-second timeout so an unhealthy cluster surfaces as a fast, clear error instead of a stuck CREATE TABLE. Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…#2519) Strict semver treats \`9.0.0-beta.2\` < \`9.0.0\`, but a nightly or release-candidate TiKV cut from the branch that contains the descending-index decoder (split_datum / decode_one_with_desc auto- detect) does carry the feature. Rejecting it forces operators to pick between bumping the production min-version to track dev-build tags or running a hand-edited binary — both worse than just letting the pre-release pass. Compare on Major.Minor.Patch only when checking the floor: a store whose base version is at or above MinTiKVVersionForDescIndex passes regardless of pre-release suffix. Stores whose base is below (e.g. \`8.5.0-beta.2\`) still fail closed, so the relaxation does not degrade the gate to "any pre-release passes". Two new test cases in TestCheckStoresMeetDescIndexMinVersion lock both directions in. Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
pkg/ddl/executor.go (1)
1145-1157: Avoid re-querying PD for every DESC table in batch create.
createTableWithInfoJob()is called once per table fromBatchCreateTableWithInfo(), so a restore containing many DESC-indexed tables now issuesGetAllStoresonce per table. Hoisting this probe to the batch caller, or caching the verdict for the current submission, would keep PD load bounded without weakening the safety fence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ddl/executor.go` around lines 1145 - 1157, createTableWithInfoJob currently calls checkTiKVSupportsDescIndex inside the per-table loop over tbInfo.Indices, causing a PD probe for every DESC-indexed table; move the TiKV-version gate out of createTableWithInfoJob so BatchCreateTableWithInfo runs the check once for the whole submission (or add a cached boolean shared for the current batch) and then pass the result into createTableWithInfoJob (or skip repeated checks) so checkTiKVSupportsDescIndex is invoked at batch level rather than per-table; update references where createTableWithInfoJob and BatchCreateTableWithInfo call or rely on the check to use the batched/cached verdict.pkg/ddl/index.go (2)
483-486: Consider using a structureddbterrorfor the clustered-PK DESC rejection.The rest of this file (and adjacent rejection paths like
dbterror.ErrUnsupportedAddColumnarIndex,dbterror.ErrUnsupportedAddPartialIndex,dbterror.ErrUnsupportedAddVectorIndex) raise errors with a typeddbterrorso clients receive a stable MySQL error code / SQLState and the error is matchable viaterror.Equal. Returningerrors.Errorf(...)here surfaces as a genericError 1105 (HY000)and isn't matchable in tests or downstream rollback handling.Suggest reusing an existing code (e.g. a
dbterror.ErrUnsupportedDescIndexif you introduce one for this feature, ordbterror.ErrUnsupportedDDLOperation) so the user-facing behaviour is consistent with neighbouring DDL validations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ddl/index.go` around lines 483 - 486, Replace the plain errors.Errorf(...) in the clustered-PK DESC rejection with a typed dbterror so callers/tests can match the error; for the condition that checks isPrimary && idxInfo.HasDescColumn() && (tblInfo.IsCommonHandle || tblInfo.PKIsHandle) return a dbterror (either introduce dbterror.ErrUnsupportedDescIndex or reuse dbterror.ErrUnsupportedDDLOperation) and generate the message with the existing GenWithStackByArgs/GenWithStack call pattern used by ErrUnsupportedAddColumnarIndex/ErrUnsupportedAddPartialIndex so the MySQL code/SQLState and terror.Equal matching are preserved.
490-492: Use a named constant instead of the magic1for descending-index-aware format versioning.The
GlobalIndexVersionfield (line 410) uses the public constantmodel.GlobalIndexVersionV1rather than a literal, setting good precedent. For consistency and clarity, introduce a similar public constant forIndexInfo.Version(e.g.,model.IndexVersionDescormodel.IndexVersionV1) inpkg/meta/model/index.goalongsidemaxKnownIndexVersion, and use it at line 491. This makes the format-version intent self-documenting and ensures the constant is available ifIsServableor other checks need to reference it in the future.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ddl/index.go` around lines 490 - 492, Replace the magic literal 1 when setting IndexInfo.Version with a named public constant: add a new exported constant (e.g., IndexVersionV1 or IndexVersionDesc) to the model package alongside maxKnownIndexVersion in the model/index.go definitions, update maxKnownIndexVersion if needed to include the new value, and then change the assignment in the code that calls idxInfo.HasDescColumn() to set idxInfo.Version = model.IndexVersionV1 (or the chosen name) so the version intent is self-documenting and available to other checks like IsServable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/ddl/executor.go`:
- Around line 5007-5012: Update the docstring for checkTiKVSupportsDescIndex to
accurately describe why the intest.InTest branch tolerates missing TiKV version
info: explain that MockStore-backed tests (MockStore) report empty or missing
store versions and therefore we intentionally set tolerateMissingVersion=true in
the intest path so the function does not treat those as failures; mention the
function name checkTiKVSupportsDescIndex and the intest.InTest condition so
reviewers understand this is not dead code and should not be removed.
In `@pkg/planner/core/planbuilder.go`:
- Around line 1278-1285: checkAllIndicesServable currently only checks indexes
in StatePublic and skips non-public writable indexes, which lets
writable-but-non-servable indexes pass planning and later be written by mutation
code (addRowIndices, rebuildUpdateRecordIndices). Update checkAllIndicesServable
to also include indexes where idx.IsIndexWritable() is true (in addition to
StatePublic) and call idx.IsServable() for those indexes, returning
idx.UnservableErr(tableInfo.Name.O) when not servable; this ensures any index
that mutation paths may write to is validated at plan time before
idx.Create()/idx.Delete() are invoked.
---
Nitpick comments:
In `@pkg/ddl/executor.go`:
- Around line 1145-1157: createTableWithInfoJob currently calls
checkTiKVSupportsDescIndex inside the per-table loop over tbInfo.Indices,
causing a PD probe for every DESC-indexed table; move the TiKV-version gate out
of createTableWithInfoJob so BatchCreateTableWithInfo runs the check once for
the whole submission (or add a cached boolean shared for the current batch) and
then pass the result into createTableWithInfoJob (or skip repeated checks) so
checkTiKVSupportsDescIndex is invoked at batch level rather than per-table;
update references where createTableWithInfoJob and BatchCreateTableWithInfo call
or rely on the check to use the batched/cached verdict.
In `@pkg/ddl/index.go`:
- Around line 483-486: Replace the plain errors.Errorf(...) in the clustered-PK
DESC rejection with a typed dbterror so callers/tests can match the error; for
the condition that checks isPrimary && idxInfo.HasDescColumn() &&
(tblInfo.IsCommonHandle || tblInfo.PKIsHandle) return a dbterror (either
introduce dbterror.ErrUnsupportedDescIndex or reuse
dbterror.ErrUnsupportedDDLOperation) and generate the message with the existing
GenWithStackByArgs/GenWithStack call pattern used by
ErrUnsupportedAddColumnarIndex/ErrUnsupportedAddPartialIndex so the MySQL
code/SQLState and terror.Equal matching are preserved.
- Around line 490-492: Replace the magic literal 1 when setting
IndexInfo.Version with a named public constant: add a new exported constant
(e.g., IndexVersionV1 or IndexVersionDesc) to the model package alongside
maxKnownIndexVersion in the model/index.go definitions, update
maxKnownIndexVersion if needed to include the new value, and then change the
assignment in the code that calls idxInfo.HasDescColumn() to set idxInfo.Version
= model.IndexVersionV1 (or the chosen name) so the version intent is
self-documenting and available to other checks like IsServable.
🪄 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: ae073a5a-31f9-4e06-acf3-8808a1c9a425
📒 Files selected for processing (5)
pkg/ddl/create_table.gopkg/ddl/executor.gopkg/ddl/index.gopkg/executor/index_merge_reader.gopkg/planner/core/planbuilder.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/executor/index_merge_reader.go
Contributor
Author
End-to-End Test SummaryEnd-to-end smoke test against a real How to testbash tests/run_desc_index_e2e.shPrerequisites
What the runner does
Test script (
|
| § | Coverage |
|---|---|
| 0 | Setup, drop leftover state, flip tidb_enable_descending_index = ON |
| 1 | Single descending column INDEX(b DESC) — metadata round-trip, forward DESC scan, reverse ASC scan, point lookup, range scan |
| 2 | Mixed-direction composite INDEX(a, b DESC) — MySQL 8.0 flagship case; verify ORDER BY a ASC, b DESC and ORDER BY a DESC, b ASC resolve without a Sort |
| 3 | PRIMARY KEY guards — clustered PK with DESC must error; NONCLUSTERED PK with DESC works |
| 4 | String column DESC |
| 5 | Expression index INDEX ((a+b) DESC) |
| 6 | UPDATE / DELETE against DESC indexes |
| 7 | Sysvar create-time-only — flip OFF, verify existing tables still work, verify new CREATE INDEX silently drops DESC |
| 8 | Cleanup |
Result
All sections passed. Highlights from the captured output:
| Scenario | Expected | Actual |
|---|---|---|
ORDER BY b DESC (forward scan on DESC index) |
30, 20, 15, 10, 5 | ✓ exact |
ORDER BY b ASC (reverse scan, plan shows keep order:true, desc) |
5, 10, 15, 20, 30 | ✓ exact |
ORDER BY a ASC, b DESC on INDEX(a, b DESC) — no Sort in plan |
(1,15)(1,10)(1,5)(2,20)(2,12)(2,8) | ✓ exact |
ORDER BY a DESC, b ASC (reverse complement) — no Sort |
(2,8)(2,12)(2,20)(1,5)(1,10)(1,15) | ✓ exact |
ORDER BY a, b (unsatisfiable by either direction — Sort inserted) |
(1,5)(1,10)(1,15)(2,8)(2,12)(2,20) | ✓ Sort present, correct order |
PRIMARY KEY (a, b DESC) CLUSTERED |
DDL error | ✓ "DESC is not supported on the columns of a clustered PRIMARY KEY" |
PRIMARY KEY (a DESC) NONCLUSTERED |
works as DESC unique secondary | ✓ 20, 10, 5, 1 |
String DESC: name VARCHAR(32) INDEX(name DESC) |
elderberry, date, cherry, banana, apple | ✓ exact |
Expression DESC: INDEX ((a+b) DESC) |
(4,30)=34, (2,20)=22, (5,15)=20, (1,10)=11, (3,5)=8 | ✓ exact, collation = D |
INFORMATION_SCHEMA.STATISTICS.COLLATION |
D for DESC, A for ASC |
✓ |
| UPDATE through DESC index | row (102, 20) visible after UPDATE | ✓ |
| DELETE through DESC index | b=5 row gone | ✓ |
| Sysvar OFF: existing DESC table | still descends correctly | ✓ |
Sysvar OFF: new CREATE TABLE … (x DESC) |
DESC silently dropped, collation = A |
✓ |
| Final | e2e test complete |
✓ |
A bug the e2e caught (and unit tests did not)
The first e2e run failed with Unsupported datum flag 252 for Int vector. Root cause was a second TiKV decoder pipeline I had missed: index-scan executors push raw per-column slices into LazyBatchColumn::Raw, where they are later decoded by the vectorised RawDatumDecoder family (decode_int_datum, decode_real_datum, …). Those per-type decoders only understand ASC flag bytes, so a descending column reached them as 0xFC / 0xFB / etc. and the chunk path errored even though split_datum had correctly sliced the span.
Fixed in tikv/tikv#19558 → 2ab251588 by introducing a single helper un_invert_if_desc(value: &[u8]) -> Cow<[u8]> shared between decode_one_with_desc (Vec path) and extract_columns_from_datum_format (chunk path) — DESC awareness lives in one place per pipeline, no duplicated complement logic. Second run after that fix succeeded across all 8 sections.
This is the kind of regression no unit test would catch; it required real RocksDB + real coprocessor + a real TiDB-issued DAG request.
…cap#2519) Addresses CodeRabbit round-3 feedback (critical). checkAllIndicesServable previously skipped every index whose state was not StatePublic, but the mutation paths in pkg/table/tables/tables.go (addRowIndices, rebuildUpdateRecordIndices, ...) gate on tables.IsIndexWritable, which returns true for StateWriteOnly and StateWriteReorganization too. That meant an older TiDB binary running DML against a table whose new DESC index was mid-DDL (in WriteOnly while TiDB N drives the schema change) would pass the planner fence, reach the mutation path, and silently corrupt the index because it does not understand the new metadata format. Mirror IsIndexWritable's exact predicate at the fence: skip StateDeleteOnly / StateDeleteReorganization (not writable, fence is a no-op there); check IsServable on every other state. Extend TestUnservableIndexRejectsQueries to lock in both directions — WriteOnly / WriteReorganization must error, DeleteOnly / DeleteReorganization must pass through. Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
Addresses CodeRabbit round-3 feedback. The previous docstring claimed "mock stores never satisfy tikv.Storage", which is misleading: the intest branch immediately below the type assertion exists precisely because MockStore-backed integration tests DO satisfy tikv.Storage, reach the PD-lookup path, and report empty Version strings. A future reader following the original comment might delete the \`tolerateMissingVersion=intest.InTest\` branch as dead code, breaking every DESC-index integration test. Rewrite the docstring to spell out both escape hatches separately: (1) backends that don't satisfy tikv.Storage at all (pure-Go mocks, etc.) short-circuit at the type assertion, and (2) MockStore (which does satisfy the trait) is handled by the intest tolerance flag. No code change. Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
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/executor.go (1)
4720-4748:⚠️ Potential issue | 🔴 CriticalTreat DESC as part of EXCHANGE PARTITION index compatibility.
Now that DESC changes persisted index metadata and key encoding,
checkTableDefCompatible()later in this file is too weak: it still compares only index-column name/length. That letsEXCHANGE PARTITIONacceptINDEX(a)vsINDEX(a DESC)as “compatible”, even though the swapped index KV no longer matches the receiving table’s metadata.Suggested fix
for i, sourceIdxCol := range sourceIdx.Columns { compatIdxCol := compatIdx.Columns[i] if sourceIdxCol.Length != compatIdxCol.Length || - sourceIdxCol.Name.L != compatIdxCol.Name.L { + sourceIdxCol.Name.L != compatIdxCol.Name.L || + sourceIdxCol.Desc != compatIdxCol.Desc { return errors.Trace(dbterror.ErrTablesDifferentMetadata) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ddl/executor.go` around lines 4720 - 4748, The compatibility check for EXCHANGE PARTITION must consider index sort order; update checkTableDefCompatible to include DESC/ASC in its index definition comparison (not just column name/length) by comparing the index part order flag produced by buildIndexColumns/indexPartSpecifications (e.g., include the per-part Expr/Length and a Desc flag or Order attribute when comparing IndexColumn/indexPartSpecifications entries); ensure the existing buildIndexColumns and hasDescIndexColumn logic are used/extended so the compatibility check rejects INDEX(a) vs INDEX(a DESC) mismatches.
🧹 Nitpick comments (1)
pkg/ddl/db_integration_test.go (1)
3168-3178: Doc-comment refers to a different name than the function.The helper is
explainHasStringbut the comment saysexplainHas reports whether…. Tiny nit — please align the name so future readers (andgo docoutput) aren't confused.🩹 Suggested fix
-// explainHas reports whether any line of an EXPLAIN rowset contains the given substring. +// explainHasString reports whether any line of an EXPLAIN rowset contains the given substring. func explainHasString(rows [][]any, substr string) bool {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ddl/db_integration_test.go` around lines 3168 - 3178, The doc comment above the helper currently says "explainHas reports..." but the function is named explainHasString; update the comment to reference explainHasString (or rename the function to explainHas if you prefer) so the doc and symbol match; edit the comment line directly above func explainHasString to read something like "explainHasString reports whether any line of an EXPLAIN rowset contains the given substring." to keep go doc and readers consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/ddl/db_integration_test.go`:
- Around line 3300-3310: The loop over tbl.Meta().Indices silently skips
assertions if index "idx_a" isn't present; update the test around the
TableByName/for loop (tbl.Meta().Indices and idx.Name.L checks) to detect
absence and fail: either set a found flag when idx.Name.L == "idx_a" and after
the loop require.True(found, "idx_a must exist"), or directly lookup the index
by name from tbl.Meta().Indices (or the table's index lookup helper) and assert
not nil before checking idx.Columns[0].Desc and idx.Version so the test fails if
"idx_a" was not created or renamed.
In `@pkg/ddl/executor.go`:
- Around line 5184-5188: The DESC/TiKV preflight (the call to
checkTiKVSupportsDescIndex guarded by hasDescIndexColumn) runs before the
IndexTypeHypo fast-path and wrongly blocks or errors for CREATE HYPO INDEX;
update the logic so this preflight is skipped for hypothetical indexes by either
moving the hasDescIndexColumn/checkTiKVSupportsDescIndex block to after the
existing early return for ast.IndexTypeHypo or by wrapping the call with an
explicit condition that returns early when the index type equals
ast.IndexTypeHypo (referencing hasDescIndexColumn, checkTiKVSupportsDescIndex,
and ast.IndexTypeHypo to locate the change).
In `@pkg/planner/core/planbuilder.go`:
- Around line 4795-4799: The servability check is being run against potentially
stale tnW.TableInfo; update buildImportInto to fetch the latest table info via
latestIS.TableByID(...) and pass that to checkAllIndicesServable instead of
tnW.TableInfo so the IMPORT INTO path validates against the current schema (use
the same table object that later logic uses, e.g., replace
checkAllIndicesServable(tnW.TableInfo) with
checkAllIndicesServable(latestTableInfo) where latestTableInfo is obtained from
latestIS.TableByID for tnW.TableInfo.ID).
---
Outside diff comments:
In `@pkg/ddl/executor.go`:
- Around line 4720-4748: The compatibility check for EXCHANGE PARTITION must
consider index sort order; update checkTableDefCompatible to include DESC/ASC in
its index definition comparison (not just column name/length) by comparing the
index part order flag produced by buildIndexColumns/indexPartSpecifications
(e.g., include the per-part Expr/Length and a Desc flag or Order attribute when
comparing IndexColumn/indexPartSpecifications entries); ensure the existing
buildIndexColumns and hasDescIndexColumn logic are used/extended so the
compatibility check rejects INDEX(a) vs INDEX(a DESC) mismatches.
---
Nitpick comments:
In `@pkg/ddl/db_integration_test.go`:
- Around line 3168-3178: The doc comment above the helper currently says
"explainHas reports..." but the function is named explainHasString; update the
comment to reference explainHasString (or rename the function to explainHas if
you prefer) so the doc and symbol match; edit the comment line directly above
func explainHasString to read something like "explainHasString reports whether
any line of an EXPLAIN rowset contains the given substring." to keep go doc and
readers consistent.
🪄 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: 0ecb4995-54d1-4e38-885d-c2928e2b12d7
📒 Files selected for processing (4)
pkg/ddl/db_integration_test.gopkg/ddl/desc_index_version_check_test.gopkg/ddl/executor.gopkg/planner/core/planbuilder.go
…ly (pingcap#2519) Addresses CodeRabbit round-4 feedback (minor). The previous loop body buried both DESC-drop assertions inside an \`if idx.Name.L == "idx_a"\` branch, so an upstream regression that fails to create idx_a (or silently renames it) would make the assertions skip and the test would falsely pass. Hoist the lookup out of the loop, capture the matching IndexInfo into a pointer, and require non-nil before checking the Desc / Version fields. Now any future change that breaks idx_a creation surfaces as a clear "expected idx_a to be present after ALTER TABLE" failure. Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
Addresses CodeRabbit round-4 feedback (major). The DESC-index TiKV preflight in createIndex ran before the IndexTypeHypo fast-path return, so \`CREATE HYPO INDEX ... DESC\` would unnecessarily make a PD round-trip and could fail outright on an old or unhealthy cluster even though hypothetical indexes are planner-only and never produce KV mutations. Wrap the existing \`hasDescIndexColumn(indexColumns)\` guard with \`!isHypo\` (\`indexOption != nil && indexOption.Tp == ast.IndexTypeHypo\`) so the gate fires only for indexes that will actually reach the storage layer. The hypo path already returns immediately after addHypoIndexIntoCtx, so no other side-effects are affected. Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
…2519) Addresses CodeRabbit round-4 feedback (major). The fence added in b0f53ae checked tnW.TableInfo, which can be stale on the IMPORT INTO path: buildImportInto deliberately switches to \`latestIS.TableByID(...)\` a few lines later because tnW.TableInfo captures the snapshot the parser saw, not the schema the import actually executes against. As a result an older TiDB could miss a writable newer-format index that exists only in the latest schema and proceed into the import write path, defeating the fence. Move the fence after the latestIS.TableByID lookup and pass \`tableInPlan.Meta()\` instead of \`tnW.TableInfo\`, so the check runs against the same metadata snapshot the rest of the import-planning code uses. Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/ddl/executor.go (1)
1145-1157: Avoid repeating the PD preflight once per DESC table in batch create.Line 1150 runs inside
createTableWithInfoJob, which is called per table fromBatchCreateTableWithInfo(Line 1312). For large batches, this can issue NGetAllStorescalls (and N×timeout behavior during PD instability). Consider hoisting/caching this check once per batch when any table has DESC columns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ddl/executor.go` around lines 1145 - 1157, The DESC-index TiKV preflight (checkTiKVSupportsDescIndex) is being invoked inside createTableWithInfoJob for each table, causing repeated PD GetAllStores calls during BatchCreateTableWithInfo; compute a single boolean (e.g., anyHasDesc) by scanning the batch's tbInfo list in BatchCreateTableWithInfo, call checkTiKVSupportsDescIndex once if any table has a desc column, and then either pass this result into createTableWithInfoJob (or set a flag/context) so createTableWithInfoJob no longer calls checkTiKVSupportsDescIndex per-table.pkg/ddl/db_integration_test.go (1)
3168-3178: Doc comment refers to wrong function name.The comment says
explainHasbut the function isexplainHasString. Per Go's godoc convention, the doc comment should begin with the symbol's name.📝 Suggested fix
-// explainHas reports whether any line of an EXPLAIN rowset contains the given substring. +// explainHasString reports whether any line of an EXPLAIN rowset contains the given substring. func explainHasString(rows [][]any, substr string) bool {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ddl/db_integration_test.go` around lines 3168 - 3178, Update the doc comment to start with the function's actual name: change the leading sentence to begin with "explainHasString" (e.g., "explainHasString reports whether any line of an EXPLAIN rowset contains the given substring.") so it follows Go's godoc convention for the explainHasString function in pkg/ddl/db_integration_test.go.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/ddl/db_integration_test.go`:
- Around line 3168-3178: Update the doc comment to start with the function's
actual name: change the leading sentence to begin with "explainHasString" (e.g.,
"explainHasString reports whether any line of an EXPLAIN rowset contains the
given substring.") so it follows Go's godoc convention for the explainHasString
function in pkg/ddl/db_integration_test.go.
In `@pkg/ddl/executor.go`:
- Around line 1145-1157: The DESC-index TiKV preflight
(checkTiKVSupportsDescIndex) is being invoked inside createTableWithInfoJob for
each table, causing repeated PD GetAllStores calls during
BatchCreateTableWithInfo; compute a single boolean (e.g., anyHasDesc) by
scanning the batch's tbInfo list in BatchCreateTableWithInfo, call
checkTiKVSupportsDescIndex once if any table has a desc column, and then either
pass this result into createTableWithInfoJob (or set a flag/context) so
createTableWithInfoJob no longer calls checkTiKVSupportsDescIndex per-table.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 646dce6b-8153-4392-b664-309fcc1150a3
📒 Files selected for processing (3)
pkg/ddl/db_integration_test.gopkg/ddl/executor.gopkg/planner/core/planbuilder.go
pingcap#2519) Adds the SQL script and runner shell that have been driving the end-to-end verification of the desc-index work against a live tiup playground cluster. Captures the full feature surface so future contributors can reproduce the result that's been posted on the PR discussion thread. Layout: * tests/desc_index_e2e.sql — 8 sections, each with the expected behaviour annotated. Covers single-column DESC, mixed-direction composite indexes (the MySQL 8.0 flagship case), PRIMARY KEY direction guards (clustered must be rejected; nonclustered accepted), string DESC, expression-index DESC, UPDATE / DELETE through DESC indexes, and the create-time-only semantics of the tidb_enable_descending_index sysvar. * tests/run_desc_index_e2e.sh — boots a tiup playground with caller-supplied binaries (TIDB_BIN / TIKV_BIN env vars, no hard-coded paths), waits for TiDB to come up, runs the SQL file via stdin + --force so the deliberate clustered-PK rejection in Section 3 doesn't abort the script, then tears the cluster down. This is the regression test that originally surfaced the missing TiKV chunk-extractor un-invert (\`Unsupported datum flag 252 for Int vector\`); the kind of issue no unit test would catch because it needs real RocksDB + real coprocessor + real TiDB-issued DAG request. Keeping the artefact alongside the rest of \`tests/\` makes the same check trivially repeatable. Usage: TIDB_BIN=/path/to/bin/tidb-server \ TIKV_BIN=/path/to/target/release/tikv-server \ ./tests/run_desc_index_e2e.sh Requires: tiup, mysql client, plus the two binaries above. The TiKV binary must include the descending-order coprocessor changes from tikv/tikv#19558. Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
takaidohigasi
force-pushed
the
feature/desc-index
branch
from
3d35a5a to
8eec79a
Compare
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the For example:
📖 For more info, you can check the "Contribute Code" section in the development guide. |
Contributor
Author
|
related to |
Contributor
|
Maybe set this to Draft until the TiKV change has been merged? Or isn't that a hard requirement? |
Contributor
Author
|
ok, I also agree to do TiKV one first. I will change the status |
ti-chi-bot
Bot
added
the
do-not-merge/work-in-progress
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 #2519
Problem Summary:
The
DESCkeyword inCREATE INDEX (col DESC)is parsed today butsilently ignored — a MySQL 5.7 compatibility behaviour TiDB has carried
forward since 2018 (see issue #2519). MySQL 8.0 supports descending
indexes natively, and users migrating from MySQL 8.0 or running queries
that need a descending sort over a composite key (e.g.
ORDER BY a ASC, b DESCagainst a covering index) currently must eitherrewrite their schema or accept a planner-inserted Sort operator.
This PR makes
DESCtake effect end-to-end: it persists through indexmetadata, the storage encoder, the planner, the read path, and round-
trips out via
SHOW CREATE TABLEandINFORMATION_SCHEMA.STATISTICS.The companion TiKV PR is tikv/tikv#19558, which teaches the
coprocessor's key decoder to recognise DESC-complemented flag bytes.
The companion documentation PR is pingcap/docs#22805.
What changed and how does it work?
Per-column
Descis added tomodel.IndexColumnand written into theindex key as the bitwise complement of the ascending memcomparable
bytes, so a forward RocksDB iterator yields the declared direction. The
planner satisfies
ORDER BYby computing whether a forward or reversecop scan matches per-column directions; mixed composites like
INDEX(a ASC, b DESC)satisfyORDER BY a ASC, b DESCwith a singleforward scan and no Sort.
Three-layer rolling-upgrade defence:
tidb_enable_descending_index(default OFF). Whenoff,
DESCis silently dropped at DDL time, preserving MySQL 5.7migration-script behaviour.
non-TiFlash store reports a TiKV version below
MinTiKVVersionForDescIndex, the DDL is rejected with a clear"upgrade TiKV" message. Prevents writing DESC-encoded keys to a
cluster that can't decode them.
IndexInfo.Versionfence: the field bumps to 1 whenever anycolumn is DESC.
IndexInfo.IsServableis checked at plan time andin INSERT planning, so an older TiDB binary reading a Version=1
index from schema JSON refuses to serve queries rather than
silently dropping the unknown field.
Commit structure (12 commits):
*: plumb per-column DESC flag through index metadataplanner: honor IndexColumn.Desc when matching sort propertycodec: add EncodeKeyWithDesc/DecodeOneWithDesc primitivestablecodec: encode DESC index columns with complemented bytesdistsql,executor,tables: wire DESC encoding through read pathscodec: auto-detect DESC flag bytes in chunk Decoderplanner,model: enforce IndexInfo.IsServable at plan timeddl: gate CREATE INDEX on TiKV cluster versionplanner: support mixed-direction composite indexesddl: regression test for DESC on expression-index partsddl: lock down sysvar create-time-only semanticsddl: clarify MinTiKVVersionForDescIndex is a release-coordinated TODOCheck List
Tests
Coverage:
pkg/util/codec— primitive round-trips, range-sentinel ordering, JSON/Vector rejectionpkg/meta/model— IndexColumn JSON round-trip, IsServable, HasDescColumnpkg/ddl— 8 dedicated tests including end-to-end INSERT+SELECT through MockStorepkg/planner/core/casetest/ruleandpkg/planner/core/issuetest— no regressionsbazel build --config=ci //... --nogo— green at tipSide effects
When
tidb_enable_descending_indexis OFF (the default), behaviour isbyte-for-byte identical to current TiDB. The
IsServablefence does notadd a regression for any existing index because
Version=0indexes areunconditionally serviceable.
Documentation
Adds
tidb_enable_descending_indexsysvar; enables previously-ignoredDESCsyntax inCREATE INDEX; brings TiDB closer to MySQL 8.0 fordescending-index behaviour. Documentation updates covering the new
sysvar entry, the
CREATE INDEXsyntax page, and the MySQLcompatibility list are tracked in pingcap/docs#22805.
Release note
Summary by CodeRabbit
New Features
Bug Fixes
Tests