*: support fts query for starter mode - #69012
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-tests-checked
release-note-none
|
@ChangRui-Ryan I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
size/XXL
|
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:
📝 WalkthroughWalkthroughStarter-only full-text search: gates FTS in expressions and DDL, adds planner rewrite rules to push FTS to TiFlash (with virtual score column and validation), updates physical scan/operator formatting and access-path filtering to TiFlash, ensures safe cloning of QueryInfo, and adds integration tests. ChangesFull-Text Search Planning and Execution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 |
|
Hi @ChangRui-Ryan. 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. |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/planner/core/fts_resolve_index_test.go (1)
87-88: ⚡ Quick winConsider clarifying the error messages for better user guidance.
The error message
"Currently 'FTS_MATCH_WORD()' in SELECT must not be placed"(used for both line 87 and line 88 test cases) may confuse users because:
- Line 87 tests
fts_match_wordin SELECT without WHERE—the actual constraint is that SELECT requires a matching WHERE clause.- Line 88 tests
fts_match_wordwrapped in an expression (* 2)—the actual constraint is that it must be used directly, not wrapped.- Lines 79-82 show that
fts_match_wordcan appear in SELECT when it matches the WHERE clause exactly.Suggested improvements:
- Line 87 scenario:
"FTS_MATCH_WORD() in SELECT requires a matching FTS_MATCH_WORD() in WHERE"- Line 88 scenario:
"FTS_MATCH_WORD() in SELECT must not be wrapped in expressions"🤖 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/fts_resolve_index_test.go` around lines 87 - 88, Update the two assertions' expected error strings to be more specific: for the assertion that checks "explain select fts_match_word('hello', title) from fts_t" (testing SELECT without WHERE) change the expected message to indicate that FTS_MATCH_WORD() in SELECT requires a matching FTS_MATCH_WORD() in WHERE, and for the assertion that checks "explain select fts_match_word('hello', title) * 2 from fts_t where fts_match_word('hello', title)" (testing wrapping in an expression) change the expected message to indicate that FTS_MATCH_WORD() in SELECT must not be wrapped in expressions; keep the checks using tk.MustContainErrMsg and the function name fts_match_word to locate the two assertions.
🤖 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/fts_resolve_index.go`:
- Around line 257-263: The TopK pushdown currently computes TopK from
planTopN.Offset + planTopN.Count even when the ORDER BY has additional
tie-breakers, which can change results; update the guard in the block that
checks planSelection, ds.PushedDownConds and planTopN.ByItems[0].Desc to also
ensure planTopN.ByItems has exactly one item (e.g., len(planTopN.ByItems) == 1)
before computing and setting queryInfo.TopK, so TopK is only pushed when the FTS
score is the sole sort key; keep the existing maxFTSTopK cap logic and the
uint32Ptr conversion.
In `@pkg/planner/core/operator/physicalop/physical_table_scan.go`:
- Around line 593-597: The explain output currently writes raw FTS query text
when normalized is false (ftsIndexBuffer.WriteString(ftsQueryInfo.QueryText)),
which ignores redaction mode; update the code around where normalized and
ftsIndexBuffer are used to consult the redaction setting and, when redaction is
enabled, write a redacted placeholder (e.g., "REDACTED" or an empty/obfuscated
string) instead of ftsQueryInfo.QueryText; ensure normalized handling remains
unchanged when redaction is disabled so that normalized ? "?" :
ftsQueryInfo.QueryText logic only writes raw text when redaction is off.
---
Nitpick comments:
In `@pkg/planner/core/fts_resolve_index_test.go`:
- Around line 87-88: Update the two assertions' expected error strings to be
more specific: for the assertion that checks "explain select
fts_match_word('hello', title) from fts_t" (testing SELECT without WHERE) change
the expected message to indicate that FTS_MATCH_WORD() in SELECT requires a
matching FTS_MATCH_WORD() in WHERE, and for the assertion that checks "explain
select fts_match_word('hello', title) * 2 from fts_t where
fts_match_word('hello', title)" (testing wrapping in an expression) change the
expected message to indicate that FTS_MATCH_WORD() in SELECT must not be wrapped
in expressions; keep the checks using tk.MustContainErrMsg and the function name
fts_match_word to locate the two assertions.
🪄 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: fcc0aa69-5229-4c3c-b03b-0b680afe75e0
📒 Files selected for processing (22)
pkg/ddl/BUILD.bazelpkg/ddl/executor.gopkg/ddl/index.gopkg/expression/BUILD.bazelpkg/expression/builtin.gopkg/expression/builtin_fts.gopkg/expression/integration_test/BUILD.bazelpkg/expression/integration_test/integration_test.gopkg/meta/model/table.gopkg/planner/core/BUILD.bazelpkg/planner/core/fts_resolve_index.gopkg/planner/core/fts_resolve_index_test.gopkg/planner/core/indexmerge_path.gopkg/planner/core/operator/logicalop/logical_datasource.gopkg/planner/core/operator/physicalop/physical_table_scan.gopkg/planner/core/operator/physicalop/tiflash_predicate_push_down.gopkg/planner/core/optimizer.gopkg/planner/core/plan_clone_utils.gopkg/planner/core/rule/logical_rules.gopkg/planner/core/stats.gopkg/planner/core/task.gopkg/sessionctx/stmtctx/stmtctx.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69012 +/- ##
================================================
+ Coverage 76.3218% 76.3233% +0.0014%
================================================
Files 2041 2063 +22
Lines 561480 576014 +14534
================================================
+ Hits 428532 439633 +11101
- Misses 132045 134447 +2402
- Partials 903 1934 +1031
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest |
|
@ChangRui-Ryan: 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. |
|
/retest |
|
@ChangRui-Ryan: 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. |
|
/retest |
|
@ChangRui-Ryan: 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. |
|
@JaySon-Huang: adding LGTM is restricted to approvers and reviewers in OWNERS files. 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. |
| FlagEliminateProjection | ||
| FlagMaxMinEliminate | ||
| FlagConstantPropagation | ||
| FlagFullTextIndexResolveWhere |
There was a problem hiding this comment.
are those flags stored somewhere, or being part of the protocol, insert here will change value of below flags, might break stored/protocol part
Do not mess up the order.
There was a problem hiding this comment.
Thanks, fixed. I kept the existing flag values stable by appending the new FTS flags and decoupling rule order from flag values with an explicit mapping, so adding these rules no longer shifts existing bits.
|
/retest |
|
@ChangRui-Ryan: 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. |
|
/retest |
|
@ChangRui-Ryan: 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. |
| if err := c.verifyArgs(args); err != nil { | ||
| return nil, err | ||
| } | ||
| if !deploymode.IsStarter() { |
There was a problem hiding this comment.
Will this will break current planner's rewrite for FTS function? From planner's view, "rewrite fts function to ilike function" should work even not in starter
There was a problem hiding this comment.
This should not affect the ILIKE fallback. The starter-mode gate is only for fts_match_word() (ast.FTSMatchWord). MATCH ... AGAINST uses the separate ast.FTSMysqlMatchAgainst builtin and its ILIKE fallback path remains unchanged.
|
|
||
| tk.MustExec("begin") | ||
| tk.MustExec("insert into fts_t values (1, 'hello', 'dirty')") | ||
| tk.MustContainErrMsg("select * from fts_t where fts_match_word('hello', title)", "Currently 'FTS_MATCH_WORD()' must be used alone") |
There was a problem hiding this comment.
This error message means that the code does nothing about the dirty txn.
|
/retest |
| // When we use the straight Join Order hint, we should disable the join reorder optimization. | ||
| flag &= ^rule.FlagJoinReOrder | ||
| } | ||
| if logic.SCtx().GetSessionVars().StmtCtx.FTSFunctionIsUsed { |
winoros
left a comment
winoros
left a comment
There was a problem hiding this comment.
Approved because it's only used in starter mode.
It needs more changes to make it generally available across all tiers.
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
|
||
| func (p *PhysicalTableScan) hasFullTextIndexPushDown() bool { | ||
| for _, idx := range p.UsedColumnarIndexes { | ||
| if idx != nil && idx.QueryInfo != nil && idx.QueryInfo.IndexType == tipb.ColumnarIndexType_TypeFulltext { |
| } else if idx.QueryInfo != nil && idx.QueryInfo.IndexType == tipb.ColumnarIndexType_TypeInverted { | ||
| invertedIndexes = append(invertedIndexes, idx.IndexInfo.Name.L) | ||
| } else if idx.QueryInfo != nil && idx.QueryInfo.IndexType == tipb.ColumnarIndexType_TypeFulltext { | ||
| ftsQueryInfo := idx.QueryInfo.GetFtsQueryInfo() |
There was a problem hiding this comment.
maybe put those to another method, already too long
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, JaySon-Huang, windtalker, winoros The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold please fix existing comments |
ti-chi-bot
Bot
added
the
do-not-merge/hold
958273e to
3599a66
Compare
|
/unhold |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
5 participants
What problem does this PR solve?
Issue Number: ref #67765
Problem Summary
TiDB already has the
FTS_MATCH_WORD()builtin and FULLTEXT index syntax, but it does not have the planner path that rewritesFTS_MATCH_WORD()predicates into TiFlash/CSE columnar full-text scans. As a result, Starter deployments cannot execute FULLTEXT index queries through the columnar FTS engine.The feature should be available only in Starter deployment mode. Creating FULLTEXT indexes or executing
FTS_MATCH_WORD()outside Starter mode should fail early instead of silently taking an unsupported path.What changed and how does it work
This PR adds the Starter-only FULLTEXT query path for TiDB:
FTS_MATCH_WORD()execution bydeploymode.IsStarter().FTS_MATCH_WORD(<query>, <column>)inWHEREto a matching FULLTEXT index on the target column.tipb.FTSQueryInfo.ORDER BY fts_match_word(...) DESC LIMIT Ninto a topK FTS scan:FTS_MATCH_WORD()usages rejected with clear planner errors, such as mismatched SELECT/WHERE expressions, unsupported wrapping expressions, or use without a matching FULLTEXT index.tipb.FTSQueryInfo/ columnar index pushdown path.Check List
Tests
E2E validation
I also validated the FTS query path with a real local cluster.
The cluster was started with
tiup playground/ lightly using the TiDB binary built from this PR. It used normal Starter deployment mode only; no FTS-specific mode was enabled, and it did not useplayground:v1.16.2-feature.fts.The tested topology included:
/Users/changrui/GITHUB/TiDB-Cloud/cloud-storage-engine./Users/changrui/GITHUB/tiflash, used as the query node.The test created a table with a FULLTEXT index, inserted sample rows, and verified both NoScore and WithScore FTS query paths.
Example SQL used in the validation:
Summary by CodeRabbit
New Features
Behavior / Validation
Tests