*: code preparation for the _tidb_commit_ts new hidden column | tidb-test=pr/2679 - #66057
Open
time-and-fate wants to merge 6 commits into
Open
*: code preparation for the _tidb_commit_ts new hidden column | tidb-test=pr/2679#66057time-and-fate wants to merge 6 commits into
time-and-fate wants to merge 6 commits into
Conversation
ti-chi-bot
Bot
added
do-not-merge/cherry-pick-not-approved
release-note-none
|
This cherry pick PR is for a release branch and has not yet been approved by triage owners. To merge this cherry pick:
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
size/XXL
|
[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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #66057 +/- ##
================================================
Coverage ? 38.2495%
================================================
Files ? 1735
Lines ? 638169
Branches ? 0
================================================
Hits ? 244097
Misses ? 369498
Partials ? 24574
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughAdds support for an extra commit-timestamp column ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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 |
…5-commit-ts-column
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/sample.go (1)
288-317:⚠️ Potential issue | 🟠 MajorAdd focused coverage for the new hidden-column sampling path.
This changes sampled-row decode-map construction for
_tidb_commit_ts, but there’s no targeted test in the provided diff that exercisesbuildSampleColAndDecodeColMapwith_tidb_rowid,_tidb_commit_ts, and both together. A bad offset or missing synthetic-column decode here will slip past the broad plan-output renumbering.I can help sketch a small table-driven unit test for the map/offset cases if you want. As per coding guidelines "For pkg/executor/** SQL behavior changes: run targeted unit test plus relevant integration test".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/sample.go` around lines 288 - 317, The sampled-row decode-map construction (in buildSampleColAndDecodeColMap) lacks targeted tests for the hidden columns `_tidb_rowid` (ExtraHandleID) and `_tidb_commit_ts` (ExtraCommitTSID) and their combinations; add a table-driven unit test that calls buildSampleColAndDecodeColMap (or the public entry that uses it) with schemaCols/cols permutations: neither hidden, only ExtraHandleID, only ExtraCommitTSID, and both together, and assert that for each case the synthetic column entries exist in colMap (keys model.ExtraHandleID and model.ExtraCommitTSID when expected) with correct ColumnInfo Offsets (equal to original len(cols) at creation) and that cols length increases appropriately; ensure the test detects mis-offsetting or missing decoder.Column so future changes to the extra-column append logic are covered.
🧹 Nitpick comments (6)
pkg/planner/core/find_best_task.go (1)
3160-3169: Please add a focused regression test for this TiKV row-size exception.This branch is subtle and easy to desync in future planner/cost backports. A small test that asserts
_tidb_commit_tsdoes not changePhysicalTableScan.getScanRowSize()would make the behavior much safer to preserve.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/find_best_task.go` around lines 3160 - 3169, Add a focused regression test that constructs a PhysicalTableScan (or calls the method that computes scan row size) and verifies that adding the pseudo-column model.ExtraCommitTSID to ts.tblCols does not change the returned size from PhysicalTableScan.getScanRowSize (which currently uses ts.tblCols, slices.Delete, and cardinality.GetTableAvgRowSize). The test should: create identical table metadata/column histograms (ts.tblColHists), compute the scan row size once with the normal columns and once after appending a column with ID model.ExtraCommitTSID to ts.tblCols, and assert the two sizes are equal; place the test in the planner/core package near other planner cost tests so future changes to getScanRowSize will be covered.pkg/planner/core/rule_partition_processor.go (1)
497-505: Add a focused regression test for the newExtraCommitTSIDbranch.This special-case now participates in
reconstructTableColNames, which is later used to rebuild the name list for partition-pruning expression parsing. If a later change drops or misorders this synthetic column, the failure mode will be a generic planner error rather than an obvious mismatch. I only see broad plan-output refreshes in this PR, not a targeted case that exercises this path directly.As per coding guidelines,
pkg/planner/**/*_test.go: "Targeted planner unit tests for pkg/planner/** rules or logical/physical plans: usego test -run <TestName> -tags=intest,deadlockand update rule testdata when needed".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/rule_partition_processor.go` around lines 497 - 505, Add a focused regression test that exercises the ExtraCommitTSID branch in reconstructTableColNames so the synthetic ExtraCommitTSName is included and preserved for partition-pruning expression parsing; create a new unit test in pkg/planner (e.g., pkg/planner/..._test.go) that constructs a table schema and partition pruning expression causing colExpr.ID == model.ExtraCommitTSID to be hit, call the code path that triggers reconstructTableColNames and assert the rebuilt FieldName list contains the synthetic ExtraCommitTSName in the expected order, run with go test -run <TestName> -tags=intest,deadlock and update rule testdata as needed to prevent regressions.pkg/planner/cascades/testdata/transformation_rules_suite_out.json (1)
38-39: Consider normalizing anonymousColumn#ids in planner goldens.This hunk is representative of the rest of the file: the logical plan shape is unchanged, but one hidden-column insertion forces broad ordinal-only churn. Canonicalizing anonymous
Column#placeholders before comparison would make future planner backports much smaller and less brittle.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/planner/cascades/testdata/transformation_rules_suite_out.json` around lines 38 - 39, The golden contains non-deterministic anonymous column ids like "Column#14" (and similar occurrences next to nodes like "Projection_2 input:[Group#2]"); update the test/golden comparison path that reads or writes transformation_rules_suite_out.json to canonicalize anonymous Column#<number> placeholders by replacing them with deterministic, ordinal placeholders (e.g., Column#1, Column#2 in first-seen order) before writing or comparing; implement this normalization using a single-pass map from original numeric ids to sequential ids keyed by the original "Column#\d+" string so the logical plan text remains unchanged but anonymous ids are stable, then regenerate the golden or update the test comparator to apply the same normalization at runtime.pkg/planner/core/operator/logicalop/logical_datasource.go (1)
635-645: Use the metadata helper as the source of truth for the extra commit-ts column.This method hand-rolls the
FieldType, ID, and name even though the same change set addsmodel.NewExtraCommitTSColInfo(). Keeping two encodings of the hidden column metadata makes it easy for planner schema and table metadata to drift if that helper later picks up extra flags or type attributes.♻️ Suggested direction
func (ds *DataSource) NewExtraCommitTSSchemaCol() *expression.Column { - tp := types.NewFieldType(mysql.TypeLonglong) - tp.SetFlag(tp.GetFlag() | mysql.UnsignedFlag) + info := model.NewExtraCommitTSColInfo() return &expression.Column{ - RetType: tp, + RetType: info.FieldType.Clone(), UniqueID: ds.SCtx().GetSessionVars().AllocPlanColumnID(), - ID: model.ExtraCommitTSID, - OrigName: fmt.Sprintf("%v.%v.%v", ds.DBName, ds.TableInfo.Name, model.ExtraCommitTSName), + ID: info.ID, + OrigName: fmt.Sprintf("%v.%v.%v", ds.DBName, ds.TableInfo.Name, info.Name), } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/operator/logicalop/logical_datasource.go` around lines 635 - 645, Replace the hand-crafted column construction in DataSource.NewExtraCommitTSSchemaCol with the canonical metadata from model.NewExtraCommitTSColInfo(): call model.NewExtraCommitTSColInfo() to obtain the ColumnInfo (and derive the RetType, ID, and name from that object) and only set UniqueID using ds.SCtx().GetSessionVars().AllocPlanColumnID(); ensure OrigName/ID/RetType come from the helper instead of being re-created in NewExtraCommitTSSchemaCol so the planner schema stays consistent with model.NewExtraCommitTSColInfo().pkg/planner/core/logical_plan_builder.go (2)
5662-5668: Prefer checking the synthetic column identity here, not just the output name.Matching only on
ColName.L == model.ExtraCommitTSName.Lis broader than the hidden system column and can also remove a derived-table or subquery alias with the same name. Checkingproj.Schema().Columns[i].ID == model.ExtraCommitTSIDwould keep the cleanup precise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/logical_plan_builder.go` around lines 5662 - 5668, The loop is removing outputs by name which can accidentally drop user columns; change the identity check to inspect the schema column ID instead: in the loop over proj.OutputNames() use proj.Schema().Columns[i].ID == model.ExtraCommitTSID (instead of comparing ColName.L to model.ExtraCommitTSName.L) before calling proj.SetOutputNames, modifying proj.Schema().Columns and proj.Exprs with slices.Delete; this ensures only the synthetic ExtraCommitTS column (identified by model.ExtraCommitTSID) is removed.
5440-5483: Please add a regression around the two DML row-layout paths.DELETE now strips
_tidb_commit_tsthrough the bitset pruning flow, while UPDATE removes it by rebuilding the projection. A regression here will only show up on joined/partitioned DML or generated-column cases, so this needs at least one focused planner/executor test.Also applies to: 5662-5668
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/logical_plan_builder.go` around lines 5440 - 5483, Add a focused regression test that exercises both DML row-layout paths so DELETE's bitset pruning (nonPruned / pruneAndBuildSingleTableColPosInfoForDelete / buildSingleTableColPosInfoForDelete / cols2PosInfos) and UPDATE's projection-rebuild path behave identically for the _tidb_commit_ts column; create cases for joined DML, partitioned tables and generated-column scenarios that previously hid the mismatch, run planner+executor and assert the final row layout (projection/column positions) consistently strips or preserves _tidb_commit_ts as intended for both DELETE and UPDATE.
🤖 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/planner/core/casetest/planstats/testdata/plan_stats_suite_out.json`:
- Around line 128-134: Reproduce the failing plan for the query `select * from t
join tp where tp.a = 10 and t.b = tp.c` and verify whether the optimizer now
prefers HashJoin with TableFullScan (nodes: "└─HashJoin", "TableFullScan")
instead of the previous index-based IndexJoin; run the optimizer
unit/integration tests and explain plan diffs, then bisect the commit range
(focus on changes to IndexJoin logic and PR `#66217`) to identify the offending
change, check that the hidden column `_tidb_commit_ts` is excluded from cost
calculations as intended, and if you find the regression in functions/methods
that compute join costs or choose IndexJoin vs HashJoin (look for IndexJoin
selection code and costEstimate routines), revert or fix the cost/comparison
logic so index-based join is preferred when appropriate and add a test asserting
the expected plan.
In `@pkg/planner/core/casetest/testdata/json_plan_suite_out.json`:
- Around line 6-23: The test fixture json_plan_suite_out.json was blanked (all
"SQL": "" and "JSONPlan": null), which removes coverage for
TestJSONPlanInExplain; restore the concrete test cases instead of wiping them by
re-populating each case's "SQL" and "JSONPlan" entries with the expected
deterministic values (updating plan output only where necessary to account for
_tidb_commit_ts changes), so TestJSONPlanInExplain and related assertions can
validate the exact plan output; locate the fixture entries and the
TestJSONPlanInExplain test to ensure the restored JSON matches the test
expectations and keep changes minimal and deterministic.
In `@pkg/planner/core/logical_plan_builder.go`:
- Around line 4770-4780: The code appends a synthetic _tidb_commit_ts column
directly to ds.Columns and ds.TblCols (via NewExtraCommitTSSchemaCol and
model.NewExtraCommitTSColInfo) but skips the DataSource registration used
elsewhere, leaving TblColsByID out of sync; fix by creating and adding the extra
commit-ts column through the same path used by neighboring synthetic columns
(call AppendTableCol or the DataSource method that registers table columns) or,
if AppendTableCol cannot be used, ensure you also register the column in
ds.TblColsByID using the new column's ID after creating commitTSCol so TblCols,
TblColsByID, schema, and names remain consistent (refer to
NewExtraCommitTSSchemaCol, model.NewExtraCommitTSColInfo, AppendTableCol, and
TblColsByID).
In `@pkg/planner/core/preprocess.go`:
- Around line 442-447: The current Enter(*ast.ColumnName) check rejects any
column named model.ExtraCommitTSName (i.e., _tidb_commit_ts) before name
resolution and uses plannererrors.ErrInternal, which incorrectly blocks
legitimate user-defined columns; either add a DDL-level guard to reserve
model.ExtraCommitTSName (mirroring how ExtraHandleName is protected in
add_column.go and ensuring upgrade/test coverage), or move the validation out of
Enter(*ast.ColumnName) into post-resolution code (after column resolution) so
the planner can detect the synthetic hidden column vs. a real user column and
then return the correct user-facing error (use dbterror.ErrWrongColumnName
rather than plannererrors.ErrInternal) — update the check locations referencing
Enter, p.stmtTp, model.ExtraCommitTSName, plannererrors.ErrInternal, and
dbterror.ErrWrongColumnName accordingly.
In
`@tests/integrationtest/r/planner/core/casetest/rule/rule_result_reorder.result`:
- Around line 32-35: The test fixtures only renumber EXPLAIN outputs and do not
cover the new preprocess rejection for references to _tidb_commit_ts; add a new
integration case alongside the existing rule_result_reorder tests that runs a
query with a direct reference to _tidb_commit_ts and asserts it fails via the
preprocess rejection path (expecting an error result/exit and matching the
rejection message), e.g., create a new .sql/.result pair for the
rule_result_reorder scenario that selects or filters on _tidb_commit_ts and
asserts the error is produced by the planner preprocessor.
In `@tests/integrationtest/r/window_function.result`:
- Around line 8-10: Add a focused integration test that asserts the preprocess
rejection for user SQL referencing the internal column _tidb_commit_ts: create a
test query that directly references _tidb_commit_ts (e.g., SELECT
_tidb_commit_ts FROM t; or SELECT * FROM t WHERE _tidb_commit_ts > 0; or ORDER
BY _tidb_commit_ts) and verify the planner/preprocess step returns the expected
error. Place the test alongside existing integration tests in the same suite,
update the result baseline to expect the rejection message, and ensure it
exercises the preprocess guard that rejects invalid user access to
_tidb_commit_ts.
---
Outside diff comments:
In `@pkg/executor/sample.go`:
- Around line 288-317: The sampled-row decode-map construction (in
buildSampleColAndDecodeColMap) lacks targeted tests for the hidden columns
`_tidb_rowid` (ExtraHandleID) and `_tidb_commit_ts` (ExtraCommitTSID) and their
combinations; add a table-driven unit test that calls
buildSampleColAndDecodeColMap (or the public entry that uses it) with
schemaCols/cols permutations: neither hidden, only ExtraHandleID, only
ExtraCommitTSID, and both together, and assert that for each case the synthetic
column entries exist in colMap (keys model.ExtraHandleID and
model.ExtraCommitTSID when expected) with correct ColumnInfo Offsets (equal to
original len(cols) at creation) and that cols length increases appropriately;
ensure the test detects mis-offsetting or missing decoder.Column so future
changes to the extra-column append logic are covered.
---
Nitpick comments:
In `@pkg/planner/cascades/testdata/transformation_rules_suite_out.json`:
- Around line 38-39: The golden contains non-deterministic anonymous column ids
like "Column#14" (and similar occurrences next to nodes like "Projection_2
input:[Group#2]"); update the test/golden comparison path that reads or writes
transformation_rules_suite_out.json to canonicalize anonymous Column#<number>
placeholders by replacing them with deterministic, ordinal placeholders (e.g.,
Column#1, Column#2 in first-seen order) before writing or comparing; implement
this normalization using a single-pass map from original numeric ids to
sequential ids keyed by the original "Column#\d+" string so the logical plan
text remains unchanged but anonymous ids are stable, then regenerate the golden
or update the test comparator to apply the same normalization at runtime.
In `@pkg/planner/core/find_best_task.go`:
- Around line 3160-3169: Add a focused regression test that constructs a
PhysicalTableScan (or calls the method that computes scan row size) and verifies
that adding the pseudo-column model.ExtraCommitTSID to ts.tblCols does not
change the returned size from PhysicalTableScan.getScanRowSize (which currently
uses ts.tblCols, slices.Delete, and cardinality.GetTableAvgRowSize). The test
should: create identical table metadata/column histograms (ts.tblColHists),
compute the scan row size once with the normal columns and once after appending
a column with ID model.ExtraCommitTSID to ts.tblCols, and assert the two sizes
are equal; place the test in the planner/core package near other planner cost
tests so future changes to getScanRowSize will be covered.
In `@pkg/planner/core/logical_plan_builder.go`:
- Around line 5662-5668: The loop is removing outputs by name which can
accidentally drop user columns; change the identity check to inspect the schema
column ID instead: in the loop over proj.OutputNames() use
proj.Schema().Columns[i].ID == model.ExtraCommitTSID (instead of comparing
ColName.L to model.ExtraCommitTSName.L) before calling proj.SetOutputNames,
modifying proj.Schema().Columns and proj.Exprs with slices.Delete; this ensures
only the synthetic ExtraCommitTS column (identified by model.ExtraCommitTSID) is
removed.
- Around line 5440-5483: Add a focused regression test that exercises both DML
row-layout paths so DELETE's bitset pruning (nonPruned /
pruneAndBuildSingleTableColPosInfoForDelete /
buildSingleTableColPosInfoForDelete / cols2PosInfos) and UPDATE's
projection-rebuild path behave identically for the _tidb_commit_ts column;
create cases for joined DML, partitioned tables and generated-column scenarios
that previously hid the mismatch, run planner+executor and assert the final row
layout (projection/column positions) consistently strips or preserves
_tidb_commit_ts as intended for both DELETE and UPDATE.
In `@pkg/planner/core/operator/logicalop/logical_datasource.go`:
- Around line 635-645: Replace the hand-crafted column construction in
DataSource.NewExtraCommitTSSchemaCol with the canonical metadata from
model.NewExtraCommitTSColInfo(): call model.NewExtraCommitTSColInfo() to obtain
the ColumnInfo (and derive the RetType, ID, and name from that object) and only
set UniqueID using ds.SCtx().GetSessionVars().AllocPlanColumnID(); ensure
OrigName/ID/RetType come from the helper instead of being re-created in
NewExtraCommitTSSchemaCol so the planner schema stays consistent with
model.NewExtraCommitTSColInfo().
In `@pkg/planner/core/rule_partition_processor.go`:
- Around line 497-505: Add a focused regression test that exercises the
ExtraCommitTSID branch in reconstructTableColNames so the synthetic
ExtraCommitTSName is included and preserved for partition-pruning expression
parsing; create a new unit test in pkg/planner (e.g., pkg/planner/..._test.go)
that constructs a table schema and partition pruning expression causing
colExpr.ID == model.ExtraCommitTSID to be hit, call the code path that triggers
reconstructTableColNames and assert the rebuilt FieldName list contains the
synthetic ExtraCommitTSName in the expected order, run with go test -run
<TestName> -tags=intest,deadlock and update rule testdata as needed to prevent
regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 24fe4c5d-11ef-4683-bb65-d8bffdea641b
📒 Files selected for processing (65)
pkg/executor/sample.gopkg/meta/model/column.gopkg/meta/model/table.gopkg/planner/cascades/testdata/stringer_suite_out.jsonpkg/planner/cascades/testdata/transformation_rules_suite_out.jsonpkg/planner/core/casetest/binaryplan/testdata/binary_plan_suite_out.jsonpkg/planner/core/casetest/cbotest/testdata/analyze_suite_out.jsonpkg/planner/core/casetest/dag/testdata/plan_suite_out.jsonpkg/planner/core/casetest/enforcempp/testdata/enforce_mpp_suite_out.jsonpkg/planner/core/casetest/hint/testdata/integration_suite_out.jsonpkg/planner/core/casetest/index/index_test.gopkg/planner/core/casetest/index/testdata/index_range_out.jsonpkg/planner/core/casetest/index/testdata/integration_suite_out.jsonpkg/planner/core/casetest/mpp/testdata/integration_suite_out.jsonpkg/planner/core/casetest/partition/testdata/partition_pruner_out.jsonpkg/planner/core/casetest/physicalplantest/physical_plan_test.gopkg/planner/core/casetest/physicalplantest/testdata/plan_suite_out.jsonpkg/planner/core/casetest/planstats/testdata/plan_stats_suite_out.jsonpkg/planner/core/casetest/pushdown/testdata/integration_suite_out.jsonpkg/planner/core/casetest/rule/rule_outer2inner_test.gopkg/planner/core/casetest/rule/testdata/derive_topn_from_window_out.jsonpkg/planner/core/casetest/rule/testdata/outer2inner_out.jsonpkg/planner/core/casetest/rule/testdata/predicate_pushdown_suite_out.jsonpkg/planner/core/casetest/rule/testdata/predicate_simplification_out.jsonpkg/planner/core/casetest/scalarsubquery/testdata/plan_suite_out.jsonpkg/planner/core/casetest/testdata/integration_suite_out.jsonpkg/planner/core/casetest/testdata/json_plan_suite_out.jsonpkg/planner/core/casetest/testdata/stats_suite_out.jsonpkg/planner/core/casetest/tpch/testdata/tpch_suite_out.jsonpkg/planner/core/casetest/vectorsearch/testdata/ann_index_suite_out.jsonpkg/planner/core/casetest/windows/testdata/window_push_down_suite_out.jsonpkg/planner/core/find_best_task.gopkg/planner/core/logical_plan_builder.gopkg/planner/core/operator/logicalop/logical_datasource.gopkg/planner/core/plan_cost_ver2.gopkg/planner/core/preprocess.gopkg/planner/core/rule_partition_processor.gopkg/planner/core/task.gotests/integrationtest/r/agg_predicate_pushdown.resulttests/integrationtest/r/collation_agg_func_disabled.resulttests/integrationtest/r/collation_agg_func_enabled.resulttests/integrationtest/r/collation_check_use_collation_disabled.resulttests/integrationtest/r/collation_check_use_collation_enabled.resulttests/integrationtest/r/executor/executor.resulttests/integrationtest/r/executor/expand.resulttests/integrationtest/r/executor/index_lookup_pushdown.resulttests/integrationtest/r/explain.resulttests/integrationtest/r/explain_easy.resulttests/integrationtest/r/explain_generate_column_substitute.resulttests/integrationtest/r/index_merge.resulttests/integrationtest/r/planner/cascades/integration.resulttests/integrationtest/r/planner/core/casetest/integration.resulttests/integrationtest/r/planner/core/casetest/pushdown/push_down.resulttests/integrationtest/r/planner/core/casetest/rule/rule_derive_topn_from_window.resulttests/integrationtest/r/planner/core/casetest/rule/rule_join_reorder.resulttests/integrationtest/r/planner/core/casetest/rule/rule_result_reorder.resulttests/integrationtest/r/planner/core/cbo.resulttests/integrationtest/r/planner/core/indexjoin.resulttests/integrationtest/r/planner/core/indexmerge_path.resulttests/integrationtest/r/planner/core/integration.resulttests/integrationtest/r/planner/core/issuetest/planner_issue.resulttests/integrationtest/r/planner/core/plan_cost_ver2.resulttests/integrationtest/r/select.resulttests/integrationtest/r/tpch.resulttests/integrationtest/r/window_function.result
Comment on lines
+128
to
+134
| "└─HashJoin 1.00 root inner join, equal:[eq(test.tp.c, test.t.b)]", | ||
| " ├─TableReader(Build) 1.00 root partition:p1 data:Selection", | ||
| " │ └─Selection 1.00 cop[tikv] eq(test.tp.a, 10), not(isnull(test.tp.c))", | ||
| " │ └─TableFullScan 6.00 cop[tikv] table:tp keep order:false, stats:partial[c:unInitialized]", | ||
| " └─IndexLookUp(Probe) 1.00 root ", | ||
| " ├─Selection(Build) 1.00 cop[tikv] not(isnull(test.t.b))", | ||
| " │ └─IndexRangeScan 1.00 cop[tikv] table:t, index:idx(b) range: decided by [eq(test.t.b, test.tp.c)], keep order:false, stats:partial[idx:allEvicted, a:allEvicted, b:allEvicted]", | ||
| " └─TableRowIDScan(Probe) 1.00 cop[tikv] table:t keep order:false, stats:partial[idx:allEvicted, a:allEvicted, b:allEvicted]" | ||
| " └─TableReader(Probe) 3.00 root data:Selection", | ||
| " └─Selection 3.00 cop[tikv] not(isnull(test.t.b))", | ||
| " └─TableFullScan 3.00 cop[tikv] table:t keep order:false, stats:partial[b:allEvicted]" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for related changes in the PR that might explain the plan change
# Check if there are cost model or join selection changes
# Look for changes related to cost estimation that might affect join selection
rg -n "commit.*ts|_tidb_commit_ts" --type go -C 3 pkg/planner/core/
# Check if there are explicit changes to join cost estimation
rg -n "IndexJoin|HashJoin" --type go pkg/planner/core/casetest/planstats/Repository: pingcap/tidb
Length of output: 9672
🏁 Script executed:
#!/bin/bash
# Find the PR description or check what other changes were made
# First, let's look at the test case itself to understand the query
# Find the test case that corresponds to the testdata change
rg -n "TestPartialStatsInExplain|TestPlanStatsLoadForCTE" --type go pkg/planner/core/casetest/planstats/
# Also check if there are changes to join selection logic or cost models
git diff --stat HEAD~1 | head -20
# Check for any changes to join ordering or index selection
rg -n "cartesian|IndexJoin.*cost|join.*selection" --type go pkg/planner/core/ | head -30Repository: pingcap/tidb
Length of output: 4132
🏁 Script executed:
#!/bin/bash
# Let's look at the actual test file to understand what queries are being tested
cat -n pkg/planner/core/casetest/planstats/plan_stats_test.go | head -50
# Look for the specific test case name
rg -n "TestPartialStatsInExplain" -A 20 pkg/planner/core/casetest/planstats/plan_stats_test.goRepository: pingcap/tidb
Length of output: 3016
🏁 Script executed:
#!/bin/bash
# Check what the PR is actually about - look for any documentation or comments
git log --oneline -1
git show --stat | head -30Repository: pingcap/tidb
Length of output: 1893
🏁 Script executed:
#!/bin/bash
# Find the specific line numbers in the testdata file to understand which query produced this plan
head -n 180 pkg/planner/core/casetest/planstats/testdata/plan_stats_suite_out.json | tail -n 60Repository: pingcap/tidb
Length of output: 3316
🏁 Script executed:
#!/bin/bash
# Look for the query labels near those line numbers in the test data
# The JSON should have field names that indicate which test case this is for
jq -r '.[] | select(.Name == "TestPartialStatsInExplain") | keys' pkg/planner/core/casetest/planstats/testdata/plan_stats_suite_out.json 2>/dev/null | head -5
# Or try to understand the structure
head -n 200 pkg/planner/core/casetest/planstats/testdata/plan_stats_suite_out.json | grep -A 2 -B 2 "TestPartialStats"Repository: pingcap/tidb
Length of output: 152
🏁 Script executed:
#!/bin/bash
# Let's look at the test code to see what queries it runs
sed -n '529,700p' pkg/planner/core/casetest/planstats/plan_stats_test.goRepository: pingcap/tidb
Length of output: 1710
🏁 Script executed:
#!/bin/bash
# Search for any changes to join cost or selection logic
# Look for changes in files that control IndexJoin vs HashJoin selection
rg -l "IndexJoin|HashJoin|join.*cost" --type go pkg/planner/core/ | head -20
# Check for changes to cost estimation that might affect this specific query
rg -n "getIndexJoinCostVer2|HashJoin.*cost" --type go -A 5 pkg/planner/core/ | head -50Repository: pingcap/tidb
Length of output: 5182
🏁 Script executed:
#!/bin/bash
# Let's look at what the old plan might have been
# Check if there's a git history or if this is showing changes
git log --oneline --all 2>/dev/null | head -5
git status 2>/dev/nullRepository: pingcap/tidb
Length of output: 673
🏁 Script executed:
#!/bin/bash
# Check if there are any comments in the test data or PR description about why this plan changed
# Look for comments in the test file around the TestPartialStatsInExplain test
sed -n '515,550p' pkg/planner/core/casetest/planstats/plan_stats_test.goRepository: pingcap/tidb
Length of output: 1209
🏁 Script executed:
#!/bin/bash
# Search for any recent changes to index join selection or join ordering
# Look in find_best_task.go which handles task selection
rg -n "IndexJoin.*cost|selectBestTask|selectBest4JoinPlan" --type go -B 2 -A 8 pkg/planner/core/find_best_task.go | head -80Repository: pingcap/tidb
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Check if there are any changes to join order/selection logic between commits
# Look for recent changes to join reorder or cost calculation
git diff a2376aa4..15b5da80 -- pkg/planner/core/ | head -100
# Check what join reorder sysvar commit did
git show a2376aa4 --stat | head -30Repository: pingcap/tidb
Length of output: 4056
🏁 Script executed:
#!/bin/bash
# Look for any changes to how IndexJoin availability is determined
# Check if there's any change to when IndexJoin can/cannot be used
rg -n "canUseIndexJoin|needIndexLookUp|selectBest4PhysicalJoinPlan" --type go -B 2 -A 8 pkg/planner/core/ | head -100Repository: pingcap/tidb
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Let's check the git log to understand what actual changes were made in this branch
git log --oneline --all | grep -i "commit.*ts\|join.*reorder\|index.*join" | head -20
# Also check the actual diff for the testdata file to see what changed
git diff -- pkg/planner/core/casetest/planstats/testdata/plan_stats_suite_out.json 2>/dev/null | head -200Repository: pingcap/tidb
Length of output: 1757
🏁 Script executed:
#!/bin/bash
# Let's see if we can find what the plan was before the merge
# Check if there's a baseline or if we can understand the context better
# Look at the specific commits that changed join logic
git show 10387fd2 --stat 2>/dev/null | head -20
git show f7b7465b --stat 2>/dev/null | head -20
# Check if there are comments in the test file about expected behavior changes
sed -n '520,580p' pkg/planner/core/casetest/planstats/plan_stats_test.goRepository: pingcap/tidb
Length of output: 3630
🏁 Script executed:
#!/bin/bash
# Try to understand if the plan change is related to the new join reorder implementation
# Check commit a2376aa4 which adds the sysvar for new join reorder
git show a2376aa4 --stat 2>/dev/null
# Check the description/comments about this change
git log --format="%B" a2376aa4 2>/dev/null | head -30Repository: pingcap/tidb
Length of output: 1657
🏁 Script executed:
#!/bin/bash
# Let's check the actual content of commit 0b23ce6f which added UnsignedFlag to _tidb_commit_ts
git show 0b23ce6f --stat 2>/dev/null
git log --format="%B" 0b23ce6f 2>/dev/null | head -30Repository: pingcap/tidb
Length of output: 1498
🏁 Script executed:
#!/bin/bash
# Let's check commit f7b7465b which fixes IndexJoin with Aggregation
# This might explain why the plan changed
git show f7b7465b 2>/dev/null | head -150Repository: pingcap/tidb
Length of output: 50369
🏁 Script executed:
#!/bin/bash
# Check what changes were made to exhaust_physical_plans.go in the IndexJoin fix
git show f7b7465b:pkg/planner/core/exhaust_physical_plans.go 2>/dev/null | grep -A 30 -B 5 "IndexJoin\|index.*agg" | head -80Repository: pingcap/tidb
Length of output: 4516
🏁 Script executed:
#!/bin/bash
# Let's look more carefully at the actual test query and see if there's an aggregation involved
# The query: "select * from t join tp where tp.a = 10 and t.b = tp.c"
# Check if there's any window function, aggregation, or subquery involved
grep -r "select \* from t join tp" pkg/planner/core/casetest/planstats/ --include="*.go" -B 2 -A 2Repository: pingcap/tidb
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Let's check if there's any comment or documentation about why the plan changed
# Search for any comments in the diff or test about this specific query
git log --all --oneline --grep="commit.*ts\|plan.*change" 2>/dev/null | head -10
# Also check if there's an issue tracker reference
rg -n "66656|66792|commit.*ts" --type go pkg/planner/core/ | head -20Repository: pingcap/tidb
Length of output: 3654
Verify this plan change is not an unintended regression from other changes in the PR.
The query select * from t join tp where tp.a = 10 and t.b = tp.c now uses HashJoin with TableFullScan instead of an index-based join. While _tidb_commit_ts is explicitly excluded from cost calculations (and thus should not affect join selection), this plan change likely stems from other modifications in the commit stack—particularly from fixes to IndexJoin logic rather than from the hidden column addition itself. Confirm this change is intentional and not a regression introduced by PR #66217 or similar refactoring.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/planner/core/casetest/planstats/testdata/plan_stats_suite_out.json`
around lines 128 - 134, Reproduce the failing plan for the query `select * from
t join tp where tp.a = 10 and t.b = tp.c` and verify whether the optimizer now
prefers HashJoin with TableFullScan (nodes: "└─HashJoin", "TableFullScan")
instead of the previous index-based IndexJoin; run the optimizer
unit/integration tests and explain plan diffs, then bisect the commit range
(focus on changes to IndexJoin logic and PR `#66217`) to identify the offending
change, check that the hidden column `_tidb_commit_ts` is excluded from cost
calculations as intended, and if you find the regression in functions/methods
that compute join costs or choose IndexJoin vs HashJoin (look for IndexJoin
selection code and costEstimate routines), revert or fix the cost/comparison
logic so index-based join is preferred when appropriate and add a test asserting
the expected plan.
Comment on lines
+6
to
+23
| "SQL": "", | ||
| "JSONPlan": null | ||
| }, | ||
| { | ||
| "SQL": "explain format = tidb_json insert into t1 values(1)", | ||
| "JSONPlan": [ | ||
| { | ||
| "id": "Insert_1", | ||
| "estRows": "N/A", | ||
| "taskType": "root", | ||
| "operatorInfo": "N/A" | ||
| } | ||
| ] | ||
| "SQL": "", | ||
| "JSONPlan": null | ||
| }, | ||
| { | ||
| "SQL": "explain format = tidb_json select count(*) from t1", | ||
| "JSONPlan": [ | ||
| { | ||
| "id": "HashAgg_12", | ||
| "estRows": "1.00", | ||
| "taskType": "root", | ||
| "operatorInfo": "funcs:count(Column#5)->Column#3", | ||
| "subOperators": [ | ||
| { | ||
| "id": "TableReader_13", | ||
| "estRows": "1.00", | ||
| "taskType": "root", | ||
| "operatorInfo": "data:HashAgg_5", | ||
| "subOperators": [ | ||
| { | ||
| "id": "HashAgg_5", | ||
| "estRows": "1.00", | ||
| "taskType": "cop[tikv]", | ||
| "operatorInfo": "funcs:count(test.t1._tidb_rowid)->Column#5", | ||
| "subOperators": [ | ||
| { | ||
| "id": "TableFullScan_10", | ||
| "estRows": "10000.00", | ||
| "taskType": "cop[tikv]", | ||
| "accessObject": "table:t1", | ||
| "operatorInfo": "keep order:false, stats:pseudo" | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| "SQL": "", | ||
| "JSONPlan": null | ||
| }, | ||
| { | ||
| "SQL": "explain format = tidb_json select * from t1", | ||
| "JSONPlan": [ | ||
| { | ||
| "id": "IndexReader_7", | ||
| "estRows": "10000.00", | ||
| "taskType": "root", | ||
| "operatorInfo": "index:IndexFullScan_6", | ||
| "subOperators": [ | ||
| { | ||
| "id": "IndexFullScan_6", | ||
| "estRows": "10000.00", | ||
| "taskType": "cop[tikv]", | ||
| "accessObject": "table:t1, index:id(id)", | ||
| "operatorInfo": "keep order:false, stats:pseudo" | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| "SQL": "", | ||
| "JSONPlan": null | ||
| }, | ||
| { | ||
| "SQL": "explain analyze format = tidb_json select * from t1, t2 where t1.id = t2.id", | ||
| "JSONPlan": [ | ||
| { | ||
| "id": "MergeJoin_8", | ||
| "estRows": "12487.50", | ||
| "actRows": "0", | ||
| "taskType": "root", | ||
| "executeInfo": "time:3.5ms, loops:1", | ||
| "operatorInfo": "inner join, left key:test.t1.id, right key:test.t2.id", | ||
| "memoryInfo": "760 Bytes", | ||
| "diskInfo": "0 Bytes", | ||
| "subOperators": [ | ||
| { | ||
| "id": "IndexReader_36(Build)", | ||
| "estRows": "9990.00", | ||
| "actRows": "0", | ||
| "taskType": "root", | ||
| "executeInfo": "time:3.47ms, loops:1, cop_task: {num: 1, max: 3.38ms, proc_keys: 0, tot_proc: 3ms, rpc_num: 1, rpc_time: 3.34ms, copr_cache_hit_ratio: 0.00, distsql_concurrency: 15}", | ||
| "operatorInfo": "index:IndexFullScan_35", | ||
| "memoryInfo": "171 Bytes", | ||
| "diskInfo": "N/A", | ||
| "subOperators": [ | ||
| { | ||
| "id": "IndexFullScan_35", | ||
| "estRows": "9990.00", | ||
| "actRows": "0", | ||
| "taskType": "cop[tikv]", | ||
| "accessObject": "table:t2, index:id(id)", | ||
| "executeInfo": "tikv_task:{time:3.3ms, loops:0}", | ||
| "operatorInfo": "keep order:true, stats:pseudo", | ||
| "memoryInfo": "N/A", | ||
| "diskInfo": "N/A" | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| "id": "IndexReader_34(Probe)", | ||
| "estRows": "9990.00", | ||
| "actRows": "0", | ||
| "taskType": "root", | ||
| "executeInfo": "time:14µs, loops:1, cop_task: {num: 1, max: 772.9µs, proc_keys: 0, rpc_num: 1, rpc_time: 735.7µs, copr_cache_hit_ratio: 0.00, distsql_concurrency: 15}", | ||
| "operatorInfo": "index:IndexFullScan_33", | ||
| "memoryInfo": "166 Bytes", | ||
| "diskInfo": "N/A", | ||
| "subOperators": [ | ||
| { | ||
| "id": "IndexFullScan_33", | ||
| "estRows": "9990.00", | ||
| "actRows": "0", | ||
| "taskType": "cop[tikv]", | ||
| "accessObject": "table:t1, index:id(id)", | ||
| "executeInfo": "tikv_task:{time:168.4µs, loops:0}", | ||
| "operatorInfo": "keep order:true, stats:pseudo", | ||
| "memoryInfo": "N/A", | ||
| "diskInfo": "N/A" | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| "SQL": "", | ||
| "JSONPlan": null |
There was a problem hiding this comment.
Restore concrete test cases instead of blanking the fixture.
Setting every SQL to "" and every JSONPlan to null removes the effective coverage for TestJSONPlanInExplain. If the plan output changed because of _tidb_commit_ts, this file should be updated to the new deterministic expectations, not wiped.
As per coding guidelines, **/{*_test.go,testdata/**,*.golden}: 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/planner/core/casetest/testdata/json_plan_suite_out.json` around lines 6 -
23, The test fixture json_plan_suite_out.json was blanked (all "SQL": "" and
"JSONPlan": null), which removes coverage for TestJSONPlanInExplain; restore the
concrete test cases instead of wiping them by re-populating each case's "SQL"
and "JSONPlan" entries with the expected deterministic values (updating plan
output only where necessary to account for _tidb_commit_ts changes), so
TestJSONPlanInExplain and related assertions can validate the exact plan output;
locate the fixture entries and the TestJSONPlanInExplain test to ensure the
restored JSON matches the test expectations and keep changes minimal and
deterministic.
Comment on lines
+442
to
+447
| case *ast.ColumnName: | ||
| if node.Name.L == model.ExtraCommitTSName.L && | ||
| (p.stmtTp == TypeSelect || p.stmtTp == TypeSetOpr || p.stmtTp == TypeUpdate || p.stmtTp == TypeDelete) { | ||
| p.err = plannererrors.ErrInternal.GenWithStack("Usage of column name '%s' is not supported for now", | ||
| model.ExtraCommitTSName.O) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "All references to _tidb_commit_ts / ExtraCommitTSName:"
rg -n --type=go 'ExtraCommitTSName|_tidb_commit_ts' pkg tests
echo
echo "DDL or preprocess guards that reserve the name:"
rg -n --type=go 'ExtraCommitTSName|_tidb_commit_ts|WrongColumnName' pkg/planner pkg/ddl pkg/meta
echo
echo "Tests/fixtures that cover user-defined _tidb_commit_ts columns:"
rg -n --glob '*test*' --glob '*.sql' --glob '*.out' '_tidb_commit_ts' tests pkgRepository: pingcap/tidb
Length of output: 5058
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== DDL add_column validation context ==="
sed -n '285,300p' pkg/ddl/add_column.go
echo
echo "=== Check if DDL validates against ExtraCommitTSName ==="
rg -A5 -B5 'ExtraCommitTSName|_tidb_commit_ts' pkg/ddl/add_column.go
echo
echo "=== Preprocess.go context around line 442-447 ==="
sed -n '435,455p' pkg/planner/core/preprocess.go
echo
echo "=== preprocess Enter function signature and statement type checks ==="
sed -n '50,100p' pkg/planner/core/preprocess.go | head -50Repository: pingcap/tidb
Length of output: 757
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Preprocess.go Enter(node) method with more context ==="
sed -n '380,450p' pkg/planner/core/preprocess.go
echo
echo "=== Type of error used ==="
rg -n 'plannererrors.ErrInternal|dbterror.ErrWrongColumnName' pkg/planner/core/preprocess.go | head -20
echo
echo "=== Check model/table.go for column metadata ==="
sed -n '88,100p' pkg/meta/model/table.goRepository: pingcap/tidb
Length of output: 3967
No DDL guard prevents _tidb_commit_ts as a user column; this preprocess check will reject real user columns.
The preprocess Enter(*ast.ColumnName) check at lines 442–447 runs during AST walk before column resolution, so it matches any column reference with name _tidb_commit_ts regardless of whether it is the synthetic hidden column or a real user-defined column. DDL validation (e.g., pkg/ddl/add_column.go) only guards against _tidb_rowid (ExtraHandleName), not _tidb_commit_ts, so users can legally create and reference columns with that name in their schemas.
Additionally, plannererrors.ErrInternal is the wrong error class for user-input validation. Similar checks in the same file (lines 996, 1416) use dbterror.ErrWrongColumnName for user-facing column name violations. Either add a DDL guard to globally reserve the name and cover existing schemas in upgrade tests, or move this validation to post-resolution so the planner can distinguish user columns from the synthetic column and emit the correct error type.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/planner/core/preprocess.go` around lines 442 - 447, The current
Enter(*ast.ColumnName) check rejects any column named model.ExtraCommitTSName
(i.e., _tidb_commit_ts) before name resolution and uses
plannererrors.ErrInternal, which incorrectly blocks legitimate user-defined
columns; either add a DDL-level guard to reserve model.ExtraCommitTSName
(mirroring how ExtraHandleName is protected in add_column.go and ensuring
upgrade/test coverage), or move the validation out of Enter(*ast.ColumnName)
into post-resolution code (after column resolution) so the planner can detect
the synthetic hidden column vs. a real user column and then return the correct
user-facing error (use dbterror.ErrWrongColumnName rather than
plannererrors.ErrInternal) — update the check locations referencing Enter,
p.stmtTp, model.ExtraCommitTSName, plannererrors.ErrInternal, and
dbterror.ErrWrongColumnName accordingly.
Comment on lines
+32
to
+35
| Sort 8000.00 root Column#6, Column#7 | ||
| └─HashAgg 8000.00 root group by:planner__core__casetest__rule__rule_result_reorder.t.d, funcs:min(Column#8)->Column#6, funcs:max(Column#9)->Column#7 | ||
| └─TableReader 8000.00 root data:HashAgg | ||
| └─HashAgg 8000.00 cop[tikv] group by:planner__core__casetest__rule__rule_result_reorder.t.d, funcs:min(planner__core__casetest__rule__rule_result_reorder.t.b)->Column#7, funcs:max(planner__core__casetest__rule__rule_result_reorder.t.c)->Column#8 | ||
| └─HashAgg 8000.00 cop[tikv] group by:planner__core__casetest__rule__rule_result_reorder.t.d, funcs:min(planner__core__casetest__rule__rule_result_reorder.t.b)->Column#8, funcs:max(planner__core__casetest__rule__rule_result_reorder.t.c)->Column#9 |
There was a problem hiding this comment.
Add coverage for the new _tidb_commit_ts rejection path.
These fixture updates only renumber existing EXPLAIN outputs. They do not lock down the new preprocess behavior called out in the PR: rejecting SQL that references _tidb_commit_ts. Please add a paired integration case that asserts the error for at least one direct reference, otherwise the main behavior change here is untested.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tests/integrationtest/r/planner/core/casetest/rule/rule_result_reorder.result`
around lines 32 - 35, The test fixtures only renumber EXPLAIN outputs and do not
cover the new preprocess rejection for references to _tidb_commit_ts; add a new
integration case alongside the existing rule_result_reorder tests that runs a
query with a direct reference to _tidb_commit_ts and asserts it fails via the
preprocess rejection path (expecting an error result/exit and matching the
rejection message), e.g., create a new .sql/.result pair for the
rule_result_reorder scenario that selects or filters on _tidb_commit_ts and
asserts the error is produced by the planner preprocessor.
Comment on lines
+8
to
10
| Projection 10000.00 root Column#7->Column#8 | ||
| └─Window 10000.00 root sum(cast(window_function.t.a, decimal(10,0) BINARY))->Column#7 over() | ||
| └─IndexReader 10000.00 root index:IndexFullScan |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden \
-g 'tests/**' \
-g 'pkg/**/testdata/**' \
-g '**/*_test.go' \
'_tidb_commit_ts'Repository: pingcap/tidb
Length of output: 38
Add a regression test for _tidb_commit_ts references.
The search confirms there are no existing tests that exercise the new preprocess rejection for user SQL referencing _tidb_commit_ts. These integration result updates are pure Column# rebaselines and do not provide coverage for the new feature. Add a focused test case covering at least one form of direct reference (SELECT, WHERE, or ORDER BY _tidb_commit_ts) to demonstrate that the preprocess guard correctly rejects invalid user access to this internal column.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integrationtest/r/window_function.result` around lines 8 - 10, Add a
focused integration test that asserts the preprocess rejection for user SQL
referencing the internal column _tidb_commit_ts: create a test query that
directly references _tidb_commit_ts (e.g., SELECT _tidb_commit_ts FROM t; or
SELECT * FROM t WHERE _tidb_commit_ts > 0; or ORDER BY _tidb_commit_ts) and
verify the planner/preprocess step returns the expected error. Place the test
alongside existing integration tests in the same suite, update the result
baseline to expect the rejection message, and ensure it exercises the preprocess
guard that rejects invalid user access to _tidb_commit_ts.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/integrationtest/r/planner/core/casetest/hint/hint.result (1)
270-314: Consider normalizing anonymousColumn#IDs in theseEXPLAINexpectations.Most of this churn is ordinal-only. That makes the hint suite noisy for internal schema changes like hidden-column additions without improving behavioral coverage. If the test harness can strip or normalize
Column#\d+, these cases would be much less brittle.Also applies to: 368-1514, 1686-1690
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integrationtest/r/planner/core/casetest/hint/hint.result` around lines 270 - 314, The EXPLAIN expectations are brittle due to numeric anonymous IDs like "Column#\d+"; update the test comparison path that reads/compares hint.result (e.g., the code that loads expected EXPLAIN outputs—look for functions named loadExpectedResult, parseExplainOutput, or the test harness comparator) to normalize those tokens before diffing by applying a regex replacement for Column#\d+ (either replace all with a fixed marker like Column#N or map them to stable ordinal names) so the assertion compares normalized EXPLAIN text; ensure the normalization runs for the hint.result comparisons (including the ranges noted) and add a small unit-test or comment validating the normalization step.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integrationtest/r/executor/explain.result`:
- Around line 88-91: Add an explicit integration test case in the explain.result
expectations that exercises the new preprocess rule rejecting references to
_tidb_commit_ts: add SELECT and EXPLAIN commands that reference
`_tidb_commit_ts` and assert the user-visible error message (instead of just
rebasing Column# shifts), update the expected output block(s) where
EXPLAIN/SELECT on `_tidb_commit_ts` should produce the rejection error, and
mirror the same addition in the other corresponding result blocks referenced in
the review so the cherry-pick actually tests the restriction.
In `@tests/integrationtest/r/expression/misc.result`:
- Around line 319-323: Add an integration test that executes a query directly
referencing the _tidb_commit_ts column (e.g., SELECT _tidb_commit_ts FROM t or
similar) and asserts that planner preprocessing rejects it (expecting a
non-success/error result), thus exercising the preprocessor guard in preprocess
(the reject-_tidb_commit_ts check). Create a new test case named clearly (e.g.,
TestRejectTidbCommitTS) in the integration test suite, run it to capture output,
and ensure the test fails when the query is accepted and passes when an error is
raised by the preprocess guard that handles _tidb_commit_ts.
---
Nitpick comments:
In `@tests/integrationtest/r/planner/core/casetest/hint/hint.result`:
- Around line 270-314: The EXPLAIN expectations are brittle due to numeric
anonymous IDs like "Column#\d+"; update the test comparison path that
reads/compares hint.result (e.g., the code that loads expected EXPLAIN
outputs—look for functions named loadExpectedResult, parseExplainOutput, or the
test harness comparator) to normalize those tokens before diffing by applying a
regex replacement for Column#\d+ (either replace all with a fixed marker like
Column#N or map them to stable ordinal names) so the assertion compares
normalized EXPLAIN text; ensure the normalization runs for the hint.result
comparisons (including the ranges noted) and add a small unit-test or comment
validating the normalization step.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a0062658-1b4c-40bd-a37a-392235f73bbc
📒 Files selected for processing (46)
tests/integrationtest/r/access_path_selection.resulttests/integrationtest/r/bindinfo/bind.resulttests/integrationtest/r/black_list.resulttests/integrationtest/r/clustered_index.resulttests/integrationtest/r/cte.resulttests/integrationtest/r/executor/aggregate.resulttests/integrationtest/r/executor/chunk_reuse.resulttests/integrationtest/r/executor/explain.resulttests/integrationtest/r/executor/issues.resulttests/integrationtest/r/executor/jointest/hash_join.resulttests/integrationtest/r/executor/partition/table.resulttests/integrationtest/r/executor/point_get.resulttests/integrationtest/r/explain-non-select-stmt.resulttests/integrationtest/r/explain_complex.resulttests/integrationtest/r/explain_complex_stats.resulttests/integrationtest/r/explain_cte.resulttests/integrationtest/r/explain_easy_stats.resulttests/integrationtest/r/explain_join_stats.resulttests/integrationtest/r/explain_shard_index.resulttests/integrationtest/r/expression/charset_and_collation.resulttests/integrationtest/r/expression/explain.resulttests/integrationtest/r/expression/issues.resulttests/integrationtest/r/expression/misc.resulttests/integrationtest/r/expression/vitess_hash.resulttests/integrationtest/r/generated_columns.resulttests/integrationtest/r/globalindex/aggregate.resulttests/integrationtest/r/naaj.resulttests/integrationtest/r/null_rejected.resulttests/integrationtest/r/planner/core/casetest/expression_rewriter.resulttests/integrationtest/r/planner/core/casetest/hint/hint.resulttests/integrationtest/r/planner/core/casetest/index/index.resulttests/integrationtest/r/planner/core/casetest/partition/integration_partition.resulttests/integrationtest/r/planner/core/casetest/physicalplantest/physical_plan.resulttests/integrationtest/r/planner/core/casetest/predicate_simplification.resulttests/integrationtest/r/planner/core/expression_rewriter.resulttests/integrationtest/r/planner/core/partition_pruner.resulttests/integrationtest/r/planner/core/physical_plan.resulttests/integrationtest/r/planner/core/plan.resulttests/integrationtest/r/planner/core/point_get_plan.resulttests/integrationtest/r/planner/core/range_scan_for_like.resulttests/integrationtest/r/planner/core/rule_constant_propagation.resulttests/integrationtest/r/planner/core/rule_outer2inner.resulttests/integrationtest/r/session/clustered_index.resulttests/integrationtest/r/subquery.resulttests/integrationtest/r/table/partition.resulttests/integrationtest/r/util/ranger.result
✅ Files skipped from review due to trivial changes (2)
- tests/integrationtest/r/executor/partition/table.result
- tests/integrationtest/r/explain_easy_stats.result
Comment on lines
+88
to
+91
| └─HashJoin 12500.00 root inner join, equal:[eq(executor__explain.tt123.e, Column#17)] | ||
| ├─TableReader(Build) 10000.00 root data:TableFullScan | ||
| │ └─TableFullScan 10000.00 cop[tikv] table:t2 keep order:false, stats:pseudo | ||
| └─Projection(Probe) 10000.00 root executor__explain.tt123.id, executor__explain.tt123.a, executor__explain.tt123.b, executor__explain.tt123.c, executor__explain.tt123.d, executor__explain.tt123.e, executor__explain.tt123.f, cast(executor__explain.tt123.b, json BINARY)->Column#15 | ||
| └─Projection(Probe) 10000.00 root executor__explain.tt123.id, executor__explain.tt123.a, executor__explain.tt123.b, executor__explain.tt123.c, executor__explain.tt123.d, executor__explain.tt123.e, executor__explain.tt123.f, cast(executor__explain.tt123.b, json BINARY)->Column#17 |
There was a problem hiding this comment.
Add an integration case for the new _tidb_commit_ts rejection.
These updates only rebaseline existing EXPLAIN output after the internal Column# shift. The PR objective also adds a new preprocess rule that rejects SQL references to _tidb_commit_ts, but there’s no new result block here asserting that user-visible error path. Please add a targeted case for SELECT/EXPLAIN on _tidb_commit_ts; otherwise this cherry-pick can pass without exercising the actual restriction.
Also applies to: 348-350, 383-397
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integrationtest/r/executor/explain.result` around lines 88 - 91, Add an
explicit integration test case in the explain.result expectations that exercises
the new preprocess rule rejecting references to _tidb_commit_ts: add SELECT and
EXPLAIN commands that reference `_tidb_commit_ts` and assert the user-visible
error message (instead of just rebasing Column# shifts), update the expected
output block(s) where EXPLAIN/SELECT on `_tidb_commit_ts` should produce the
rejection error, and mirror the same addition in the other corresponding result
blocks referenced in the review so the cherry-pick actually tests the
restriction.
Comment on lines
319
to
323
| desc format='brief' select ifnull("aaaa", a) from t; | ||
| id estRows task access object operator info | ||
| Projection 10000.00 root aaaa->Column#4 | ||
| Projection 10000.00 root aaaa->Column#5 | ||
| └─TableReader 10000.00 root data:TableFullScan | ||
| └─TableFullScan 10000.00 cop[tikv] table:t keep order:false, stats:pseudo |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Integration coverage:\n'
rg -n -C2 '\b_tidb_commit_ts\b' tests/integrationtest || true
printf '\nPlanner references:\n'
rg -n -C2 '\b_tidb_commit_ts\b' pkg/planner/core || trueRepository: pingcap/tidb
Length of output: 2701
🏁 Script executed:
# Find any test files that might test _tidb_commit_ts rejection or special handling
rg -l 'commit.?ts' tests/integrationtest/
# Check what changed in the PR by looking at the test file itself
head -n 330 tests/integrationtest/r/expression/misc.result | tail -n 30
# Search for any test cases that use SELECT with _tidb_commit_ts or test its rejection
rg -A5 -B5 'commit_ts|commit.ts' tests/integrationtest/ --type sqlRepository: pingcap/tidb
Length of output: 1308
🏁 Script executed:
# Search for ifnull handling or column number changes
rg -n 'ifnull|IFNULL' pkg/planner/ pkg/expression/ | head -30
# Look for any recent changes related to column numbering or extra columns
rg -n 'ExtraCommitTSName|_tidb_commit_ts' pkg/ --type go -l
# Check if there's a function that processes column references or validates them
rg -n 'preprocessor|reject|_tidb_commit_ts' pkg/planner/ pkg/expression/ -A 3 | head -50Repository: pingcap/tidb
Length of output: 10091
🏁 Script executed:
# Look at preprocess.go specifically for _tidb_commit_ts handling
rg -n '_tidb_commit_ts|ExtraCommitTSName' pkg/planner/core/preprocess.go -A 5 -B 5
# Search for any validation that rejects certain column names
rg -n 'reject|invalid.*column|special.*column' pkg/planner/core/preprocess.go -i | head -20
# Check if there's a function that validates column references
ast-grep --pattern 'func $_(ctx $_) validateColumn($_, $_) $_{$$$}' pkg/planner/core/Repository: pingcap/tidb
Length of output: 620
Add integration test coverage for _tidb_commit_ts rejection.
This hunk updates the EXPLAIN slot id. However, the integration suite lacks a negative SQL test case asserting that direct user references to _tidb_commit_ts are rejected. The preprocessor guard at pkg/planner/core/preprocess.go:443–446 enforces this rejection, but there is no integration test exercising it. Add a case to tests/integrationtest/ that verifies this error is properly raised.
As per coding guidelines, use pushd tests/integrationtest && ./run-tests.sh -r <TestName> && popd to record the test output.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integrationtest/r/expression/misc.result` around lines 319 - 323, Add
an integration test that executes a query directly referencing the
_tidb_commit_ts column (e.g., SELECT _tidb_commit_ts FROM t or similar) and
asserts that planner preprocessing rejects it (expecting a non-success/error
result), thus exercising the preprocessor guard in preprocess (the
reject-_tidb_commit_ts check). Create a new test case named clearly (e.g.,
TestRejectTidbCommitTS) in the integration test suite, run it to capture output,
and ensure the test fails when the query is accepted and passes when an error is
raised by the preprocess guard that handles _tidb_commit_ts.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/planner/core/logical_plan_builder.go`:
- Around line 803-806: The code currently excludes NATURAL JOIN columns by
comparing the output name (using model.ExtraHandleName, model.ExtraCommitTSName,
model.ExtraPhysTblIDName) which wrongly filters user columns that are aliased to
those names; update the checks in the NATURAL JOIN logic to inspect the paired
schema columns (lColumns[i] and rColumns[i]) and compare their internal
IDs/metadata (e.g., model.ExtraCommitTSID / the equivalent hidden column ID or
flag on the Column/Field structure) instead of FieldName.ColName; apply the same
change to the other occurrences referenced (the similar branches around the
other ranges) so synthetic columns are excluded by their intrinsic ID/hidden
marker rather than by visible alias.
- Around line 5662-5668: The loop blindly removes any output named
model.ExtraCommitTSName.L which can drop user-visible columns; update the
deletion condition in the loop that iterates proj.OutputNames() (and removes
entries from proj.Schema().Columns and proj.Exprs) to first inspect the
corresponding projected schema column (proj.Schema().Columns[i]) and only delete
when that column is the synthetic commit-ts (e.g., by checking a reserved column
ID or that the column is marked hidden), otherwise leave it intact; modify the
condition around proj.SetOutputNames / proj.Schema().Columns = / proj.Exprs = to
include that extra check.
- Around line 5440-5448: The current loop clears nonPruned based only on
FieldName.ColName (names[i].ColName.L == model.ExtraCommitTSName.L), which also
removes ordinary columns aliased `_tidb_commit_ts`; change the check to detect
the synthetic commit-ts column using the underlying schema column identity
instead of the output name: for each names[i], ensure you inspect its referenced
Column/ColumnInfo (e.g., names[i].Column or names[i].ColumnInfo) and only Clear
nonPruned when that column exists and its Column.ID matches the synthetic
`_tidb_commit_ts` schema ID (i.e., the unique ID used for the extra commit-ts
column), leaving other aliased ordinary columns untouched so TblColPosInfo
mapping remains correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4a2e4998-ce1f-433c-81f3-4fab9283729e
📒 Files selected for processing (1)
pkg/planner/core/logical_plan_builder.go
Comment on lines
+803
to
+806
| // Natural join should ignore _tidb_rowid and _tidb_commit_ts | ||
| if name.ColName.L == model.ExtraHandleName.L || | ||
| name.ColName.L == model.ExtraCommitTSName.L || | ||
| name.ColName.L == model.ExtraPhysTblIDName.L { |
There was a problem hiding this comment.
Key the NATURAL JOIN exclusion on the synthetic column ID, not the output name.
These branches now ignore any column named _tidb_commit_ts. A derived table can legally expose a normal column with that alias, and NATURAL JOIN should still treat it as a common column. Match against the paired schema column (lColumns[i] / rColumns[i]) by model.ExtraCommitTSID or hidden metadata instead of FieldName.ColName.
🔧 Minimal direction
- for _, name := range lNames {
- if name.ColName.L == model.ExtraHandleName.L ||
- name.ColName.L == model.ExtraCommitTSName.L ||
- name.ColName.L == model.ExtraPhysTblIDName.L {
+ for i, name := range lNames {
+ if lColumns[i].ID == model.ExtraHandleID ||
+ lColumns[i].ID == model.ExtraCommitTSID ||
+ lColumns[i].ID == model.ExtraPhysTblID {
continue
}Also applies to: 817-820, 853-856
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/planner/core/logical_plan_builder.go` around lines 803 - 806, The code
currently excludes NATURAL JOIN columns by comparing the output name (using
model.ExtraHandleName, model.ExtraCommitTSName, model.ExtraPhysTblIDName) which
wrongly filters user columns that are aliased to those names; update the checks
in the NATURAL JOIN logic to inspect the paired schema columns (lColumns[i] and
rColumns[i]) and compare their internal IDs/metadata (e.g.,
model.ExtraCommitTSID / the equivalent hidden column ID or flag on the
Column/Field structure) instead of FieldName.ColName; apply the same change to
the other occurrences referenced (the similar branches around the other ranges)
so synthetic columns are excluded by their intrinsic ID/hidden marker rather
than by visible alias.
Comment on lines
+5440
to
+5448
| nonPruned := bitset.New(uint(len(names))) | ||
| nonPruned.SetAll() | ||
| // Always prune the `_tidb_commit_ts` column. | ||
| for i, name := range names { | ||
| if name.ColName.L == model.ExtraCommitTSName.L { | ||
| nonPruned.Clear(uint(i)) | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
DELETE pruning will also strip ordinary columns aliased _tidb_commit_ts.
This loop clears nonPruned by FieldName.ColName only. In a multi-table DELETE, a joined subquery/view can project a regular column with that alias; clearing it here shifts later ordinals and corrupts the TblColPosInfo mapping. This needs to distinguish the synthetic column by schema column ID, not by output name.
🔧 Safer direction
-func pruneAndBuildColPositionInfoForDelete(
- names []*types.FieldName,
+func pruneAndBuildColPositionInfoForDelete(
+ names []*types.FieldName,
+ cols []*expression.Column,
tblID2Handle map[int64][]util.HandleCols,
tblID2Table map[int64]table.Table,
hasFK bool,
) (TblColPosInfoSlice, *bitset.BitSet, error) {
@@
- for i, name := range names {
- if name.ColName.L == model.ExtraCommitTSName.L {
+ for i := range names {
+ if cols[i].ID == model.ExtraCommitTSID {
nonPruned.Clear(uint(i))
continue
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/planner/core/logical_plan_builder.go` around lines 5440 - 5448, The
current loop clears nonPruned based only on FieldName.ColName
(names[i].ColName.L == model.ExtraCommitTSName.L), which also removes ordinary
columns aliased `_tidb_commit_ts`; change the check to detect the synthetic
commit-ts column using the underlying schema column identity instead of the
output name: for each names[i], ensure you inspect its referenced
Column/ColumnInfo (e.g., names[i].Column or names[i].ColumnInfo) and only Clear
nonPruned when that column exists and its Column.ID matches the synthetic
`_tidb_commit_ts` schema ID (i.e., the unique ID used for the extra commit-ts
column), leaving other aliased ordinary columns untouched so TblColPosInfo
mapping remains correct.
Comment on lines
+5662
to
+5668
| for i := len(proj.OutputNames()) - 1; i >= 0; i-- { | ||
| if proj.OutputNames()[i].ColName.L == model.ExtraCommitTSName.L { | ||
| proj.SetOutputNames(slices.Delete(proj.OutputNames(), i, i+1)) | ||
| proj.Schema().Columns = slices.Delete(proj.Schema().Columns, i, i+1) | ||
| proj.Exprs = slices.Delete(proj.Exprs, i, i+1) | ||
| } | ||
| } |
There was a problem hiding this comment.
Filter the synthetic commit-ts column by ID in UPDATE too.
This projection step has the same alias-collision problem: UPDATE ... JOIN (SELECT 1 AS _tidb_commit_ts) s ... would drop s._tidb_commit_ts even though it's a normal user-visible column. Check the projected schema column ID or hidden flag before deleting.
🔧 Minimal fix
for i := len(proj.OutputNames()) - 1; i >= 0; i-- {
- if proj.OutputNames()[i].ColName.L == model.ExtraCommitTSName.L {
+ if proj.Schema().Columns[i].ID == model.ExtraCommitTSID {
proj.SetOutputNames(slices.Delete(proj.OutputNames(), i, i+1))
proj.Schema().Columns = slices.Delete(proj.Schema().Columns, i, i+1)
proj.Exprs = slices.Delete(proj.Exprs, i, i+1)
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/planner/core/logical_plan_builder.go` around lines 5662 - 5668, The loop
blindly removes any output named model.ExtraCommitTSName.L which can drop
user-visible columns; update the deletion condition in the loop that iterates
proj.OutputNames() (and removes entries from proj.Schema().Columns and
proj.Exprs) to first inspect the corresponding projected schema column
(proj.Schema().Columns[i]) and only delete when that column is the synthetic
commit-ts (e.g., by checking a reserved column ID or that the column is marked
hidden), otherwise leave it intact; modify the condition around
proj.SetOutputNames / proj.Schema().Columns = / proj.Exprs = to include that
extra check.
Member
Author
|
/retest |
1 similar comment
Member
Author
|
/retest |
|
@time-and-fate: 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. |
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
1 participant
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.
Manual cherry-pick of #65620
What problem does this PR solve?
Issue Number: ref #64281
Problem Summary:
What changed and how does it work?
_tidb_commit_tsnew hidden column | tidb-test=pr/2635 #64610. Please see the PR description there.pkg/planner/core/preprocess.goto disallow usages of_tidb_commit_tsin the SQL.Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Behavior Changes
Tests