pkg/planner, pkg/sessionctx: keep native TiCI FTS plan when LIKE fallback rejects syntax | tidb-test=feature/fts tiflash=feature/fts tikv=feature/fts#68525
Conversation
…back rejects syntax
ti-chi-bot
Bot
added
release-note
|
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. |
|
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:
📝 WalkthroughWalkthroughRemoves build-time non-viable FTS MATCH tracking so rewrite only records predicate-match; optimizer no longer forces FTS→LIKE fallback early, native TiCI planning errors drive fallback, TiCI index selection respects IGNORE INDEX hints, a TiCI estimate feature flag is added, and regression tests/docs updated. ChangesFTS Alternative-Plan Heuristic Refactoring
Sequence DiagramsequenceDiagram
participant Rewriter as expression_rewriter.matchAgainstToExpression
participant Builder as PlanBuilder
participant Optimizer as buildAndOptimizeLogicalPlanRound
participant TiCI as core.DoOptimize
Rewriter->>Builder: MarkPredicateMatch()
Builder->>Optimizer: predicate-match signal exposed
Optimizer->>TiCI: Invoke native TiCI analysis / index analysis
TiCI-->>Optimizer: Success OR FTSLikeFallbackError
Optimizer->>Optimizer: Cost comparison and winner selection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/planner/optimize.go (1)
487-493:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUnsafe type assertion can panic in fallback error handling.
At Line 491,
err.(*base.FTSLikeFallbackError)can panic whenerris wrapped, even thougherrors.Assucceeded. UsefallbackErr.Unwrap()directly (and keep a non-nil original error fallback).Suggested fix
func maybeArmFTSLikeFallback( sessVars *variable.SessionVars, builder *core.PlanBuilder, err error, ) (discardRound bool, retErr error) { if err == nil { return false, nil } + origErr := err var fallbackErr *base.FTSLikeFallbackError if !stderrors.As(err, &fallbackErr) { return false, err } - err = err.(*base.FTSLikeFallbackError).Unwrap() + err = fallbackErr.Unwrap() if fallbackErr.Cause == nil { - return false, err + if err != nil { + return false, err + } + return false, origErr }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/planner/optimize.go` around lines 487 - 493, Replace the unsafe type assertion err.(*base.FTSLikeFallbackError).Unwrap() with a direct call to fallbackErr.Unwrap() (using the value obtained via errors.As) and ensure you preserve a non-nil original error if Unwrap() returns nil; specifically, in the fallback handling block where you currently declare var fallbackErr *base.FTSLikeFallbackError and call errors.As(err, &fallbackErr), use fallbackErr.Unwrap() to get the inner error, and if that result is nil keep the original err as the fallback before returning.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/planner/optimize.go`:
- Around line 487-493: Replace the unsafe type assertion
err.(*base.FTSLikeFallbackError).Unwrap() with a direct call to
fallbackErr.Unwrap() (using the value obtained via errors.As) and ensure you
preserve a non-nil original error if Unwrap() returns nil; specifically, in the
fallback handling block where you currently declare var fallbackErr
*base.FTSLikeFallbackError and call errors.As(err, &fallbackErr), use
fallbackErr.Unwrap() to get the inner error, and if that result is nil keep the
original err as the fallback before returning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 03f0419c-459c-43a6-a88d-0b4320e87fa4
📒 Files selected for processing (6)
docs/note/planner/rule/rule_ai_notes.mdpkg/planner/core/casetest/tici/tici_test.gopkg/planner/core/expression_rewriter.gopkg/planner/core/planbuilder.gopkg/planner/optimize.gopkg/sessionctx/stmtctx/stmtctx.go
💤 Files with no reviewable changes (1)
- pkg/planner/core/planbuilder.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature/fts #68525 +/- ##
===================================================
+ Coverage 39.4214% 39.4249% +0.0034%
===================================================
Files 1725 1725
Lines 478059 478098 +39
===================================================
+ Hits 188458 188490 +32
- Misses 272613 272619 +6
- Partials 16988 16989 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
|
/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. |
|
/hold |
ti-chi-bot
Bot
added
the
do-not-merge/hold
|
/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 |
|
/retest-required |
|
/unhold |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
…om/AilinKid/tidb into codex/fts-keep-native-fallback-fix
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/sessionctx/vardef/tidb_vars.go (1)
939-940: 💤 Low valueConsider enhancing the comment with optimizer context.
The comment clearly explains what the variable controls, but could be more specific about its use in the optimizer/planner context.
📝 Optional comment enhancement
- // TiDBEnableTiCIEstimate indicates whether to call TiCI to estimate full-text search row counts. + // TiDBEnableTiCIEstimate indicates whether the optimizer should call TiCI to estimate full-text search row counts during cost calculation. TiDBEnableTiCIEstimate = "tidb_enable_tici_estimate"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/sessionctx/vardef/tidb_vars.go` around lines 939 - 940, Update the comment for the constant TiDBEnableTiCIEstimate to state that this session variable is consulted by the query optimizer/planner during cost estimation to decide whether to call TiCI for estimating full-text search row counts, and note that toggling it can affect plan selection and cost calculations (reference symbol: TiDBEnableTiCIEstimate in tidb_vars.go).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/planner/core/casetest/tici/stats_test.go`:
- Around line 47-48: Capture the current global value of
vardef.TiDBEnableTiCIEstimate before changing it, then set it to "on" for the
test using tk.MustExec, and defer a cleanup that restores the original captured
value (not hard-coded "on"); update both occurrences that set/reset the global
(the tk.MustExec calls around vardef.TiDBEnableTiCIEstimate at the top of the
test and the similar block at lines ~82-85) so the deferred call uses the saved
original string/value to restore the exact prior global state.
---
Nitpick comments:
In `@pkg/sessionctx/vardef/tidb_vars.go`:
- Around line 939-940: Update the comment for the constant
TiDBEnableTiCIEstimate to state that this session variable is consulted by the
query optimizer/planner during cost estimation to decide whether to call TiCI
for estimating full-text search row counts, and note that toggling it can affect
plan selection and cost calculations (reference symbol: TiDBEnableTiCIEstimate
in tidb_vars.go).
🪄 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: 7964314c-b30f-4d7b-b1dd-65ff1ef065b4
📒 Files selected for processing (5)
pkg/planner/core/casetest/tici/BUILD.bazelpkg/planner/core/casetest/tici/stats_test.gopkg/planner/core/stats.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/sysvar.go
| tk.MustExec(fmt.Sprintf("set global %s = on", vardef.TiDBEnableTiCIEstimate)) | ||
| defer tk.MustExec(fmt.Sprintf("set global %s = on", vardef.TiDBEnableTiCIEstimate)) |
There was a problem hiding this comment.
Restore the original global TiCI-estimate value instead of forcing ON at cleanup.
Line 48 always resets to ON, which can leak global state into other tests. Capture the original value first and restore that exact value in cleanup.
Suggested patch
tk := testkit.NewTestKit(t, store)
-tk.MustExec(fmt.Sprintf("set global %s = on", vardef.TiDBEnableTiCIEstimate))
-defer tk.MustExec(fmt.Sprintf("set global %s = on", vardef.TiDBEnableTiCIEstimate))
+orig := tk.MustQuery(fmt.Sprintf("select @@global.%s", vardef.TiDBEnableTiCIEstimate)).Rows()[0][0].(string)
+tk.MustExec(fmt.Sprintf("set global %s = on", vardef.TiDBEnableTiCIEstimate))
+defer tk.MustExec(fmt.Sprintf("set global %s = %s", vardef.TiDBEnableTiCIEstimate, orig))As per coding guidelines, "**/*_test.go: Keep test changes minimal and deterministic; avoid broad golden/testdata churn unless required."
Also applies to: 82-85
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/planner/core/casetest/tici/stats_test.go` around lines 47 - 48, Capture
the current global value of vardef.TiDBEnableTiCIEstimate before changing it,
then set it to "on" for the test using tk.MustExec, and defer a cleanup that
restores the original captured value (not hard-coded "on"); update both
occurrences that set/reset the global (the tk.MustExec calls around
vardef.TiDBEnableTiCIEstimate at the top of the test and the similar block at
lines ~82-85) so the deferred call uses the saved original string/value to
restore the exact prior global state.
|
/retest |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: qw4990, terry1purcell, winoros 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 |
|
/retest |
1 similar comment
|
/retest |
winoros
changed the title
|
/retest |
|
/cherrypick release-fts-202602 |
|
@winoros: new pull request created to branch 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 ti-community-infra/tichi repository. |
6 participants
What problem does this PR solve?
Issue Number: close #68489
Problem Summary:
With
@@tidb_opt_enable_alternative_logical_plans = 1, the planner could discard a valid native TiCI FTS plan before native planning actually ran. When the LIKE fallback then rejected boolean prefix syntax such asstainles*, the query failed even though the native FTS path was executable.What changed and how does it work?
nonViableFTSMatchheuristic invalidation path from round-1 build state.MATCHsignal only for scheduling the fallback round.FTSLikeFallbackError.MATCH ... AGAINST.docs/note/planner/rule/rule_ai_notes.md.Check List
Tests
Test command:
make failpoint-enable && (GOCACHE=/private/tmp/index-join-refactor-go-build go test ./pkg/planner/core/casetest/tici -run TestTiCIAlternativeLogicalPlansKeepNativePrefixPlan --tags=intest; rc=$?; make failpoint-disable; exit $rc)Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Documentation