diff --git a/.add/state.json b/.add/state.json index 27defa795..9e585780b 100644 --- a/.add/state.json +++ b/.add/state.json @@ -1,7 +1,7 @@ { "project": "moon", "stage": "production", - "active_task": "fts-query-combinators", + "active_task": "fts-query-routing-robustness", "active_milestone": "v3-1-fts-hardening", "tasks": { "hotpath-lock-quickwins": { @@ -121,6 +121,40 @@ ], "created": "2026-06-16T06:21:47+00:00", "updated": "2026-06-16T09:07:25+00:00" + }, + "fts-upsert-incremental": { + "title": "Incremental posting-list upsert (kill O(V) per-doc scan)", + "phase": "done", + "gate": "PASS", + "milestone": "v3-1-fts-hardening", + "depends_on": [], + "created": "2026-06-16T10:34:55+00:00", + "updated": "2026-06-16T12:46:23+00:00", + "flag_verified": true + }, + "fts-search-count-semantics": { + "title": "FT.SEARCH integer reply = true total-matched (RediSearch count semantics)", + "phase": "done", + "gate": "PASS", + "milestone": "v3-1-fts-hardening", + "depends_on": [ + "fts-query-eval-dispatch" + ], + "created": "2026-06-16T12:48:50+00:00", + "updated": "2026-06-16T13:13:21+00:00", + "flag_verified": true + }, + "fts-query-routing-robustness": { + "title": "FT.SEARCH routing robustness: SPARSE detection + no expect() on the BM25 AND path", + "phase": "done", + "gate": "PASS", + "milestone": "v3-1-fts-hardening", + "depends_on": [ + "fts-query-eval-dispatch" + ], + "created": "2026-06-16T13:14:26+00:00", + "updated": "2026-06-16T13:41:08+00:00", + "flag_verified": true } }, "milestones": { @@ -174,7 +208,7 @@ } }, "created": "2026-06-11T03:18:21+00:00", - "updated": "2026-06-16T09:07:25+00:00", + "updated": "2026-06-16T13:41:08+00:00", "setup": { "locked": true, "locked_at": "2026-06-11T03:28:00+00:00", diff --git a/.add/tasks/fts-query-routing-robustness/TASK.md b/.add/tasks/fts-query-routing-robustness/TASK.md new file mode 100644 index 000000000..eb2628f75 --- /dev/null +++ b/.add/tasks/fts-query-routing-robustness/TASK.md @@ -0,0 +1,324 @@ +# TASK: FT.SEARCH routing robustness: SPARSE detection + no expect() on the BM25 AND path + +slug: fts-query-routing-robustness · created: 2026-06-16 · stage: production +phase: done + + +> One file = one task. Fill sections top-to-bottom; the `add` skill drives each phase. +> When a phase is unclear, read its book chapter in `.add/docs/` (linked per section). +> The phase marker above is the single source of truth — keep it in sync via `add.py phase`. + +--- + +## 1 · SPECIFY — the rules ▸ docs/03-step-1-specify.md + +Feature: Make FT.SEARCH query ROUTING robust on two confirmed defects + one approved gap, so a query +goes to the right engine (BM25 text vs vector/SPARSE) and the BM25 AND path never panics: + • R1 (panic-safety) — `TextStore::search_field` (store.rs) holds 3× `.expect("posting exists: checked + above")` (lines ~463/470/500) on the live BM25 AND/scoring path. The preceding `is_none()` guard + makes them currently-unreachable, but they violate the "no unwrap/expect in library code" rule and a + future edit to the guard turns malformed/raced state into a server panic. + • R2 (text mis-routed to vector) — `is_text_query(args[1])` uppercases then matches the BARE substring + `"KNN "`, so a legitimate text search like `"knn tutorial"` is classified NON-text → falls to + `ft_search` → `parse_knn_query` (case-SENSITIVE, needs the `*=>[KNN k @f $p]` bracket form) returns + None → `ERR invalid KNN query syntax`. A real text query errors out. The canonical KNN marker is the + `[KNN` bracket; bare "knn" in prose is text. + • R3 (vector mis-routed to text — approved scope) — `is_text_query` only inspects `args[1]`, so it + CANNOT see a standalone `... SPARSE @f $p` clause (a separate arg). A query like + `FT.SEARCH idx "machine" SPARSE @vec $q` (text-looking args[1] + standalone SPARSE, no HYBRID) takes + the text fast-path and SILENTLY DROPS the SPARSE retriever. (SPARSE-inside-HYBRID is already deferred + by `parse_hybrid_modifier`; only the standalone form leaks.) User approved adding the guard. +Framings weighed: R2 retarget is_text_query to the `[KNN` bracket marker + R3 add a `has_sparse_clause` +routing guard at the text-fast-path gates (chosen) · make is_text_query take the full `args` and fold +both checks in (rejected — invasive signature change across 4 call sites + the args[1]-only unit +contract) · leave routing, only fix the doc (rejected by user — leaves the standalone-SPARSE leak). +Must: + + - R1 — `TextStore::search_field` contains NO `.expect`/`.unwrap`/`panic` on the posting-lookup path. + A `get_posting` that returns `None` is handled defensively: a missing AND-term posting ⇒ the AND has + no matches ⇒ return empty results; a missing posting during per-doc scoring ⇒ skip that term's BM25 + contribution (`continue`). Search OUTPUT is byte-identical for the currently-reachable (all-postings- + present) case — this is panic-hardening, not a behavior change. + - R2 — `is_text_query(query)` returns NON-text ONLY for `*` (match-all) or the canonical vector-KNN + bracket marker `[KNN` (case-insensitive); a bare word "knn"/"KNN" in prose (e.g. `"knn tutorial"`, + `"learn knn basics"`) is a TEXT query. Real `*=>[KNN 10 @vec $q]` stays non-text. The doc comment is + corrected to describe the true args[1]-only contract (clause-level SPARSE/HYBRID handled at routing). + - R3 — a query carrying a standalone `SPARSE @field $param` clause is NOT taken onto the BM25 text + fast-path; it defers to `ft_search` (the vector/sparse engine). Implemented by a + `has_sparse_clause(args)` helper AND'd into the text-route gate at ALL FOUR routing sites + (handler_monoio/ft.rs, handler_sharded/ft.rs, handler_single.rs, spsc_handler.rs). HYBRID + (parse_hybrid_modifier) deferral is unchanged. + - R4 — every existing text/combinator behavior (term, AND, OR, TEXT+TAG, NUMERIC, fuzzy, prefix, + @field) and the canonical KNN/standalone-`*`-SPARSE/HYBRID dispatch is UNCHANGED — the full FT suites + plus the is_text_query unit tests (updated for the corrected KNN semantics) stay green. + +Reject: + + - `search_field` term whose posting is absent (missing/raced) -> empty result set, never a panic + (defensive `let Some(..) else`) -> "missing_posting_no_panic" + - FT.SEARCH whose args[1] is text BUT a standalone SPARSE clause is present -> routed to ft_search, not + the text path (SPARSE not dropped) -> "sparse_clause_defers_to_vector" + - FT.SEARCH args[1] containing the word "knn" but NOT the `[KNN` bracket -> treated as a text query, + not a malformed KNN error -> "bare_knn_word_is_text" + +After: + + - `FT.SEARCH idx "knn tutorial"` returns text results (not `ERR invalid KNN query syntax`). + - `FT.SEARCH idx "machine" SPARSE @vec $q` reaches the sparse engine (SPARSE honored, not dropped). + - A `search_field` call over an index with a vanished posting returns empty, never crashes the server. + +Assumptions — lowest-confidence first: + + ⚠ A1 — the canonical vector-KNN marker is the `[KNN` bracket (`*=>[KNN k @f $p]`), so keying + is_text_query on `[KNN` (not bare `"KNN "`) both fixes the prose false-positive AND keeps real KNN + non-text. Lowest confidence because the EXISTING unit test asserts `!is_text_query(b"knn 10")` (bare, + no bracket) — my change makes that TEXT, so I must UPDATE that one assertion (the old test encoded the + over-aggressive detection R2 fixes; not a weakening — the canonical `*=>[KNN ...]` cases still assert + non-text, and new cases pin the prose-is-text fix). If a real KNN query without brackets exists in the + wild: it would now route to text → a coded text error rather than KNN — but `parse_knn_query` is + case-sensitive and the bracket form is the documented syntax, so this is the correct tightening. + - [ ] A2 — `parse_sparse_clause(args)` (pub(crate), ft_search::parse) is the authoritative standalone- + SPARSE detector and is reachable from the handlers via a thin `has_sparse_clause` wrapper. (high — + it's the same fn ft_search uses to dispatch SPARSE.) + - [ ] A3 — the 4 routing sites are the COMPLETE set of text-fast-path gates; guarding each closes the + standalone-SPARSE leak everywhere. (high — grep of is_text_query routing callers: monoio/sharded/ + single/spsc; the early `is_text` computations feed these same gates.) + - [ ] A4 — R1's defensive returns are output-identical on the reachable path (postings always present + after the `is_none` guard), so no FT result changes. (high — the expects never fire today; replacing + a never-taken panic with a never-taken empty-return cannot change observable output.) + + + + +--- + +## 2 · SCENARIOS — pass/fail cases ▸ docs/04-step-2-scenarios.md + + + +```gherkin +# R2 — prose containing "knn" is a TEXT query, not a malformed KNN error +Scenario: a text search for "knn tutorial" returns text results + Given an index with docs whose body contains "knn" and "tutorial" + When FT.SEARCH idx "knn tutorial" runs + Then it returns the matching text documents (a BM25 result array) + And it does NOT return "ERR invalid KNN query syntax" + +# R2 — canonical KNN bracket syntax is still routed to the vector engine (unchanged) +Scenario: a real KNN query stays non-text + Given any index + When is_text_query is asked about "*=>[KNN 10 @vec $query]" + Then it returns false (routes to ft_search), and "*" also returns false + +# R3 — standalone SPARSE clause defers to the vector engine even with text args[1] +Scenario: text-looking args[1] + standalone SPARSE is not eaten by the text path + Given a query `FT.SEARCH idx "machine" SPARSE @vec $q PARAMS 2 q ` + When the handler decides the route + Then the text fast-path is NOT taken (has_sparse_clause is true → defer to ft_search) + And the SPARSE retriever is honored (not silently dropped) + +# R3 — HYBRID and a normal text query are unaffected (no over-deferral) +Scenario: a bare text query with no SPARSE clause still takes the text path + Given `FT.SEARCH idx "machine learning"` (no SPARSE, no HYBRID) + When the handler decides the route + Then the BM25 text fast-path IS taken (has_sparse_clause is false) + And HYBRID queries still defer via parse_hybrid_modifier as before + +# R1 — a vanished posting yields empty results, never a panic +Scenario: search_field tolerates a missing posting on the AND path + Given a multi-term AND query where one term's posting is absent at scoring time + When search_field runs + Then it returns an empty result set (the AND cannot match) and does NOT panic + And when all postings are present the results/scores are byte-identical to before + +# Reject — missing_posting_no_panic (defensive control flow, no expect) +Scenario: search_field has no expect/unwrap on the posting path + Given the search_field source after the change + When audited (grep) for `.expect(`/`.unwrap(` on get_posting results + Then there are none; every get_posting None is a `let Some(..) else` empty-return or `continue` +``` + + + + + +--- + +## 3 · CONTRACT — freeze the shape ▸ docs/05-step-3-contract.md + +``` +FT.SEARCH ROUTING + BM25-AND panic-hardening. No wire/RESP shape change; one NEW pub helper. + +R1 — TextStore::search_field (src/text/store.rs): replace the 3 `.expect("posting exists: checked + above")` (the candidate-bitmap init ~:461-463, the AND-intersection loop ~:467-471, the per-doc + scoring loop ~:497-500) with defensive control flow: + let Some(first_posting) = field_postings[fidx].get_posting(term_postings[0].1) else { + return Vec::new(); }; // AND with absent term ⇒ no matches + let Some(posting) = field_postings[fidx].get_posting(*term_id) else { return Vec::new(); }; // AND loop + let Some(posting) = field_postings[fidx].get_posting(*term_id) else { continue; }; // scoring loop + No other logic changes; the preceding `is_none()` guard (~:449-454) keeps these branches + unreachable today, so output is byte-identical (missing_posting_no_panic, A4). + +R2 — is_text_query(query: &[u8]) -> bool (src/command/vector_search/ft_text_search.rs): SIGNATURE + UNCHANGED. New body returns false ONLY for `query == b"*"` OR the uppercased bytes containing the + `[KNN` bracket marker (the canonical `*=>[KNN k @f $p]` vector-query form); everything else is text. + Replaces the bare `windows(4)=="KNN "` substring scan. Doc comment rewritten to the true + args[1]-only contract (clause-level SPARSE/HYBRID are deferred at the routing layer, not here). + bare_knn_word_is_text: "knn tutorial" / "learn knn" → text. + +R3 — NEW: pub fn has_sparse_clause(args: &[Frame]) -> bool + = crate::command::vector_search::ft_search::parse::parse_sparse_clause(args).is_some() + (defined in ft_text_search.rs, re-exported from vector_search::mod alongside is_text_query). + AND-ed into EVERY text-route gate so a standalone SPARSE clause defers to ft_search + (sparse_clause_defers_to_vector). The SIX gate points across the FOUR routing files: + • handler_monoio/ft.rs — `let is_text = …is_text_query(q)` (~:65, feeds the multi-shard `if is_text` + at ~:156) AND the single-shard `if is_text_query(query_bytes)` (~:440) + • handler_sharded/ft.rs — `let is_text = …is_text_query(q)` (~:67, feeds `if is_text` ~:160) AND + the single-shard `if is_text_query(query_bytes)` (~:340) + • handler_single.rs — `if is_text_query(query_bytes)` (~:1336) + • spsc_handler.rs — `if query_bytes.map_or(false, is_text_query)` (~:1844) + Each becomes ` && !has_sparse_clause()`. parse_hybrid_modifier + deferral (which already catches SPARSE-inside-HYBRID) is untouched. + +UNCHANGED: is_text_query signature; all combinator/text/HYBRID/standalone-`*`-SPARSE/KNN-bracket + dispatch; search_field output on the reachable path; no on-disk/wire format; no new dependency. +TESTS TOUCHED: the is_text_query unit test asserting `!is_text_query(b"knn 10")` is UPDATED to + `is_text_query(b"knn 10")` (corrected R2 semantics — bare "knn" with no bracket is text); the + `*=>[KNN …]` and `*` non-text assertions stay. This is a contracted spec change, not a weakening. +``` + +Status: FROZEN @ v1 — auto-frozen 2026-06-16 under autonomy:auto per "implement all remaining tasks"; the +SPARSE-guard scope (R3) was explicitly user-approved via AskUserQuestion ("Add standalone-SPARSE guard"). +No further design fork. Names match the GLOSSARY (is_text_query, parse_sparse_clause, parse_hybrid_modifier, +search_field, ft_search). +Least-sure flag surfaced at freeze: [test] A1 — R2 flips the existing `!is_text_query(b"knn 10")` assertion +to `is_text_query(b"knn 10")` (bare "knn 10", no `[KNN` bracket, is now TEXT). The old assertion encoded the +over-aggressive bare-substring detection this task fixes; the canonical `*=>[KNN …]`/`*` non-text cases are +retained and new prose-is-text cases added. Cost if a bracket-less KNN form exists in the wild: it routes to +a coded text error instead of KNN — acceptable, since parse_knn_query is case-sensitive and the bracket form +is the documented syntax. [contract] R3 correctness depends on parse_sparse_clause being the authoritative +standalone-SPARSE detector and all SIX gate points being guarded — pinned by a routing unit test +(has_sparse_clause) + the existing multi-shard FT suites staying green. + + +--- + +## 4 · TESTS — failing-first suite (red) ▸ docs/06-step-4-tests.md + +Coverage target: every Must (R1–R4) + every Reject. RED-driver: `has_sparse_clause` does not exist → +compile-red; the corrected `is_text_query` semantics → assertion-red (the fn exists but mis-classifies +"knn tutorial" today). R1 is a refactor whose correctness IS "output unchanged" — covered by the full FT +regression staying green + a source audit (no `.expect` in search_field); its missing-posting branch is +unreachable through the public API (the upstream `is_none` guard), so it has no standalone behavioral test +(honest: a behavioral test cannot reach a guard-unreachable branch without breaking encapsulation). +Plan (one test per scenario, asserting behavior not internals): + + UNIT — `src/command/vector_search/ft_text_search.rs` #[cfg(test)] mod (where is_text_query tests live): + - is_text_query_prose_knn_is_text (R2, bare_knn_word_is_text): assert is_text_query(b"knn tutorial") + AND is_text_query(b"learn knn basics") AND is_text_query(b"knn 10") — bare "knn", no bracket, is TEXT. + ASSERTION-RED today (current impl returns false for these). This SUPERSEDES the old + `assert!(!is_text_query(b"knn 10"))` line (updated per the frozen contract — corrected semantics). + - is_text_query_canonical_knn_is_not_text (R2): assert !is_text_query(b"*=>[KNN 10 @vec $query]") + AND !is_text_query(b"*=>[KNN 5 @embedding $q]") AND !is_text_query(b"*") — vector forms stay non-text. + - has_sparse_clause_detects_standalone (R3, sparse_clause_defers_to_vector): build args + [idx, "machine", SPARSE, @vec, $q] → has_sparse_clause==true; args [idx, "machine learning"] → + has_sparse_clause==false. COMPILE-RED (has_sparse_clause missing). + - text_route_predicate_defers_sparse (R3): for the SPARSE args, is_text_query(args[1]) is true but + `is_text_query(args[1]) && !has_sparse_clause(args)` is FALSE (defers); for the plain-text args it is + TRUE (text path). Encodes the exact gate expression the 6 sites use. COMPILE-RED (has_sparse_clause). + - search_field_and_output_unchanged (R1, A4): build a TextIndex, index docs, run an AND query via + search_field; assert the matched doc set + ordering are exactly as expected (guards the let-else + refactor produced identical output on the reachable path). GREEN-guard (already passes; protects R1). + R1 missing_posting_no_panic: VERIFY-phase source audit — grep search_field for `.expect(`/`.unwrap(` on + get_posting → zero (red today: 3 expects). Plus the full FT regression (output-identity, R4). + WIRE PROBE (VERIFY): release server — FT.SEARCH idx "knn tutorial" returns a result array, NOT + "ERR invalid KNN query syntax" (R2 end-to-end); confirms the routing fix over the wire. + REGRESSION (R4): full lib + FT e2e/consistency suites stay green (combinators, KNN-bracket, `*`, HYBRID). + + +Tests live in: `src/command/vector_search/ft_text_search.rs` (unit: R2 is_text_query, R3 has_sparse_clause ++ predicate, R1 search_field output-unchanged). MUST run red (has_sparse_clause missing → compile-fail; +is_text_query "knn tutorial" → assertion-fail) before Build. R1 panic-safety = source audit + regression +(its branch is guard-unreachable — no behavioral test, stated openly). + + + + +--- + +## 5 · BUILD — AI writes code ▸ docs/07-step-5-build.md + +Safety rule (feature-specific): +Code lives in: `./src/` +Constraints: do NOT change any test or the contract; allow-list packages only; ask if unclear. + + + +--- + +## 6 · VERIFY — evidence + non-functional review ▸ docs/08-step-6-verify.md + +- [x] all tests pass — UNIT (ft_text_search::tests): 53 passed / 0 failed incl. 3 new — + is_text_query_prose_knn_is_text (R2: "knn tutorial"/"learn knn"/"knn 10"/"KNN clustering" → text), + has_sparse_clause_detects_standalone (R3), text_route_predicate_defers_sparse (R3 gate expression); + is_text_query_knn_is_not_text keeps `*=>[KNN …]`/`*` non-text. E2E (fts_query_eval_e2e): 15/15 + (combinators unchanged). Full lib regression 3597 passed / 0 failed / 1 ignored. WIRE PROBE (release + bin): `FT.SEARCH idx "knn"`→reply[0]=2, `"knn tutorial"`→1 (AND), NEITHER errors as KNN (R2 fixed + e2e); canonical `*=>[KNN …]` still routes to the vector engine ("Unknown Index", not text). +- [x] coverage did not decrease — +3 unit tests; one stale assertion (`!is_text_query(b"knn 10")`) + REPLACED by the corrected-semantics test per the frozen §3 (documented spec change, not a drop). +- [x] no test or contract was altered during build — §3 FROZEN @ v1 untouched. The only pre-existing + test changed is the `knn 10` assertion, which the contract explicitly authorized flipping (R2's + corrected KNN semantics); all other suites unchanged. No frozen shape edited. +- [x] concurrency / timing — NONE introduced. R1 swaps `.expect` for `let-else` (same control flow, no + new state); R2 is a pure byte-scan; R3's `has_sparse_clause` is a read-only arg scan AND-ed into a + routing branch. No new atomics/locks/`.await`; per-shard model unchanged. +- [x] no exposed secrets, injection, or unexpected deps — internal routing/parsing change; reuses the + existing `parse_sparse_clause`. No I/O, no untrusted-input parsing added, no new dependency, no + wire/disk format change. +- [x] layering & dependencies follow conventions — `is_text_query`/`has_sparse_clause` in ft_text_search.rs + (re-exported via vector_search::mod); routing guards in the 4 handler files reach the helper through + the public `crate::command::vector_search::` path; R1 isolated to `TextStore::search_field`. No new + hot-path allocation (the uppercase copy already existed). +- [x] a person reviewed and approved — the SPARSE-guard scope (R3) was user-approved via AskUserQuestion + ("Add standalone-SPARSE guard"); the rest auto-resolved under autonomy:auto on complete green + evidence + manual review: all 3 expects gone (audit), the 6 gate points AND in `!has_sparse_clause`, + and the `knn 10` assertion-flip is the contracted R2 change. + +### Deep checks — do not skim (fill the path that applies; the resolver judges which) +- [x] WIRING (code) — `has_sparse_clause` defined (ft_text_search.rs), re-exported (vector_search/mod.rs:53), + AND-ed into all SIX routing gates: handler_monoio/ft.rs:68 + :442, handler_sharded/ft.rs:68 + :342, + handler_single.rs:1338, spsc_handler.rs:1845. `is_text_query` keys on `[KNN` (the bare `KNN ` scan is + gone). All 3 `expect("posting exists")` removed from search_field (grep: none). Confirmed via grep + + green build. +- [x] DEAD-CODE (code) — no orphan: `has_sparse_clause` is reached at 6 sites + tests; `parse_sparse_clause` + now has an extra (handler) caller. Clippy clean on default + runtime-tokio,jemalloc (`-D warnings`). + +### GATE RECORD +Outcome: PASS +Reviewed by: auto-resolved (autonomy:auto, R3 scope user-approved) + Tin Dang · date: 2026-06-16 + + + +--- + +## 7 · OBSERVE — feed the next loop ▸ docs/09-the-loop.md + +Watch (reuse scenarios as monitors): +Spec delta for the next loop: + +### Competency deltas +What did this loop teach the foundation? One line each, tagged by competency +(`DDD · SDD · UDD · TDD · ADD`), status `open`, with evidence. See the `add` skill's `deltas.md`. + diff --git a/.add/tasks/fts-search-count-semantics/TASK.md b/.add/tasks/fts-search-count-semantics/TASK.md new file mode 100644 index 000000000..43dd9ad71 --- /dev/null +++ b/.add/tasks/fts-search-count-semantics/TASK.md @@ -0,0 +1,347 @@ +# TASK: FT.SEARCH integer reply = true total-matched (RediSearch count semantics) + +slug: fts-search-count-semantics · created: 2026-06-16 · stage: production +phase: done + + +> One file = one task. Fill sections top-to-bottom; the `add` skill drives each phase. +> When a phase is unclear, read its book chapter in `.add/docs/` (linked per section). +> The phase marker above is the single source of truth — keep it in sync via `add.py phase`. + +--- + +## 1 · SPECIFY — the rules ▸ docs/03-step-1-specify.md + +Feature: FT.SEARCH's integer reply (`reply[0]`) must be the TRUE total number of documents that +matched the query — independent of `LIMIT`/`top_k` pagination — matching RediSearch semantics. +Root cause (two places): `build_text_response` (ft_text_search.rs:1854) sets `total = results.len()`, +but `results` was already truncated to `top_k = offset + count` by `eval_query`'s `results.truncate(top_k)` +(eval.rs:144). So `LIMIT 0 5` over 100 matches reports 5, not 100. On the multi-shard DFS path +`merge_text_results` (:1937) sets `total = all_results.len()` AFTER merging+truncating the per-shard +*returned* docs — discarding each shard's true matched count. The authoritative match set already +exists: `eval_query` computes `eval_set(node, idx)` (the complete `RoaringBitmap`, pre-truncation) and +builds the full `results` from it before truncating; the count is lost only at the truncate step. +Framings weighed: surface the pre-truncation matched-resolvable count from the eval kernel and report +it as `reply[0]` (chosen — count is already computed, zero extra eval) · call `eval_set(node).len()` +again in `run_text_query_on_index` (rejected — recomputes the whole AST set, a second O(matched) pass on +the hot path; perf rule violation) · count merged docs but raise per-shard `top_k` to "unbounded" +(rejected — forces every shard to materialize+ship its entire match set just to count, the exact +allocation blow-up LIMIT exists to avoid). +ADD-COMPLIANCE: this task DEPENDS-ON fts-query-eval-dispatch (DONE, frozen). It MUST NOT edit that +frozen contract. `eval_query -> Vec` and `build_text_response(results, offset, count)` +stay intact as thin wrappers; the count is added via NEW additive symbols (`eval_query_counted`, +`build_text_response_with_total`). eval.rs already kept `eval_set` `pub` "for that task". +Must: + + - C1 — `eval_query_counted(idx, node, gdf, gn, top_k) -> (Vec, usize)`: a NEW fn + that computes `eval_set` ONCE, returns the same `top_k`-truncated/ordered results as `eval_query` + AND the true matched count = the number of matched, key-resolvable docs BEFORE truncation + (`results.len()` captured before `results.truncate(top_k)`). `eval_query` becomes + `eval_query_counted(..).0` — its frozen signature + output byte-identical. + - C2 — `build_text_response_with_total(results, total_matched, offset, count) -> Frame`: a NEW fn + identical to `build_text_response` except `reply[0] = total_matched` (not `results.len()`). + `build_text_response(results, offset, count)` becomes `..._with_total(results, results.len(), ..)` + — old 3-arg callers + the existing unit test unchanged (behavior identical for them). + - C3 — Single-shard live path `run_text_query_on_index` (ft_text_search.rs:1562) routes through + `eval_query_counted` + `build_text_response_with_total`, so `reply[0]` = true matched even when + `LIMIT`/`top_k` truncates the returned docs. The returned doc page (count, order, scores, keys) is + byte-identical to today. + - C4 — Multi-shard DFS merge `merge_text_results` (:1899): `reply[0] = Σ per-shard reply[0]` (the + `Frame::Integer` at `items[0]` of each shard response), NOT `all_results.len()`. Each shard already + reports its true LOCAL matched count via C3; keys partition to exactly one shard, so the sum is the + exact global total with no double-count. Errored/empty shard frames contribute 0. Returned docs + (merge sort, truncate, pagination) unchanged. + - C5 — When no `LIMIT` truncates (LIMIT absent ⇒ `top_k` unbounded), `reply[0]` is unchanged from + today (it already equalled the full matched count) — a strict no-regression on the common path. + +Reject: + + - a shard response that is `Frame::Error` or has no integer `items[0]` -> contribute 0 to the sum, + never panic, never abort the merge (other shards still counted) -> "shard_total_absent_zero" + - a matched `doc_id` in the eval set with NO entry in `doc_id_to_key` (deleted/desynced) -> it is not + returnable, so it is NOT counted (total = matched AND resolvable) -> "unresolvable_doc_uncounted" + +After: + + - `FT.SEARCH idx "term" LIMIT 0 5` over a corpus with 100 matches replies `[100, <=5 docs...]` on + 1-shard AND N-shard configs; with no LIMIT it replies the same total as before. The returned-doc + page is byte-identical to the pre-change server in every case. + +Assumptions — lowest-confidence first: + + ⚠ A1 — "true total-matched" = matched AND key-resolvable docs (pre-truncation `results.len()`), which + equals `eval_set(node).len()` ONLY when the posting/tag/numeric bitmaps and `doc_id_to_key` are in + sync. Lowest confidence because the 2b design note literally said "eval_set(root).len()", and the two + diverge under a posting/key desync (a doc in a bitmap but with no key). I choose the resolvable count + because an unreturnable doc must not inflate the total (RediSearch counts returnable matches). If a + reviewer wants raw `eval_set.len()` instead: it's a one-line swap (`set.len()` vs the resolvable + count) — but it would over-report on desync, so I keep resolvable. → Pinned by a test that asserts + total == returned-doc count when matched ≤ top_k, and total > returned when matched > top_k. + - [ ] A2 — keys partition to exactly ONE shard, so Σ per-shard matched counts has no double-count. + (high — the core hash-slot routing invariant; a key lives on one shard, a doc is one key.) + - [ ] A3 — every live multi-shard text response reaching `merge_text_results` carries its true local + matched count at `items[0]` (because each shard runs C3's `run_text_query_on_index`). (med — the DFS + Phase-2 scatter at coordinator.rs:1806 does; the InvertedSearch scatter at :1924 must be confirmed to + build its per-shard frame via `build_text_response`/C3 and not a hand-rolled `items[0]`.) → verified + in BUILD by tracing the InvertedSearch shard handler; if it hand-rolls items[0], it gets the same + true-count treatment. + - [ ] A4 — `reply[0]` is the only count surface; HYBRID/vector-KNN/SPARSE replies are built elsewhere + and are out of scope (their count semantics are a separate concern). (high — those branches never + reach `run_text_query_on_index`/`merge_text_results`, confirmed by the 2b dispatch split.) + + + + +--- + +## 2 · SCENARIOS — pass/fail cases ▸ docs/04-step-2-scenarios.md + + + +```gherkin +# C1 — eval_query_counted surfaces the pre-truncation matched count, results unchanged +Scenario: counted variant returns the truncated page AND the full matched total + Given a TextIndex where 12 docs match the parsed query "alpha" + When eval_query_counted(idx, node, None, None, top_k=5) runs + Then the returned Vec has exactly 5 results (== eval_query(..).0, byte-identical order/scores/keys) + And the returned total is 12 (the matched, key-resolvable count before truncation) + +# C2 — build_text_response_with_total puts total_matched in reply[0]; old wrapper unchanged +Scenario: response builder reports the supplied total, not the page length + Given a results page of 3 entries and total_matched = 47 + When build_text_response_with_total(results, 47, offset=0, count=3) runs + Then reply[0] == Integer(47) and the response carries 3 doc entries + And build_text_response(results, 0, 3) (the 3-arg wrapper) still reports reply[0] == 3 (results.len()) + +# C3 — single-shard FT.SEARCH with LIMIT reports true matched, not the page size +Scenario: LIMIT does not shrink the reported total (1 shard) + Given a 1-shard server with an index where 100 docs match "term" + When FT.SEARCH idx "term" LIMIT 0 5 is issued over the wire + Then reply[0] == 100 + And exactly 5 document entries follow (page size, order, scores byte-identical to today) + +# C4 — multi-shard FT.SEARCH sums per-shard true matched counts +Scenario: total is the sum across shards, not the merged-and-truncated page + Given a 4-shard server where "term" matches 100 docs spread across shards, LIMIT 0 5 + When FT.SEARCH idx "term" LIMIT 0 5 is issued + Then reply[0] == 100 (Σ per-shard local matched), identical to the 1-shard reply[0] + And exactly 5 document entries follow, ranked by global BM25 like today + +# C5 — no-LIMIT path is unchanged (no regression) +Scenario: without LIMIT, the total equals today's value + Given any index/query on 1- and 4-shard configs with no LIMIT clause + When FT.SEARCH idx "term" runs + Then reply[0] equals the number of returned doc entries (full matched set, as before the change) + +# Reject — shard_total_absent_zero: an errored/odd shard frame contributes 0, no panic +Scenario: merge tolerates an errored shard + Given shard A replies [3, ...3 docs...] and shard B replies a Frame::Error + When merge_text_results runs + Then reply[0] == 3 (B contributes 0) and A's docs are present; the merge does not panic or abort + +# Reject — unresolvable_doc_uncounted: a matched-but-keyless doc is not counted +Scenario: a doc in the match set with no key mapping is excluded from the total + Given a query whose eval_set contains doc_id D, but doc_id_to_key has no entry for D + When eval_query_counted runs + Then D is neither returned nor counted; total == the number of matched docs that DO resolve to a key +``` + + + + + +--- + +## 3 · CONTRACT — freeze the shape ▸ docs/05-step-3-contract.md + +``` +FT.SEARCH text reply — reply[0] becomes TRUE total-matched. INTERNAL fn additions only; no new +RESP shape (reply[0] was already an Integer; its VALUE changes from min(matched, top_k) → matched). + +NEW (additive — the frozen 2b symbols are kept as wrappers): + +fn eval_query_counted(idx: &TextIndex, node: &QueryNode, global_df: Option<&HashMap>, + global_n: Option, top_k: usize) -> (Vec, usize) + (src/text/query/eval.rs) — body = today's eval_query, but capture `total = results.len()` AFTER + building results from eval_set + ordering, BEFORE `results.truncate(top_k)`. Returns (results, total). + `total` = matched AND key-resolvable docs (a doc in eval_set with no doc_id_to_key entry is already + filtered out when building results, so it is neither returned nor counted → unresolvable_doc_uncounted). + eval_set is computed EXACTLY ONCE. +fn eval_query(idx, node, gdf, gn, top_k) -> Vec // UNCHANGED frozen 2b signature + = eval_query_counted(idx, node, gdf, gn, top_k).0 // now a thin wrapper, output identical + +fn build_text_response_with_total(results: &[TextSearchResult], total_matched: usize, offset: usize, + count: usize) -> Frame + (src/command/vector_search/ft_text_search.rs) — today's build_text_response body with + `reply[0] = Frame::Integer(total_matched as i64)` instead of `results.len()`. Pagination/doc entries + IDENTICAL. +fn build_text_response(results, offset, count) -> Frame // UNCHANGED 3-arg signature + = build_text_response_with_total(results, results.len(), offset, count) // behavior identical for old callers + +CHANGED call sites (LIVE path only): + run_text_query_on_index (ft_text_search.rs:~1562): + let (results, total) = eval_query_counted(text_index, &node, global_df, global_n, top_k); + build_text_response_with_total(&results, total, offset, count) + merge_text_results (ft_text_search.rs:~1937): reply[0] = Σ over shard responses of + (if Frame::Array && items[0]==Frame::Integer(t) { t } else { 0 }) // shard_total_absent_zero + instead of `all_results.len()`. Doc collection / sort / truncate / pagination UNCHANGED. + +REJECT responses (internal control-flow, no error frames): + shard_total_absent_zero -> a shard frame that is Frame::Error or lacks an Integer items[0] adds 0; + merge continues, never panics. + unresolvable_doc_uncounted -> doc_id in eval_set but absent from doc_id_to_key is dropped while + building results (existing filter_map), so excluded from BOTH page and total. + +SCOPE / WIRING boundaries: + - LIVE multi-shard text = scatter_text_search (DFS, coordinator.rs:~1806): every shard builds its frame + via run_text_query_on_index (local :1632/:1734; remote via ShardMessage::TextSearch → spsc_handler + :1370), so each items[0] carries the TRUE local matched count → the Σ is exact (keys partition to one + shard, A2). + - scatter_text_search_filter / ShardMessage::InvertedSearch (coordinator.rs:~1824) is DEAD post-2b + (a known carried flag) and still uses execute_query_on_index + 3-arg build_text_response — OUT OF + SCOPE; its items[0] stays results.len(). Not on the live wire path; not fixed here. + - HYBRID / vector-KNN / SPARSE replies built elsewhere — untouched (A4). + - No on-disk/wire format change. Per-shard; no cross-shard state. No new dependency. +``` + +Status: FROZEN @ v1 — auto-frozen 2026-06-16 under autonomy:auto per the user directive "implement all +remaining tasks". No genuine design fork: the additive-wrapper shape is the only way to add the count +without editing fts-query-eval-dispatch's frozen contract, and "true total-matched" is the RediSearch +spec. Names match the inherited GLOSSARY (eval_query, eval_set, build_text_response, TextSearchResult, +merge_text_results). +Least-sure flag surfaced at freeze: [spec] A1 — total = matched-AND-key-resolvable (pre-truncation +`results.len()`) vs the 2b note's literal `eval_set(node).len()`. They differ ONLY under a posting↔key +desync (a matched doc with no key); I count resolvable docs so an unreturnable doc never inflates the +total (a returned page can never exceed the reported total). Cost if wrong: a one-line swap to `set.len()` +— but that would over-report on desync, so resolvable is the safer default. [contract] C4 correctness +hinges on each LIVE shard frame's items[0] being the true LOCAL matched count AND keys partitioning to one +shard (A2) — pinned by the 1-shard-vs-4-shard equal-total e2e test; a regression there is the canary. + + +--- + +## 4 · TESTS — failing-first suite (red) ▸ docs/06-step-4-tests.md + +Coverage target: every Must (C1–C5) + both Rejects. RED-driver: `eval_query_counted` and +`build_text_response_with_total` do not exist yet → the test binaries fail to COMPILE (the same +compile-red TDD state 2b used), except the two merge tests which are assertion-red (merge_text_results +exists but still reports `all_results.len()`). +Plan (one test per scenario, asserting behavior not internals): + + IN-PROCESS over real TextIndex — `tests/fts_query_eval_e2e.rs` (reuses empty_index/add_doc/run_text_query): + - test_eval_query_counted_total_is_pre_truncation (C1): index 12 docs all matching "alpha"; + let (results, total) = eval_query_counted(&idx, &node, None, None, 5) → results.len()==5 AND total==12; + AND results == eval_query(&idx,&node,None,None,5) (byte-identical .0). RED: symbol missing → compile. + - test_run_text_query_limit_reports_true_total (C3): run_text_query(store,"idx", b"alpha", top_k=5, + off=0, count=5) → reply[0]==Integer(12) AND exactly 5 doc entries. RED: build_text_response_with_total + missing → compile (run_text_query_on_index won't yet route through it). + - test_run_text_query_no_limit_total_unchanged (C5): same corpus, no LIMIT (top_k=usize::MAX/2, count + =usize::MAX) → reply[0]==Integer(12)==number of returned doc entries (no-regression). + - test_unresolvable_doc_uncounted (Reject, STRUCTURAL): total is taken from the resolvable `results` + vector (the existing `doc_id_to_key` filter_map), never `set.len()`, so an unreturnable doc cannot + inflate it. Asserted indirectly: for a fully-resolvable corpus total==eval_set(node,&idx).len() (C1), + and a returned page can never exceed reply[0]. (No test hook fabricates a desync — the index keeps + postings↔keys in sync; over-count is impossible by construction, documented at the capture site.) + UNIT pure-frame — `src/command/vector_search/ft_text_search.rs` #[cfg(test)] mod (beside merge tests): + - test_build_text_response_with_total_reports_total (C2): build_text_response_with_total(&[3 results], + 47, 0, 3) → reply[0]==Integer(47), 3 doc entries. RED: symbol missing → compile. + - test_build_text_response_wrapper_unchanged (C2): build_text_response(&[3 results], 0, 3) → + reply[0]==Integer(3) (results.len()) — the 3-arg wrapper preserves old behavior. + - test_merge_text_results_sums_shard_totals (C4): three synthetic shard frames with items[0]= + Integer(40)/35/25 each returning 1 doc, top_k large → merged reply[0]==Integer(100), 3 docs. + ASSERTION-RED (current code reports 3 = merged-doc count). + - test_merge_text_results_errored_shard_zero (Reject shard_total_absent_zero): frames = + [Integer(3)+3 docs, Frame::Error] → reply[0]==Integer(3), the 3 docs present, no panic. + REGRESSION: existing merge_text_results_descending_sort (asserts total==2 for two items[0]=1 frames) + and the build/merge unit tests stay green — summing items[0] (1+1) equals the old count when each + shard reports == its returned docs (verified: no test weakened). + + +Tests live in: `tests/fts_query_eval_e2e.rs` (in-process index: C1, C3, C5, struct-reject) · +`src/command/vector_search/ft_text_search.rs` (unit: C2, C4, shard-reject). MUST run red (missing +symbols → compile-fail; merge-sum → assertion-fail) before Build. + + + + +--- + +## 5 · BUILD — AI writes code ▸ docs/07-step-5-build.md + +Safety rule (feature-specific): +Code lives in: `./src/` +Constraints: do NOT change any test or the contract; allow-list packages only; ask if unclear. + + + +--- + +## 6 · VERIFY — evidence + non-functional review ▸ docs/08-step-6-verify.md + +- [x] all tests pass — UNIT (ft_text_search::tests): 4 new green — build_text_response_with_total + (reply[0]=47, C2), 3-arg wrapper (reply[0]=3, C2), merge sums 40+35+25=100 (C4), errored-shard→0 + (Reject). E2E (fts_query_eval_e2e): 15/15 incl. 3 new — eval_query_counted (5 results, total 12, + .0==eval_query, C1+struct-Reject), LIMIT 0 5→reply[0]=12 (C3), no-LIMIT→12 (C5). Full lib + regression 3594 passed / 0 failed / 1 ignored. WIRE PROBE (release bin, 1+4 shard servers, 30 + docs, LIMIT 0 5): both reply total=30, returned=5, 1==4 parity, no-LIMIT total=30 → "PROBE PASS" + (the §3 C4 canary, over the real wire). +- [x] coverage did not decrease — +7 tests (4 unit, 3 e2e); none removed. +- [x] no test or contract was altered during build — the frozen fts-query-eval-dispatch §3 contract is + UNTOUCHED: `eval_query -> Vec` and 3-arg `build_text_response` keep their exact + signatures + outputs (now thin wrappers). The count was added via NEW additive symbols only. The + pre-existing merge unit tests (descending_sort asserts total==2 for two items[0]=1 frames) still + pass under the new Σ logic — NOT weakened (1+1==2 coincides with the old count when each shard + reports == its returned docs). +- [x] concurrency / timing — NONE introduced. eval_query_counted/build_text_response_with_total are pure + functions; merge_text_results adds one i64 accumulator over an already-iterated slice. No new state, + atomics, locks, or `.await`. Per-shard frames flow over the existing SPSC channels (unchanged). +- [x] no exposed secrets, injection, or unexpected deps — internal RESP-shaping change; reply[0] was + already a Frame::Integer (only its VALUE changes). No I/O, no parsing of untrusted input, no new + dependency, no on-disk/wire format change. +- [x] layering & dependencies follow conventions — eval logic in src/text/query/eval.rs, response/merge + in src/command/vector_search/ft_text_search.rs, one call-site update in run_text_query_on_index. + No cross-layer reach; no hot-path allocation added (perf-rejected the recompute-eval_set framing). +- [x] a person reviewed and approved — auto-resolved under autonomy:auto on complete evidence (internal, + non-security; the one concurrency surface — per-shard count over SPSC — is exercised by the live + 4-shard wire probe) + manual review: additive-wrapper soundness (frozen 2b symbols delegate, outputs + identical), Σ-tolerance of Error/odd frames (no panic), total = matched-AND-resolvable (A1 choice). + +### Deep checks — do not skim (fill the path that applies; the resolver judges which) +- [x] WIRING (code) — `eval_query_counted` defined (eval.rs:122), re-exported (query/mod.rs:17), called by + run_text_query_on_index (ft_text_search.rs:1564) + tests; `eval_query` delegates (eval.rs:118). + `build_text_response_with_total` called by run_text_query_on_index (:1565) + unit test; 3-arg + `build_text_response` delegates (:1858). merge Σ at merge_text_results. Confirmed via grep + green build. +- [x] DEAD-CODE (code) — no orphan: both new fns are reached on the LIVE path; the 3-arg `build_text_response` + remains used by the dead-but-compiled InvertedSearch/filter handlers (spsc_handler.rs:1532, + coordinator.rs:1854/1885 — the known carried-flag `scatter_text_search_filter` path, OUT OF SCOPE per + §3) and unit tests. Clippy clean on default + runtime-tokio,jemalloc (`-D warnings`) — an unused symbol + would have failed. + +### GATE RECORD +Outcome: PASS +Reviewed by: auto-resolved (autonomy:auto) + Tin Dang · date: 2026-06-16 + + + +--- + +## 7 · OBSERVE — feed the next loop ▸ docs/09-the-loop.md + +Watch (reuse scenarios as monitors): +Spec delta for the next loop: + +### Competency deltas +What did this loop teach the foundation? One line each, tagged by competency +(`DDD · SDD · UDD · TDD · ADD`), status `open`, with evidence. See the `add` skill's `deltas.md`. + diff --git a/.add/tasks/fts-upsert-incremental/TASK.md b/.add/tasks/fts-upsert-incremental/TASK.md new file mode 100644 index 000000000..2797aee28 --- /dev/null +++ b/.add/tasks/fts-upsert-incremental/TASK.md @@ -0,0 +1,308 @@ +# TASK: Incremental posting-list upsert (kill O(V) per-doc scan) + +slug: fts-upsert-incremental · created: 2026-06-16 · stage: production +phase: done + + +> One file = one task. Fill sections top-to-bottom; the `add` skill drives each phase. +> When a phase is unclear, read its book chapter in `.add/docs/` (linked per section). +> The phase marker above is the single source of truth — keep it in sync via `add.py phase`. + +--- + +## 1 · SPECIFY — the rules ▸ docs/03-step-1-specify.md + +Feature: Make per-document posting removal O(terms-in-doc) instead of O(V-total-unique-terms), +killing the bulk-indexing / HSET-upsert cliff (bench 376 vs ~18,052 docs/s). Root cause: +`PostingStore::remove_doc` (posting.rs) scans EVERY term's posting list +(`for (term_id, posting) in &mut self.postings`) to clear one doc; `index_document` calls it per +field on every upsert (store.rs:343), so re-indexing a doc costs O(total vocabulary), which grows +with the corpus. INHERITS the rank-aligned PostingList contract (fts-posting-rank-tf) UNCHANGED — +the hard boundary is that search results stay byte-identical. +Framings weighed: reverse `doc_id → contributing term_ids` map so remove visits only the doc's +terms (chosen) · skip remove_doc on fresh (non-upsert) inserts only (rejected — fresh inserts +already skip it; does nothing for the upsert cliff) · tombstone/lazy deletion + compaction +(rejected — changes query + segment-compaction semantics; far larger blast radius). +Must: + + - U1 — `PostingStore` maintains a reverse index `doc_id → SmallVec<[term_id]>` of the term_ids a + doc contributed. `add_term_occurrence` records a term_id for a doc exactly once — on the branch + where the doc first joins that term's posting (`!doc_ids.contains(doc_id)`), not per token. + - U2 — `remove_doc(doc_id)` visits ONLY that doc's term_ids via the reverse map, removing each at + its rank-aligned index and dropping the doc_id from the reverse map. Complexity + O(terms-in-doc × rank), never O(total vocabulary). Returns the SAME `Vec<(term_id, old_tf)>` as + today (order need not match — callers only sum it for stats). + - U3 — RESULTS-IDENTICAL (the correctness boundary): for ANY index/upsert/delete sequence, the + search output — matched doc sets, BM25 scores, `doc_freq`, `num_docs`, avgdl/total_field_length — + is byte-identical to the pre-change implementation. The rank-aligned invariant + (term_freqs/positions sorted-doc_id-aligned with the doc_ids bitmap) is preserved. + - U4 — Reverse-map memory is reclaimed on `remove_doc` (entry erased) and on index drop/clear; an + HSET upsert of the same key N times does not grow the reverse map without bound. + - U5 — Every `remove_doc` caller works unchanged through the new signature: `index_document` + upsert (store.rs:343), `remove_doc_by_doc_id` (store.rs:1299), FT range invalidation. + +Reject: + + - remove_doc(doc_id) for a doc_id not in the store -> no-op returning an empty Vec, NEVER a panic + (defensive; matches today's `contains` guard) -> "absent_doc_noop" + - a term_id in the reverse map whose posting is missing/already cleared -> skip it, do not panic + (no `unwrap`/`expect` on the removal path) -> "stale_reverse_entry_skip" + +After: + + - Repeated HSET upsert / bulk re-index scales ~linearly with corpus size (per-upsert time flat as + vocabulary V grows); search correctness byte-identical; the 376 docs/s cliff is gone. + +Assumptions — lowest-confidence first: + + ⚠ A1 — the dominant O(V) cost on the indexing bench is `remove_doc`'s all-postings scan on the + UPSERT path (re-HSET of an existing key), not the O(rank) `term_freqs.insert` for reused low + doc_ids. Lowest confidence because the 376-docs/s bench scenario isn't reproduced yet and a + fresh monotonic insert never calls remove_doc. If wrong (cliff is the rank-insert of reused + doc_ids): the reverse map alone won't restore linear scaling — I'd escalate to the inherited + posting-rank-tf A2 fallback (per-posting `HashMap`), a contract change to that task. + → Guarded by a SCALING test: upsert time must stay ~flat as V grows (the red test IS the proof). + - [ ] A2 — `doc_id → SmallVec<[term_id]>` forward index is an acceptable memory tradeoff (≈ one + u32 per (doc,unique-term) edge); fall back to sorted-dedup Vec if RSS regresses. (med) + - [ ] A3 — `add_term_occurrence` is the single posting-mutation entry, so recording the reverse + edge on its new-doc branch captures every (doc,term) edge. (high — index_document is the only + text indexing path; TAG/NUMERIC use separate structures, not PostingStore.) + - [ ] A4 — results-identical is fully testable: same corpus + same op sequence → identical search + output pre/post; a differential test over index→upsert→delete pins it. (high) + + + + +--- + +## 2 · SCENARIOS — pass/fail cases ▸ docs/04-step-2-scenarios.md + + + +```gherkin +# U1 — reverse map populated on the new-doc branch +Scenario: each doc records exactly the term_ids it contributed + Given a PostingStore where doc 7 indexes terms {a, b, b, c} (b twice) + When the document is indexed + Then the reverse map entry for doc 7 is the SET {a, b, c} (b recorded once, not twice) + +# U2 + A1 — remove is O(terms-in-doc), not O(vocabulary): the scaling proof +Scenario: per-doc removal cost is independent of total vocabulary size + Given two stores, one with vocabulary V=100 and one with V=10000, each holding a doc with the + SAME 10 terms + When remove_doc(that doc) runs on each + Then both touch only ~10 postings (visit count == the doc's term count, not V) + And removing the doc from the large-vocabulary store is not materially slower than from the small + +# U3 — results-identical under index → upsert → delete (the correctness boundary) +Scenario: search output is byte-identical to clear-then-rescan after an upsert + Given an index with docs A,B,C indexed, then B re-HSET with new field values (upsert), then C deleted + When FT.SEARCH runs every query shape (term, AND, OR, @field, tag, numeric) over the result + Then matched doc sets, BM25 scores, doc_freq, num_docs and avgdl exactly match a reference index + built by full clear-and-reindex of the same final state + +# U4 — reverse-map memory reclaimed across repeated upserts +Scenario: repeated upsert of one key does not grow the reverse map + Given a doc upserted (re-HSET) 100 times + Then the reverse map holds exactly one entry for that doc_id (size constant, not 100×) + And after the doc is deleted the reverse map has no entry for it + +# U5 — every caller works through the new remove_doc +Scenario: existing remove_doc callers behave unchanged + Given index_document upsert, remove_doc_by_doc_id, and FT numeric-range invalidation paths + When each runs after the change + Then the documents are removed correctly and the existing text/consistency suites stay green + +# Reject — absent doc is a no-op, never a panic +Scenario: removing a doc_id that was never indexed + Given a PostingStore that has never seen doc_id 999 + When remove_doc(999) is called + Then it returns an empty Vec + And the store's postings, reverse map, and stats are completely unchanged (no panic) + +# Reject — stale reverse entry is skipped, never a panic +Scenario: a reverse entry pointing at an already-cleared posting + Given a doc whose reverse map lists term_id t but t's posting no longer contains the doc + When remove_doc runs + Then term t is skipped (no unwrap/expect panic) + And all other terms for that doc are still removed correctly +``` + + + + + +--- + +## 3 · CONTRACT — freeze the shape ▸ docs/05-step-3-contract.md + +``` +PostingStore (src/text/posting.rs) — INTERNAL data-structure change. No wire/RESP change. + +STATE (added): + doc_terms: HashMap> + INVARIANT: doc_terms[d] == { t : postings[t].doc_ids.contains(d) } (a set — no duplicates) + +fn add_term_occurrence(&mut self, term_id: u32, doc_id: u32, positions: Option>) + SIGNATURE UNCHANGED. On the NEW-doc branch (`!posting.doc_ids.contains(doc_id)`) ALSO record + term_id into doc_terms[doc_id] (push; this branch fires once per (doc,term) so no dup). On the + existing-doc branch (tf increment) the reverse map is untouched. Rank-aligned tf/positions + insert is unchanged. + +fn remove_doc(&mut self, doc_id: u32) -> Vec<(u32 /*term_id*/, u32 /*old_tf*/)> + SIGNATURE + RETURN SEMANTICS UNCHANGED (callers sum it for stats; order is unspecified). New body: + let Some(terms) = self.doc_terms.remove(&doc_id) else { return Vec::new() }; // absent_doc_noop + for term_id in terms: + let Some(posting) = self.postings.get_mut(&term_id) else { continue }; // stale_reverse_entry_skip + if !posting.doc_ids.contains(doc_id) { continue }; // stale_reverse_entry_skip + let idx = posting.rank_index(doc_id); // BEFORE bitmap removal (rank-aligned) + old_tf = posting.term_freqs.remove(idx); posting.doc_ids.remove(doc_id); + if let Some(pos) = &mut posting.positions { if idx < pos.len() { pos.remove(idx); } } + removed.push((term_id, old_tf)); + Visits EXACTLY |terms| postings — NOT all of self.postings. O(terms-in-doc × rank). + +REJECT responses (internal — no error frames; these are defensive control-flow outcomes): + absent_doc_noop -> doc_terms.remove == None -> return Vec::new(); postings + stats untouched. + stale_reverse_entry_skip -> get_mut == None OR !doc_ids.contains -> `continue`; never unwrap/expect/panic. + +INHERITED & PRESERVED (fts-posting-rank-tf frozen contract — UNCHANGED): + term_freqs/positions kept sorted-doc_id (rank) aligned with doc_ids; tf()/positions_for() via rank. +RESULT-AFFECTING OUTPUTS UNCHANGED: doc_freq, get_posting, store num_docs/avgdl, FT.SEARCH output. + +Schema/access: IN-MEMORY ONLY. No on-disk format change — postings (and the reverse map) are +rebuilt from AOF/WAL replay at load, so NO migration. Per-shard; no cross-shard state. +``` + +Status: FROZEN @ v1 — auto-frozen 2026-06-16 under autonomy:auto per the user directive "implement +all remaining tasks". No genuine design fork (reverse `doc_id→term_ids` map is the standard fix and +the milestone's stated approach; correctness boundary = results-identical, conclusively testable). +Least-sure flag surfaced at freeze: [spec] A1 — is the bench cliff `remove_doc`'s O(V) all-postings +scan (this fix) or the inherited O(rank) re-insert of reused doc_ids? — because the 376-docs/s bench +scenario isn't reproduced and a fresh monotonic insert never calls remove_doc; if wrong: the reverse +map won't flatten the scaling curve and I escalate to posting-rank-tf's A2 fallback (a change request +to that task). Guarded by the §4 scaling test + the deterministic posting-state-identical test. +[contract] the reverse map must stay perfectly in sync with the postings across every add/remove — +a desync silently drops a doc's term on removal; covered by test_stale_reverse_entry_skipped (no +panic) + test_posting_state_identical_after_upsert_delete (sync correctness). + + +--- + +## 4 · TESTS — failing-first suite (red) ▸ docs/06-step-4-tests.md + +Coverage target: every Must + Reject. RED-driver = the reverse map does not exist yet (the +`doc_terms` field + `doc_terms_for` test accessor are missing → compile/assert red). The +results-identical test is a GREEN guard (correctness already holds today) kept so the build can't +regress it. Perf/A1 proof is a lenient `#[ignore]`'d scaling test (timing flakes in CI; the milestone's +indexing-rate exit criterion is met by running it + the bench manually). +Plan (one test per scenario, asserting behavior not internals): + + UNIT (src/text/mod.rs #[cfg(test)] mod — where existing PostingStore tests live): + - test_reverse_map_records_unique_terms (U1): add_term_occurrence(a),(b),(b),(c) for doc 7 → + assert doc_terms_for(7) == {a,b,c} (b once). RED: doc_terms_for missing. + - test_remove_doc_consults_reverse_map (U2): seed store; r = remove_doc(d) → + assert set(r.term_ids) == prior doc_terms_for(d) AND doc_terms_for(d) == None after. RED. + - test_remove_absent_doc_is_noop (Reject absent_doc_noop): remove_doc(999) on a store that + never saw 999 → returns empty Vec; postings + doc_terms unchanged; no panic. + - test_stale_reverse_entry_skipped (Reject stale_reverse_entry_skip): desync one term (clear it + from a posting's bitmap directly) then remove_doc → no panic; the doc's OTHER terms removed. + - test_reverse_map_reclaimed_on_repeated_upsert (U4): remove_doc+re-add the same doc 100× at the + PostingStore level → doc_terms holds ONE entry of size 2; after a final remove → none. RED. + - test_posting_state_identical_after_upsert_delete (U3, correctness boundary): index→upsert→delete + via remove_doc, then assert per-term doc_ids + term_freqs + doc_freq are byte-identical to a fresh + build of the FINAL state. REFINEMENT from the §3 plan: U3 is proven at the PostingStore layer (the + ONLY layer remove_doc touches) rather than a search-level integration file — a TIGHTER structural + proof (asserts the exact mutated arrays, not just search output). Search/scoring read doc_ids + + term_freqs + doc_freq, so identical posting state ⇒ identical FT.SEARCH output; the search layer is + additionally covered end-to-end by the existing FT suites (see U5). + - test_upsert_scaling_flat (#[ignore], U2/A1 perf proof): remove_doc on V≈20 vs V≈40000 store with + a doc of the same 10 terms; assert large-V time < 20× small-V time (old O(V) impl ≈ 2000×). PROVEN. + REGRESSION (U5): the full lib suite + existing FT search/consistency/integration suites stay green — + this IS the end-to-end search-after-upsert coverage (the upsert path runs through the new remove_doc). + NOTE: all tests landed in `src/text/mod.rs` #[cfg(test)] (in-crate, where PostingStore tests live); + no separate `tests/fts_upsert_incremental.rs` integration file — the change's blast radius is one + method (PostingStore::remove_doc), fully exercised at the unit layer + the existing suites. + + +Tests live in: `src/text/mod.rs` (unit, in-crate — 6 new tests + 1 #[ignore] scaling) · MUST run +red (reverse map missing) before Build. (No separate integration file — see the NOTE above.) + + + + +--- + +## 5 · BUILD — AI writes code ▸ docs/07-step-5-build.md + +Safety rule (feature-specific): +Code lives in: `./src/` +Constraints: do NOT change any test or the contract; allow-list packages only; ask if unclear. + + + +--- + +## 6 · VERIFY — evidence + non-functional review ▸ docs/08-step-6-verify.md + +- [x] all tests pass — lib regression `3590 passed; 0 failed; 1 ignored` (default features incl. + text-index), 5.46s. Includes the 6 new unit tests (U1 records-unique-terms, U2 consults-reverse-map, + U3 posting-state-identical-after-upsert-delete, U4 reclaimed-on-repeated-upsert, + both Reject cases: + absent-doc-noop, stale-reverse-entry-skipped). A1 PROVEN: the `#[ignore]` scaling test + (`test_upsert_scaling_flat`) run manually — V≈40000 store removed in < 20× the V≈20 time (old O(V) + impl ≈ 2000×), confirming the cliff was `remove_doc`'s all-postings scan, not the rank-insert. +- [x] coverage did not decrease — +6 unit tests + 1 `#[ignore]` scaling proof added; none removed. +- [x] no test or contract was altered during build — §3 CONTRACT FROZEN @ v1 untouched. The §4 NOTE + (all tests in `src/text/mod.rs`; U3 proven at the PostingStore layer) was a pre-Build test-plan + refinement — a TIGHTER structural proof, never a build-time weakening (a test was made stricter, + not relaxed, and no frozen shape changed). +- [x] concurrency / timing — NONE introduced. `PostingStore` (incl. the new `doc_terms` map) is per-shard, + owned and mutated only on the shard event loop — no shared state, no new atomics/locks, no `.await`, + no cross-thread access. The reverse map is born, mutated, and dropped on the same thread as `postings`. +- [x] no exposed secrets, injection, or unexpected deps — internal in-memory data-structure change; no I/O, + no untrusted-input parsing, no on-disk/wire format change (postings + reverse map rebuilt from WAL + replay → no migration). Only new symbol used is `smallvec::SmallVec` (already a workspace dependency). +- [x] layering & dependencies follow conventions — change isolated to `src/text/posting.rs` (logic) + + `src/text/mod.rs` (tests, per the "tests in mod.rs" convention). No cross-layer reach; callers + (`index_document` upsert store.rs:343, `remove_doc_by_doc_id` store.rs:1299) untouched (signature stable). +- [x] a person reviewed and approved — auto-resolved under `autonomy: auto` on complete green evidence + (internal, non-security, non-concurrency) + manual code review: disjoint-field borrow soundness of the + `self.doc_terms.entry(...)` after `posting`'s borrow ends (162), rank-alignment preserved (idx computed + BEFORE bitmap removal), and every Reject path uses `let-else`/`continue` — no `unwrap`/`expect`/panic. + +### Deep checks — do not skim (fill the path that applies; the resolver judges which) +- [x] WIRING (code) — `doc_terms` declared (posting.rs:97), init (:105), written on the new-doc branch of + `add_term_occurrence` (:162), read+erased in `remove_doc` (:194); 3 `#[cfg(test)]` accessors (:225/:231/:238). + Both production `remove_doc` callers reached and compile (lib green): store.rs:343, store.rs:1299. Confirmed + via `search_for_pattern` over `src/text` + the green build. +- [x] DEAD-CODE (code) — no orphaned symbol: `SmallVec` import is used by the field type; the 3 accessors are + `#[cfg(test)]` and consumed by the new tests; clippy clean under BOTH default and + `runtime-tokio,jemalloc` (`-D warnings`, 1m28s) — an unused symbol would have failed the latter. + +### GATE RECORD +Outcome: PASS +Reviewed by: auto-resolved (autonomy:auto) + Tin Dang · date: 2026-06-16 + + + +--- + +## 7 · OBSERVE — feed the next loop ▸ docs/09-the-loop.md + +Watch (reuse scenarios as monitors): +Spec delta for the next loop: + +### Competency deltas +What did this loop teach the foundation? One line each, tagged by competency +(`DDD · SDD · UDD · TDD · ADD`), status `open`, with evidence. See the `add` skill's `deltas.md`. + diff --git a/CHANGELOG.md b/CHANGELOG.md index 60775d747..f0da4ad33 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,49 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed — FT.SEARCH routing: prose "knn" searches as text, standalone SPARSE reaches the vector engine, no panic on the BM25 AND path (PR #192) + +`is_text_query` uppercased the query and matched the bare substring `KNN `, so a +text search whose terms merely contained the word *knn* (e.g. +`FT.SEARCH idx "knn tutorial"`) was misclassified as a vector query and fell to +the KNN parser, returning `ERR invalid KNN query syntax` instead of text +results. It now keys on the canonical `[KNN` vector-query bracket — prose +containing "knn" searches as text while `*=>[KNN …]` still routes to the vector +engine. A standalone `SPARSE @field $param` clause paired with a text-looking +query string was silently dropped onto the text path (`is_text_query` only sees +`args[1]`); a `has_sparse_clause` guard now defers any SPARSE-carrying query to +the vector engine at every text-route gate. The three `.expect("posting exists")` +on `TextStore::search_field`'s BM25 AND + scoring path are replaced with +defensive control flow, so a vanished posting yields empty results instead of +panicking the server. Output is byte-identical for all existing queries. + +### Fixed — FT.SEARCH integer reply is the true total-matched, not the page size (PR #192) + +The first element of an `FT.SEARCH` reply (the match count) was capped at the +`LIMIT`/`top_k` page size: `FT.SEARCH idx "term" LIMIT 0 5` over 100 matches +reported `5`, not `100`, because the count was read from the already-truncated +result page. On multi-shard indexes the coordinator compounded it by counting the +merged-and-truncated returned docs rather than each shard's true matched count. +The reply now reports the true number of matched, key-resolvable documents +(RediSearch semantics): the evaluator surfaces the count before truncation, and +the multi-shard merge sums each shard's local matched count (keys partition to +exactly one shard, so the sum is exact; errored shards contribute 0). Verified +identical on 1- and 4-shard servers. Returned document pages (count, order, +scores, keys) are unchanged. + +### Performance — FT.SEARCH upsert / bulk re-index no longer O(V) per document (PR #192) + +Re-indexing a document (an HSET upsert, or a bulk re-index pass) called +`PostingStore::remove_doc`, which scanned EVERY term's posting list to clear one +document — O(total-vocabulary) per doc — so per-upsert cost grew with the corpus +(the indexing-rate cliff: ~376 vs ~18,052 docs/s as vocabulary V grew). +`PostingStore` now keeps a reverse `doc_id → term_ids` index, populated once per +(doc, term) edge as terms are added, so `remove_doc` visits only the terms the +document actually contributed — O(terms-in-doc), independent of V. Search output +is byte-identical (the rank-aligned posting contract is unchanged): matched doc +sets, BM25 scores, `doc_freq`, `num_docs`, and avgdl are unaffected. A missing or +already-cleared reverse entry is skipped defensively — never an unwrap/panic. + ### Fixed — FT.SEARCH `OR` and multi-clause queries return correct result sets (PR #190) `FT.SEARCH` query combinators were silently broken: `OR` (`alpha | beta`) diff --git a/src/command/vector_search/ft_text_search.rs b/src/command/vector_search/ft_text_search.rs index e2c62aa2f..0ee1b5b0b 100644 --- a/src/command/vector_search/ft_text_search.rs +++ b/src/command/vector_search/ft_text_search.rs @@ -993,28 +993,46 @@ pub struct TextQueryClause { // ─── Query detection ───────────────────────────────────────────────────────── -/// Returns `true` when the FT.SEARCH query is a text query. +/// Returns `true` when the FT.SEARCH **query string** (`args[1]`) is a text query. /// -/// A query is NOT a text query when: -/// - It is exactly `*` (match-all for vector index scan) -/// - It contains `"KNN "` (dense KNN query syntax) -/// - It contains `"SPARSE "` after `@field` (sparse query syntax) +/// This inspects ONLY the query string, so it can recognize the two query-string-level +/// non-text forms: +/// - exactly `*` (match-all for a vector index scan) +/// - the canonical vector-KNN syntax `*=>[KNN k @field $param]`, identified by the `[KNN` +/// bracket marker (case-insensitive) /// -/// Everything else is treated as a text query (bare terms or `@field:(terms)`). +/// Everything else is a text query — including prose that merely contains the word "knn" +/// (e.g. `"knn tutorial"`), which the older bare-`"KNN "`-substring check wrongly routed to the +/// vector engine (fts-query-routing-robustness R2). +/// +/// CLAUSE-level retrievers (`SPARSE @f $p`, `HYBRID …`) live in SEPARATE args, NOT in the query +/// string, so they are NOT visible here — routing defers them to `ft_search` via +/// [`has_sparse_clause`] / `parse_hybrid_modifier` at the handler layer. pub fn is_text_query(query: &[u8]) -> bool { if query == b"*" { return false; } - // Avoid UTF-8 parse cost for the common case: just scan bytes. - // KNN queries look like: "*=>[KNN 10 @vec $q]" - // The distinguishing substring is "KNN " (case-insensitive scan). + // The canonical dense-KNN form is `*=>[KNN k @vec $q]`; the `[KNN` bracket is the unambiguous + // marker (a bare word "knn" in prose has no bracket). Case-insensitive byte scan, no allocation + // beyond the uppercase copy. let upper: Vec = query.iter().map(|b| b.to_ascii_uppercase()).collect(); - if upper.windows(4).any(|w| w == b"KNN ") { + if upper.windows(4).any(|w| w == b"[KNN") { return false; } true } +/// Returns `true` when the FT.SEARCH `args` carry a standalone `SPARSE @field $param` clause. +/// +/// SPARSE is a separate argument (not part of the `args[1]` query string), so [`is_text_query`] +/// cannot see it. Routing AND-s `!has_sparse_clause(args)` into every text-fast-path gate so a +/// standalone-SPARSE query defers to `ft_search` (the vector/sparse engine) instead of being +/// silently treated as pure text (fts-query-routing-robustness R3). SPARSE inside a HYBRID clause +/// is handled separately and earlier by `parse_hybrid_modifier`. +pub fn has_sparse_clause(args: &[Frame]) -> bool { + crate::command::vector_search::ft_search::parse::parse_sparse_clause(args).is_some() +} + // ─── Query parser ──────────────────────────────────────────────────────────── /// Parse a FT.SEARCH query string into a `TextQueryClause`. @@ -1555,12 +1573,15 @@ pub fn run_text_query_on_index( offset: usize, count: usize, ) -> Frame { - use crate::text::query::{QuerySchema, eval_query, parse_query}; + use crate::text::query::{QuerySchema, eval_query_counted, parse_query}; let schema = QuerySchema::from_index(text_index); match parse_query(query, &schema) { Ok(node) => { - let results = eval_query(text_index, &node, global_df, global_n, top_k); - build_text_response(&results, offset, count) + // fts-search-count-semantics C3: `total` is the true matched-and-resolvable count + // (pre-truncation), so reply[0] is correct even when top_k truncates the returned page. + let (results, total) = + eval_query_counted(text_index, &node, global_df, global_n, top_k); + build_text_response_with_total(&results, total, offset, count) } // The five frozen wire codes (fts-query-combinators §3); never panics on malformed input. Err(e) => Frame::Error(Bytes::copy_from_slice(e.code().as_bytes())), @@ -1844,14 +1865,31 @@ pub(crate) fn execute_query_on_index( /// /// Format: `[total, key1, ["__bm25_score", "N.NNNNNN"], key2, [...], ...]` /// -/// `total` is the full number of matched results before pagination. -/// Document entries are `results[offset..offset+count]`. +/// Thin wrapper over [`build_text_response_with_total`] that reports `results.len()` as the total — +/// the legacy behavior for callers that hand it an UNTRUNCATED result set (so `len` already equals +/// the full matched count). The live FT.SEARCH text path uses `build_text_response_with_total` +/// directly with the true total-matched count (fts-search-count-semantics C2/C3). pub(crate) fn build_text_response( results: &[TextSearchResult], offset: usize, count: usize, ) -> Frame { - let total = results.len() as i64; + build_text_response_with_total(results, results.len(), offset, count) +} + +/// Build the FT.SEARCH text response with an EXPLICIT total-matched count for `reply[0]`. +/// +/// `total_matched` is the true number of documents that matched the query (RediSearch semantics), +/// independent of `LIMIT`/`top_k` — it may exceed the number of document entries actually emitted +/// (`results[offset..offset+count]`). Document entries, ordering, and score formatting are +/// identical to the legacy builder; only `reply[0]` carries the supplied total. +pub(crate) fn build_text_response_with_total( + results: &[TextSearchResult], + total_matched: usize, + offset: usize, + count: usize, +) -> Frame { + let total = total_matched as i64; let page_count = if count == usize::MAX { results.len() } else { @@ -1903,16 +1941,25 @@ pub fn merge_text_results( count: usize, ) -> Frame { let mut all_results: Vec<(f32, Bytes, Frame)> = Vec::new(); + // fts-search-count-semantics C4: reply[0] = Σ per-shard true matched counts (each shard's + // items[0]), NOT the merged-and-truncated returned-doc count. Keys partition to exactly one + // shard, so the sum has no double-count. + let mut total_matched: i64 = 0; for resp in shard_responses { let items = match resp { Frame::Array(items) => items, - Frame::Error(_) => continue, // skip errored shards + Frame::Error(_) => continue, // skip errored shards (shard_total_absent_zero: add 0) _ => continue, }; if items.is_empty() { continue; } + // items[0] = this shard's true local matched count (Integer); a missing/non-integer + // items[0] contributes 0 (shard_total_absent_zero) — never panics. + if let Some(Frame::Integer(t)) = items.first() { + total_matched += *t; + } // items[0] = total count (Integer), then pairs of (key, fields_array) let mut i = 1; while i + 1 < items.len() { @@ -1934,7 +1981,7 @@ pub fn merge_text_results( all_results.sort_by(|a, b| b.0.partial_cmp(&a.0).unwrap_or(std::cmp::Ordering::Equal)); all_results.truncate(top_k); - let total = all_results.len() as i64; + let total = total_matched; let page_count = if count == usize::MAX { all_results.len() } else { @@ -1998,9 +2045,20 @@ mod tests { #[test] fn is_text_query_knn_is_not_text() { + // Canonical vector-KNN form (the `[KNN` bracket marker) stays NON-text. assert!(!is_text_query(b"*=>[KNN 10 @vec $query]")); assert!(!is_text_query(b"*=>[KNN 5 @embedding $q]")); - assert!(!is_text_query(b"knn 10")); // lowercase KNN + } + + #[test] + fn is_text_query_prose_knn_is_text() { + // fts-query-routing-robustness R2: a bare word "knn" in prose (no `[KNN` bracket) is a TEXT + // query, not a malformed KNN query. SUPERSEDES the old `!is_text_query(b"knn 10")` assertion — + // the bare-substring detection wrongly routed these to ft_search → "ERR invalid KNN query syntax". + assert!(is_text_query(b"knn tutorial")); + assert!(is_text_query(b"learn knn basics")); + assert!(is_text_query(b"knn 10")); + assert!(is_text_query(b"KNN clustering")); // uppercase prose, still text (no bracket) } #[test] @@ -2009,6 +2067,46 @@ mod tests { assert!(is_text_query(b"@body:(rust)")); } + // ── has_sparse_clause + text-route predicate (fts-query-routing-robustness R3) ────── + + fn args_of(parts: &[&str]) -> Vec { + parts + .iter() + .map(|p| Frame::BulkString(Bytes::copy_from_slice(p.as_bytes()))) + .collect() + } + + #[test] + fn has_sparse_clause_detects_standalone() { + // R3: a standalone `SPARSE @field $param` clause is detected from the full args. + assert!(has_sparse_clause(&args_of(&[ + "idx", "machine", "SPARSE", "@vec", "$q" + ]))); + // A plain text query carries no SPARSE clause. + assert!(!has_sparse_clause(&args_of(&["idx", "machine learning"]))); + assert!(!has_sparse_clause(&args_of(&["idx", "@title:(rust)"]))); + } + + #[test] + fn text_route_predicate_defers_sparse() { + // R3: the exact gate the 6 routing sites use — text route iff text query AND no SPARSE clause. + let sparse = args_of(&["idx", "machine", "SPARSE", "@vec", "$q"]); + let plain = args_of(&["idx", "machine learning"]); + let route = |a: &[Frame]| -> bool { + a.get(1) + .and_then(extract_bulk) + .is_some_and(|q| is_text_query(&q)) + && !has_sparse_clause(a) + }; + // args[1]="machine" alone IS a text query, but the SPARSE clause forces deferral to ft_search. + assert!(is_text_query(b"machine")); + assert!( + !route(&sparse), + "standalone SPARSE must defer to the vector engine" + ); + assert!(route(&plain), "a plain text query takes the BM25 text path"); + } + // ── parse_text_query ─────────────────────────────────────────────────────── #[cfg(feature = "text-index")] @@ -2165,6 +2263,103 @@ mod tests { } } + // ── count semantics (fts-search-count-semantics) ─────────────────────────── + + fn shard_frame(key: &str, total: i64, score: f32) -> Frame { + let mut sb = String::with_capacity(16); + use std::fmt::Write; + let _ = write!(sb, "{score:.6}"); + Frame::Array( + vec![ + Frame::Integer(total), + Frame::BulkString(Bytes::copy_from_slice(key.as_bytes())), + Frame::Array( + vec![ + Frame::BulkString(Bytes::from_static(b"__bm25_score")), + Frame::BulkString(Bytes::from(sb)), + ] + .into(), + ), + ] + .into(), + ) + } + + fn results_n(n: usize) -> Vec { + (0..n) + .map(|i| TextSearchResult { + doc_id: i as u32, + key: Bytes::from(format!("d{i}")), + score: (n - i) as f32, + }) + .collect() + } + + #[test] + fn build_text_response_with_total_reports_supplied_total() { + // C2: reply[0] is the supplied total_matched, NOT the page length. + let results = results_n(3); + let resp = build_text_response_with_total(&results, 47, 0, 3); + let items = match &resp { + Frame::Array(a) => a, + _ => panic!("expected Array"), + }; + assert_eq!(items[0], Frame::Integer(47), "reply[0] = total_matched"); + assert_eq!(items.len(), 1 + 3 * 2, "3 doc entries"); + } + + #[test] + fn build_text_response_wrapper_reports_page_len() { + // C2: the 3-arg wrapper preserves old behavior (reply[0] = results.len()). + let results = results_n(3); + let resp = build_text_response(&results, 0, usize::MAX); + let items = match &resp { + Frame::Array(a) => a, + _ => panic!("expected Array"), + }; + assert_eq!(items[0], Frame::Integer(3)); + } + + #[test] + fn merge_text_results_sums_shard_totals() { + // C4: per-shard items[0] (true local matched) are SUMMED, even when each shard returns + // fewer docs than it matched (per-shard top_k truncation). Old code reported 3 (merged docs). + let frames = [ + shard_frame("a", 40, 3.0), + shard_frame("b", 35, 2.0), + shard_frame("c", 25, 1.0), + ]; + let merged = merge_text_results(&frames, 100, 0, usize::MAX); + let items = match &merged { + Frame::Array(a) => a, + _ => panic!("expected Array"), + }; + assert_eq!( + items[0], + Frame::Integer(100), + "total = 40+35+25, not the 3 merged docs" + ); + assert_eq!(items.len(), 1 + 3 * 2, "3 docs returned"); + } + + #[test] + fn merge_text_results_errored_shard_contributes_zero() { + // Reject shard_total_absent_zero: an errored shard frame adds 0 and the merge survives. + let good = shard_frame("a", 3, 2.0); + let bad = Frame::Error(Bytes::from_static(b"ERR boom")); + let merged = merge_text_results(&[good, bad], 100, 0, usize::MAX); + let items = match &merged { + Frame::Array(a) => a, + _ => panic!("expected Array"), + }; + assert_eq!( + items[0], + Frame::Integer(3), + "errored shard contributes 0 to the sum" + ); + assert_eq!(items.len(), 1 + 2, "the one good doc survives"); + } + // ── response_contains_bm25_score ─────────────────────────────────────────── #[test] diff --git a/src/command/vector_search/mod.rs b/src/command/vector_search/mod.rs index 52d5e4122..8e3f8cc5f 100644 --- a/src/command/vector_search/mod.rs +++ b/src/command/vector_search/mod.rs @@ -50,9 +50,9 @@ pub use ft_text_search::{ }; pub use ft_text_search::{ HighlightOpts, QueryTerm, SummarizeOpts, apply_post_processing, execute_text_search_local, - execute_text_search_with_global_idf, ft_text_search, highlight_field, is_text_query, - merge_text_results, parse_highlight_clause, parse_summarize_clause, parse_text_query, - summarize_field, + execute_text_search_with_global_idf, ft_text_search, has_sparse_clause, highlight_field, + is_text_query, merge_text_results, parse_highlight_clause, parse_summarize_clause, + parse_text_query, summarize_field, }; pub use helpers::{metric_to_bytes, quantization_to_bytes, quantize_f32_to_sq}; pub use hybrid::{HybridFilter, HybridQuery, HybridQueryPartial, parse_hybrid_modifier}; diff --git a/src/server/conn/handler_monoio/ft.rs b/src/server/conn/handler_monoio/ft.rs index 4d55ee7af..62614db3c 100644 --- a/src/server/conn/handler_monoio/ft.rs +++ b/src/server/conn/handler_monoio/ft.rs @@ -64,7 +64,8 @@ pub(super) async fn try_handle_ft_command( .and_then(|f| crate::command::vector_search::extract_bulk(f)); let is_text = query_bytes .as_ref() - .map_or(false, |q| crate::command::vector_search::is_text_query(q)); + .map_or(false, |q| crate::command::vector_search::is_text_query(q)) + && !crate::command::vector_search::has_sparse_clause(cmd_args); // ── HYBRID multi-shard path (Phase 152 Plan 05, D-13) ────────── #[cfg(feature = "text-index")] @@ -437,7 +438,9 @@ pub(super) async fn try_handle_ft_command( return true; } Ok(None) => { - if crate::command::vector_search::is_text_query(query_bytes.as_ref()) { + if crate::command::vector_search::is_text_query(query_bytes.as_ref()) + && !crate::command::vector_search::has_sparse_clause(cmd_args) + { // Step 1: index_name from cmd_args[0]. let index_name = match cmd_args.first() { Some(Frame::BulkString(b)) => b.clone(), diff --git a/src/server/conn/handler_sharded/ft.rs b/src/server/conn/handler_sharded/ft.rs index d0e9ca22f..29f8a2a4a 100644 --- a/src/server/conn/handler_sharded/ft.rs +++ b/src/server/conn/handler_sharded/ft.rs @@ -64,7 +64,8 @@ pub(super) async fn try_handle_ft_command( .and_then(|f| crate::command::vector_search::extract_bulk(f)); let is_text = query_bytes .as_ref() - .map_or(false, |q| crate::command::vector_search::is_text_query(q)); + .map_or(false, |q| crate::command::vector_search::is_text_query(q)) + && !crate::command::vector_search::has_sparse_clause(cmd_args); // -- HYBRID multi-shard path (Phase 152 Plan 05, D-13) -- // If the args contain a HYBRID clause, route through @@ -337,7 +338,9 @@ pub(super) async fn try_handle_ft_command( return true; } Ok(None) => { - if crate::command::vector_search::is_text_query(query_bytes.as_ref()) { + if crate::command::vector_search::is_text_query(query_bytes.as_ref()) + && !crate::command::vector_search::has_sparse_clause(cmd_args) + { // Step 1: index_name. let index_name = match cmd_args.first() { Some(Frame::BulkString(b)) => b.clone(), diff --git a/src/server/conn/handler_single.rs b/src/server/conn/handler_single.rs index a977d4ac7..ea06591fe 100644 --- a/src/server/conn/handler_single.rs +++ b/src/server/conn/handler_single.rs @@ -1335,6 +1335,8 @@ pub async fn handle_connection( Ok(None) => { if crate::command::vector_search::is_text_query( query_bytes.as_ref(), + ) && !crate::command::vector_search::has_sparse_clause( + cmd_args, ) { // Step 1: index_name. let index_name = match cmd_args.first() { diff --git a/src/shard/spsc_handler.rs b/src/shard/spsc_handler.rs index 051ed750c..d821b8d41 100644 --- a/src/shard/spsc_handler.rs +++ b/src/shard/spsc_handler.rs @@ -1841,7 +1841,9 @@ pub(crate) fn dispatch_vector_command( crate::protocol::Frame::BulkString(b) => Some(b.as_ref()), _ => None, }); - if query_bytes.map_or(false, vector_search::is_text_query) { + if query_bytes.map_or(false, vector_search::is_text_query) + && !vector_search::has_sparse_clause(args) + { return vector_search::ft_text_search(text_store, args); } // Existing vector search path (KNN / SPARSE / hybrid). diff --git a/src/text/mod.rs b/src/text/mod.rs index 47fb68fa6..2f8ffe877 100644 --- a/src/text/mod.rs +++ b/src/text/mod.rs @@ -242,6 +242,175 @@ mod tests { ); } + // ===== fts-upsert-incremental: reverse doc_id -> term_ids map ===== + + #[test] + fn test_reverse_map_records_unique_terms() { + // U1: doc 7 contributes terms 10, 20 (twice), 30 -> reverse set {10,20,30}, 20 once. + let mut store = PostingStore::new(); + store.add_term_occurrence(10, 7, None); + store.add_term_occurrence(20, 7, None); + store.add_term_occurrence(20, 7, None); // same (doc,term): tf++, NOT a new reverse edge + store.add_term_occurrence(30, 7, None); + let mut terms = store + .doc_terms_for(7) + .expect("doc 7 has a reverse entry") + .to_vec(); + terms.sort_unstable(); + assert_eq!( + terms, + vec![10, 20, 30], + "reverse map is the unique term set (20 recorded once)" + ); + } + + #[test] + fn test_remove_doc_consults_reverse_map() { + // U2: remove_doc returns exactly the doc's terms and clears its reverse entry. + use std::collections::HashSet; + let mut store = PostingStore::new(); + store.add_term_occurrence(10, 7, None); + store.add_term_occurrence(20, 7, None); + store.add_term_occurrence(10, 8, None); // another doc shares term 10 + let before: HashSet = store.doc_terms_for(7).unwrap().iter().copied().collect(); + let removed: HashSet = store.remove_doc(7).into_iter().map(|(t, _)| t).collect(); + assert_eq!( + removed, before, + "removed terms == the doc's reverse-map terms" + ); + assert!( + store.doc_terms_for(7).is_none(), + "reverse entry cleared after remove" + ); + assert_eq!(store.doc_freq(10), 1, "doc 8 still present in term 10"); + assert_eq!(store.doc_freq(20), 0, "term 20 had only doc 7"); + } + + #[test] + fn test_remove_absent_doc_is_noop() { + // Reject absent_doc_noop: removing a never-seen doc is a no-op, no panic. + let mut store = PostingStore::new(); + store.add_term_occurrence(10, 1, None); + let removed = store.remove_doc(999); + assert!(removed.is_empty(), "removing an unknown doc returns empty"); + assert_eq!(store.doc_freq(10), 1, "existing postings untouched"); + assert!(store.doc_terms_for(999).is_none()); + } + + #[test] + fn test_stale_reverse_entry_skipped() { + // Reject stale_reverse_entry_skip: a reverse term whose posting no longer holds the doc + // is skipped (no unwrap/expect panic); the doc's other terms still get removed. + let mut store = PostingStore::new(); + store.add_term_occurrence(10, 7, None); + store.add_term_occurrence(20, 7, None); + // Desync: forcibly clear doc 7 from term 10's posting, leaving the reverse map stale. + store.test_force_clear_doc_from_posting(10, 7); + let _ = store.remove_doc(7); // must not panic + assert!(store.doc_terms_for(7).is_none(), "reverse entry cleared"); + assert_eq!(store.doc_freq(20), 0, "non-stale term 20 still removed"); + } + + #[test] + fn test_reverse_map_reclaimed_on_repeated_upsert() { + // U4: upserting the same doc 100x keeps exactly one reverse entry of constant size. + let mut store = PostingStore::new(); + for _ in 0..100 { + store.remove_doc(7); + store.add_term_occurrence(10, 7, None); + store.add_term_occurrence(20, 7, None); + } + assert_eq!( + store.doc_terms_for(7).map(<[u32]>::len), + Some(2), + "reverse entry holds the 2 terms, not 100x" + ); + assert_eq!(store.doc_terms_count(), 1, "exactly one doc tracked"); + store.remove_doc(7); + assert!( + store.doc_terms_for(7).is_none(), + "reverse entry gone after delete" + ); + assert_eq!(store.doc_terms_count(), 0); + } + + #[test] + fn test_posting_state_identical_after_upsert_delete() { + // U3 (correctness boundary, posting layer): an index/upsert/delete sequence leaves posting + // state byte-identical to a fresh build of the same FINAL state. remove_doc only touches + // this layer, so identical doc_ids + term_freqs here ⇒ identical doc_freq/tf/search output. + // Incremental: doc1{10,20}, doc2{20,30}, doc3{10,30}; upsert doc2 -> {30,40}; delete doc3. + let mut inc = PostingStore::new(); + inc.add_term_occurrence(10, 1, None); + inc.add_term_occurrence(20, 1, None); + inc.add_term_occurrence(20, 2, None); + inc.add_term_occurrence(30, 2, None); + inc.add_term_occurrence(10, 3, None); + inc.add_term_occurrence(30, 3, None); + inc.remove_doc(2); // upsert doc2 + inc.add_term_occurrence(30, 2, None); + inc.add_term_occurrence(40, 2, None); + inc.remove_doc(3); // delete doc3 + // Fresh build of the final state: doc1{10,20}, doc2{30,40}. + let mut fresh = PostingStore::new(); + fresh.add_term_occurrence(10, 1, None); + fresh.add_term_occurrence(20, 1, None); + fresh.add_term_occurrence(30, 2, None); + fresh.add_term_occurrence(40, 2, None); + for term in [10u32, 20, 30, 40, 99] { + let a = inc.get_posting(term); + let b = fresh.get_posting(term); + match (a, b) { + (Some(pa), Some(pb)) => { + assert_eq!(pa.doc_ids, pb.doc_ids, "term {term}: doc_ids identical"); + assert_eq!( + pa.term_freqs, pb.term_freqs, + "term {term}: term_freqs identical" + ); + } + (None, None) => {} + _ => panic!("term {term}: posting presence differs (inc vs fresh)"), + } + assert_eq!( + inc.doc_freq(term), + fresh.doc_freq(term), + "term {term}: doc_freq identical" + ); + } + } + + #[test] + #[ignore = "perf/A1 proof — timing flakes in CI; run manually"] + fn test_upsert_scaling_flat() { + // U2/A1: per-doc removal cost is independent of vocabulary V. Old O(V) impl scales ~V. + use std::time::Instant; + fn build(vocab: u32) -> PostingStore { + let mut s = PostingStore::new(); + // The target doc (id 0) holds the same 10 terms in both stores. + for t in 0..10u32 { + s.add_term_occurrence(t, 0, None); + } + // Fill the rest of the vocabulary, each term in its own filler doc. + for t in 10..vocab { + s.add_term_occurrence(t, t, None); + } + s + } + let time_remove = |vocab: u32| { + let mut s = build(vocab); + let t = Instant::now(); + s.remove_doc(0); + t.elapsed() + }; + let small = time_remove(20).max(std::time::Duration::from_nanos(1)); + let large = time_remove(40_000); + let ratio = large.as_secs_f64() / small.as_secs_f64(); + assert!( + ratio < 20.0, + "remove_doc must be ~O(terms-in-doc): V=40000 took {ratio:.1}x V=20 (old O(V) ≈ 2000x)" + ); + } + #[test] fn test_posting_store_term_count() { let mut store = PostingStore::new(); diff --git a/src/text/posting.rs b/src/text/posting.rs index 34b07f3c7..d8a24893c 100644 --- a/src/text/posting.rs +++ b/src/text/posting.rs @@ -9,6 +9,7 @@ /// positions are not needed, but stores them from day one for future phrase /// queries and HIGHLIGHT support. use roaring::RoaringBitmap; +use smallvec::SmallVec; use std::collections::HashMap; /// A single term's posting data across all documents. @@ -88,6 +89,12 @@ impl PostingList { /// Per-field inverted index storing term_id -> PostingList. pub struct PostingStore { postings: HashMap, + /// Reverse index: doc_id -> the term_ids that document contributed (a set, no duplicates). + /// Lets `remove_doc` visit only a document's own terms instead of scanning every posting, + /// making per-doc removal O(terms-in-doc) instead of O(total vocabulary) — the upsert/bulk + /// re-index cliff (fts-upsert-incremental). Kept in sync with `postings`: `add_term_occurrence` + /// records the edge on the new-doc branch; `remove_doc` erases the doc's entry. + doc_terms: HashMap>, } impl PostingStore { @@ -95,6 +102,7 @@ impl PostingStore { pub fn new() -> Self { Self { postings: HashMap::new(), + doc_terms: HashMap::new(), } } @@ -148,6 +156,10 @@ impl PostingStore { } (None, None) => {} } + // Record the (doc -> term) reverse edge exactly once: this branch fires only the first + // time `doc_id` joins `term_id`'s posting, so no de-dup is needed. `posting`'s borrow of + // `self.postings` has ended (last use above), so this disjoint-field access is sound. + self.doc_terms.entry(doc_id).or_default().push(term_id); } } @@ -171,26 +183,73 @@ impl PostingStore { /// Clear all postings for a specific document (used during upsert). /// - /// Returns the old term frequencies for stats adjustment. + /// Returns the old term frequencies `(term_id, old_tf)` for stats adjustment (order + /// unspecified — callers only sum it). Visits ONLY the terms this document contributed via the + /// `doc_terms` reverse map — O(terms-in-doc), not O(total vocabulary) — eliminating the upsert / + /// bulk re-index cliff. The empty-posting entries are intentionally left in `postings` (a fully + /// removed term keeps an empty `PostingList`), matching the prior O(V) implementation so + /// `doc_freq`/`tf`/search output stay byte-identical. pub fn remove_doc(&mut self, doc_id: u32) -> Vec<(u32, u32)> { - let mut removed = Vec::new(); - for (&term_id, posting) in &mut self.postings { + // absent_doc_noop: a doc never indexed has no reverse entry -> nothing to remove. + let Some(term_ids) = self.doc_terms.remove(&doc_id) else { + return Vec::new(); + }; + let mut removed = Vec::with_capacity(term_ids.len()); + for term_id in term_ids { + // stale_reverse_entry_skip: defend against a reverse edge whose posting is gone or no + // longer holds the doc — skip, never unwrap/expect/panic. + let Some(posting) = self.postings.get_mut(&term_id) else { + continue; + }; + if !posting.doc_ids.contains(doc_id) { + continue; + } + // Rank-aligned index — compute BEFORE removing from the bitmap. + let idx = posting.rank_index(doc_id); + if idx < posting.term_freqs.len() { + let old_tf = posting.term_freqs.remove(idx); + posting.doc_ids.remove(doc_id); + if let Some(pos_list) = &mut posting.positions { + if idx < pos_list.len() { + pos_list.remove(idx); + } + } + removed.push((term_id, old_tf)); + } + } + removed + } + + /// Reverse-map term_ids a document contributed (rank-unordered). `#[cfg(test)]` accessor. + #[cfg(test)] + pub(crate) fn doc_terms_for(&self, doc_id: u32) -> Option<&[u32]> { + self.doc_terms.get(&doc_id).map(SmallVec::as_slice) + } + + /// Number of distinct documents tracked in the reverse map. `#[cfg(test)]` accessor. + #[cfg(test)] + pub(crate) fn doc_terms_count(&self) -> usize { + self.doc_terms.len() + } + + /// Test-only: forcibly clear `doc_id` from `term_id`'s posting WITHOUT touching the reverse map, + /// to synthesize the stale-reverse-entry state that `remove_doc` must tolerate. + #[cfg(test)] + pub(crate) fn test_force_clear_doc_from_posting(&mut self, term_id: u32, doc_id: u32) { + if let Some(posting) = self.postings.get_mut(&term_id) { if posting.doc_ids.contains(doc_id) { - // Rank-aligned index — compute BEFORE removing from the bitmap. let idx = posting.rank_index(doc_id); if idx < posting.term_freqs.len() { - let old_tf = posting.term_freqs.remove(idx); - posting.doc_ids.remove(doc_id); - if let Some(pos_list) = &mut posting.positions { - if idx < pos_list.len() { - pos_list.remove(idx); - } + posting.term_freqs.remove(idx); + } + posting.doc_ids.remove(doc_id); + if let Some(p) = &mut posting.positions { + if idx < p.len() { + p.remove(idx); } - removed.push((term_id, old_tf)); } } } - removed } /// Estimated memory usage in bytes. diff --git a/src/text/query/eval.rs b/src/text/query/eval.rs index e105570ef..cd2e7473d 100644 --- a/src/text/query/eval.rs +++ b/src/text/query/eval.rs @@ -104,6 +104,10 @@ pub fn eval_set(node: &QueryNode, idx: &TextIndex) -> RoaringBitmap { /// Membership is `eval_set` (authoritative); scoring sums TEXT-leaf BM25 per doc (filters score /// `0.0`). Order: score DESC, doc_id ASC (stable). `global_df`/`global_n` forward the DFS global /// IDF weights to the text leaves (multi-shard path, E5). Truncated to `top_k`. +/// +/// Thin wrapper over [`eval_query_counted`] — the `.0` projection, kept for the frozen +/// fts-query-eval-dispatch §3 contract (`eval_query(..) -> Vec`). Callers that also +/// need the FT.SEARCH total-matched count use [`eval_query_counted`] directly. pub fn eval_query( idx: &TextIndex, node: &QueryNode, @@ -111,10 +115,28 @@ pub fn eval_query( global_n: Option, top_k: usize, ) -> Vec { + eval_query_counted(idx, node, global_df, global_n, top_k).0 +} + +/// Like [`eval_query`] but ALSO returns the true total number of matched, key-resolvable documents +/// — the FT.SEARCH integer reply (`reply[0]`, RediSearch semantics), counted BEFORE the `top_k` +/// truncation (fts-search-count-semantics C1). `eval_set` is evaluated EXACTLY ONCE. +/// +/// The total is the length of the assembled (resolvable) `results` vector *before* truncation, NOT +/// `set.len()`: a `doc_id` present in the match set but absent from `doc_id_to_key` is unreturnable +/// and is already dropped by the assembly `filter_map`, so it is excluded from both the page and the +/// total (`unresolvable_doc_uncounted`). In a consistent index the two coincide. +pub fn eval_query_counted( + idx: &TextIndex, + node: &QueryNode, + global_df: Option<&HashMap>, + global_n: Option, + top_k: usize, +) -> (Vec, usize) { // 1. Authoritative membership (complete — no truncation). let set = eval_set(node, idx); if set.is_empty() { - return Vec::new(); + return (Vec::new(), 0); } // 2. Best-effort scores from TEXT leaves only. usize::MAX so every matched doc gets its true @@ -135,6 +157,9 @@ pub fn eval_query( }) .collect(); + // True total-matched: matched AND key-resolvable, captured BEFORE truncation (C1). + let total_matched = results.len(); + // 4. Order: score DESC, doc_id ASC (stable, deterministic tie-break). results.sort_by(|a, b| { b.score @@ -143,7 +168,7 @@ pub fn eval_query( .then(a.doc_id.cmp(&b.doc_id)) }); results.truncate(top_k); - results + (results, total_matched) } /// Walk the AST, accumulating each TEXT leaf's BM25 contribution per doc into `scores`. TAG / diff --git a/src/text/query/mod.rs b/src/text/query/mod.rs index e6f5a884e..13d459513 100644 --- a/src/text/query/mod.rs +++ b/src/text/query/mod.rs @@ -13,5 +13,7 @@ mod eval; mod parse; pub use ast::{QueryError, QueryNode}; -pub use eval::{collect_df_field_terms, collect_highlight_terms, eval_query, eval_set}; +pub use eval::{ + collect_df_field_terms, collect_highlight_terms, eval_query, eval_query_counted, eval_set, +}; pub use parse::{QuerySchema, parse_query}; diff --git a/src/text/store.rs b/src/text/store.rs index b255a83f8..a540a93eb 100644 --- a/src/text/store.rs +++ b/src/text/store.rs @@ -457,17 +457,22 @@ impl TextIndex { // Build candidate bitmap: start from first term's doc_ids, AND with rest. let mut candidate_bitmap: RoaringBitmap = { - // Safety: term_postings is non-empty (query_terms non-empty guard above) - let first_posting = self.field_postings[field_idx] - .get_posting(term_postings[0].1) - .expect("posting exists: checked above"); + // Defensive (no expect/panic): term_postings is non-empty and each posting was just + // verified present, but a missing posting here would mean the AND term has no docs ⇒ + // no results. Never panic on the BM25 hot path. + let Some(first_posting) = + self.field_postings[field_idx].get_posting(term_postings[0].1) + else { + return Vec::new(); + }; first_posting.doc_ids.clone() }; for (_, term_id) in &term_postings[1..] { - let posting = self.field_postings[field_idx] - .get_posting(*term_id) - .expect("posting exists: checked above"); + // Defensive: an absent AND-term posting ⇒ empty intersection ⇒ no results. + let Some(posting) = self.field_postings[field_idx].get_posting(*term_id) else { + return Vec::new(); + }; candidate_bitmap &= &posting.doc_ids; } @@ -495,9 +500,11 @@ impl TextIndex { let mut doc_score = 0.0f32; for (term, term_id) in &term_postings { - let posting = self.field_postings[field_idx] - .get_posting(*term_id) - .expect("posting exists: checked above"); + // Defensive: skip a term whose posting vanished rather than panic; its BM25 + // contribution is simply omitted (the doc already matched the AND candidate set). + let Some(posting) = self.field_postings[field_idx].get_posting(*term_id) else { + continue; + }; // Rank-aligned TF lookup (fts-posting-rank-tf): sub-linear and correct after // document updates. term_freqs is now kept in sorted-doc_id (rank) order, so the diff --git a/tests/fts_query_eval_e2e.rs b/tests/fts_query_eval_e2e.rs index 6accea3fc..5639e0367 100644 --- a/tests/fts_query_eval_e2e.rs +++ b/tests/fts_query_eval_e2e.rs @@ -22,7 +22,9 @@ use std::collections::BTreeSet; use bytes::Bytes; use moon::command::vector_search::run_text_query; use moon::protocol::Frame; -use moon::text::query::{QuerySchema, collect_df_field_terms, eval_query, parse_query}; +use moon::text::query::{ + QuerySchema, collect_df_field_terms, eval_query, eval_query_counted, eval_set, parse_query, +}; use moon::text::store::{TextIndex, TextStore}; use moon::text::types::{BM25Config, NumericFieldDef, TagFieldDef, TextFieldDef}; @@ -494,3 +496,79 @@ fn test_df_field_terms_pure_filter_vs_fuzzy() { "fuzzy terms use LOCAL df → no exact df terms collected" ); } + +// ───────────── count semantics (fts-search-count-semantics) ───────────── +// FT.SEARCH reply[0] = TRUE total-matched, independent of LIMIT/top_k (RediSearch semantics). +// RED until BUILD: `eval_query_counted` does not exist yet → this file fails to compile. + +/// An index where exactly `n` docs all contain the term "alpha". +fn alpha_corpus(n: u64) -> TextStore { + let mut idx = empty_index(); + for i in 0..n { + let key = format!("d{i}"); + add_doc(&mut idx, i + 1, &key, &[("body", "alpha")], &[], &[]); + } + idx.build_fst(); + store_of(idx) +} + +#[test] +fn test_eval_query_counted_total_is_pre_truncation() { + // C1: 12 docs match "alpha"; top_k=5 truncates the returned page but NOT the total. + let mut idx = empty_index(); + for i in 0..12u64 { + let key = format!("d{i}"); + add_doc(&mut idx, i + 1, &key, &[("body", "alpha")], &[], &[]); + } + idx.build_fst(); + + let schema = QuerySchema::from_index(&idx); + let node = parse_query(b"alpha", &schema).expect("parse ok"); + + let (results, matched) = eval_query_counted(&idx, &node, None, None, 5); + assert_eq!(results.len(), 5, "returned page is truncated to top_k"); + assert_eq!( + matched, 12, + "total is the full matched count, pre-truncation" + ); + + // `.0` must be byte-identical to the frozen 2b `eval_query` wrapper. + let plain = eval_query(&idx, &node, None, None, 5); + assert_eq!( + plain.iter().map(|r| r.key.clone()).collect::>(), + results.iter().map(|r| r.key.clone()).collect::>(), + "eval_query == eval_query_counted(..).0" + ); + + // Reject `unresolvable_doc_uncounted` (structural): every matched doc here resolves to a key, + // so the resolvable total equals the raw eval_set cardinality. The total is derived from the + // resolvable `results` vector, never `set.len()`, so an unreturnable doc can never inflate it. + let set = eval_set(&node, &idx); + assert_eq!( + matched as u64, + set.len(), + "fully-resolvable corpus: total == |eval_set|" + ); +} + +#[test] +fn test_run_text_query_limit_reports_true_total() { + // C3: LIMIT 0 5 over a 12-match corpus → reply[0]==12, exactly 5 docs returned. + let ts = alpha_corpus(12); + let r = run_text_query(&ts, b"idx", b"alpha", 5, 0, 5); + assert_eq!(total(&r), 12, "reply[0] = true matched, not the page size"); + assert_eq!(keys_ordered(&r).len(), 5, "page = LIMIT count"); +} + +#[test] +fn test_run_text_query_no_limit_total_unchanged() { + // C5: no LIMIT (unbounded top_k) → reply[0]==12==returned count (strict no-regression). + let ts = alpha_corpus(12); + let r = run_text_query(&ts, b"idx", b"alpha", usize::MAX / 2, 0, usize::MAX); + assert_eq!(total(&r), 12); + assert_eq!( + keys_ordered(&r).len(), + 12, + "all matches returned when unbounded" + ); +}