pkg/planner, pkg/sessionctx: keep native TiCI FTS plan when LIKE fallback rejects syntax (#68525)#68572
Conversation
…back rejects syntax
ti-chi-bot
added
ok-to-test
winoros
changed the title
📝 WalkthroughWalkthroughThis PR fixes a bug where FTS alternative-plan heuristics incorrectly invalidated native TiCI plans. It removes the problematic ChangesFTS Fallback and TiCI Estimation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes span planner logic, system variables, and test infrastructure across 10+ files, but follow a coherent narrative: fix the FTS fallback signaling (planbuilder, expression_rewriter, optimize), gate estimation (stats), add hint support (datasource), and test comprehensively. No deep semantic complexity; straightforward flag/method replacements and conditional logic adjustments. Moderate spread requires reviewing the interconnected signaling changes across multiple files. Possibly related issues
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)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-fts-202602 #68572 +/- ##
=======================================================
Coverage ? 39.4147%
=======================================================
Files ? 1725
Lines ? 478098
Branches ? 0
=======================================================
Hits ? 188441
Misses ? 272673
Partials ? 16984
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/note/planner/rule/rule_ai_notes.md (1)
64-64: 💤 Low valueOptional: Standardize flag style for Go idiom.
The command uses
--tags=intest(double-dash), while Go convention is-tags=intest(single-dash). Both work, but single-dash is more idiomatic for Go tooling. Note that this file already has inconsistency between line 21 (single-dash) and line 46 (double-dash).♻️ Optional flag style adjustment
- `make failpoint-enable && (go test ./pkg/planner/core/casetest/tici -run TestTiCIAlternativeLogicalPlansKeepNativePrefixPlan --tags=intest; rc=$?; make failpoint-disable; exit $rc)` + `make failpoint-enable && (go test ./pkg/planner/core/casetest/tici -run TestTiCIAlternativeLogicalPlansKeepNativePrefixPlan -tags=intest; rc=$?; make failpoint-disable; exit $rc)`🤖 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 `@docs/note/planner/rule/rule_ai_notes.md` at line 64, Replace the double-dash Go flag in the command snippet so it uses the idiomatic single-dash form: change `--tags=intest` to `-tags=intest` in the command that runs `go test` for `TestTiCIAlternativeLogicalPlansKeepNativePrefixPlan`; ensure similar consistency elsewhere in this file (e.g., line with single-dash at the earlier example) so all `go test` invocations use `-tags=...`.
🤖 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-49: The test currently forces the global TiCI-estimate var back
to ON in the defer which can leak state; before calling
tk.MustExec(fmt.Sprintf("set global %s = on", vardef.TiDBEnableTiCIEstimate))
capture the current global value (e.g. via tk.MustQuery(fmt.Sprintf("select
@@global.%s", vardef.TiDBEnableTiCIEstimate)) into a local variable origVal) and
in the defer restore that original value with tk.MustExec(fmt.Sprintf("set
global %s = %s", vardef.TiDBEnableTiCIEstimate, origVal)); update the code
around the tk.MustExec calls so the test always restores the prior setting
instead of hardcoding "on".
---
Nitpick comments:
In `@docs/note/planner/rule/rule_ai_notes.md`:
- Line 64: Replace the double-dash Go flag in the command snippet so it uses the
idiomatic single-dash form: change `--tags=intest` to `-tags=intest` in the
command that runs `go test` for
`TestTiCIAlternativeLogicalPlansKeepNativePrefixPlan`; ensure similar
consistency elsewhere in this file (e.g., line with single-dash at the earlier
example) so all `go test` invocations use `-tags=...`.
🪄 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: 9a9f066d-2656-4603-85cd-091fa2de47bf
📒 Files selected for processing (12)
docs/note/planner/rule/rule_ai_notes.mdpkg/planner/core/casetest/tici/BUILD.bazelpkg/planner/core/casetest/tici/stats_test.gopkg/planner/core/casetest/tici/tici_test.gopkg/planner/core/expression_rewriter.gopkg/planner/core/operator/logicalop/logical_datasource.gopkg/planner/core/planbuilder.gopkg/planner/core/stats.gopkg/planner/optimize.gopkg/sessionctx/stmtctx/stmtctx.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/sysvar.go
💤 Files with no reviewable changes (1)
- pkg/planner/core/planbuilder.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 after the test.
The deferred reset hardcodes ON, so this test can leak global state when the previous value was OFF, which makes later tests order-dependent.
Suggested fix
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))
+origTiCIEstimate := tk.MustQuery(
+ fmt.Sprintf("select @@global.%s", vardef.TiDBEnableTiCIEstimate),
+).Rows()[0][0]
+tk.MustExec(fmt.Sprintf("set global %s = on", vardef.TiDBEnableTiCIEstimate))
+defer tk.MustExec(fmt.Sprintf("set global %s = %v", vardef.TiDBEnableTiCIEstimate, origTiCIEstimate))As per coding guidelines, "Keep test changes minimal and deterministic; avoid broad golden/testdata churn unless required."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tk.MustExec(fmt.Sprintf("set global %s = on", vardef.TiDBEnableTiCIEstimate)) | |
| defer tk.MustExec(fmt.Sprintf("set global %s = on", vardef.TiDBEnableTiCIEstimate)) | |
| tk := testkit.NewTestKit(t, store) | |
| origTiCIEstimate := tk.MustQuery( | |
| fmt.Sprintf("select @@global.%s", vardef.TiDBEnableTiCIEstimate), | |
| ).Rows()[0][0] | |
| tk.MustExec(fmt.Sprintf("set global %s = on", vardef.TiDBEnableTiCIEstimate)) | |
| defer tk.MustExec(fmt.Sprintf("set global %s = %v", vardef.TiDBEnableTiCIEstimate, origTiCIEstimate)) |
🤖 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 - 49, The test
currently forces the global TiCI-estimate var back to ON in the defer which can
leak state; before calling tk.MustExec(fmt.Sprintf("set global %s = on",
vardef.TiDBEnableTiCIEstimate)) capture the current global value (e.g. via
tk.MustQuery(fmt.Sprintf("select @@global.%s", vardef.TiDBEnableTiCIEstimate))
into a local variable origVal) and in the defer restore that original value with
tk.MustExec(fmt.Sprintf("set global %s = %s", vardef.TiDBEnableTiCIEstimate,
origVal)); update the code around the tk.MustExec calls so the test always
restores the prior setting instead of hardcoding "on".
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: qw4990, 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 |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
4 participants
This is an automated cherry-pick of #68525
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
Release Notes
New Features
tidb_enable_tici_estimatesystem variable to control search index row-count estimation behavior.Bug Fixes
Tests