fix(store): OR the rarest query tokens instead of ANDing all of them (#1177) - #1230
Conversation
`_escape_fts5_query` joined every whitespace token with spaces, which FTS5 reads as an implicit AND. Requiring every token to be present is zero-recall on the multi-word natural-language queries the lane exists to serve: measured over 503 distinct logged prompts against a live 44,584-belief store, it returned nothing for 28.7% of user turns and 100% of harness blocks. `search_beliefs`, `search_beliefs_scored` and the federated peer search now build the MATCH expression by ORing the three lowest-document- frequency tokens. Zero-hit rate goes to 0.0% on both arms while holding 91.3% of the conjunctive lane's top-20 hits, indistinguishable from a full OR's 91.4% and at a ninth of its cost (4.91 ms vs 43.67 ms p50 on the user arm). Rarity is resolved through FTS5's own tokenizer via two per-connection TEMP virtual tables — no migration, nothing written to the database file, and safe on a read-only DB. Reusing the Python tokenizer instead would resolve only 62.9% of tokens against 99.7%, and unresolved tokens collapse to df 0 and masquerade as the rarest, which is the failure #1158 already records for the IDF-clip lane. The expression carries the original tokens rather than the stems it ranked by, because the porter stemmer is not idempotent: 416 of that store's 15,208 terms stem again to something else. `_escape_fts5_query` is retained for callers that do want every token present. `test_multi_word_query_implicit_and` asserted the old contract and is rewritten to assert the new one, including that the belief containing every token still ranks first. Refs #1177, #1158
Six properties, each chosen so that reverting the corresponding piece of the fix fails one of them: the cliff itself (a multi-word query where no belief holds every term returns hits, with the conjunctive builder shown still reproducing the empty result on the same fixture, so the test cannot pass by the corpus happening to contain every token); the trim ordering by document frequency rather than query position; resolution through FTS5's own tokenizer for the underscore and diacritic cases where the Python tokenizer disagrees; the expression carrying original tokens rather than re-emitted stems; escaping still holding for FTS5 operators, punctuation and embedded quotes; and the fallback to a full OR when the probe is unavailable. Refs #1177, #1158
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @robotrocketscience, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
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 |
robotrocketscience
added
the
attn:review
Reviewer's GuideSwitches the FTS5 search lane from implicitly AND-ing all query tokens to OR-ing the rarest few tokens by corpus document frequency, using per-connection TEMP vocab/probe tables, with robust fallbacks and regression tests to ensure recall, correctness, and safety of query escaping. Sequence diagram for the updated rarest-token FTS5 MATCH constructionsequenceDiagram
actor User
participant MemoryStore
participant SQLite
User->>MemoryStore: search_beliefs(query)
activate MemoryStore
MemoryStore->>MemoryStore: _fts5_match_expression(query)
alt [no tokens]
MemoryStore-->>MemoryStore: return ""
else [tokens present]
alt [len(tokens) <= _FTS5_RAREST_TERMS]
MemoryStore->>MemoryStore: _escape_fts5_query_disjunctive(query)
MemoryStore-->>MemoryStore: MATCH expression (OR all tokens)
else [len(tokens) > _FTS5_RAREST_TERMS]
MemoryStore->>MemoryStore: _fts5_rarest_tokens(tokens, keep)
MemoryStore->>MemoryStore: _ensure_fts5_query_probe()
alt [probe available]
MemoryStore->>SQLite: CREATE TEMP fts5_vocab_1177 / fts5_probe_1177 / fts5_probe_terms_1177
MemoryStore->>SQLite: INSERT tokens INTO temp.fts5_probe_1177
MemoryStore->>SQLite: SELECT term, doc FROM temp.fts5_probe_terms_1177
MemoryStore->>SQLite: SELECT doc FROM temp.fts5_vocab_1177 WHERE term = ?
MemoryStore-->>MemoryStore: rarest tokens by df
MemoryStore-->>MemoryStore: MATCH expression (OR rarest tokens)
else [probe unavailable or no tokens resolved]
MemoryStore->>MemoryStore: _escape_fts5_query_disjunctive(query)
MemoryStore-->>MemoryStore: MATCH expression (OR all tokens)
end
end
end
MemoryStore->>SQLite: SELECT ... FROM beliefs_fts WHERE beliefs_fts MATCH ?
MemoryStore-->>User: list[Belief]
deactivate MemoryStore
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PR-size soft capThis PR is over the advisory size threshold:
Bigger PRs collide with more open work, which under the parallel-session workflow tends to produce repeated This is advisory only — nothing is blocked. If the size is intentional (large refactor, module removal, generated code), apply the |
|
[claim:review:Toug:2026-07-31T02:58:09Z] |
|
Reviewed against a build of the branch head. This is good work and I have no What I verified independently
Two things I want to call out as good because they are the kind of thing that Nit 1 — the trim is order-dependent when document frequencies tie
I initially thought this was significant. It is not, and I want to be explicit
Sorting on Nit 2 —
|
robotrocketscience
added
the
ready-to-merge
|
[release:review:Toug:2026-07-31T03:21:50Z] |
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged c6ba460 → |
robotrocketscience
removed
the
attn:review
|
[claim:review:Kulili:2026-07-31T03:45:15Z] |
|
[release:review:Kulili:2026-07-31T03:45:29Z] |
1 participant
Implements the ratified operator disposition on #1177: pursue the recall fix on the existing numpy lane, price the trade explicitly, and do not carry the substrate swap along with it. The FTS5-for-numpy/scipy swap is untouched and stays parked.
The defect is also written up as a sub-item of the #1158 umbrella ("FTS5 MATCH is built as an implicit AND over every whitespace token"), so this closes that sub-item rather than opening a third issue for the same surface.
The defect
_escape_fts5_queryquotes each whitespace token and joins with spaces, which FTS5 reads as an implicit AND. No single belief contains every word of a natural-language question, so the lane goes silent on exactly the query shape aUserPromptSubmitprompt has.Measured over 503 distinct logged prompts against a live 44,584-belief store. Arms reported separately, because pooling them is misleading in both directions — 66% of the corpus is harness blocks, and they behave nothing like user turns:
Pricing the trade, which is what the disposition asked for
The full OR is the option the issue's Mechanism section actually specifies, and it is the expensive one — 43.67 ms p50 on the user arm against the conjunctive lane's 1.01 ms. It also buys nothing over the trim. Agreement with the conjunctive lane's top-20, on the 122 user queries where that lane returned anything at all:
So the middle option the disposition anticipated is not a compromise — it is the same answer for a ninth of the cost, and going past 3 tokens is measurably free of benefit. That is why the constant is 3 and not a round number.
What that agreement figure does and does not say. It is measured only where the conjunctive lane returned hits, because there is no reference set anywhere else. On the 49 user queries and 332 harness blocks where it returned nothing, every hit is new and unverified against a ground truth — this PR establishes that the lane stops being silent, not that the new hits are well-ranked. Ranking quality on those is a bench question, not a claim made here.
Rarity is corpus df, not a stopword list. On a memory store about retrieval,
retrievalcarries df 2,443 andhowonly 516 — sohowsurvives the trim andretrievalis dropped. That reads wrong and measures right: the trim is candidate generation feeding bm25, which then ranks. I checked this specifically because the output looked like a bug.No migration, no new dependency
Document frequencies come from two per-connection TEMP virtual tables — an
fts5vocaboverbeliefs_fts, and an empty probe table declared with the sameporter unicode61tokenizer. Nothing is written to the database file, there is no schema change, and the path works on a read-only DB. Given #1161, avoiding a migration was the whole risk profile. An SQLite built withoutfts5vocabdegrades to a full OR rather than to an exception or back to zero recall.Two facts established by measurement, both of which would have been silent defects
Tokens are resolved through FTS5's own tokenizer, not
aelfrice.bm25.tokenize_stemmed. The two disagree on underscores and diacritics. Ranking query tokens against the index with the Python tokenizer resolves 62.9% of them against the native path's 99.7% — and every unresolved token collapses to df 0 and masquerades as the rarest. That is precisely the failure #1158 records for the IDF-clip lane, and my first measurement pass had it: the rarest-N numbers were wrong until I checked the resolution rate and redid them.The expression carries the original tokens, never the stems it ranked by. The porter stemmer is not idempotent. Checked across the whole live vocabulary rather than spot-checked: 416 of 15,208 terms stem again to something else (
abus→abu,acceler→accel). Re-emitting a stem would silently stop matching those documents.Tests
tests/test_fts5_recall_1177.py— 15 tests over six properties, each chosen so that reverting the corresponding piece of the fix fails one of them. Mutation-checked rather than assumed:search_beliefsto the implicit ANDThe cliff test asserts the conjunctive builder still returns empty on the same fixture, so it cannot pass by the corpus happening to contain every token.
One existing test asserted the old contract and is rewritten rather than deleted:
test_multi_word_query_implicit_andpinnedids == {"b1"}. It now asserts the disjunctive contract and that the belief containing every token still ranks first, which is the property that makes partial matching safe.Full suite: 6544 passed, 69 skipped, 71 xfailed.
Refs #1177, #1158
Summary by Sourcery
Change FTS5 belief search to use an OR over the rarest query tokens instead of an implicit AND over all tokens, eliminating zero-recall behavior for natural-language queries while keeping latency acceptable.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: