feat(fts): add with_index_limit to the indexed FTS leaf execs - #9174
feat(fts): add with_index_limit to the indexed FTS leaf execs#9174hamersaw wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The limit adjustment is a clone-and-edit operation that preserves each leaf’s segment, mask, scorer, prefilter, and query state while resetting exec-local metrics. Explicit struct literals make future field additions fail compilation until their preservation is considered, which is safer than reconstructing through public constructors; the regression coverage also exercises both UUID-bound and pre-resolved segment selections.
e6d4424 to
f75098b
Compare
948ab93 to
5c410fe
Compare
Over-fetching a bounded FTS arm changes exactly one thing — `FtsSearchParams::limit` — but these nodes have no `with_fetch`, because the cut lives in the index params rather than in a fetch node. Callers have therefore rebuilt the leaf through a public constructor, which silently drops whatever that constructor does not take. For `MatchQueryExec` that is five things: the segment selection, the overlay block, the external mask, and the prepared and shared scorers. Losing the first three is not a recall detail — a leaf scoped to explicit segments widens to all committed ones, re-admitting documents the plan excluded as superseded, so a caller asking only for more candidates gets a different answer. The reconstruct-to-edit pattern is the bug, not any particular missing field: it re-breaks every time a field is added, and it has already done so twice. `with_index_limit` copies the node whole and changes the limit, so there is nothing to forget. Metrics start fresh; the tokenized-query cache carries over, since it is a pure function of the unchanged query. Added to `MatchQueryExec`, `PhraseQueryExec` and `CompoundQueryExec`. `HybridCompoundQueryExec` needs the same but is still `pub(crate)`, so it belongs with the change that makes it public. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QBv2WLSq5oy6kR2gVrfFDh
f75098b to
24327c0
Compare
5c410fe to
22c4cc6
Compare
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The patch is unchanged from the previously assessed revision and remains compatible with the updated stacked base. Limit adjustment still preserves each leaf’s segment, mask, scorer, prefilter, and query state while resetting exec-local metrics; this clone-and-edit API remains preferable to reconstructing through constructors that can widen the search domain.
Stacked on #9172 (chain: #9160 → #9167 → #9172 → this).
The problem is the pattern, not a field
Over-fetching a bounded FTS arm changes exactly one thing —
FtsSearchParams::limit— but these nodes have nowith_fetch, because the cut lives in the index params rather than in a fetch node. So callers rebuild the leaf through a public constructor, which silently drops whatever that constructor does not take.For
MatchQueryExecthat is five things:new?segment_selectionoverlay_blockexternal_maskprepared_queryshared_scorerLosing the first three is not a recall detail. A leaf scoped to explicit segments widens to
AllCommitted, re-admitting documents the plan excluded as superseded — so a caller asking only for more candidates gets a different answer.This has already broken twice in review on downstream work (once for
MatchQueryExec, once forPhraseQueryExec), which is the argument for fixing the shape rather than the instances: reconstruct-to-edit re-breaks every time a field is added to one of these execs, and the compiler cannot see it because the constructor call still type-checks.The change
with_index_limit(&self, limit)copies the node whole and changes the limit. Nothing to forget, and nothing to re-forget when a field is added.Metrics start fresh, since a new exec owns its own. The tokenized-query cache carries over — it is a pure function of
query, which is unchanged, so re-tokenizing would be pure waste.Added to
MatchQueryExec,PhraseQueryExecandCompoundQueryExec.HybridCompoundQueryExecneeds the same, but it is stillpub(crate); its builder belongs with #9166, which makes the type public.Test
with_index_limit_preserves_the_execution_domaintargets the hazard directly rather than the happy path: a leaf built withnew_with_segment_uuidskeeps its explicit UUIDs after the limit changes, and one built withnew_with_segmentskeeps its preset metadata. Both would silently widen under the reconstruct pattern.24
io::exec::ftstests pass, clippy clean.Downstream
Lets sophon's WAL over-fetch stop reconstructing leaves. It also restores over-fetch for shapes that had to give it up for exactly this reason: phrase leaves are currently pinned at
k(lancedb/sophon#7830) andCompoundQueryExecnever had it, both because neither could be rebuilt safely. With this they get real over-fetch, so the base arm stops under-delivering on those shapes once the PK block-list drops superseded rows.🤖 Generated with Claude Code
https://claude.ai/code/session_01QBv2WLSq5oy6kR2gVrfFDh