planner: fix gen-col mistakenly resolved after duplicate expression index substitution #67692
planner: fix gen-col mistakenly resolved after duplicate expression index substitution #67692AilinKid wants to merge 4 commits intopingcap:masterfrom
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
do-not-merge/work-in-progress
|
Hi @AilinKid. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughColumn virtual-expression index resolution now prefers exact column-ID matches and selects a unique fallback only when unambiguous; added tests for this behavior and a planner regression EXPLAIN test; two Bazel test rule tweaks (shard_count and test deps) updated. Changes
Sequence Diagram(s)(Skipped — changes are local logic and tests; do not meet criteria for a sequence diagram.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/issuetest/planner_issue_test.go`:
- Around line 35-45: The test only inspects the first LogicalSelection found by
findFirstLogicalSelection, which misses regressions in other nodes (e.g.,
LogicalSort); update the test to traverse the entire optimized plan (not just
the first match) and assert the transformed expressions for both selection and
sort nodes—either replace findFirstLogicalSelection with a recursive walker that
collects all LogicalSelection and LogicalSort nodes or add explicit assertions
for logicalop.LogicalSort expressions (and similarly update the other
occurrences referenced around lines 219-227) so the test validates ORDER BY
LOWER(name), id is rewritten correctly across the whole plan.
🪄 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: 7273ec29-376e-45d8-b590-c676c30c1189
📒 Files selected for processing (4)
pkg/planner/core/issuetest/BUILD.bazelpkg/planner/core/issuetest/planner_issue_test.gopkg/planner/core/rule_generate_column_substitute.gotests/realtikvtest/addindextest4/BUILD.bazel
| duplicatedExpr = true | ||
| } | ||
| if duplicatedExpr { | ||
| *ambiguousExprs = append(*ambiguousExprs, col.VirtualExpr) |
There was a problem hiding this comment.
Following the API design of Go's built-in slices package, directly returning the slice here is the standard approach.
There was a problem hiding this comment.
Good point. Updated in 2c29f27. The helper now returns the updated []expression.Expression directly instead of mutating a *[]expression.Expression, which is cleaner and closer to the usual Go slice style.
0xPoe
left a comment
0xPoe
left a comment
There was a problem hiding this comment.
It seems we also disable the rewrite for this case:
create table space (
workspace_id binary(16) not null,
id binary(16) not null,
name varchar(200) not null,
status enum('CURRENT','ARCHIVED') not null,
primary key (workspace_id, id)
);
create index idx_lower_name on space(workspace_id, (lower(name)), id);
analyze table space;
explain format='plan_tree'
select id, name
from space
where workspace_id = x'00000000000000000000000000000001'
order by lower(name), id
limit 3;
-- expect: uses idx_lower_name, no Sort
create index idx_status_lower_name on space(workspace_id, status,
(lower(name)), id);
explain format='plan_tree'
select id, name
from space
where workspace_id = x'00000000000000000000000000000001'
order by lower(name), id
limit 3;Is this expected?
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #67692 +/- ##
================================================
- Coverage 77.7686% 77.1052% -0.6634%
================================================
Files 1987 1969 -18
Lines 550609 550790 +181
================================================
- Hits 428201 424688 -3513
- Misses 121488 126050 +4562
+ Partials 920 52 -868
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Thanks, I checked this case locally, and this matches what More importantly, So what changes here is the cost-based choice, not rewrite availability. In this reproducer the table is empty and the plan is using pseudo stats; |
|
/retest-required |
|
@AilinKid: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
I tested it with master and this patch: Environment
Repro SQLDROP DATABASE IF EXISTS test67692;
CREATE DATABASE test67692;
USE test67692;
CREATE TABLE space (
workspace_id BINARY(16) NOT NULL,
id BINARY(16) NOT NULL,
name VARCHAR(200) NOT NULL,
status ENUM('CURRENT','ARCHIVED') NOT NULL,
PRIMARY KEY (workspace_id, id)
);
INSERT INTO space
WITH RECURSIVE seq(n) AS (
SELECT CAST(1 AS SIGNED)
UNION ALL
SELECT n + 1 FROM seq WHERE n < 200
)
SELECT
x'00000000000000000000000000000001' AS workspace_id,
UNHEX(LPAD(HEX(n), 32, '0')) AS id,
CONCAT('backend-', LPAD(CAST(n AS CHAR), 3, '0')) AS name,
IF(n % 3 = 0, 'ARCHIVED', 'CURRENT') AS status
FROM seq;
CREATE INDEX idx_lower_name ON space(workspace_id, (LOWER(name)), id);
ANALYZE TABLE space;
CREATE INDEX idx_status_lower_name ON space(workspace_id, status, (LOWER(name)), id);
ANALYZE TABLE space;
EXPLAIN FORMAT='brief'
SELECT id, name
FROM space
WHERE workspace_id = x'00000000000000000000000000000001'
ORDER BY LOWER(name), id
LIMIT 3;Clean master
|
thx @0xPoe got it, I also notice some test failure from unit test |
AilinKid
force-pushed
the
codex/fix-issue-67552
branch
from
2c29f27 to
dd05fce
Compare
ti-chi-bot
Bot
added
size/M
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/expression/column.go (1)
738-752:⚠️ Potential issue | 🟡 MinorClarify intent of the two-pass scan and confirm fallback mitigation.
EqualByExprAndIDalready returns true whenUniqueIDmatches, so the new first loop is a priority override: when two schema columns share the sameVirtualExpr, prefer the one whoseUniqueIDequals the target instead of the first positional match. A one-line comment would make this non-obvious intent clear to future readers.Regarding the fallback risk: this function is called only as a fallback recovery path (in
resolve_indices.goandphysical_index_reader.go) when the normalResolveIndicescall fails with an error. Upstream callers already have comments acknowledging "duplicate virtual expression column matched," indicating awareness of the ambiguity. The function's behavior when the target column'sUniqueIDis absent and multiple schema columns share the sameVirtualExpris that it picks the first positional match. While this is imperfect, the fallback-only invocation pattern and upstream error recovery mitigate the risk in practice.📝 Proposed clarifying comment
func (col *Column) resolveIndicesByVirtualExpr(ctx EvalContext, schema *Schema) bool { + // First prefer an exact UniqueID match so that when multiple schema columns + // share the same VirtualExpr (e.g. two expression indexes on LOWER(name)), + // we bind to the intended hidden generated column rather than the first + // positional match found by EqualByExprAndID. for i, c := range schema.Columns { if c.EqualColumn(col) { col.Index = i return true } } for i, c := range schema.Columns { if c.EqualByExprAndID(ctx, col) { col.Index = i return true } } return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/expression/column.go` around lines 738 - 752, Add a one-line comment above resolveIndicesByVirtualExpr explaining the two-pass scan: first loop prefers exact positional match via EqualColumn to override when UniqueID matches the target, second loop uses EqualByExprAndID (which also returns true on UniqueID match) to fall back to expression+ID matching; also note this function is only used as a fallback recovery path (e.g., when ResolveIndices fails) and that if UniqueID is absent and multiple schema columns share the same VirtualExpr the function will pick the first positional match, which is an acknowledged imperfect fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/expression/column.go`:
- Around line 738-752: Add a one-line comment above resolveIndicesByVirtualExpr
explaining the two-pass scan: first loop prefers exact positional match via
EqualColumn to override when UniqueID matches the target, second loop uses
EqualByExprAndID (which also returns true on UniqueID match) to fall back to
expression+ID matching; also note this function is only used as a fallback
recovery path (e.g., when ResolveIndices fails) and that if UniqueID is absent
and multiple schema columns share the same VirtualExpr the function will pick
the first positional match, which is an acknowledged imperfect fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9b8d3bd4-39ca-4b03-a6a3-09a9e7d0715c
📒 Files selected for processing (5)
br/pkg/metautil/BUILD.bazelpkg/expression/column.gopkg/expression/column_test.gopkg/importsdk/BUILD.bazelpkg/planner/core/issuetest/planner_issue_test.go
✅ Files skipped from review due to trivial changes (2)
- pkg/importsdk/BUILD.bazel
- br/pkg/metautil/BUILD.bazel
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/planner/core/issuetest/planner_issue_test.go
AilinKid
changed the title
|
/retest-required |
|
@AilinKid: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
[LGTM Timeline notifier]Timeline:
|
0xPoe
left a comment
0xPoe
left a comment
There was a problem hiding this comment.
Thanks!
Overall looks good to me.
Could you please update the PR body as well? I believe it still contains the old statement explaining the previous fixes and changes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/expression/column.go`:
- Around line 739-754: The current loop in the schema resolution records only
the first EqualByExprAndID match into fallbackIdx and uses it even if multiple
columns share the same virtual expression; update the logic in the loop that
iterates schema.Columns (the block using EqualColumn and EqualByExprAndID) to
instead collect/count all EqualByExprAndID candidates (e.g., track candidate
index and a count of matches) and only set col.Index to the fallback when the
count equals 1; keep the existing immediate return on exact EqualColumn, but
change the post-loop check to use the unique-candidate condition (count==1)
before assigning col.Index to the recorded candidate index and returning true,
otherwise treat as unresolved.
🪄 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: 87cd31be-2274-4d31-b709-5b3f97ece979
📒 Files selected for processing (1)
pkg/expression/column.go
|
/retest-required |
|
|
/retest-required |
|
/retest-required |
|
/test unit-test |
|
@AilinKid: The specified target(s) for Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
AilinKid
force-pushed
the
codex/fix-issue-67552
branch
from
f665353 to
9196209
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: qw4990 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 |
4 participants
What problem does this PR solve?
Issue Number: ref #67552
Problem Summary:
When a table has multiple expression indexes backed by different hidden generated columns but sharing the same virtual expression, virtual-expression index resolution may bind an expression to the wrong hidden column. This can produce an invalid IndexLookUp plan and trigger errors such as
Unexpected missing column 12on real TiKV.What changed and how does it work?
This PR updates virtual-expression column resolution to prefer the exact column identity before falling back to expression equality.
Column.resolveIndicesByVirtualExprnow checksEqualColumnfirst, so a column whoseUniqueIDexists in the selected schema resolves to that exact column even if another schema column with the sameVirtualExprappears earlier. If there is no exact column match, it falls back to expression equality only when that fallback identifies exactly one candidate; ambiguous fallback candidates remain unresolved instead of choosing the first match.A regression unit test covers the tie-breaking behavior by constructing two columns with the same
VirtualExprand asserting that resolution chooses the exactUniqueIDmatch instead of the earlier expression-equal column. The same test also verifies that a target without an exact match does not resolve through an ambiguous expression-equality fallback. The planner issue test keeps coverage for the reported query shape and verifies that the expression index path is still selected.Check List
Tests
Manual test steps:
tiup playground nightly --db.binpath=/private/tmp/tidb-issue67552-clean/bin/tidb-server --tiflash=0Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Bug Fixes
Tests
Chores