planner, meta: avoid unsafe index on virtual generated temporal columns#67985
planner, meta: avoid unsafe index on virtual generated temporal columns#67985expxiaoli wants to merge 1 commit intopingcap:masterfrom
Conversation
ti-chi-bot
Bot
added
release-note
|
Hi @expxiaoli. 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. |
📝 WalkthroughWalkthroughIntroduces detection of virtual generated temporal-with-date columns and updates planner index-hint resolution to exclude indexes containing them from normal access paths; adds tests and small build config updates to ensure hinted matches produce warnings (not key-not-found errors) when such filtered indexes are referenced (unless ignored). Changes
Sequence Diagram(s)(Skipped — changes are focused within planner hint-resolution logic and test coverage; no multi-component external runtime sequence diagram generated.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.11.4)Command failed 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
🧹 Nitpick comments (1)
pkg/planner/core/casetest/index/index_test.go (1)
107-125: Move issue-52520 regression into its own test function.This new block tests virtual generated temporal columns, which is unrelated to
TestInvisibleIndex's scope (invisible-index visibility toggling). Embedding it here makes the regression harder to discover via test name, hurts targeted test runs (-run), and will conflate failures. Consider extracting it to a dedicatedTestIssue52520VirtualGeneratedTemporalIndex(still usingRunTestUnderCascades) in the same file.As per coding guidelines, code SHOULD remain maintainable for future readers and SHOULD be self-documenting through clear naming and structure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/casetest/index/index_test.go` around lines 107 - 125, Extract the virtual generated temporal column assertions (the entire block that creates t_issue_52520, sets sql_mode, inserts the invalid date, then runs the index/no-index queries and warning checks) out of the existing TestInvisibleIndex test and place them into a new, self-contained test function named TestIssue52520VirtualGeneratedTemporalIndex; keep the same setup/teardown and use RunTestUnderCascades with the same testKit calls (MustExec, MustNoIndexUsed, MustQuery, etc.) so behavior is identical but scoped to the new test, ensuring the new test references RunTestUnderCascades and preserves the same checks for "virtual generated temporal column".
🤖 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/planbuilder.go`:
- Around line 1197-1214: The current hint-resolution can pick a filtered unsafe
index from virtualGeneratedTemporalWithDateIndexes after publicPaths are
checked, causing USE INDEX(i) to wrongly resolve to a safe prefix; change the
resolution to consider both publicPaths and
virtualGeneratedTemporalWithDateIndexes together with exact-match precedence:
implement a single resolver (or extend
getVirtualGeneratedTemporalWithDateIndexByName) that accepts both lists, first
checks for exact matches in publicPaths then in
virtualGeneratedTemporalWithDateIndexes (and returns an explicit “inapplicable
filtered-unsafe” result if the exact match is only in the filtered set),
otherwise count prefix matches across both sets and return the one prefix match
only if the combined count == 1; update the code that forces a path (the branch
that currently inspects publicPaths then virtualGeneratedTemporalWithDateIndexes
around the USE INDEX handling) to use this combined resolver so exact matches
take precedence and inapplicable warnings are emitted correctly.
---
Nitpick comments:
In `@pkg/planner/core/casetest/index/index_test.go`:
- Around line 107-125: Extract the virtual generated temporal column assertions
(the entire block that creates t_issue_52520, sets sql_mode, inserts the invalid
date, then runs the index/no-index queries and warning checks) out of the
existing TestInvisibleIndex test and place them into a new, self-contained test
function named TestIssue52520VirtualGeneratedTemporalIndex; keep the same
setup/teardown and use RunTestUnderCascades with the same testKit calls
(MustExec, MustNoIndexUsed, MustQuery, etc.) so behavior is identical but scoped
to the new test, ensuring the new test references RunTestUnderCascades and
preserves the same checks for "virtual generated temporal column".
🪄 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: 8110ce2b-908d-4cb9-8d8e-4660a087d1c4
📒 Files selected for processing (3)
pkg/meta/model/column.gopkg/planner/core/casetest/index/index_test.gopkg/planner/core/planbuilder.go
ti-chi-bot
Bot
added
size/L
|
[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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/planner/core/casetest/index/index_test.go (1)
142-144: Unused setup int_issue_52520_prefixleaves coverage gaps.The table defines
idx_safe(c)(on a regular column) andidx_unsafe(b)(on the virtual generated date column), but the only query (line 144) references a non-existent indexidx. As a result:
idx_unsafeis never exercised, so the assertion "indexes containing a virtual generated temporal column are filtered out when mixed with other indexes on the same table" is not actually covered here.idx_safeis never exercised, so there's no regression check that safe indexes on the same table remain usable/hintable.- The table name suggests "prefix" but no prefix index is defined.
Consider either dropping the unused columns/indexes to match the intent (just verifying that the not-found error path still fires), or adding assertions against
idx_safe(usable) andidx_unsafe(filtered with the virtual-generated-temporal warning) to justify the richer schema.🧪 Suggested additional coverage
tk.MustExec("drop table if exists t_issue_52520_prefix") tk.MustExec("create table t_issue_52520_prefix (a varchar(32), c int, b date as (a), key idx_safe(c), key idx_unsafe(b))") tk.MustContainErrMsg("select /* issue:52520 */ c from t_issue_52520_prefix use index(idx) where c = 1", "Key 'idx' doesn't exist") + // idx_safe on a regular column should still be usable. + tk.MustUseIndex("select /* issue:52520 */ c from t_issue_52520_prefix use index(idx_safe) where c = 1", "idx_safe") + // idx_unsafe contains the virtual generated temporal column and must be filtered with a warning. + tk.MustNoIndexUsed("select /* issue:52520 */ b from t_issue_52520_prefix use index(idx_unsafe)") + tk.MustQuery("show warnings").CheckContain("virtual generated temporal column")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/casetest/index/index_test.go` around lines 142 - 144, The test setup for t_issue_52520_prefix creates two indexes (idx_safe on c and idx_unsafe on virtual column b) but the assertion only references a non-existent idx, leaving idx_safe/idx_unsafe untested; either simplify the CREATE TABLE to remove the unused index/virtual column if you only want to assert the not-found path, or add explicit assertions: call tk.MustQuery / tk.MustContainErrMsg with use index(idx_safe) to confirm idx_safe is selectable/hintable, and a query using use index(idx_unsafe) (or an optimizer path that would consider it) to assert it is filtered out due to the virtual-generated temporal column (and that the proper warning/message is produced); adjust the table definition to include a real prefix index if you intended to test prefix behavior and update the assertion accordingly.
🤖 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/planbuilder.go`:
- Around line 1389-1392: In checkTblIndexForPointPlan (in point_get_plan.go) add
the same unsafe-index filter used in getPossibleAccessPaths: when iterating
tbl.Indices and considering an index (the loop that checks idxInfo.Unique,
idxInfo.State and hint availability), call
indexContainsVirtualGeneratedTemporalWithDateColumn(tblInfo, index) and skip the
index if it returns true so the point-get planner rejects indexes that contain
virtual generated temporal-with-date columns.
---
Nitpick comments:
In `@pkg/planner/core/casetest/index/index_test.go`:
- Around line 142-144: The test setup for t_issue_52520_prefix creates two
indexes (idx_safe on c and idx_unsafe on virtual column b) but the assertion
only references a non-existent idx, leaving idx_safe/idx_unsafe untested; either
simplify the CREATE TABLE to remove the unused index/virtual column if you only
want to assert the not-found path, or add explicit assertions: call tk.MustQuery
/ tk.MustContainErrMsg with use index(idx_safe) to confirm idx_safe is
selectable/hintable, and a query using use index(idx_unsafe) (or an optimizer
path that would consider it) to assert it is filtered out due to the
virtual-generated temporal column (and that the proper warning/message is
produced); adjust the table definition to include a real prefix index if you
intended to test prefix behavior and update the assertion accordingly.
🪄 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: ca273f28-1870-4136-805d-3297a8c85ca8
📒 Files selected for processing (5)
pkg/importsdk/BUILD.bazelpkg/meta/model/column.gopkg/planner/core/casetest/index/BUILD.bazelpkg/planner/core/casetest/index/index_test.gopkg/planner/core/planbuilder.go
✅ Files skipped from review due to trivial changes (1)
- pkg/planner/core/casetest/index/BUILD.bazel
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/meta/model/column.go
|
/retest-required |
|
@expxiaoli: 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. |
|
@expxiaoli: 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. |
1 participant
What problem does this PR solve?
Issue Number: close #52520
Problem Summary:
When a virtual generated temporal column is indexed, the value stored in the index can be computed under the
sql_modeused when the row was written, while reading the virtual generated column from the table path recomputes it under the currentsql_mode.For example, a virtual generated
DATEcolumn based onvarcharinput may produce different results for invalid dates such as2020-02-31after switchingsql_modetoALLOW_INVALID_DATES. As a result, queries using the index and queries not using the index may return inconsistent results.What changed and how does it work?
This PR prevents TiDB from using indexes that contain virtual generated temporal columns with a date part, including
DATE,DATETIME, andTIMESTAMP.ColumnInfo.IsVirtualGeneratedTemporalWithDateColumn()to identify virtual generated columns whose target type contains a date part.USE INDEX, optimizerUSE_INDEXhints, andIGNORE INDEXbehavior around issue inconsistent data and index for virtual generated column #52520.Check List
Tests
Side effects
Documentation
Validation
Used
Readyprofile because this creates a local commit with code changes and PR-ready message. Commands run:Release note
Summary by CodeRabbit
Bug Fixes
Tests