Fix prepared fulltext score threshold; push prepared distance bounds - #27765
Conversation
…atrixorigin#27400) A MATCH can only be answered from the index, so the planner refuses the rewrite -- 20105 -- whenever a document the index never returns (relevance 0) could satisfy the predicate. collectDrivingFullTextMatches ran that test on a LITERAL threshold. A '?' is still a ParamRef at PREPARE, so nothing was harvested, and EXECUTE failed even for an ordinary `> 0`: MATCH(body) AGAINST(? IN NATURAL LANGUAGE MODE) > 0 -> 1,2,3 MATCH(body) AGAINST(? IN NATURAL LANGUAGE MODE) > ? -> 20105 Re-planning at EXECUTE does not help: rebuildPreparePlan builds from the parse tree, where the marker is still a marker. So carry the test to the engine instead. fulltextRuntimeScoreGuard takes the ACTUAL operator and the ACTUAL threshold off the predicate and asks the same question about a relevance of 0 -- `0 <op> ?` -- as an optional trailing table-function argument, which both fulltext_index_scan and fulltext2_search raise 20105 on when it is true. Nothing is attached when every threshold is a literal, so an unprepared query carries no runtime cost. A runtime threshold now behaves exactly like the same literal at every value: `> ?` and `>= ?` work where the literal works, and still raise 20105 where it does (`> -1`, `>= 0`). That closes the whole shape rather than the reported value. The conjuncts are ANDed, not ORed. `MATCH > ? AND MATCH < ?` is satisfiable at relevance 0 only when BOTH halves are, so ORing refused (0,5) -- a query the identical literals are accepted for. A literal conjunct that already excludes relevance 0 makes the rewrite safe whatever the runtime half is, and emits no guard at all. isExecutionConstantExpr is shared out to utils.go because the distinction that matters is execution-constant vs per-row, not literal vs non-literal: a '?' folds once before a scan, a column reference never can. Covered by unit tests (planner guard 81.5%, engine guard 100%, zeroRelevanceSatisfies 100%) and a BVT matrix over classic FULLTEXT and FULLTEXT2: term-only, threshold-only and both parameterized, positive/zero/negative bounds, CAST(? AS DOUBLE), both operand orders, conjunctions, repeated execution, reuse after a refusal, with the literal forms as controls. Binary-protocol COM_STMT_PREPARE is not reachable from mo-tester; it shares this plan path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
getDistRangeFromFilters peels `distfn(v, lit) <op> K` off the filter list and
pushes it onto the vector index reader, so the query collapses to one Vector
Index Scan. mergeUpperBound/mergeLowerBound demanded a numeric LITERAL, so a
prepared bound lost the pushdown entirely:
literal dist < 5 -> Vector Index Scan on idx
param dist < ? -> Join
-> Table Scan (Filter: l2_distance(...) < ...)
-> Vector Index Scan on idx
Same rows, a base-table scan and a join more work. The consumer side already
handles it -- vectorscan constant-folds the bound before the scan and fails
loudly if it does not reduce to a number -- so only the planner refused to
create one.
Accept a non-literal bound only into an EMPTY slot, where there is no tightness
to compare, and only when it is constant for the whole execution. A per-row
expression (a column reference) must still stay a residual filter: it has no
single value to fold. That is the matrixorigin#25639 case, and
TestGetDistRangeFromFiltersKeepsTightestBound caught a first version of this
change that tested merely "not a literal" -- the reason the condition is
isExecutionConstantExpr and not GetLit() == nil.
A second bound on the same side still falls back to a residual filter once
either side is non-literal, in both orders, so no bound is ever dropped.
Covered by unit tests (mergeUpperBound 95%, mergeLowerBound 85%) over all four
operators in both operand roles, plus a BVT case that asserts the PLAN SHAPE --
the pushdown is invisible in the results, so a results-only case would pass just
as happily with it gone.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
cpegeric
changed the title
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
I found two correctness regressions in the IVFFLAT part. The FULLTEXT runtime-guard design and its classic/FULLTEXT2 paths look sound, but the distance-bound optimization is not safe to merge yet.
[P1] Preserve NULL semantics for a prepared distance bound
mergeUpperBound / mergeLowerBound now peel a ParamRef out of the SQL filter and make the index reader its sole consumer. A prepared parameter can legally be NULL, though. At EXECUTE, parameter replacement turns the bound into a NULL literal; constant folding visits IndexReaderParam.DistRange but leaves that NULL literal as NULL, and vectorDistanceBound then rejects it with IVF distance bound did not fold to a numeric literal.
For example:
prepare s from 'select id from t
where l2_distance(v, ''[1,1,1]'') < ?
order by l2_distance(v, ''[1,1,1]'') limit 2';
set @d = null;
execute s using @d;The predicate is UNKNOWN for every row, so the correct result is an empty set; before this optimization it remained a residual filter and produced that result. This PR turns the valid query into an error. Please represent a runtime-NULL range as an empty result (or retain an equivalent runtime predicate/guard), and cover non-NULL -> NULL -> non-NULL reuse for both lower and upper bounds.
Relevant flow: apply_indices_vector.go:950-965/990-1005 -> rule/constant_fold.go:107-110 -> vectorindex/ivfflat/relation_search.go:444-452.
[P1] Do not classify round(?, per_row_column) as execution-constant
isExecutionConstantExpr follows only fn.Args[0] for every whitelisted wrapper. ROUND has a value-affecting second argument, so round(?, u.lim) varies per row even though its first argument is a parameter. The helper currently returns true, the distance predicate is peeled into one global DistRange, constant folding cannot eliminate the ColRef, and the reader fails with the same “did not fold” error. Before this PR the predicate stayed residual and executed normally.
I added a temporary counterexample on exact head 18a62532e013a1ba5cdc2e47ba821c69affbdb41 asserting that
l2_distance(v, '[1,2,3]') < round(?, per_row_digits)stays residual; it failed because the PR removed the filter and installed the expression as UpperBound. Please validate all value-affecting wrapper arguments (and wrapper arity), not only argument 0, and add this case to TestIsExecutionConstantExpr / the distance-range tests.
Focused existing planner and table-function tests pass on the exact head. These counterexamples are outside the current BVT matrix, which uses only non-NULL scalar bounds and a bare per-row column.
aptend
left a comment
aptend
left a comment
There was a problem hiding this comment.
I independently reproduced two blocking IVFFLAT regressions on exact head 18a62532e013a1ba5cdc2e47ba821c69affbdb41: a prepared NULL distance bound turns valid three-valued SQL into an execution error, and round(?, per_row_column) is misclassified as one execution-wide value. I also found a fulltext literal/parameter parity gap outside the current matrix. The existing focused and full package tests pass, as do focused -race -count=3 runs, but temporary counterexample tests fail for all three cases.
Validation performed after make thirdparties and make cgo: full pkg/sql/plan, pkg/sql/colexec/table_function, and pkg/vectorindex/ivfflat tests; focused race runs; exact-head counterexamples. The temporary tests were removed afterward.
Two correctness regressions from the prepared-distance-bound pushdown, both reported in review of matrixorigin#27400. 1. A NULL prepared bound turned a valid query into an error. A parameter may legally bind NULL. `distance < NULL` is UNKNOWN for every row, so the answer is the empty set -- which is what the residual filter produced before the bound was peeled. After peeling, the range is the predicate's ONLY consumer, and vectorDistanceBound rejected the folded NULL literal with "IVF distance bound did not fold to a numeric literal". Reproduced on the unfixed head: set @d = null; execute s using @d; -> ERROR 20301 invalid input: IVF distance bound did not fold to a numeric literal vectorDistanceBound now separates "evaluated to NULL" from "is not a number at all", and the range reports the empty set for the first while still failing loudly on the second. That mirrors how a NULL query vector is already handled one layer up, where vectorscan's RequestAt returns no request rather than an error. 2. isExecutionConstantExpr accepted a per-row wrapper argument. It followed fn.Args[0] for every whitelisted wrapper, so `round(?, per_row_col)` was reported constant even though ROUND's second argument is value-affecting and varies per row. The bound was then peeled into one scan-wide range that cannot fold. The walk is now per-wrapper: CAST checks only argument 0 (argument 1 is the target TYPE, not a value), while ROUND/FLOOR/CEIL check every argument, and an expression must actually contain a parameter to qualify. Worth recording for the reviewer: this one is latent, not live. MO's ROUND refuses a non-constant second argument outright -- on an indexed table AND on a table with no index -- so `round(?, per_row_col)` never reaches the rule from SQL. It is reachable only by hand-building the plan expression, which is what the reported counterexample does. Fixed anyway: the helper is shared with the fulltext guard, and a classification this rule gets wrong is exactly the matrixorigin#25639 failure mode. Covered by TestVectorDistanceBoundNull, the ROUND cases in TestIsExecutionConstantExpr, and BVT coverage of non-NULL -> NULL -> non-NULL reuse on both bounds plus both-NULL. Mutation-checked: removing the NULL detection fails the BVT on exactly the two NULL statements. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The runtime branch harvested a MATCH threshold for ANY comparison operator once
the bound was a parameter, while the literal test below it accepts only `>` and
`>=`. `MATCH(...) < c` is refused for every literal c -- a relevance-0 document
satisfies it whenever c > 0, and nothing satisfies it when c <= 0 -- so:
MATCH(body) AGAINST('fox') < 0 -> 20105
MATCH(body) AGAINST(?) < ? -> executes and returns the empty set at ? = 0
The parameter form gained an evaluation path the literal form does not have, and
the comment claiming the two behave identically at every value was simply wrong.
Split the two decisions instead of conflating them. The VALUE is what a runtime
threshold hides, and the engine guard re-checks it -- that is why `> ?` is
harvested even though `> -1` is refused. The OPERATOR is known at plan time on
both paths, so the runtime branch is now gated on `>` / `>=` as well, and `< ?`
raises 20105 exactly as `< 0` does. No shape the literal path accepts is lost.
`=` and `<>` were already refused on both paths (they fall to the default arm) and
stay refused. They are conservatively refused rather than wrong: `MATCH = 0.5` and
`MATCH <> 0` ARE membership-implying and could drive the index. That is a missed
optimisation, uniform across both paths, and out of scope here.
TestDrivingHarvestLiteralParameterParity states the invariant at the level it
actually holds: not "the same answer for every value" -- the first version of the
test asserted that and failed on the intended `> -1` case -- but "no operator is
harvestable via a parameter unless some literal makes it harvestable". It covers
all six operators in both operand orders, and includes `=` / `<>` precisely because
adding one path without the other is how this appeared. BVT adds the `< ?` and
`<= ?` cases against their literal twins.
Mutation-checked: removing the operator gate fails the parity test on `<`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
aptend
left a comment
aptend
left a comment
There was a problem hiding this comment.
Deep re-review of exact head 5fe815e. I read the complete review/comment/reply/thread history, compared the increment since 438a602, and rechecked the full planner, FULLTEXT/FULLTEXT2, and IVFFLAT diff. The prior IVFFLAT NULL-bound, execution-constant-wrapper, literal/parameter eligibility, and FULLTEXT2 guard-order blockers are materially closed. The new commit evaluates the runtime zero-relevance guard before both engines’ NULL-pattern paths and covers safe→unsafe operator reuse; direct engine tests reproduce the previous counterexample and now return the established 20105 refusal. The existing NULL/reuse, range-intersection, wrapper-arity, and operator-parity behavior remains intact. No new allocation, retained state, goroutine, lock, or wait edge is added. Exact-head full tests for pkg/sql/plan, pkg/sql/colexec/table_function, and pkg/vectorindex/ivfflat pass; the focused race matrix passes three runs; go vet and diff checks pass. Two exact-head CI jobs are still queued/running, with no failure reported. No blocking correctness, lifecycle, concurrency, performance, or compatibility issue remains.
Merge Queue Status
This pull request spent 56 minutes 34 seconds in the queue, including 56 minutes 10 seconds running CI. Required conditions to merge
|
5 participants
What type of PR is this?
Which issue(s) this PR fixes:
issue #27400
What this PR does / why we need it:
Fix prepared fulltext score threshold; push prepared distance bounds
Fixes #27400
What was broken
A
MATCH()can only be answered from the index. So the planner refuses the rewrite — error 20105 — whenever a document the index never returns (relevance 0) could satisfy the predicate.collectDrivingFullTextMatchesran that test against a literal threshold. A?is still aParamRefat PREPARE time, so nothing was harvested and EXECUTE failed even for an ordinary> 0:Re-planning at EXECUTE doesn't help:
rebuildPreparePlanbuilds from the parse tree, where the marker is still a marker.Fix 1 — carry the test to the engine
fulltextRuntimeScoreGuardtakes the actual operator and the actual threshold off the predicate and asks the same question about a relevance of 0 —0 <op> ?— as an optional trailing table-function argument. Bothfulltext_index_scanandfulltext2_searchraise 20105 on it when true. Nothing is attached when every threshold is a literal, so unprepared queries carry no runtime cost.A runtime threshold now behaves exactly like the same literal at every value:
> 0>= 0.001>= 0> -1c > MATCH(...)That closes the whole shape, not just the reported value —
>= ?works now too.Conjuncts are ANDed, not ORed.
MATCH > ? AND MATCH < ?is satisfiable at relevance 0 only when both halves are; ORing refused(0,5), a query the identical literals are accepted for. A literal conjunct that already excludes relevance 0 makes the rewrite safe whatever the runtime half is, and emits no guard at all.Fix 2 — prepared distance bounds (ivfflat)
Same root cause, different symptom.
mergeUpper/LowerBounddemanded a numeric literal, so a prepared bound lost the range pushdown:Same rows, a base-table scan and a join more work. The consumer already handled it —
vectorscanconstant-folds the bound and fails loudly if it doesn't reduce to a number — only the planner refused to create one.The distinction that matters
Not literal vs non-literal, but execution-constant vs per-row. A
?folds once before a scan; a column reference never can. Shared asisExecutionConstantExprinutils.go.This is load-bearing: a first version of fix 2 tested merely "not a literal" and broke
TestGetDistRangeFromFiltersKeepsTightestBound, the #25639 guard. A per-row bound still stays a residual filter, and a second same-side bound still falls back in both orders, so no bound is ever dropped.Scope notes
getDistRangeFromFiltershas one caller,apply_indices_ivfflat.go.COM_STMT_PREPAREisn't reachable from mo-tester; it shares this plan path and the issue reports the same failure for both.Testing
Unit coverage of new code:
checkFulltextZeroRelevanceGuardzeroRelevanceSatisfiesfulltextRuntimeScoreGuardmergeUpperBoundmergeLowerBoundisExecutionConstantExprRemainder is unreachable binder-error branches.
BVT —
fulltext_prepare_score_threshold.sql(95 statements) andvector_ivf_prepare_dist_range.sql(37). The vector case asserts the plan shape, since the pushdown is invisible in results.Mutation-checked, each independently:
> -1/>= 0, both index typesBaseline comparison on the
fulltext,fulltext2andvectorsuites: only the two new cases differ (12 and 1 baseline failures respectively); every other file identical.fulltext2is byte-identical, confirming its existing failures are pre-existing type-metadata drift.Commits
b8a39cd1c8— fix(fulltext): let a prepared score threshold drive the MATCH index ([Bug]: Prepared fulltext score threshold parameter prevents MATCH index rewrite #27400)854a703412— perf(ivfflat): push a prepared distance bound into the index reader