[DNM] executor: optimize range scan for cached table#67431
Open
you06 wants to merge 49 commits into
Open
Conversation
Cache c.GetType(ctx) into a local variable in Constant.EvalInt and Constant.EvalReal to avoid redundant method calls. Also remove the sleep 1200 between work file iterations in the cached table scan run script. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
For Constant expressions without ParamMarker, directly access RetType to get the flag instead of calling GetType(sctx), reducing overhead in integer comparison operations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…located FieldType
Add getTypeNonAlloc that fills a caller-provided FieldType buffer on the
stack for ParamMarker constants instead of allocating via NewFieldType.
Replace all GetType calls in Eval{Int,Real,String,Decimal,Time,Duration,
JSON,VectorFloat32} with this non-allocating variant to reduce GC pressure
in hot paths.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
For cached tables, getSnapshotRow() always returns nil, making the merge logic in getOneRow() unnecessary overhead. Add a dedicated nextForCacheTable() method that reads directly from addedRowsIter, skipping the snapshot row merge and reducing per-row cost. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Introduce memRowsBatchIterForTable for cached table scans. It decodes rows into chunks and applies vectorized filters when enabled. Add a benchmark comparing row vs batch iter.
Add a specialized decoder for fixed-length int-handle row keys to reduce overhead in mem table scans, and add tests/bench updates.
- Add wide-row decode and mixed-append benchmarks\n- Optimize chunk fixed-len appends and fixed AppendNull\n- Cache per-column decode metadata in ChunkDecoder\n- Add correctness tests and cached-table-scan work plans
Pass buf and preAlloc parameters through to decodeIndexKvGeneral and decodeIndexKvForClusteredIndexVersion1 so callers (DecodeIndexKVEx) can reuse pre-allocated slices. Replace CutIndexKeyNew with CutIndexKeyTo to write into the pre-allocated result slice directly and use reEncodeHandleTo instead of reEncodeHandle to append in-place. Remove the now-unused reEncodeHandle helper. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ue decoding Add IndexRestoredDecoder to cache the colIDs map and BytesDecoder across rows, avoiding per-row allocations in decodeRestoredValues. Introduce arena-based encodeOldDatumToArena and DecodeToBytesNoHandleInto to further reduce allocations during index KV decoding in the general path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add Time.AppendString() that formats date/datetime/timestamp via direct byte writes instead of fmt.Sprintf and string concatenation, eliminating allocations. Rewrite Time.String() to delegate to AppendString. In DumpTextRow, use AppendString with the existing tmp buffer to avoid hack.Slice(t.String()) allocations. Also bump tmp buffer from 20 to 32 bytes to accommodate the longest datetime+fsp string. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cache HandleStatus, time.Location, and EvalContext in memIndexReader so they are computed once per scan instead of on every row decode. Also use DecodeColumnValueWithDatum to avoid intermediate allocations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Introduce a per-cached-table result set cache that stores query result sets keyed by plan digest and parameter hash. The cache is invalidated when the table lease expires, avoiding stale reads without eviction logic. Includes unit tests for hit/miss, capacity limits, and concurrency safety. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Ensure the cached table result set cache is properly invalidated when data may become stale: on data reload, lease renewal failure, and before acquiring a write lock. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add CanCacheResultSet to determine whether a query's result set can be served from the cached table result set cache. The check rejects queries with non-deterministic functions (NOW, RAND, UUID, etc.), user/session variable references, FOR UPDATE locks, DML context, and non-cached tables. Includes comprehensive unit tests covering all rejection paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add BuildResultCacheKey to construct cache keys from plan digest and parameter hashes, enabling per-query result set caching for cached tables. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…d tables Wire up the result set cache by wrapping top-level executors with CachedResultExec. Move ResultCacheKey to the table package so it can be referenced across executor and planner without import cycles, and expose GetCachedResult/PutCachedResult on the CachedTable interface. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add metrics (hit/miss counters, memory gauge, eviction counter), slow log field (Result_cache_hit), EXPLAIN ANALYZE runtime stats, and the StmtCtx.ReadFromResultCache flag to provide visibility into result set cache behavior on cached tables. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…e fixes Add secondary paramBytes verification on cache lookups to guard against FNV-1a hash collisions. Fall back to OriginalSQL hashing when plan cache parameterization is bypassed (e.g. non-prepared plan cache disabled). Exclude PointGetPlan and BatchPointGetPlan from result set caching since they bypass plan cache parameterization. Remove obsolete workplan files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Store TIMESTAMP columns in UTC in datum cache with tracked indices for per-session timezone conversion. Add timezone-aware test for cached table result set cache. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cover projection, desc scan, filters, timezone conversion, multi-chunk desc, and benchmark for the cached datum fast-path iterator. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cherry-pick of 1ee4b3c from PR pingcap#66640 (Part 5). Adds CachedIndexDatumData and BuildCachedIndexDatumData for pre-decoding index KV entries into datum maps at cache load time. The executor builder now looks up index datum caches via GetCachedIndexDatumData interface and populates indexDatumCaches on UnionScanExec.
… condition checks Cherry-pick of def0558 from PR pingcap#66640 (Part 5). - Reuse us.mutableRow field instead of allocating a new MutRow per Next() call in both Next and nextForCacheTable. - Add direct-to-chunk datum write fast path in nextForCacheTable when no virtual columns or conditions exist. - Skip EvalBool when conditions slice is empty in memIndexReader.getMemRows and memRowsIterForIndex.Next. - Reuse mutableRow in memCachedDatumIter instead of allocating via MutRowFromDatums on every iteration. Note: removeRedundantAccessConditions was already cherry-picked in Part 4.
Cherry-pick of e21ed51 from PR pingcap#66640 (Part 5). Guard against nil timezone in EncodeMySQLTime and keep index decode buffer preallocation to avoid unnecessary allocations.
Cherry-pick of d143283 from PR pingcap#66640 (Part 5). Fix cached datum decode handling and improve result cache memory accounting consistency. Update test assertions accordingly.
Cherry-pick of 1cde4d3 from PR pingcap#66640 (Part 5). Add GetCachedDatumDataForMemBuffer and GetCachedIndexDatumDataForMemBuffer methods that validate the MemBuffer matches the current cacheData generation, preventing stale datum cache usage across lease boundaries.
Cherry-pick of 979f905 from PR pingcap#66640 (Part 5). Use origin default values when building datum cache for columns that are missing from the encoded row data, ensuring newly added columns with defaults are properly filled.
Cherry-pick of 10fc5ba from PR pingcap#66640 (Part 5). Prefer GetCachedDatumDataForMemBuffer and GetCachedIndexDatumDataForMemBuffer over unpinned accessors when looking up datum caches in handleCachedTable, ensuring the datum cache matches the same cacheData generation as the MemBuffer being read.
Cherry-pick of 57df0aa from PR pingcap#66640 (Part 5). - Add kvRangesCoverFullTable guard to only use datum cache fast path when the scan covers the full table record range and txn membuffer has no overriding entries. - Add safety checks in CachedResultExec for schema match validation. - Add comprehensive tests for datum cache fallback and backfill edge cases.
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. 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
do-not-merge/invalid-title
do-not-merge/needs-linked-issue
do-not-merge/needs-tests-checked
do-not-merge/release-note-label-needed
you06
changed the title
|
Review failed due to infrastructure/execution failure after retries. Please re-trigger review. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
sig/planner
|
[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 |
|
Hi @you06. 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. |
📝 WalkthroughWalkthroughThis PR introduces in-memory result set caching for cached tables in TiDB. It adds a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Planner as Planner Core
participant Builder as Executor Builder
participant CachedResultExec as CachedResultExec
participant WrappedExec as Wrapped Executor
participant CachedTable as CachedTable
participant Metrics
Client->>Planner: Execute Query
Planner->>Planner: CanCacheResultSet(AST, Plan)
Planner->>Planner: BuildResultCacheKey(params)
Planner->>Builder: Build Executor
Builder->>CachedResultExec: wrapWithResultCache()
CachedResultExec->>CachedTable: GetCachedResult(key, paramBytes)
alt Cache Hit
CachedTable-->>CachedResultExec: cached chunks + schema
CachedResultExec->>CachedResultExec: Open() - hit path
CachedResultExec->>Metrics: ResultCacheHitCounter++
CachedResultExec->>Client: Serve cached chunks
else Cache Miss
CachedTable-->>CachedResultExec: nil (miss)
CachedResultExec->>WrappedExec: Open()
CachedResultExec->>WrappedExec: Next() - collect chunks
CachedResultExec->>Metrics: ResultCacheMissCounter++
CachedResultExec->>Client: Serve executor results
CachedResultExec->>CachedResultExec: Close() - backfill
CachedResultExec->>CachedTable: PutCachedResult(key, paramBytes, chunks)
end
sequenceDiagram
participant Session
participant MemReader as MemIndexReader
participant CachedTable as CachedDatumData
participant Decoder as IndexRestoredDecoder
Session->>MemReader: Scan cached table index
MemReader->>CachedTable: GetCachedIndexDatumData(indexID)
alt Datum Cache Available
CachedTable-->>MemReader: indexDatumCache (pre-decoded)
MemReader->>MemReader: Use cached datums directly
MemReader->>MemReader: Apply filter conditions
else Datum Cache Unavailable
CachedTable-->>MemReader: nil
MemReader->>Decoder: DecodeIndexKVEx(key, value)
Decoder->>Decoder: Use cached decoder
Decoder-->>MemReader: decoded columns
MemReader->>MemReader: Apply filters
end
MemReader->>Session: Return rows
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the 📖 For more info, you can check the "Contribute Code" section in the development guide. Notice: To remove the For example:
📖 For more info, you can check the "Contribute Code" section in the development guide. |
|
@you06: 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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #67431 +/- ##
=================================================
- Coverage 77.7970% 49.0438% -28.7533%
=================================================
Files 2023 329 -1694
Lines 556490 70751 -485739
=================================================
- Hits 432933 34699 -398234
+ Misses 121809 31972 -89837
- Partials 1748 4080 +2332
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/util/rowcodec/decoder.go (1)
143-148:⚠️ Potential issue | 🟠 MajorAdd a regression test for
loc == niltimestamp decoding.This changes both datum-map and chunk decoding to skip timezone conversion when no location is configured. Because this is a bug fix, please add a targeted test that decodes a non-zero
TIMESTAMPthrough both paths withloc == nil.As per coding guidelines, "MUST add a regression test and verify it fails before fix and passes after fix for bug fixes."
Also applies to: 270-274, 452-465
🧹 Nitpick comments (12)
pkg/expression/constant.go (1)
246-259: Consider logging or returning error on failure for debuggability.The new
getTypeNonAllocsilently returnsnilwhenGetUserVarfails, whereas the originalGetTypelogs a warning vialogutil.BgLogger().Warn. This reduces observability when debugging parameter resolution issues in hot paths.Since this is a performance-critical path, the trade-off may be intentional, but consider whether silent failure could mask issues during development or testing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/expression/constant.go` around lines 246 - 259, The getTypeNonAlloc function currently swallows errors from c.ParamMarker.GetUserVar and returns nil, reducing observability; update getTypeNonAlloc toSurface the failure (either by logging the error via logutil.BgLogger().Warn with context or by returning an error value) instead of silently returning nil: locate getTypeNonAlloc (and the c.ParamMarker.GetUserVar call), ensure types.InitUnspecifiedFieldType(buf) and types.InferParamTypeFromDatum(&dt, buf) remain unchanged on success, and on GetUserVar failure emit a warning including the error and parameter identifier (or change the signature to return ( *types.FieldType, error ) so callers can handle the error) to preserve debuggability in the hot path.pkg/util/hint/hint.go (1)
415-418: Consider explicit conflict handling for opposite plan-cache hints.If both
ignore_plan_cache()anduse_plan_cache()are present, both flags become true with no parser-level warning. Adding an explicit conflict warning here would make behavior clearer.♻️ Suggested parser-side conflict warning
case HintIgnorePlanCache: + if stmtHints.UsePlanCache { + warns = append(warns, ErrWarnConflictingHint.FastGenByArgs("IGNORE_PLAN_CACHE() conflicts with USE_PLAN_CACHE()")) + } stmtHints.IgnorePlanCache = true case HintUsePlanCache: + if stmtHints.IgnorePlanCache { + warns = append(warns, ErrWarnConflictingHint.FastGenByArgs("USE_PLAN_CACHE() conflicts with IGNORE_PLAN_CACHE()")) + } stmtHints.UsePlanCache = true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/util/hint/hint.go` around lines 415 - 418, Both plan-cache hints currently set flags independently (case HintIgnorePlanCache sets stmtHints.IgnorePlanCache and case HintUsePlanCache sets stmtHints.UsePlanCache) so if both appear no parser-level conflict is reported; change each case to first check whether the opposite flag is already true (e.g., when setting stmtHints.IgnorePlanCache check stmtHints.UsePlanCache and vice‑versa) and emit a parser-level warning about conflicting hints via the existing warning API (or add a small helper to record a parse warning) instead of silently setting both; ensure the check references HintIgnorePlanCache, HintUsePlanCache, stmtHints.IgnorePlanCache and stmtHints.UsePlanCache so the conflict is detected and reported.pkg/util/rowcodec/bench_test.go (1)
100-106: Use a fixed timezone in the benchmark to keep runs deterministic.Line 100 and Line 105 use
time.Local, which can vary across environments and alter decode behavior/cost for timestamp paths. Prefertime.UTC(or an explicit fixed zone) for stable comparisons.As per coding guidelines "Keep test changes minimal and deterministic; avoid broad golden/testdata churn unless required."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/util/rowcodec/bench_test.go` around lines 100 - 106, Replace non-deterministic time.Local with a fixed timezone in the benchmark: update the calls to enc.Encode(...) and rowcodec.NewChunkDecoder(...) that currently pass time.Local so they use time.UTC (or an explicit fixed zone) instead, ensuring the Encode and NewChunkDecoder invocations (and any related benchmark setup using time.Local) use the fixed timezone for stable deterministic runs.pkg/util/chunk/chunk_test.go (1)
778-796: Exercise the first null-bitmap byte boundary in this regression.With only four appends, this never crosses into
nullBitmap[1]. Extending the pattern past 8 rows would cover the boundary where append bugs usually show up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/util/chunk/chunk_test.go` around lines 778 - 796, The test TestColumnNullBitmapAfterAppend only appends 4 rows so it never exercises the second null-bitmap byte; update the test to append past the 8-row boundary (e.g., use NewChunkWithCapacity(fieldTypes, 16) and perform the AppendInt64/AppendNull pattern until at least 9 rows) and then assert IsNull/values for entries across the byte boundary (reference functions NewChunkWithCapacity, AppendInt64, AppendNull, NumRows, Column, IsNull, GetRow).pkg/executor/builder.go (1)
1505-1557: Avoid re-encoding ranger's reserve logic in the executor builder.This filter-elision path is correctness-sensitive. If ranger's
shouldReserverules move, UnionScan can silently diverge from the range builder. A shared helper would keep both sides in lockstep.As per coding guidelines, "Code SHOULD remain maintainable for future readers with basic TiDB familiarity, including readers who are not experts in the specific subsystem/feature."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/builder.go` around lines 1505 - 1557, The removeRedundantAccessConditions function reimplements ranger's shouldReserve logic which risks divergence; extract the reservation-checking logic into a shared helper used by both the ranger and this executor builder (or call the existing ranger.ShouldReserve/appropriate exported function) so UnionScan doesn't re-encode rules locally. Change removeRedundantAccessConditions (and any callers) to rely on that shared helper (referencing removeRedundantAccessConditions, accessConds, idxCols, idxColLens) instead of computing fullLenColIDs and canonical-hash comparisons here, ensuring the canonical-hash cloning behavior is preserved inside the shared helper if needed to avoid races.pkg/util/rowcodec/decoder.go (1)
673-719: Spell out the lifetime contract ofDecodeToBytesNoHandleInto.The returned
values[i]slices now aliasarena, so they become invalid as soon as the caller reuses or resets that arena. Please document that explicitly, along with the requirement thatvaluesis large enough for the highestoutputOffset, so this helper is harder to misuse.As per coding guidelines, "Comments SHOULD explain non-obvious intent, constraints, invariants, concurrency guarantees, SQL/compatibility contracts, or important performance trade-offs, and SHOULD NOT restate what the code already makes clear."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/util/rowcodec/decoder.go` around lines 673 - 719, Document the lifetime and sizing contract for DecodeToBytesNoHandleInto: update the function comment for DecodeToBytesNoHandleInto to explicitly state that returned values[i] may alias the provided arena (via encodeOldDatumToArena) and therefore become invalid as soon as the caller reuses or resets arena (caller must copy any needed bytes before reusing arena), and also state that the caller must ensure the values slice is large enough to contain entries for the maximum index referenced by outputOffset (i.e., values must have length > max(outputOffset) and will be reused/cleared by the call). Also mention that the caller should reset arena between rows (arena = arena[:0]) as already expected.pkg/executor/mem_reader.go (3)
1407-1407: Consider pointer comparison for timezone check.The string comparison
sessionLoc.String() != time.UTC.String()allocates strings. A more efficient check would be pointer comparison, though this requires care sincetime.UTCis a specific pointer value.♻️ Suggested optimization (optional)
- needTZConvert := len(tsColProjected) > 0 && sessionLoc.String() != time.UTC.String() + needTZConvert := len(tsColProjected) > 0 && sessionLoc != time.UTCNote: This works because
time.UTCis a package-level variable. However, if session location could be a different*time.Locationinstance that happens to represent UTC, the pointer comparison would incorrectly return true. The string comparison is safer but slower.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/mem_reader.go` at line 1407, The current allocation-heavy timezone check uses sessionLoc.String() != time.UTC.String(); replace this with a pointer comparison to avoid allocations by changing the condition to check sessionLoc != nil && sessionLoc != time.UTC when computing needTZConvert (the variables involved are needTZConvert, tsColProjected and sessionLoc), ensuring you guard against a nil sessionLoc; only use this pointer comparison if semantic equivalence of pointer identity to UTC is acceptable in the codepath.
1351-1356: Simplify mutableRow initialization check.The check
iter.mutableRow.ToRow().Chunk() == nilis an indirect way to detect if the mutableRow was initialized. Consider tracking initialization state explicitly or initializing unconditionally in the constructor.The current pattern works but is non-obvious to future readers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/mem_reader.go` around lines 1351 - 1356, The initialization check for iter.mutableRow using iter.mutableRow.ToRow().Chunk() == nil is non-obvious; change to a clear initialization strategy: either initialize mutableRow unconditionally where it's used (call chunk.MutRowFromTypes(iter.retFieldTypes) if iter.mutableRow is nil) using a direct nil check on iter.mutableRow, or (preferably) initialize mutableRow in the iterator constructor/creator (e.g., New... or whatever builds the iterator) so the runtime path in the filter application (inside the loop that currently does iter.mutableRow.SetDatums(iter.datumRow...)) can assume mutableRow is already created; update references to iter.mutableRow initialization accordingly (look for mutableRow, MutRowFromTypes, and the iterator constructor/newter functions) and remove the indirect ToRow().Chunk() check.
300-317: TIMESTAMP timezone conversion iterates tsColIndices for each output column.The nested loop in
projectCachedIndexDatumschecks alltsColIndicesfor each output offset. For indexes with many columns and multiple TIMESTAMP columns, this could add overhead.Consider building a set or using the projected index directly if performance becomes a concern in profiles.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/mem_reader.go` around lines 300 - 317, projectCachedIndexDatums currently loops over m.indexDatumCache.TsColIndices for every output offset to find TIMESTAMP columns; this is O(n*m) and can be optimized by precomputing a lookup before the outer loop. Change projectCachedIndexDatums to build a set/map (e.g., map[int]struct{}) of ts offsets from m.indexDatumCache.TsColIndices (or otherwise derive the projected timestamp offsets directly) and then, inside the output loop, check membership via the map (using offset) before doing the time conversion that uses m.loc, d.GetMysqlTime(), t.ConvertTimeZone and d.SetMysqlTime.pkg/executor/mem_reader_test.go (1)
228-247: Helper function builds test data with mismatched schema.
buildTestDatumCachecreates a 3-column schema (int64, varchar, int64) but the mock test data at lines 825-826 uses tuples like{int64(1), "pinned", int64(10)}which matches this schema. However, the function is used throughout the tests with this schema, while theCachedDatumDataconsumer inmem_reader.goexpects field types to align with table columns.This is fine for unit testing purposes, but consider adding a comment clarifying the schema for maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/mem_reader_test.go` around lines 228 - 247, The helper buildTestDatumCache constructs a 3-column CachedDatumData with field types (int64, varchar(64), int64) but the test consumer expects table-aligned types; add an inline comment above buildTestDatumCache that documents the exact schema it builds (column 0: int64, column 1: varchar(64), column 2: int64) and clarify that tests supply rows matching that schema so future readers know why the types differ from production table schemas.pkg/table/tables/cached_datum.go (1)
163-168: Handle status check assumes single-column int handle after switch.The
hdStatusdetermination at line 166 checkstps[colsLen]for unsigned flag. This is correct because:
- For
PKIsHandle: single PK column appended at indexcolsLen- For
IsCommonHandle: multiple columns appended, buthdStatusisn't used for common handles- For default (ExtraHandle): single int64 appended
The logic is correct but could benefit from a brief comment explaining why only checking index
colsLenis sufficient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/table/tables/cached_datum.go` around lines 163 - 168, The handle-status check uses tps[colsLen] to decide unsigned-ness but lacks an explanatory comment; add a brief comment above the block (around the hdStatus, colsLen, and tps usage) clarifying that checking tps[colsLen] is sufficient because for PKIsHandle a single PK column is appended at index colsLen, for ExtraHandle the single int64 handle is appended there, and IsCommonHandle does not use hdStatus—this will make the rationale for using tps[colsLen] with tablecodec.HandleDefault/tablecodec.HandleIsUnsigned clear to future readers.pkg/tablecodec/tablecodec.go (1)
1962-1975: Restored values are copied into preallocated resultValues.The decode path now:
- Decodes into
preAlloc[:colsLen]viaCutIndexKeyTo- Decodes restored values into a separate slice
- Copies restored values over the initial decode
This is correct but note that step 1's work is discarded when restored values exist. Consider whether step 1 can be skipped when
segs.RestoredValues != nil.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tablecodec/tablecodec.go` around lines 1962 - 1975, The code currently decodes into resultValues via CutIndexKeyTo and then discards that work when segs.RestoredValues != nil; move the restored-values branch above the call to CutIndexKeyTo so you skip populating resultValues when restored data exists, and instead call CutIndexKeyTo in a mode that doesn't decode into the full result buffer (e.g., pass preAlloc[:0] or support a nil/empty dest) to only obtain keySuffix when needed; update the logic around resultValues, preAlloc, CutIndexKeyTo, segs.RestoredValues, and decodeRestoredValuesV5 so restored values are decoded directly into resultValues and the initial decode is avoided.
🤖 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/executor/builder.go`:
- Around line 6419-6421: The cache-lock update for BatchPointGet incorrectly
runs for INSERT statements because the condition lacks b.inInsertStmt; modify
the condition that guards cachedTable.UpdateLockForRead(...) to also check
!b.inInsertStmt (i.e., if not InExplainStmt and not inDeleteStmt and not
inUpdateStmt and not inInsertStmt) so INSERT/UPDATE/DELETE paths all skip
UpdateLockForRead, keeping cache-lease behavior aligned with
handleCachedTable().
In `@pkg/executor/cached_result_exec.go`:
- Around line 128-131: Open() can observe a specific cache generation that may
be invalidated later; currently Close() always back-fills using
cachedTable.PutCachedResult (lines with e.collecting, e.cacheKey, e.paramBytes,
e.collectedChunks, e.resultSchema) which can reintroduce stale rows after
invalidateResultCache() or lease rollover. Fix by capturing a generation token
or a reference to the result-cache instance in Open() (e.g., store on the
executor struct), then in Close() compare the stored token/instance against the
current cache generation and only call PutCachedResult when they match; drop the
backfill when the token/instance has changed so stale results are not written.
In `@pkg/executor/mem_reader_test.go`:
- Around line 3-27: The test fails to compile because the context package is
used (context.Background() in mem_reader_test.go) but not imported; update the
import block in pkg/executor/mem_reader_test.go to add the "context" package so
references like context.Background() resolve (locate the import list near the
top of mem_reader_test.go where packages such as "testing" and "time" are
declared).
- Around line 1-27: The file mem_reader_test.go in package executor is missing
the standard TiDB Apache-2.0 license header; add the canonical TiDB license
comment block at the top of pkg/executor/mem_reader_test.go (above the package
declaration) by copying the header from a nearby Go source file in the repo and
updating the year if needed so the file begins with the proper license header
before package executor.
In `@pkg/planner/core/casetest/plancache/plan_cache_suite_test.go`:
- Around line 1891-1900: Drop the existing binding created with use_plan_cache()
before installing the new binding that uses ignore_plan_cache(): invoke the
testkit drop-binding call (using tk.MustExec) for the same normalized SQL
("select * from <tableName> where pk >= ?") that was used when creating the
use_plan_cache() binding, then create the ignore_plan_cache() binding and run
the execute/check assertions; this ensures you remove the earlier binding
replacement/selection behavior and exercise only the ignore_plan_cache() policy.
In `@pkg/planner/core/result_cache_check_test.go`:
- Around line 79-105: The test currently logs and skips if the optimizer doesn't
choose the exact plan, letting regressions silently pass; change the checks to
assert the plan type explicitly. Replace the switch+default logging in the
PointGet and BatchPointGet blocks with an assertion like require.IsType(t,
&core.PointGetPlan{}, q.plan) (and require.IsType(t, &core.BatchPointGetPlan{},
q.plan) for the batch test) so the test fails if the optimizer no longer
produces that plan; keep the subsequent require.False(t,
core.CanCacheResultSet(q.stmtNode, q.plan, false)) check unchanged to verify
non-cacheability. Ensure you reference q.plan and q.stmtNode in the assertions
to locate the code.
In `@pkg/planner/core/result_cache_key.go`:
- Around line 142-150: The cache key uses a fixed reference date to compute
timezone offset in appendTZOffset, which can produce stale cache hits across DST
transitions; change the offset calculation to use the current time in the target
location (e.g., time.Now().In(loc).Zone()) instead of time.Date(2000,1,1,...),
keep the nil loc -> time.UTC fallback, preserve the binary encoding of the
offset and the existing appendLengthPrefixedBytes call with loc.String() so the
key still contains both numeric offset and location name.
In `@pkg/table/tables/cache.go`:
- Around line 454-463: getOrCreateResultCache currently returns whatever
c.resultCache.Load() yields after a failed CompareAndSwap, which can be nil if
invalidateResultCache removed it concurrently; change getOrCreateResultCache to
loop: load the current value, if non-nil return it, otherwise allocate a new
resultSetCache and attempt CompareAndSwap(nil, rc) and if it succeeds return rc,
if it fails repeat the loop until a non-nil cache is observed—this ensures
callers (e.g., where rc.put(...) is invoked) never receive a nil resultCache
even under concurrent invalidateResultCache calls.
In `@pkg/table/tables/cached_datum.go`:
- Around line 1-14: Update the license header year in
pkg/table/tables/cached_datum.go from 2025 to 2026; locate the top-of-file
copyright block (the Apache License header) and change the year value to 2026 so
it matches other new files in the PR (e.g., decoder_test.go).
In `@pkg/types/time.go`:
- Around line 2910-2943: In AppendString, guard the current "fast path" that
writes digits directly by validating Year() < 10000 and Microsecond() < 1000000
and that Fsp() is within 0..6; if any of those bounds are violated, fall back to
the safe numeric formatting path (e.g., call the generic formatter/String() or a
helper that uses strconv/fmt to format each component) instead of emitting
punctuation; update the AppendString implementation (and checks around the
mysql.TypeDate branch) to perform these bounds checks before using
appendInt4/appendInt2 and the digits array so out-of-range Years or Microsecond
values render via the fallback numeric formatting.
In `@pkg/util/chunk/pool.go`:
- Around line 115-119: PutChunk currently does an O(n²) duplicate-column sweep
by, for each column c, scanning remaining indices and niling matches; replace
this with an O(n) dedupe using a seen set. In PutChunk iterate once over
chk.columns (or fields) and maintain a map[*Column]struct{} called seen; on each
index i check if chk.columns[i] is nil or already in seen — if already seen set
chk.columns[i]=nil, otherwise add the pointer to seen. This removes the inner
loop and preserves the existing behavior of nil-ing duplicate column pointers.
In `@pkg/util/rowcodec/bench_test.go`:
- Around line 109-113: The call to decoder.DecodeToChunk in bench_test.go is
missing the required commitTS uint64 argument; update the
decoder.DecodeToChunk(rowData, kv.IntHandle(r), chk) call to include a commitTS
value as the second parameter (e.g. decoder.DecodeToChunk(rowData, commitTS,
kv.IntHandle(r), chk)), using an existing commitTS variable if available or a
sensible constant (0 or a test-specific timestamp) so the call matches the
signature DecodeToChunk(rowData []byte, commitTS uint64, handle kv.Handle, chk
*chunk.Chunk).
---
Nitpick comments:
In `@pkg/executor/builder.go`:
- Around line 1505-1557: The removeRedundantAccessConditions function
reimplements ranger's shouldReserve logic which risks divergence; extract the
reservation-checking logic into a shared helper used by both the ranger and this
executor builder (or call the existing ranger.ShouldReserve/appropriate exported
function) so UnionScan doesn't re-encode rules locally. Change
removeRedundantAccessConditions (and any callers) to rely on that shared helper
(referencing removeRedundantAccessConditions, accessConds, idxCols, idxColLens)
instead of computing fullLenColIDs and canonical-hash comparisons here, ensuring
the canonical-hash cloning behavior is preserved inside the shared helper if
needed to avoid races.
In `@pkg/executor/mem_reader_test.go`:
- Around line 228-247: The helper buildTestDatumCache constructs a 3-column
CachedDatumData with field types (int64, varchar(64), int64) but the test
consumer expects table-aligned types; add an inline comment above
buildTestDatumCache that documents the exact schema it builds (column 0: int64,
column 1: varchar(64), column 2: int64) and clarify that tests supply rows
matching that schema so future readers know why the types differ from production
table schemas.
In `@pkg/executor/mem_reader.go`:
- Line 1407: The current allocation-heavy timezone check uses
sessionLoc.String() != time.UTC.String(); replace this with a pointer comparison
to avoid allocations by changing the condition to check sessionLoc != nil &&
sessionLoc != time.UTC when computing needTZConvert (the variables involved are
needTZConvert, tsColProjected and sessionLoc), ensuring you guard against a nil
sessionLoc; only use this pointer comparison if semantic equivalence of pointer
identity to UTC is acceptable in the codepath.
- Around line 1351-1356: The initialization check for iter.mutableRow using
iter.mutableRow.ToRow().Chunk() == nil is non-obvious; change to a clear
initialization strategy: either initialize mutableRow unconditionally where it's
used (call chunk.MutRowFromTypes(iter.retFieldTypes) if iter.mutableRow is nil)
using a direct nil check on iter.mutableRow, or (preferably) initialize
mutableRow in the iterator constructor/creator (e.g., New... or whatever builds
the iterator) so the runtime path in the filter application (inside the loop
that currently does iter.mutableRow.SetDatums(iter.datumRow...)) can assume
mutableRow is already created; update references to iter.mutableRow
initialization accordingly (look for mutableRow, MutRowFromTypes, and the
iterator constructor/newter functions) and remove the indirect ToRow().Chunk()
check.
- Around line 300-317: projectCachedIndexDatums currently loops over
m.indexDatumCache.TsColIndices for every output offset to find TIMESTAMP
columns; this is O(n*m) and can be optimized by precomputing a lookup before the
outer loop. Change projectCachedIndexDatums to build a set/map (e.g.,
map[int]struct{}) of ts offsets from m.indexDatumCache.TsColIndices (or
otherwise derive the projected timestamp offsets directly) and then, inside the
output loop, check membership via the map (using offset) before doing the time
conversion that uses m.loc, d.GetMysqlTime(), t.ConvertTimeZone and
d.SetMysqlTime.
In `@pkg/expression/constant.go`:
- Around line 246-259: The getTypeNonAlloc function currently swallows errors
from c.ParamMarker.GetUserVar and returns nil, reducing observability; update
getTypeNonAlloc toSurface the failure (either by logging the error via
logutil.BgLogger().Warn with context or by returning an error value) instead of
silently returning nil: locate getTypeNonAlloc (and the c.ParamMarker.GetUserVar
call), ensure types.InitUnspecifiedFieldType(buf) and
types.InferParamTypeFromDatum(&dt, buf) remain unchanged on success, and on
GetUserVar failure emit a warning including the error and parameter identifier
(or change the signature to return ( *types.FieldType, error ) so callers can
handle the error) to preserve debuggability in the hot path.
In `@pkg/table/tables/cached_datum.go`:
- Around line 163-168: The handle-status check uses tps[colsLen] to decide
unsigned-ness but lacks an explanatory comment; add a brief comment above the
block (around the hdStatus, colsLen, and tps usage) clarifying that checking
tps[colsLen] is sufficient because for PKIsHandle a single PK column is appended
at index colsLen, for ExtraHandle the single int64 handle is appended there, and
IsCommonHandle does not use hdStatus—this will make the rationale for using
tps[colsLen] with tablecodec.HandleDefault/tablecodec.HandleIsUnsigned clear to
future readers.
In `@pkg/tablecodec/tablecodec.go`:
- Around line 1962-1975: The code currently decodes into resultValues via
CutIndexKeyTo and then discards that work when segs.RestoredValues != nil; move
the restored-values branch above the call to CutIndexKeyTo so you skip
populating resultValues when restored data exists, and instead call
CutIndexKeyTo in a mode that doesn't decode into the full result buffer (e.g.,
pass preAlloc[:0] or support a nil/empty dest) to only obtain keySuffix when
needed; update the logic around resultValues, preAlloc, CutIndexKeyTo,
segs.RestoredValues, and decodeRestoredValuesV5 so restored values are decoded
directly into resultValues and the initial decode is avoided.
In `@pkg/util/chunk/chunk_test.go`:
- Around line 778-796: The test TestColumnNullBitmapAfterAppend only appends 4
rows so it never exercises the second null-bitmap byte; update the test to
append past the 8-row boundary (e.g., use NewChunkWithCapacity(fieldTypes, 16)
and perform the AppendInt64/AppendNull pattern until at least 9 rows) and then
assert IsNull/values for entries across the byte boundary (reference functions
NewChunkWithCapacity, AppendInt64, AppendNull, NumRows, Column, IsNull, GetRow).
In `@pkg/util/hint/hint.go`:
- Around line 415-418: Both plan-cache hints currently set flags independently
(case HintIgnorePlanCache sets stmtHints.IgnorePlanCache and case
HintUsePlanCache sets stmtHints.UsePlanCache) so if both appear no parser-level
conflict is reported; change each case to first check whether the opposite flag
is already true (e.g., when setting stmtHints.IgnorePlanCache check
stmtHints.UsePlanCache and vice‑versa) and emit a parser-level warning about
conflicting hints via the existing warning API (or add a small helper to record
a parse warning) instead of silently setting both; ensure the check references
HintIgnorePlanCache, HintUsePlanCache, stmtHints.IgnorePlanCache and
stmtHints.UsePlanCache so the conflict is detected and reported.
In `@pkg/util/rowcodec/bench_test.go`:
- Around line 100-106: Replace non-deterministic time.Local with a fixed
timezone in the benchmark: update the calls to enc.Encode(...) and
rowcodec.NewChunkDecoder(...) that currently pass time.Local so they use
time.UTC (or an explicit fixed zone) instead, ensuring the Encode and
NewChunkDecoder invocations (and any related benchmark setup using time.Local)
use the fixed timezone for stable deterministic runs.
In `@pkg/util/rowcodec/decoder.go`:
- Around line 673-719: Document the lifetime and sizing contract for
DecodeToBytesNoHandleInto: update the function comment for
DecodeToBytesNoHandleInto to explicitly state that returned values[i] may alias
the provided arena (via encodeOldDatumToArena) and therefore become invalid as
soon as the caller reuses or resets arena (caller must copy any needed bytes
before reusing arena), and also state that the caller must ensure the values
slice is large enough to contain entries for the maximum index referenced by
outputOffset (i.e., values must have length > max(outputOffset) and will be
reused/cleared by the call). Also mention that the caller should reset arena
between rows (arena = arena[:0]) as already expected.
🪄 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: d2c05633-a5ce-4ac7-95df-c7d891a5caf2
📒 Files selected for processing (67)
pkg/executor/BUILD.bazelpkg/executor/adapter.gopkg/executor/adapter_slow_log.gopkg/executor/builder.gopkg/executor/cached_result_exec.gopkg/executor/cached_result_exec_test.gopkg/executor/mem_reader.gopkg/executor/mem_reader_test.gopkg/executor/prepared.gopkg/executor/slow_query.gopkg/executor/union_scan.gopkg/expression/builtin_compare.gopkg/expression/constant.gopkg/expression/util.gopkg/infoschema/tables.gopkg/metrics/executor.gopkg/metrics/metrics.gopkg/parser/ast/misc.gopkg/planner/core/BUILD.bazelpkg/planner/core/casetest/plancache/BUILD.bazelpkg/planner/core/casetest/plancache/plan_cache_suite_test.gopkg/planner/core/plan_cache.gopkg/planner/core/plan_cache_utils.gopkg/planner/core/result_cache_check.gopkg/planner/core/result_cache_check_test.gopkg/planner/core/result_cache_key.gopkg/planner/core/result_cache_key_test.gopkg/server/internal/column/column.gopkg/sessionctx/stmtctx/stmtctx.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/session.gopkg/sessionctx/variable/setvar_affect.gopkg/sessionctx/variable/slow_log.gopkg/sessionctx/variable/sysvar.gopkg/table/table.gopkg/table/tables/BUILD.bazelpkg/table/tables/cache.gopkg/table/tables/cache_test.gopkg/table/tables/cache_tz_test.gopkg/table/tables/cached_datum.gopkg/table/tables/cached_datum_test.gopkg/table/tables/result_cache.gopkg/table/tables/result_cache_test.gopkg/tablecodec/BUILD.bazelpkg/tablecodec/bench_test.gopkg/tablecodec/tablecodec.gopkg/tablecodec/tablecodec_test.gopkg/types/field_type.gopkg/types/time.gopkg/types/time_test.gopkg/util/chunk/chunk_test.gopkg/util/chunk/column.gopkg/util/chunk/pool.gopkg/util/chunk/pool_test.gopkg/util/codec/codec.gopkg/util/codec/codec_test.gopkg/util/execdetails/execdetails.gopkg/util/execdetails/execdetails_test.gopkg/util/execdetails/runtime_stats.gopkg/util/hint/hint.gopkg/util/rowcodec/BUILD.bazelpkg/util/rowcodec/bench_test.gopkg/util/rowcodec/common.gopkg/util/rowcodec/decoder.gopkg/util/rowcodec/decoder_test.gopkg/util/rowcodec/encoder.gopkg/util/rowcodec/row.go
Comment on lines
6419
to
6421
| if !b.ctx.GetSessionVars().StmtCtx.InExplainStmt && !b.inDeleteStmt && !b.inUpdateStmt { | ||
| tbl.(table.CachedTable).UpdateLockForRead(context.Background(), b.ctx.GetStore(), startTS, leaseDuration) | ||
| cachedTable.UpdateLockForRead(context.Background(), b.ctx.GetStore(), startTS, leaseDuration) | ||
| } |
There was a problem hiding this comment.
Keep BatchPointGet's cache-lock guard aligned with the scan path.
handleCachedTable() skips UpdateLockForRead() for INSERT/UPDATE/DELETE, but this branch omits b.inInsertStmt. That makes cache-lease behavior depend on plan shape: INSERT ... SELECT that lands on BatchPointGet can still refresh/load the table cache while the scan paths do not.
Suggested fix
- if !b.ctx.GetSessionVars().StmtCtx.InExplainStmt && !b.inDeleteStmt && !b.inUpdateStmt {
+ if !b.ctx.GetSessionVars().StmtCtx.InExplainStmt && !b.inDeleteStmt && !b.inUpdateStmt && !b.inInsertStmt {
cachedTable.UpdateLockForRead(context.Background(), b.ctx.GetStore(), startTS, leaseDuration)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/executor/builder.go` around lines 6419 - 6421, The cache-lock update for
BatchPointGet incorrectly runs for INSERT statements because the condition lacks
b.inInsertStmt; modify the condition that guards
cachedTable.UpdateLockForRead(...) to also check !b.inInsertStmt (i.e., if not
InExplainStmt and not inDeleteStmt and not inUpdateStmt and not inInsertStmt) so
INSERT/UPDATE/DELETE paths all skip UpdateLockForRead, keeping cache-lease
behavior aligned with handleCachedTable().
Comment on lines
+128
to
+131
| // Back-fill the cache only after the wrapped executor closes successfully. | ||
| if e.collecting { | ||
| e.cachedTable.PutCachedResult(e.cacheKey, e.paramBytes, e.collectedChunks, e.resultSchema) | ||
| } |
There was a problem hiding this comment.
Pin the backfill to the cache generation observed in Open().
After invalidateResultCache() on a lease rollover or write lock, a long-running statement can still reach Lines 129-131 and populate the current cache with rows computed from the old snapshot. That reintroduces stale results immediately after invalidation. Capture a generation token/result-cache instance in Open() and drop the backfill when it no longer matches at Close().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/executor/cached_result_exec.go` around lines 128 - 131, Open() can
observe a specific cache generation that may be invalidated later; currently
Close() always back-fills using cachedTable.PutCachedResult (lines with
e.collecting, e.cacheKey, e.paramBytes, e.collectedChunks, e.resultSchema) which
can reintroduce stale rows after invalidateResultCache() or lease rollover. Fix
by capturing a generation token or a reference to the result-cache instance in
Open() (e.g., store on the executor struct), then in Close() compare the stored
token/instance against the current cache generation and only call
PutCachedResult when they match; drop the backfill when the token/instance has
changed so stale results are not written.
Comment on lines
+1
to
+27
| package executor | ||
|
|
||
| import ( | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/pingcap/tidb/pkg/executor/internal/exec" | ||
| "github.com/pingcap/tidb/pkg/expression" | ||
| "github.com/pingcap/tidb/pkg/kv" | ||
| "github.com/pingcap/tidb/pkg/meta/model" | ||
| "github.com/pingcap/tidb/pkg/parser/ast" | ||
| pmodel "github.com/pingcap/tidb/pkg/parser/model" | ||
| "github.com/pingcap/tidb/pkg/parser/mysql" | ||
| "github.com/pingcap/tidb/pkg/sessionctx/stmtctx" | ||
| "github.com/pingcap/tidb/pkg/store/mockstore" | ||
| "github.com/pingcap/tidb/pkg/table" | ||
| "github.com/pingcap/tidb/pkg/table/tables" | ||
| "github.com/pingcap/tidb/pkg/tablecodec" | ||
| "github.com/pingcap/tidb/pkg/types" | ||
| "github.com/pingcap/tidb/pkg/util/chunk" | ||
| "github.com/pingcap/tidb/pkg/util/codec" | ||
| "github.com/pingcap/tidb/pkg/util/mock" | ||
| "github.com/pingcap/tidb/pkg/util/rowcodec" | ||
| "github.com/pingcap/tidb/pkg/util/sqlexec" | ||
| "github.com/stretchr/testify/require" | ||
| "github.com/tikv/client-go/v2/tikv" | ||
| ) |
There was a problem hiding this comment.
Missing standard TiDB license header.
New Go source files must include the standard Apache 2.0 license header. As per coding guidelines, copy from a nearby file and update the year if needed.
📄 Proposed fix: Add license header
+// Copyright 2026 PingCAP, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
package executor📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| package executor | |
| import ( | |
| "testing" | |
| "time" | |
| "github.com/pingcap/tidb/pkg/executor/internal/exec" | |
| "github.com/pingcap/tidb/pkg/expression" | |
| "github.com/pingcap/tidb/pkg/kv" | |
| "github.com/pingcap/tidb/pkg/meta/model" | |
| "github.com/pingcap/tidb/pkg/parser/ast" | |
| pmodel "github.com/pingcap/tidb/pkg/parser/model" | |
| "github.com/pingcap/tidb/pkg/parser/mysql" | |
| "github.com/pingcap/tidb/pkg/sessionctx/stmtctx" | |
| "github.com/pingcap/tidb/pkg/store/mockstore" | |
| "github.com/pingcap/tidb/pkg/table" | |
| "github.com/pingcap/tidb/pkg/table/tables" | |
| "github.com/pingcap/tidb/pkg/tablecodec" | |
| "github.com/pingcap/tidb/pkg/types" | |
| "github.com/pingcap/tidb/pkg/util/chunk" | |
| "github.com/pingcap/tidb/pkg/util/codec" | |
| "github.com/pingcap/tidb/pkg/util/mock" | |
| "github.com/pingcap/tidb/pkg/util/rowcodec" | |
| "github.com/pingcap/tidb/pkg/util/sqlexec" | |
| "github.com/stretchr/testify/require" | |
| "github.com/tikv/client-go/v2/tikv" | |
| ) | |
| // Copyright 2026 PingCAP, Inc. | |
| // | |
| // Licensed under the Apache License, Version 2.0 (the "License"); | |
| // you may not use this file except in compliance with the License. | |
| // You may obtain a copy of the License at | |
| // | |
| // http://www.apache.org/licenses/LICENSE-2.0 | |
| // | |
| // Unless required by applicable law or agreed to in writing, software | |
| // distributed under the License is distributed on an "AS IS" BASIS, | |
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | |
| // See the License for the specific language governing permissions and | |
| // limitations under the License. | |
| package executor | |
| import ( | |
| "testing" | |
| "time" | |
| "github.com/pingcap/tidb/pkg/executor/internal/exec" | |
| "github.com/pingcap/tidb/pkg/expression" | |
| "github.com/pingcap/tidb/pkg/kv" | |
| "github.com/pingcap/tidb/pkg/meta/model" | |
| "github.com/pingcap/tidb/pkg/parser/ast" | |
| pmodel "github.com/pingcap/tidb/pkg/parser/model" | |
| "github.com/pingcap/tidb/pkg/parser/mysql" | |
| "github.com/pingcap/tidb/pkg/sessionctx/stmtctx" | |
| "github.com/pingcap/tidb/pkg/store/mockstore" | |
| "github.com/pingcap/tidb/pkg/table" | |
| "github.com/pingcap/tidb/pkg/table/tables" | |
| "github.com/pingcap/tidb/pkg/tablecodec" | |
| "github.com/pingcap/tidb/pkg/types" | |
| "github.com/pingcap/tidb/pkg/util/chunk" | |
| "github.com/pingcap/tidb/pkg/util/codec" | |
| "github.com/pingcap/tidb/pkg/util/mock" | |
| "github.com/pingcap/tidb/pkg/util/rowcodec" | |
| "github.com/pingcap/tidb/pkg/util/sqlexec" | |
| "github.com/stretchr/testify/require" | |
| "github.com/tikv/client-go/v2/tikv" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/executor/mem_reader_test.go` around lines 1 - 27, The file
mem_reader_test.go in package executor is missing the standard TiDB Apache-2.0
license header; add the canonical TiDB license comment block at the top of
pkg/executor/mem_reader_test.go (above the package declaration) by copying the
header from a nearby Go source file in the repo and updating the year if needed
so the file begins with the proper license header before package executor.
Comment on lines
+3
to
+27
| import ( | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/pingcap/tidb/pkg/executor/internal/exec" | ||
| "github.com/pingcap/tidb/pkg/expression" | ||
| "github.com/pingcap/tidb/pkg/kv" | ||
| "github.com/pingcap/tidb/pkg/meta/model" | ||
| "github.com/pingcap/tidb/pkg/parser/ast" | ||
| pmodel "github.com/pingcap/tidb/pkg/parser/model" | ||
| "github.com/pingcap/tidb/pkg/parser/mysql" | ||
| "github.com/pingcap/tidb/pkg/sessionctx/stmtctx" | ||
| "github.com/pingcap/tidb/pkg/store/mockstore" | ||
| "github.com/pingcap/tidb/pkg/table" | ||
| "github.com/pingcap/tidb/pkg/table/tables" | ||
| "github.com/pingcap/tidb/pkg/tablecodec" | ||
| "github.com/pingcap/tidb/pkg/types" | ||
| "github.com/pingcap/tidb/pkg/util/chunk" | ||
| "github.com/pingcap/tidb/pkg/util/codec" | ||
| "github.com/pingcap/tidb/pkg/util/mock" | ||
| "github.com/pingcap/tidb/pkg/util/rowcodec" | ||
| "github.com/pingcap/tidb/pkg/util/sqlexec" | ||
| "github.com/stretchr/testify/require" | ||
| "github.com/tikv/client-go/v2/tikv" | ||
| ) |
There was a problem hiding this comment.
Missing context import causes compilation failure.
The test file uses context.Background() at lines 605 and 700, but the context package is not imported.
🐛 Proposed fix
import (
+ "context"
"testing"
"time"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import ( | |
| "testing" | |
| "time" | |
| "github.com/pingcap/tidb/pkg/executor/internal/exec" | |
| "github.com/pingcap/tidb/pkg/expression" | |
| "github.com/pingcap/tidb/pkg/kv" | |
| "github.com/pingcap/tidb/pkg/meta/model" | |
| "github.com/pingcap/tidb/pkg/parser/ast" | |
| pmodel "github.com/pingcap/tidb/pkg/parser/model" | |
| "github.com/pingcap/tidb/pkg/parser/mysql" | |
| "github.com/pingcap/tidb/pkg/sessionctx/stmtctx" | |
| "github.com/pingcap/tidb/pkg/store/mockstore" | |
| "github.com/pingcap/tidb/pkg/table" | |
| "github.com/pingcap/tidb/pkg/table/tables" | |
| "github.com/pingcap/tidb/pkg/tablecodec" | |
| "github.com/pingcap/tidb/pkg/types" | |
| "github.com/pingcap/tidb/pkg/util/chunk" | |
| "github.com/pingcap/tidb/pkg/util/codec" | |
| "github.com/pingcap/tidb/pkg/util/mock" | |
| "github.com/pingcap/tidb/pkg/util/rowcodec" | |
| "github.com/pingcap/tidb/pkg/util/sqlexec" | |
| "github.com/stretchr/testify/require" | |
| "github.com/tikv/client-go/v2/tikv" | |
| ) | |
| import ( | |
| "context" | |
| "testing" | |
| "time" | |
| "github.com/pingcap/tidb/pkg/executor/internal/exec" | |
| "github.com/pingcap/tidb/pkg/expression" | |
| "github.com/pingcap/tidb/pkg/kv" | |
| "github.com/pingcap/tidb/pkg/meta/model" | |
| "github.com/pingcap/tidb/pkg/parser/ast" | |
| pmodel "github.com/pingcap/tidb/pkg/parser/model" | |
| "github.com/pingcap/tidb/pkg/parser/mysql" | |
| "github.com/pingcap/tidb/pkg/sessionctx/stmtctx" | |
| "github.com/pingcap/tidb/pkg/store/mockstore" | |
| "github.com/pingcap/tidb/pkg/table" | |
| "github.com/pingcap/tidb/pkg/table/tables" | |
| "github.com/pingcap/tidb/pkg/tablecodec" | |
| "github.com/pingcap/tidb/pkg/types" | |
| "github.com/pingcap/tidb/pkg/util/chunk" | |
| "github.com/pingcap/tidb/pkg/util/codec" | |
| "github.com/pingcap/tidb/pkg/util/mock" | |
| "github.com/pingcap/tidb/pkg/util/rowcodec" | |
| "github.com/pingcap/tidb/pkg/util/sqlexec" | |
| "github.com/stretchr/testify/require" | |
| "github.com/tikv/client-go/v2/tikv" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/executor/mem_reader_test.go` around lines 3 - 27, The test fails to
compile because the context package is used (context.Background() in
mem_reader_test.go) but not imported; update the import block in
pkg/executor/mem_reader_test.go to add the "context" package so references like
context.Background() resolve (locate the import list near the top of
mem_reader_test.go where packages such as "testing" and "time" are declared).
Comment on lines
+1891
to
+1900
| tk.MustExec(fmt.Sprintf( | ||
| "CREATE BINDING FOR select * from %s where pk >= ? USING select /*+ ignore_plan_cache() */ * from %s where pk >= ?", | ||
| tableName, tableName, | ||
| )) | ||
| tk.MustExec("execute st using @a") | ||
| tk.MustQuery(`select @@last_plan_from_cache`).Check(testkit.Rows("0")) | ||
| tk.MustExec("execute st using @a") | ||
| tk.MustQuery(`select @@last_plan_from_cache`).Check(testkit.Rows("0")) | ||
| tk.MustExec("execute st using @a") | ||
| tk.MustQuery(`select @@last_plan_from_cache`).Check(testkit.Rows("0")) |
There was a problem hiding this comment.
Drop the earlier binding before installing ignore_plan_cache().
Lines 1891-1894 target the same normalized SQL as the use_plan_cache() binding created on Lines 1880-1883. Without an explicit DROP BINDING, the final assertions depend on binding replacement/selection behavior instead of only the policy being tested.
Suggested fix
+ tk.MustExec(fmt.Sprintf("DROP BINDING FOR select * from %s where pk >= ?", tableName))
tk.MustExec(fmt.Sprintf(
"CREATE BINDING FOR select * from %s where pk >= ? USING select /*+ ignore_plan_cache() */ * from %s where pk >= ?",
tableName, tableName,
))As per coding guidelines, "Keep test changes minimal and deterministic; avoid broad golden/testdata churn unless required."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| tk.MustExec(fmt.Sprintf( | |
| "CREATE BINDING FOR select * from %s where pk >= ? USING select /*+ ignore_plan_cache() */ * from %s where pk >= ?", | |
| tableName, tableName, | |
| )) | |
| tk.MustExec("execute st using @a") | |
| tk.MustQuery(`select @@last_plan_from_cache`).Check(testkit.Rows("0")) | |
| tk.MustExec("execute st using @a") | |
| tk.MustQuery(`select @@last_plan_from_cache`).Check(testkit.Rows("0")) | |
| tk.MustExec("execute st using @a") | |
| tk.MustQuery(`select @@last_plan_from_cache`).Check(testkit.Rows("0")) | |
| tk.MustExec(fmt.Sprintf("DROP BINDING FOR select * from %s where pk >= ?", tableName)) | |
| tk.MustExec(fmt.Sprintf( | |
| "CREATE BINDING FOR select * from %s where pk >= ? USING select /*+ ignore_plan_cache() */ * from %s where pk >= ?", | |
| tableName, tableName, | |
| )) | |
| tk.MustExec("execute st using `@a`") | |
| tk.MustQuery(`select @@last_plan_from_cache`).Check(testkit.Rows("0")) | |
| tk.MustExec("execute st using `@a`") | |
| tk.MustQuery(`select @@last_plan_from_cache`).Check(testkit.Rows("0")) | |
| tk.MustExec("execute st using `@a`") | |
| tk.MustQuery(`select @@last_plan_from_cache`).Check(testkit.Rows("0")) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/planner/core/casetest/plancache/plan_cache_suite_test.go` around lines
1891 - 1900, Drop the existing binding created with use_plan_cache() before
installing the new binding that uses ignore_plan_cache(): invoke the testkit
drop-binding call (using tk.MustExec) for the same normalized SQL ("select *
from <tableName> where pk >= ?") that was used when creating the
use_plan_cache() binding, then create the ignore_plan_cache() binding and run
the execute/check assertions; this ensures you remove the earlier binding
replacement/selection behavior and exercise only the ignore_plan_cache() policy.
Comment on lines
+454
to
+463
| func (c *cachedTable) getOrCreateResultCache() *resultSetCache { | ||
| if rc := c.resultCache.Load(); rc != nil { | ||
| return rc | ||
| } | ||
| rc := newResultSetCache() | ||
| if c.resultCache.CompareAndSwap(nil, rc) { | ||
| return rc | ||
| } | ||
| return c.resultCache.Load() | ||
| } |
There was a problem hiding this comment.
Retry the CAS here instead of returning a possibly nil cache.
If another goroutine installs a cache and invalidateResultCache() removes it before Line 462, getOrCreateResultCache() returns nil and Line 488 panics on rc.put(...).
Suggested fix
func (c *cachedTable) getOrCreateResultCache() *resultSetCache {
- if rc := c.resultCache.Load(); rc != nil {
- return rc
- }
- rc := newResultSetCache()
- if c.resultCache.CompareAndSwap(nil, rc) {
- return rc
- }
- return c.resultCache.Load()
+ for {
+ if rc := c.resultCache.Load(); rc != nil {
+ return rc
+ }
+ rc := newResultSetCache()
+ if c.resultCache.CompareAndSwap(nil, rc) {
+ return rc
+ }
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/table/tables/cache.go` around lines 454 - 463, getOrCreateResultCache
currently returns whatever c.resultCache.Load() yields after a failed
CompareAndSwap, which can be nil if invalidateResultCache removed it
concurrently; change getOrCreateResultCache to loop: load the current value, if
non-nil return it, otherwise allocate a new resultSetCache and attempt
CompareAndSwap(nil, rc) and if it succeeds return rc, if it fails repeat the
loop until a non-nil cache is observed—this ensures callers (e.g., where
rc.put(...) is invoked) never receive a nil resultCache even under concurrent
invalidateResultCache calls.
Comment on lines
+1
to
+14
| // Copyright 2025 PingCAP, Inc. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
There was a problem hiding this comment.
License header year is 2025.
The copyright year is 2025. Given the current date context is March 2026, this should likely be updated to 2026 for consistency with other new files in this PR (e.g., decoder_test.go uses 2026).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/table/tables/cached_datum.go` around lines 1 - 14, Update the license
header year in pkg/table/tables/cached_datum.go from 2025 to 2026; locate the
top-of-file copyright block (the Apache License header) and change the year
value to 2026 so it matches other new files in the PR (e.g., decoder_test.go).
Comment on lines
+2910
to
+2943
| // AppendString appends the formatted time string to buf and returns the result. | ||
| // It avoids fmt/bytes.Buffer; allocations occur only if buf needs to grow. | ||
| func (t Time) AppendString(buf []byte) []byte { | ||
| buf = appendInt4(buf, t.Year()) | ||
| buf = append(buf, '-') | ||
| buf = appendInt2(buf, t.Month()) | ||
| buf = append(buf, '-') | ||
| buf = appendInt2(buf, t.Day()) | ||
|
|
||
| if t.Type() == mysql.TypeDate { | ||
| return buf | ||
| } | ||
|
|
||
| buf = append(buf, ' ') | ||
| buf = appendInt2(buf, t.Hour()) | ||
| buf = append(buf, ':') | ||
| buf = appendInt2(buf, t.Minute()) | ||
| buf = append(buf, ':') | ||
| buf = appendInt2(buf, t.Second()) | ||
|
|
||
| fsp := t.Fsp() | ||
| if fsp > 0 { | ||
| buf = append(buf, '.') | ||
| micro := t.Microsecond() | ||
| digits := [6]byte{ | ||
| byte('0' + micro/100000), | ||
| byte('0' + (micro/10000)%10), | ||
| byte('0' + (micro/1000)%10), | ||
| byte('0' + (micro/100)%10), | ||
| byte('0' + (micro/10)%10), | ||
| byte('0' + micro%10), | ||
| } | ||
| buf = append(buf, digits[:fsp]...) | ||
| } |
There was a problem hiding this comment.
Guard the fast path for out-of-range year and fractional fields.
This path assumes Year() < 10000 and Microsecond() < 1000000. Time can still carry larger representable values before normalization/validation, and those will render as punctuation here (:000, :00000) instead of numeric text. Please keep the fast path for normal values, but fall back to numeric formatting when those bounds are exceeded.
Suggested fallback
func (t Time) AppendString(buf []byte) []byte {
- buf = appendInt4(buf, t.Year())
+ year := t.Year()
+ if year < 10000 {
+ buf = appendInt4(buf, year)
+ } else {
+ buf = strconv.AppendInt(buf, int64(year), 10)
+ }
buf = append(buf, '-')
buf = appendInt2(buf, t.Month())
buf = append(buf, '-')
buf = appendInt2(buf, t.Day())
@@
fsp := t.Fsp()
if fsp > 0 {
buf = append(buf, '.')
micro := t.Microsecond()
- digits := [6]byte{
- byte('0' + micro/100000),
- byte('0' + (micro/10000)%10),
- byte('0' + (micro/1000)%10),
- byte('0' + (micro/100)%10),
- byte('0' + (micro/10)%10),
- byte('0' + micro%10),
- }
- buf = append(buf, digits[:fsp]...)
+ if micro < 1000000 {
+ digits := [6]byte{
+ byte('0' + micro/100000),
+ byte('0' + (micro/10000)%10),
+ byte('0' + (micro/1000)%10),
+ byte('0' + (micro/100)%10),
+ byte('0' + (micro/10)%10),
+ byte('0' + micro%10),
+ }
+ buf = append(buf, digits[:fsp]...)
+ } else {
+ frac := FormatIntWidthN(micro, 6)
+ buf = append(buf, frac[:fsp]...)
+ }
}
return buf
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/types/time.go` around lines 2910 - 2943, In AppendString, guard the
current "fast path" that writes digits directly by validating Year() < 10000 and
Microsecond() < 1000000 and that Fsp() is within 0..6; if any of those bounds
are violated, fall back to the safe numeric formatting path (e.g., call the
generic formatter/String() or a helper that uses strconv/fmt to format each
component) instead of emitting punctuation; update the AppendString
implementation (and checks around the mysql.TypeDate branch) to perform these
bounds checks before using appendInt4/appendInt2 and the digits array so
out-of-range Years or Microsecond values render via the fallback numeric
formatting.
Comment on lines
+115
to
+119
| for j := i + 1; j < len(fields); j++ { | ||
| if chk.columns[j] == c { | ||
| chk.columns[j] = nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid quadratic duplicate-column scanning in PutChunk.
At Line 115, scanning the remaining columns for every index makes PutChunk O(n²). This path is hot, so wide chunks can see measurable recycle overhead. Prefer O(n) dedup via a seen set of *Column.
⚙️ Proposed O(n) fix
func (p *Pool) PutChunk(fields []*types.FieldType, chk *Chunk) {
+ seen := make(map[*Column]struct{}, len(fields))
for i, f := range fields {
c := chk.columns[i]
- // Some chunk columns are references to other columns. Avoid putting the
- // same Column back into the pool multiple times.
if c == nil {
continue
}
- for j := i + 1; j < len(fields); j++ {
- if chk.columns[j] == c {
- chk.columns[j] = nil
- }
+ if _, ok := seen[c]; ok {
+ chk.columns[i] = nil
+ continue
}
+ seen[c] = struct{}{}
c.reset()
switch elemLen := getFixedLen(f); elemLen {
case VarElemLen:
p.varLenColPool.Put(c)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for j := i + 1; j < len(fields); j++ { | |
| if chk.columns[j] == c { | |
| chk.columns[j] = nil | |
| } | |
| } | |
| func (p *Pool) PutChunk(fields []*types.FieldType, chk *Chunk) { | |
| seen := make(map[*Column]struct{}, len(fields)) | |
| for i, f := range fields { | |
| c := chk.columns[i] | |
| if c == nil { | |
| continue | |
| } | |
| if _, ok := seen[c]; ok { | |
| chk.columns[i] = nil | |
| continue | |
| } | |
| seen[c] = struct{}{} | |
| c.reset() | |
| switch elemLen := getFixedLen(f); elemLen { | |
| case VarElemLen: | |
| p.varLenColPool.Put(c) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/util/chunk/pool.go` around lines 115 - 119, PutChunk currently does an
O(n²) duplicate-column sweep by, for each column c, scanning remaining indices
and niling matches; replace this with an O(n) dedupe using a seen set. In
PutChunk iterate once over chk.columns (or fields) and maintain a
map[*Column]struct{} called seen; on each index i check if chk.columns[i] is nil
or already in seen — if already seen set chk.columns[i]=nil, otherwise add the
pointer to seen. This removes the inner loop and preserves the existing behavior
of nil-ing duplicate column pointers.
Comment on lines
+109
to
+113
| for range b.N { | ||
| chk.Reset() | ||
| for r := 0; r < batchSize; r++ { | ||
| if err := decoder.DecodeToChunk(rowData, kv.IntHandle(r), chk); err != nil { | ||
| b.Fatal(err) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== DecodeToChunk method definitions =="
rg -nP --type=go -C2 'func\s*\(.*\)\s*DecodeToChunk\s*\('
echo
echo "== DecodeToChunk call sites in benchmarks =="
rg -nP --type=go -C2 '\.DecodeToChunk\s*\(' --iglob '*bench_test.go'Repository: pingcap/tidb
Length of output: 1563
🏁 Script executed:
rg -n 'DecodeToChunk' pkg/util/rowcodec/bench_test.go -B2 -A2Repository: pingcap/tidb
Length of output: 389
🏁 Script executed:
rg -n 'DecodeToChunk' pkg/util/rowcodec --type=go -B1 -A1 | head -60Repository: pingcap/tidb
Length of output: 3907
DecodeToChunk call missing required commitTS argument at line 112.
The call signature requires 4 arguments: DecodeToChunk(rowData []byte, commitTS uint64, handle kv.Handle, chk *chunk.Chunk). Line 112 provides only 3. This is a compile-time blocker.
Fix
- if err := decoder.DecodeToChunk(rowData, kv.IntHandle(r), chk); err != nil {
+ if err := decoder.DecodeToChunk(rowData, 0, kv.IntHandle(r), chk); err != nil {
b.Fatal(err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for range b.N { | |
| chk.Reset() | |
| for r := 0; r < batchSize; r++ { | |
| if err := decoder.DecodeToChunk(rowData, kv.IntHandle(r), chk); err != nil { | |
| b.Fatal(err) | |
| for range b.N { | |
| chk.Reset() | |
| for r := 0; r < batchSize; r++ { | |
| if err := decoder.DecodeToChunk(rowData, 0, kv.IntHandle(r), chk); err != nil { | |
| b.Fatal(err) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/util/rowcodec/bench_test.go` around lines 109 - 113, The call to
decoder.DecodeToChunk in bench_test.go is missing the required commitTS uint64
argument; update the decoder.DecodeToChunk(rowData, kv.IntHandle(r), chk) call
to include a commitTS value as the second parameter (e.g.
decoder.DecodeToChunk(rowData, commitTS, kv.IntHandle(r), chk)), using an
existing commitTS variable if available or a sensible constant (0 or a
test-specific timestamp) so the call matches the signature DecodeToChunk(rowData
[]byte, commitTS uint64, handle kv.Handle, chk *chunk.Chunk).
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.
Summary
Test plan
make serverbuilds successfullySummary by CodeRabbit
New Features
tidb_plan_cache_policyto control plan caching eligibility (allorhint_only)Performance Improvements
Metrics & Monitoring