diff --git a/.add/milestones/v3-1-fts-hardening/MILESTONE.md b/.add/milestones/v3-1-fts-hardening/MILESTONE.md new file mode 100644 index 000000000..17669fc6d --- /dev/null +++ b/.add/milestones/v3-1-fts-hardening/MILESTONE.md @@ -0,0 +1,78 @@ +# MILESTONE: FTS Hardening + +goal: Moon's full-text search is trustworthy and competitive: every query combinator (term, AND, OR, TEXT+TAG, NUMERIC) returns correct results with true total-matched counts, and indexing plus high-DF queries run without the O(V)/O(M^2) cliffs the 2026-06-16 benchmark exposed. +rationale: new-major (split 1/3) — headline slice of the v3 "secondary-engine correctness & parity" theme opened from the 2026-06-16 deep review + 4-feature GCloud benchmark (FTS was the benchmark's biggest gap vs RediSearch and was previously un-benchmarked). No closed milestone (v1 shared-nothing, v2/v2-1 throughput) covers FTS correctness/parity. This slice owns FTS's O(M^2)/O(V) perf cliffs + the OR/combo/count correctness defects + a latent query-routing panic. Graph (v3-2) and Vector/KV (v3-3) are sibling slices. +stage: production · status: active · created: 2026-06-16 + +> SDD living doc for this milestone. Keep it THIN: breadth, shared decisions, and +> exit criteria only — per-task detail lives in each `.add/tasks//TASK.md`, +> written just-in-time. Update this doc whenever a task reveals a milestone gap. + +## Scope +In: +- High-DF term query no longer O(M^2): replace the O(N) `.position(|id| id == doc_id)` TF lookup + with a rank-based lookup (e.g. `RoaringBitmap::rank`). `src/text/store.rs:507,773`. + (bench: term_hi 419 ms / 19 qps vs RediSearch 2 ms / 3,813 qps.) +- Bulk indexing no longer O(V) per doc: replace the per-doc posting upsert scan with an + incremental update. `src/text/posting.rs:139`. (bench: 376 vs 18,052 docs/s, ~48×.) +- `OR` (`|`) returns the union of matched docs; `TEXT+TAG` combined query returns the intersection + (non-empty when matches exist). (bench: OR total 10 vs 2,072; combo 0 vs 253.) +- FT.SEARCH reply reports the true total-matched count, not the returned-page count. + (bench: TAG 10 vs 5,064 — an FT.SEARCH protocol deviation.) +- Query routing + robustness: `is_text_query()` recognizes `SPARSE` (no misroute of sparse-vector + queries to the BM25 path) and the FT query path removes its 3× `expect()` panics. + `src/command/vector_search/ft_text_search.rs:1004`, `src/text/store.rs:463`. + +Out: +- BM25 ranking-quality tuning / new analyzers — this is correctness of result SETS + counts + speed, + not relevance-score changes. +- Aggregation/GROUPBY speed (bench 6 vs 11 qps) and NUMERIC-range query speed (~3× slower) — noted, + deferred perf, not correctness defects. +- Vector-search QPS/recall (v3-3 / a later effort) and graph (v3-2). + +## Shared decisions & glossary deltas (living — every task must honor these) +- FT.SEARCH count semantics = RediSearch's: the integer reply is the TOTAL matched; paging is + separate. Every query task honors this once `fts-search-count-semantics` freezes it. +- TDD red/green: each correctness defect lands a FAILING test first (wrong union / zero combo / + wrong count / SPARSE misroute), then the fix (CLAUDE.md Rule 3). +- New/changed parser/eval paths get a fuzz target (CLAUDE.md Fuzzing) and NEVER panic on malformed + input — return `Frame::Error`, no `expect`/`unwrap` on the query path. +- No new hot-path allocations on the dispatch/query path; no new `unsafe` without approved SAFETY. + +## Shared / risky contracts (freeze these first) +- FT.SEARCH total-count semantics (matched vs returned) — a wire-visible reply contract every query + path shares and clients depend on. Freeze first. -> owning task `fts-search-count-semantics` +- Posting-list TF lookup API (rank-based) — the shape the high-DF fix and the count/eval paths both + call; wrong here re-does downstream query work. -> owning task `fts-posting-rank-tf` + +## Tasks (breadth-first decomposition; detail lives in each TASK.md) +- [x] fts-posting-rank-tf depends-on: none — replace O(N) TF `.position()` + with rank-based lookup; kills the high-DF O(M^2) cliff. DONE 2026-06-16 (gate PASS, commits + 45b3db8+a7e816d). Also fixed a latent BM25-misalignment bug; froze the rank-aligned PostingList + TF contract (term_freqs/positions sorted-doc_id-aligned, tf()/positions_for() via rank). +- [ ] fts-upsert-incremental depends-on: none — replace O(V) per-doc posting + upsert scan with incremental update; fast bulk indexing. +- [ ] fts-query-combinators depends-on: none — `OR` (`|`) unions and + `TEXT+TAG`/`TEXT+NUMERIC` combos intersect; correct matched sets. Code trace upgraded this from + two point-bugs to a missing PARSER layer; froze a query GRAMMAR+AST+eval_set contract @ v1 + (2026-06-16) and SPLIT the build into 2a+2b: + - [ ] fts-query-combinators (2a) depends-on: none — recursive-descent + `parse_query` → `QueryNode`/`QueryError` + grammar + parser fuzz target. Owns the frozen contract. + - [ ] fts-query-eval-dispatch (2b) depends-on: fts-query-combinators — `eval_set` (RoaringBitmap + union/intersect) + ft_text_search dispatch rewrite + wire reply. Inherits the frozen contract. +- [ ] fts-search-count-semantics depends-on: fts-query-eval-dispatch — FT.SEARCH reply = true + total-matched count = `eval_set(root).len()` (RediSearch semantics). Counts over 2b's matched set. +- [ ] fts-query-routing-robustness depends-on: none — `is_text_query()` recognizes + `SPARSE`; remove 3× `expect()` on the FT query path. + +## Exit criteria (observable; map each to the task that delivers it) +- [ ] A high-DF term (~5% of docs) returns without the O(M^2) cliff — a 100K-doc query-latency test + holds well under the old 419 ms (rank-based, not linear scan). (← fts-posting-rank-tf) +- [ ] Bulk indexing scales ~linearly (no O(V) per-doc scan) — an indexing-rate test shows the cliff + gone vs the 376 docs/s baseline. (← fts-upsert-incremental) +- [ ] `OR` returns |A ∪ B| matched docs and `TEXT+TAG` returns the non-empty intersection — + correctness tests over a known corpus. (← fts-query-combinators) +- [ ] FT.SEARCH's integer reply equals the true matched count (not the page size) — test asserts the + total over a corpus larger than the returned page. (← fts-search-count-semantics) +- [ ] A `SPARSE @field …` query routes to the vector path (not BM25), and malformed FT input returns + an error frame (no panic) — routing + fuzz/negative tests. (← fts-query-routing-robustness) diff --git a/.add/milestones/v3-2-graph-correctness/MILESTONE.md b/.add/milestones/v3-2-graph-correctness/MILESTONE.md new file mode 100644 index 000000000..8026cb7d9 --- /dev/null +++ b/.add/milestones/v3-2-graph-correctness/MILESTONE.md @@ -0,0 +1,57 @@ +# MILESTONE: Graph Correctness & Cypher Filtering + +goal: Moon's graph queries are correct: Cypher MATCH narrows on inline node-property predicates instead of full-scanning the label, directional traversal covers incoming/Both edges post-compaction, and node labels >= 32 are no longer silently dropped. +rationale: new-major (split 2/3) — part of the v3 "secondary-engine correctness & parity" theme from the 2026-06-16 deep review + 4-feature benchmark. No closed milestone (v1 shared-nothing, v2/v2-1 throughput) covers graph-query correctness. This slice owns the three graph defects the review/benchmark surfaced; FTS (v3-1) and Vector/KV (v3-3) are sibling slices. Correctness only — Moon's native graph build + 1-hop already beat FalkorDB (bench §11.4). +stage: production · status: planned · created: 2026-06-16 + +> SDD living doc for this milestone. Keep it THIN: breadth, shared decisions, and +> exit criteria only — per-task detail lives in each `.add/tasks//TASK.md`, +> written just-in-time. Update this doc whenever a task reveals a milestone gap. + +## Scope +In: +- Cypher MATCH narrows on an inline node-property predicate (e.g. `MATCH (a {id:N})`) instead of + full-scanning the label. (bench: `cypher_match_rows` = 14,991 ≈ |E| vs FalkorDB's filtered 4; + Moon Cypher point-query 40 qps.) +- `Direction::Incoming` and `Direction::Both` traversals return incoming edges after compaction — + CSR currently stores only outgoing. `src/graph/traversal.rs:188–191`. +- Node label storage supports labels with id >= 32 (the current 32-bit bitmap truncates). + `src/graph/csr/mod.rs:161` (`if label < 32`). + +Out: +- Full openCypher coverage / general predicate pushdown beyond inline node-property equality — only + the point-filter the benchmark exercised is in scope. +- Graph throughput — native GRAPH.ADDNODE/ADDEDGE build + native 1-hop already lead FalkorDB + (bench §11.4); this milestone is correctness, not speed. +- Cross-shard graph — single-keyspace per CLAUDE.md, unchanged here. + +## Shared decisions & glossary deltas (living — every task must honor these) +- TDD red/green: each defect lands a FAILING test first (wrong rows / missing incoming edge / + dropped label), then the fix (CLAUDE.md Rule 3). +- Malformed Cypher must never panic — return an error frame (parser/eval defensiveness). +- New/changed graph commands keep their `scripts/test-consistency.sh` + `scripts/test-commands.sh` + entries (CLAUDE.md New Commands). +- No new `unsafe` without explicit user approval + a `// SAFETY:` comment. + +## Shared / risky contracts (freeze these first) +- Cypher inline-predicate evaluation semantics — how `{prop:val}` narrows the candidate set (index + probe vs filtered scan) and what it returns. Wrong shape re-does every downstream Cypher query. + -> owning task `graph-cypher-inline-filter` +- Incoming-edge representation — reverse adjacency vs on-demand scan; a layout choice the label and + traversal work both read. -> owning task `graph-incoming-edges` + +## Tasks (breadth-first decomposition; detail lives in each TASK.md) +- [ ] graph-cypher-inline-filter depends-on: none — Cypher MATCH narrows on inline node-property + predicate instead of full-scanning the label; point-query returns only the matching node's edges. +- [ ] graph-incoming-edges depends-on: none — CSR traversal returns incoming / Both-direction + edges post-compaction (reverse adjacency or incoming index). +- [ ] graph-label-bitmap-overflow depends-on: none — node label storage supports id >= 32 without + silent truncation. + +## Exit criteria (observable; map each to the task that delivers it) +- [ ] A Cypher point-query `MATCH (a {id:N})-[]->(b)` returns only node N's edges (returned rows == + expected, not ≈|E|) — test asserts the narrowed row count. (← graph-cypher-inline-filter) +- [ ] On a compacted graph, `Direction::Incoming` and `Both` return the incoming edges (non-empty + where they should be) — test on a post-compaction graph. (← graph-incoming-edges) +- [ ] A node assigned label id >= 32 (e.g. a 40-label graph) is matched by its label query — no + silent drop; test with >= 33 distinct labels. (← graph-label-bitmap-overflow) diff --git a/.add/milestones/v3-3-vector-kv-polish/MILESTONE.md b/.add/milestones/v3-3-vector-kv-polish/MILESTONE.md new file mode 100644 index 000000000..a3ef0538a --- /dev/null +++ b/.add/milestones/v3-3-vector-kv-polish/MILESTONE.md @@ -0,0 +1,65 @@ +# MILESTONE: Vector & KV Latent-Correctness + Hot-Path Polish + +goal: Moon's vector segments decode at the correct code length (SQ8 included) and FT.INFO reports true cross-segment doc counts, and the KV/vector command hot paths honor the no-alloc and parking_lot lock-discipline rules. +rationale: new-major (split 3/3) — the v3 "secondary-engine correctness & parity" theme's latent-correctness + rule-compliance slice from the 2026-06-16 deep review. Bundles the two latent vector-correctness traps (none block the current call paths, hence they survived to review) with the small hot-path-alloc / lock-discipline list. FTS (v3-1) and Graph (v3-2) are sibling slices. +stage: production · status: planned · created: 2026-06-16 + +> SDD living doc for this milestone. Keep it THIN: breadth, shared decisions, and +> exit criteria only — per-task detail lives in each `.add/tasks//TASK.md`, +> written just-in-time. Update this doc whenever a task reveals a milestone gap. + +## Scope +In: +- SQ8 segment `code_len = bytes_per_code - 4` is wrong for SQ8's 8-byte trailer — corrected decode + across the segment lifecycle (search / merge / persistence). `src/vector/segment/compaction.rs:554` + + `immutable.rs` / `mutable.rs`. (Latent P0; same family as the v0.3.0-deferred code_len note.) +- FT.INFO `num_docs` sums mutable + immutable segments, not just the mutable one. + `src/command/vector_search/ft_info.rs:42`. +- FT.SEARCH avoids the ~3.2 MB `key_hash_to_key` clone per query (borrow / `Arc` the map). +- KV `INCR`/`DECR` write the integer via `itoa` to a buffer — no per-op `String` alloc on the hot + path. `src/command/string/string_write.rs:312,317`. +- The command-dispatch path uses `parking_lot::RwLock`, not `std::sync::RwLock`, for the ACL table. + `src/shard/event_loop.rs:65`, `src/command/connection.rs:374,460`. + +Out: +- Re-quantization / new vector codecs — only the existing SQ8 decode length is in scope, not new + formats. +- Vector-search QPS/recall competitiveness vs RediSearch (bench §10.5) — a larger HNSW/quant effort, + deferred. +- Broader allocation/lock audit beyond these named sites. + +## Shared decisions & glossary deltas (living — every task must honor these) +- On-disk segment compatibility: the `code_len` fix MUST either decode existing SQ8 segments + correctly or bump the segment format version with a documented migration — never silently mis-read + persisted data (CLAUDE.md persistence/reload care; cf. the CWD-reload trap). +- No new hot-path allocations in command / protocol / event_loop / io (CLAUDE.md Allocations): + `itoa` / `SmallVec` / borrow only. +- `parking_lot` locks only; never hold a lock across `.await` (CLAUDE.md Lock Handling). +- TDD red/green per fix; no new `unsafe` without approved SAFETY. + +## Shared / risky contracts (freeze these first) +- SQ8 on-disk `code_len` / segment-trailer layout — the byte format read at decode. Wrong or + un-versioned here corrupts persisted indexes. Freeze the decode contract (and migration stance) + before touching the read path. -> owning task `vector-sq8-code-len` + +## Tasks (breadth-first decomposition; detail lives in each TASK.md) +- [ ] vector-sq8-code-len depends-on: none — fix `code_len` for SQ8's 8-byte trailer; + correct decode across search / merge / persistence (latent P0). +- [ ] vector-ftinfo-num-docs depends-on: none — FT.INFO `num_docs` sums all segments + (mutable + immutable), not just mutable. +- [ ] vector-search-keyhash-noclone depends-on: none — drop the ~3.2 MB `key_hash_to_key` clone per + FT.SEARCH (borrow / `Arc`). +- [ ] kv-incr-itoa depends-on: none — `INCR`/`DECR` via `itoa`-to-buffer; no + `String` alloc on the hot path. +- [ ] kv-dispatch-lock-discipline depends-on: none — ACL / dispatch `std::sync::RwLock` -> + `parking_lot::RwLock`. + +## Exit criteria (observable; map each to the task that delivers it) +- [ ] An SQ8 immutable segment decodes vectors at the correct length — recall parity with the + pre-compaction mutable segment (within quant tolerance) across search / merge / reload; test. (← vector-sq8-code-len) +- [ ] FT.INFO `num_docs` == total docs across mutable + immutable after a compaction; test. (← vector-ftinfo-num-docs) +- [ ] FT.SEARCH performs no per-query 3 MB `key_hash` clone — allocation probe / throughput test + shows the clone gone. (← vector-search-keyhash-noclone) +- [ ] `INCR`/`DECR` allocate no `String` on the hot path — `itoa` path asserted (test / no-alloc check). (← kv-incr-itoa) +- [ ] No `std::sync::RwLock` remains on the command-dispatch path — ACL table is `parking_lot`; + audit/grep + test. (← kv-dispatch-lock-discipline) diff --git a/.add/state.json b/.add/state.json index 4f7b5cd87..27defa795 100644 --- a/.add/state.json +++ b/.add/state.json @@ -1,8 +1,8 @@ { "project": "moon", "stage": "production", - "active_task": "ft-yield-costfree-monoio", - "active_milestone": "v2-1-throughput-polish", + "active_task": "fts-query-combinators", + "active_milestone": "v3-1-fts-hardening", "tasks": { "hotpath-lock-quickwins": { "title": "Eliminate per-command global locks & syscall-level quick wins", @@ -88,6 +88,39 @@ "created": "2026-06-15T11:57:05+00:00", "updated": "2026-06-15T14:39:07+00:00", "flag_verified": true + }, + "fts-posting-rank-tf": { + "title": "Rank-based posting TF lookup (kill the high-DF O(M^2) cliff)", + "phase": "done", + "gate": "PASS", + "milestone": "v3-1-fts-hardening", + "depends_on": [ + "none" + ], + "created": "2026-06-16T04:51:36+00:00", + "updated": "2026-06-16T05:40:44+00:00", + "flag_verified": true + }, + "fts-query-combinators": { + "title": "OR unions + TEXT+TAG intersects \u2014 correct combinator result sets", + "phase": "done", + "gate": "PASS", + "milestone": "v3-1-fts-hardening", + "depends_on": [], + "created": "2026-06-16T05:44:57+00:00", + "updated": "2026-06-16T09:07:25+00:00", + "flag_verified": true + }, + "fts-query-eval-dispatch": { + "title": "Evaluate the query AST to matched sets + wire FT.SEARCH dispatch (2b)", + "phase": "done", + "gate": "PASS", + "milestone": "v3-1-fts-hardening", + "depends_on": [ + "fts-query-combinators" + ], + "created": "2026-06-16T06:21:47+00:00", + "updated": "2026-06-16T09:07:25+00:00" } }, "milestones": { @@ -114,10 +147,34 @@ "status": "done", "created": "2026-06-15T11:57:00+00:00", "updated": "2026-06-15T14:49:42+00:00" + }, + "v3-1-fts-hardening": { + "title": "FTS Hardening", + "goal": "Moon's full-text search is trustworthy and competitive: every query combinator (term, AND, OR, TEXT+TAG, NUMERIC) returns correct results with true total-matched counts, and indexing plus high-DF queries run without the O(V)/O(M^2) cliffs the 2026-06-16 benchmark exposed.", + "stage": "production", + "status": "active", + "created": "2026-06-16T04:42:21+00:00", + "updated": "2026-06-16T04:42:21+00:00" + }, + "v3-2-graph-correctness": { + "title": "Graph Correctness & Cypher Filtering", + "goal": "Moon's graph queries are correct: Cypher MATCH narrows on inline node-property predicates instead of full-scanning the label, directional traversal covers incoming/Both edges post-compaction, and node labels >= 32 are no longer silently dropped.", + "stage": "production", + "status": "planned", + "created": "2026-06-16T04:42:42+00:00", + "updated": "2026-06-16T04:42:42+00:00" + }, + "v3-3-vector-kv-polish": { + "title": "Vector & KV Latent-Correctness + Hot-Path Polish", + "goal": "Moon's vector segments decode at the correct code length (SQ8 included) and FT.INFO reports true cross-segment doc counts, and the KV/vector command hot paths honor the no-alloc and parking_lot lock-discipline rules.", + "stage": "production", + "status": "planned", + "created": "2026-06-16T04:42:42+00:00", + "updated": "2026-06-16T04:42:42+00:00" } }, "created": "2026-06-11T03:18:21+00:00", - "updated": "2026-06-15T14:49:42+00:00", + "updated": "2026-06-16T09:07:25+00:00", "setup": { "locked": true, "locked_at": "2026-06-11T03:28:00+00:00", diff --git a/.add/tasks/fts-posting-rank-tf/TASK.md b/.add/tasks/fts-posting-rank-tf/TASK.md new file mode 100644 index 000000000..580cf5d75 --- /dev/null +++ b/.add/tasks/fts-posting-rank-tf/TASK.md @@ -0,0 +1,318 @@ +# TASK: Rank-based posting TF lookup (kill the high-DF O(M^2) cliff) + +slug: fts-posting-rank-tf · created: 2026-06-16 · stage: production · risk: high · autonomy: conservative +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: Rank-aligned posting-list TF lookup — correct AND sub-linear term-frequency reads for BM25. +Framings weighed: rank-aligned parallel arrays (term_freqs/positions kept in sorted-doc_id order; lookup via `RoaringBitmap::rank` → O(log N)) (chosen) · per-posting `HashMap` tf (O(1), alignment-free, +mem) · sorted `Vec<(doc_id,tf)>` + binary_search (replaces the parallel arrays) +Must: + + - M1 — TF lookup for `(term_id, doc_id)` returns the document's TRUE term frequency, including + AFTER a document is updated / re-indexed (HSET on an existing key). [fixes the latent + insertion-order-vs-sorted-bitmap misalignment — see A1] + - M2 — TF lookup is sub-linear in posting-list length: a high-DF term query over M candidate docs + costs O(M·log N), not O(M·N) (no per-candidate `.position()` linear scan). [kills the O(M²) cliff] + - M3 — both BM25 read sites use the lookup: `search_field` (store.rs:504) and the expanded-term + `search_field_or` path (store.rs:770). + - M4 — no regression on the already-correct path: for a corpus inserted in ascending doc_id order + with no updates, BM25 scores AND result ordering are byte-identical to today. + - M5 — `term_freqs` and the parallel `positions` (when `Some`) stay consistent with `doc_ids` + under add / upsert / remove — the alignment invariant is explicit and maintained in ONE place + (the `add_term_occurrence` / `remove_doc` data structure), not re-derived per read site. + +Reject: + + - doc_id absent from the term's posting list -> tf = 0 (defined default; BM25 treats the term as + not occurring in that doc — current `.unwrap_or(0.0)`). -> "tf_absent" (= 0, not an error) + - (No client-facing error codes: this is an internal index path; malformed query/input is rejected + upstream. The sole "rejection" is the absent-doc default above.) + +After: + + - A high-DF term query (term in ~5% of 100K docs) no longer exhibits the O(M²) blow-up — latency + drops from the measured ~419 ms toward RediSearch-class. + - BM25 scores are correct even after document updates (the re-index misalignment bug is gone). + - The PostingList TF representation is the FROZEN contract `fts-upsert-incremental` builds on. + +Assumptions — lowest-confidence first: + + ⚠ A1 — This task is NOT perf-only. The same path carries a latent CORRECTNESS bug: `index_document` + re-indexes an updated doc with its SAME doc_id (store.rs:343,355) via `remove_doc` + re-add, and + the re-add does `doc_ids.insert()` (sorted position in the bitmap) but `term_freqs.push()` (END of + the vec, posting.rs:96-97). So re-indexing a doc whose id is NOT the max in a shared posting list + misaligns `term_freqs` against `doc_ids.iter()` (sorted) → wrong TF → corrupted BM25 for that whole + term. Lowest confidence because the milestone scoped this as a perf fix, but a rank-based lookup is + only correct if `term_freqs` is sorted-aligned — so the fix NECESSARILY corrects the bug. If wrong + (ship "perf-only", keep push-to-end): `rank()` would index the wrong TF and corrupt scores worse + than today, silently. → I assume scope INCLUDES the correctness fix. + - [ ] A2 — Representation = rank-aligned parallel arrays (keep `doc_ids` RoaringBitmap + `term_freqs` + /`positions` sorted-aligned; lookup via `rank`) over a per-posting `HashMap`. It's the + freeze-first contract `fts-upsert-incremental` inherits: arrays hold current memory + O(log N) + lookup but make upsert insert O(N) (shift at rank); a HashMap is O(1) lookup+insert but adds a map + per term. If wrong: the upsert task re-opens this contract. + - [ ] A3 — `doc_ids` MUST remain a RoaringBitmap (query-eval AND/OR candidate set-ops depend on it); + only the TF side changes. (high confidence) + - [ ] A4 — `positions[i]` moves in lockstep with `term_freqs[i]` under the new alignment so phrase / + HIGHLIGHT data stays correct. (high confidence) + - [ ] A5 — for the already-correct ascending-insert path, `rank(doc_id) == old position`, so results + are unchanged there. (high confidence) + + + + +--- + +## 2 · SCENARIOS — pass/fail cases ▸ docs/04-step-2-scenarios.md + + + +```gherkin +# ── M1 / A1 — the correctness fix (headline) ───────────────────────────── +Scenario: TF stays correct after a low-id document is updated + Given term "alpha" is indexed into doc 3 (tf=1), then doc 5 (tf=1), then doc 8 (tf=1) + When doc 3 is updated (re-HSET) so "alpha" now occurs 4 times in doc 3 + Then FT.SEARCH "alpha" scores doc 3 with TF=4 and docs 5 and 8 with TF=1 each + And the term_freqs of docs 5 and 8 are NOT scrambled by doc 3's re-insertion # no cross-doc corruption + +# ── M2 — sub-linear lookup, no O(M^2) cliff ────────────────────────────── +Scenario: High-DF term query does not scan the whole posting per candidate + Given a 100K-doc index where term "common" occurs in ~5000 docs + When FT.SEARCH "common" runs under concurrency + Then per-query latency is sub-linear in posting length — far under the ~419 ms O(M*N) baseline + And the BM25 results are identical to the linear-scan reference # speed gained, answers unchanged + +# ── M3 — both read sites use the lookup ────────────────────────────────── +Scenario: Expanded-term (OR / fuzzy) path uses the same correct TF lookup + Given the updated-doc-3 setup above, queried via the expanded-term path (search_field_or) + When that query runs + Then doc 3's TF is read as 4 — identical to the direct search_field path + And no read site keeps an inline .iter().position() TF scan + +# ── M4 — no regression on the already-correct path ─────────────────────── +Scenario: Ascending-insert corpus produces identical BM25 to before + Given a corpus inserted purely in ascending doc_id order with no document updates + When FT.SEARCH runs for any term + Then BM25 scores and result ordering are byte-identical to the pre-change implementation + +# ── M5 — positions stay aligned with TF ────────────────────────────────── +Scenario: Position data stays aligned with TF after an update + Given a position-tracked index where low-id doc 3 is updated (as above) + When the positions for doc 3 are read + Then they belong to doc 3 (consistent with its tf=4), not a neighbouring doc + And positions[i] still corresponds to the i-th doc_id in sorted order + +# ── Reject — tf_absent (defined default, not an error) ─────────────────── +Scenario: A term absent from a document yields TF 0 without panic + Given term "rare" does NOT occur in doc 7 (doc 7 not in "rare"'s posting) + When BM25 scores doc 7 for "rare" + Then the TF contribution is 0 # unwrap_or(0.0) default + And the lookup does not panic and does not return a neighbouring doc's tf +``` + + + + + +--- + +## 3 · CONTRACT — freeze the shape ▸ docs/05-step-3-contract.md + +Internal data-structure contract (not a wire endpoint). The frozen shape is the PostingList TF +representation + its lookup API in `src/text/posting.rs`, consumed by the BM25 read sites in +`src/text/store.rs`. Wire protocol (FT.SEARCH request/reply) is UNCHANGED. + +``` +INVARIANT (frozen) — rank-alignment of PostingList: + term_freqs[i] and (when positions = Some) positions[i] correspond to the i-th doc_id of + `doc_ids` in ASCENDING (sorted / rank) order. For a present doc_id d: + idx(d) = doc_ids.rank(d) as usize - 1 // rank(d) = count of ids <= d (roaring 0.11) + term_freqs is RANK-aligned, NOT insertion-aligned. Maintained in exactly one place + (PostingStore::add_term_occurrence / remove_doc) — never re-derived at a read site. + +READ API (impl PostingList, src/text/posting.rs): + fn tf(&self, doc_id: u32) -> u32 + doc_ids.contains(doc_id) -> term_freqs[idx(doc_id)] // sub-linear (rank), no O(N) scan + else -> 0 // "tf_absent" default — never panics + fn positions_for(&self, doc_id: u32) -> Option<&[u32]> + Some(&positions[idx]) when tracked AND present, else None + +WRITES preserve the invariant (impl PostingStore): + add_term_occurrence(term_id, doc_id, positions): + existing doc -> i = idx(doc_id); term_freqs[i] += 1; positions[i].extend(..) + NEW doc -> doc_ids.insert(doc_id); i = idx(doc_id); + term_freqs.insert(i, 1); positions.insert(i, ..) // INSERT-at-rank, NOT push + remove_doc(doc_id) -> i = idx(doc_id); term_freqs.remove(i); positions.remove(i); doc_ids.remove + +READ SITES updated (src/text/store.rs): + search_field (~:504) and search_field_or (~:770): the inline + doc_ids.iter().position(|id| id==doc_id).map(|i| term_freqs[i]).unwrap_or(0.0) + becomes posting.tf(doc_id) as f32. (contains() short-circuit at ~:765 may stay — O(1).) + +OUT OF CONTRACT: + - doc_ids stays a RoaringBitmap; query-eval AND/OR candidate set-ops unchanged. + - No on-disk / persistence format change — PostingList is the in-memory mutable-segment struct. + - BM25 formula, k1/b, analyzer pipeline: untouched. +``` + +Status: FROZEN @ v1 — approved by Tin Dang (2026-06-16). Lowest-confidence flags surfaced + accepted at freeze. +Least-sure flag surfaced at freeze: [spec] fixing the misalignment CHANGES BM25 output for previously-corrupted post-update cases — that is the bug being corrected, NOT a regression, so M4's no-regression golden must come from an ascending-insert corpus (the path already correct); if wrong, a snapshot of the OLD buggy scores would mis-fail. [contract] rank-aligned arrays make `add_term_occurrence` O(rank) on insert (shift `term_freqs`/`positions`) — the write cost `fts-upsert-incremental` inherits; if high-churn indexing regresses, that task re-tunes the structure it inherits here. + + + + +--- + +## 4 · TESTS — failing-first suite (red) ▸ docs/06-step-4-tests.md + +Coverage target: all 6 scenarios; new `posting.rs` `tf`/`positions_for`/insert-at-rank lines covered. +Plan (one test per scenario, asserting behavior not internals): + + - test_tf_correct_after_low_id_update (M1/A1): add_term_occurrence for "alpha" in order doc3,doc5,doc8; + then remove_doc(3) + re-add doc3 ×4; assert tf(3)==4 AND tf(5)==1 AND tf(8)==1. + [RED today: push-to-end makes tf(3) read a neighbour → assertion fails before the fix] + - test_high_df_query_sublinear (M2): ~100K docs, term in ~5%; assert query latency far under the + linear baseline (best-of-K guard, like perf_v0112) AND results identical to a linear-scan reference. + - test_expanded_path_same_tf (M3): the doc3-update setup queried via search_field_or; assert its TF==4 + and equals the search_field path; assert no inline `.position()` TF scan remains at either read site. + - test_ascending_insert_unchanged (M4): ascending corpus, no updates; assert BM25 scores + ordering + equal a captured pre-change reference (golden from the CORRECT ascending path). + - test_positions_aligned_after_update (M5): position-tracked; after doc3 update assert positions_for(3) + belongs to doc3 (aligned with tf=4). + - test_tf_absent_is_zero (reject "tf_absent"): tf(absent_doc)==0, no panic, no neighbour's tf returned. + + +Tests live in: `tests/fts_posting_rank_tf.rs` · MUST run red (missing `tf()` / push-to-end bug) 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 — 7/7 new (`tests/fts_posting_rank_tf.rs`); full lib suite 3584/0; + `text::` 144/0; `text::store::tests` 15/0. Tokio+text-index parity `cargo check + --no-default-features --features runtime-tokio,jemalloc,text-index,graph` → clean + (change has zero runtime-specific code). `cargo fmt --check` clean; `cargo clippy + -- -D warnings` clean (no posting.rs/store.rs warnings). +- [x] coverage did not decrease — net +7 tests; they cover the previously-UNTESTED + update-misalignment path (distinct tfs after a low-id re-index), which no prior + test exercised. M4 keeps the already-correct ascending path green. +- [x] no test or contract was altered during build — §3 CONTRACT FROZEN @ v1 untouched; + red suite (tests phase) went green via `src/text/{posting,store}.rs` edits only. +- [x] concurrency / timing of the risky operation is safe — BM25 search + (`search_field`/`search_field_or`, store.rs:506,767 reading `posting.tf`) runs + SYNCHRONOUSLY on the owning shard's event loop under an exclusive `&mut TextStore` + borrow: the off-event-loop `FtSearchPlan::Yield` path (PR #179) is dense-KNN-only on + an owned `SearchSnapshot` and routes every TEXT shape to `Sync` (ft_search/dispatch.rs), + so it never reads postings. Writes (`add_term_occurrence` via HSET auto-index) run on + the SAME single thread under the SAME `&mut`. No concurrent reader/writer of + `PostingStore`; the change preserves the prior mutation discipline (read indexes the + same Vec, write mutates the same Vec — neither adds shared mutability). No residue. +- [x] no exposed secrets, injection openings, or unexpected dependencies — pure in-memory + index re-indexing; no I/O, no new crate, no on-disk format change (postings rebuilt + from HSET replay on reload). `tf()`/`positions_for()` are panic-free (`contains` guard + + `.get().unwrap_or(0)`), so malformed/absent doc_ids degrade to tf 0, never a crash. + No security finding. +- [x] layering & dependencies follow CONVENTIONS.md — change confined to `src/text/`: + `posting.rs` owns the structure, `store.rs` the read sites. New methods hang off the + existing public `PostingList`; `rank_index` is private. No new module, no cross-layer + dependency, no hot-path allocation introduced (rank/insert on existing Vecs). +- [x] a person reviewed and approved the change + + +### Deep checks — do not skim (fill the path that applies; the resolver judges which) +- [x] WIRING (code) — `tf()` referenced at store.rs:506 and store.rs:767 (the ONLY two BM25 + read sites; confirmed by serena pattern sweep of `src/text` — store.rs:891 `.position()` + is an unrelated TAG-separator byte scan, not a posting read). `rank_index()` (private) is + called by `tf`, `positions_for`, `add_term_occurrence` (×2 branches) and `remove_doc`. +- [x] DEAD-CODE (code) — the old linear-scan blocks were REMOVED (not left behind); the stale + "RESEARCH Pitfall 1" note was rewritten, not orphaned. clippy reports no dead code. + ⚠ ONE flag: `positions_for()` is currently exercised only by its M5 conformance test — + there is NO production reader of `positions` yet (positions are stored "from day one for + future phrase queries"; verified the only `.positions` accesses are the write path + + `estimated_bytes`). It is the frozen-contract forward accessor (the rank-aligned + counterpart to `tf()`) that `fts-upsert-incremental` inherits and phrase/HIGHLIGHT will + consume — intentional contract API, not accidental dead code. Surfaced, not buried. +- [ ] SEMANTIC (prose / non-code) — n/a (code change). + +### GATE RECORD +Outcome: PASS (human gate — risk: high · autonomy: conservative) +Lowest-confidence item surfaced + accepted at the gate: `positions_for()` is contract API +referenced only by its conformance test today (no production reader of positions until phrase +queries / fts-upsert-incremental land) — accepted as intentional frozen-contract forward API. +No security or concurrency residue; no test/contract weakened. +Reviewed by: Tin Dang · date: 2026-06-16 + + + +--- + +## 7 · OBSERVE — feed the next loop ▸ docs/09-the-loop.md + +Watch (reuse scenarios as monitors): BM25 score correctness after document re-index (M1 +scenario as a regression monitor) · high-DF term-query p99 latency vs the ~419 ms O(M²) +baseline (M2 scenario) · FT.SEARCH ranking stability under update-heavy workloads. +Spec delta for the next loop: a milestone-scoped "perf fix" hid a latent correctness bug — +the rank-based lookup is only sound if `term_freqs` is sorted-aligned, so perf and correctness +were inseparable. fts-upsert-incremental now inherits the FROZEN rank-aligned contract: its +known cost is O(rank) insert (shift `term_freqs`/`positions`); if high-churn indexing regresses, +that task re-tunes the representation (the A2 HashMap fallback) rather than re-opening this one. + +### 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`. + +- [SDD · open] A milestone scoped a task as "perf-only" but grounded code investigation in + Specify found a latent correctness bug on the same path (insertion-order term_freqs vs + sorted-bitmap read) — perf and correctness were inseparable. Lesson: re-derive task scope + from the code during Specify, don't inherit the milestone's framing verbatim. (evidence: + A1 flag; the misalignment was real and is fixed + tested.) +- [TDD · open] The bug survived the code's entire prior life because every existing test used + EQUAL term frequencies, which mask alignment errors. Lesson: index/posting fixtures must use + DISTINCT, asymmetric values so a positional misalignment changes an observable output. + (evidence: test_tf_correct_after_low_id_update needs a=5,b=2,c=3 to catch it; equal tfs pass + under the buggy code.) +- [ADD · open] risk:high + autonomy:conservative correctly forced a human gate on a change that + looked mechanically green — the surfaced flag (positions_for is test-only-referenced forward + API) was a real judgement call the auto-gate should not have silently swallowed. (evidence: + unguarded_high_risk_auto guard held; gate presented + human-approved.) diff --git a/.add/tasks/fts-query-combinators/TASK.md b/.add/tasks/fts-query-combinators/TASK.md new file mode 100644 index 000000000..56655f2c0 --- /dev/null +++ b/.add/tasks/fts-query-combinators/TASK.md @@ -0,0 +1,420 @@ +# TASK: OR unions + TEXT+TAG intersects — correct combinator result sets + +slug: fts-query-combinators · created: 2026-06-16 · stage: production · risk: high · autonomy: conservative +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 query-combinator parser + evaluator — a proper recursive-descent parser over +the RediSearch query subset (terms with modifiers · implicit-AND juxtaposition · OR `|` · grouping +`( )` · field clauses `@f:` · TAG `{a|b}` · NUMERIC `[min max]`) producing a small AST, evaluated to +correct matched doc-id SETS via RoaringBitmap union/intersection — replacing the current ad-hoc +string-slicing parser that drops `|` and mis-tokenizes second `@field:` clauses. +Framings weighed: full recursive-descent AST parser + RoaringBitmap evaluator reusing the existing +per-clause leaves (search_field / search_field_or / search_tag / numeric) (chosen) · thin clause-combiner +bolt-on over the old parser (rejected — won't compose to @a:x|@b:y, multi-tag, numeric combos) · two +point-fixes for just `t1|t2` and `@text:.. @tag:{..}` (rejected — re-opened on the next combo shape) +Must: + + - M1 — UNION: `a | b` (default field or within `@f:(a|b)`) returns the UNION of {docs matching a} ∪ + {docs matching b}, NOT their intersection. The `|` token is parsed as a boolean-OR node, never + discarded. [fixes Defect 1: tokenize_with_modifiers drops `|` → AND, ft_text_search.rs:1095] + - M2 — MULTI-CLAUSE INTERSECTION (generic): a query with ≥2 field clauses — any mix of TEXT + `@t:words`, TAG `@g:{v}`, NUMERIC `@n:[lo hi]` — returns the INTERSECTION of each clause's matched + doc set. Each TAG/NUMERIC clause is dispatched to its own evaluator (search_tag / numeric range), + NEVER word-tokenized into text terms. [fixes Defect 2 generically: parse_field_targeted_query + slices after first `:`, ft_text_search.rs:1273 — covers @text+@tag, @text+@num, @tag-first] + - M3 — GROUPING: `( … )` groups a sub-expression; `@f:(a | b)` applies the union scoped to field f; + groups nest and compose with AND/OR. + - M4 — PRECEDENCE is explicit + documented, matching RediSearch DIALECT 2: ladder is AND + (juxtaposition, highest) > term-modifiers (`%`fuzzy / `*`prefix) > OR `|` (lowest); parentheses + override. So `a b | c d` ≡ `(a b) | (c d)`. [VERIFIED — redis.io query_syntax, see A1] + - M5 — NO REGRESSION on paths that already work: single term, all-Exact space-AND, fuzzy `%t%` / + prefix `t*`, and a single `@field:term(s)` clause return byte-identical matched sets + ordering to + today. (The known-correct AND path and modifier handling are preserved, not rewritten away.) + - M6 — Correct matched SET + a STABLE ordering is the contract. Combined BM25 scoring across OR + branches is BEST-EFFORT (reuse existing per-term BM25; exact union-score formula is NOT frozen + here). True total-matched COUNT is OUT — owned by `fts-search-count-semantics`, which depends-on + and counts over this set contract. + - M7 — NEVER PANIC on malformed/partial input: the parser returns `Frame::Error` with a named code; + no `expect`/`unwrap`/`panic!` on the FT query path. (Aligns with `fts-query-routing-robustness`; + a fuzz target covers the new parser per CLAUDE.md.) + +Reject: + + - unbalanced `(` `)` / `{` `}` / `[` `]` -> "syntax_error" (Frame::Error, never panic) + - empty query, or an empty group `()` with no terms -> "empty_query" + - `@name:` where name is not a schema field -> "unknown_field" + - NUMERIC filter not two parseable numbers, or min > max -> "numeric_filter_invalid" + - TAG filter with no values `{}` / `{ }` -> "tag_filter_invalid" + - a term that matches no document -> NOT an error: contributes the empty set to its node (tf_absent + semantics, consistent with [[project_v3_1_fts_hardening]] fts-posting-rank-tf). The whole query + returning zero matches is a valid empty reply, not an error. + +After: + + - OR queries return unions (bench term-OR: ~2,072, not ~10); TEXT+TAG and TEXT+NUMERIC combos return + intersections (bench combo: ~253, not 0); multi-`@clause` queries dispatch each clause correctly. + - The query grammar + AST + matched-set semantics are the FROZEN contract `fts-search-count-semantics` + counts over; the old string-slicing parser (pre_parse_field_filter / parse_field_targeted_query / + the `|`-dropping tokenizer) is retired on the FT.SEARCH path. + +Assumptions — lowest-confidence first: + + ⚠ A1 (now the lowest-confidence-but-DELIBERATE call) — SCOPE: a NEW recursive-descent parser+AST + REPLACES the ad-hoc parser on the FT.SEARCH path (parse_text_query / parse_field_targeted_query / + pre_parse_field_filter / the `|`-dropping tokenizer), reusing search_field / search_field_or / + search_tag / numeric as evaluator leaves. Lowest confidence because the dispatch wiring in + ft_text_search.rs is broad (~1400-1810) and some exotic already-working shape (e.g. quoted phrase, + HYBRID-adjacent text) may still route through an old path; full replacement risks a silent + regression there. If wrong: a compatibility shim / larger integration surface. → I assume full + replacement on the pure-text path with M5 (byte-identical on working shapes) as the guard; + HYBRID/vector/SPARSE dispatch is untouched. (Mitigated by M5 regression scenarios + the existing + 144 text unit + 15 store-search tests as the no-regression net.) + - [x] A2 — PRECEDENCE: AND > term-modifiers > OR, `a b | c d` ≡ `(a b) | (c d)`, parens override. + VERIFIED against RediSearch DIALECT 2 (redis.io query_syntax; the DIALECT-1→2 breaking change + confirms `hello world | "goodbye" moon` now parses as the two-branch union). No longer an open risk. + - [ ] A3 — doc-id space is SHARED across TEXT/TAG/NUMERIC (verified: `ensure_doc_id`, store.rs:242), + so RoaringBitmap set-ops compose directly with no id/hash translation. (high — verified in trace) + - [ ] A4 — one AST allocation PER FT.SEARCH COMMAND is acceptable (per-command, not per-key/per-doc; + precedent: the authorized per-query `FtSearchPlan` box). No new per-key hot-path alloc. (high) + - [ ] A5 — term modifiers (`%fuzzy%`, prefix `*`, exact) stay term-level leaves and compose under the + new grammar unchanged; fuzzy/prefix still expand via search_field_or as today. (high) + - [ ] A6 — NOT/negation (`-term`) and SLOP/INORDER/optional (`~`) are OUT of v1 scope (the benchmark + did not exercise them; not a correctness defect today). If present in current code, left untouched; + if absent, stays absent — noted, not silently dropped. (high) + + + + +--- + +## 2 · SCENARIOS — pass/fail cases ▸ docs/04-step-2-scenarios.md + + + +```gherkin +# ── M1 — OR is a union, not an AND ──────────────────────────────────────── +Scenario: term OR returns the union, not the intersection + Given docs where "alpha" matches {1,2,3} and "beta" matches {3,4,5} in the default field + When FT.SEARCH runs query "alpha | beta" + Then the matched doc set is {1,2,3,4,5} # union (5), not the AND {3} (~the old ~10 bug) + And every returned key actually contains alpha OR beta + +# ── M2a — TEXT + TAG combo intersects (the 0-result bug) ────────────────── +Scenario: TEXT and TAG clauses intersect instead of returning zero + Given docs where @body:"foo" matches {1,2,3,4} and @tag:{bar} matches {3,4,5} + When FT.SEARCH runs query "@body:foo @tag:{bar}" + Then the matched doc set is {3,4} (the intersection), NOT empty + And the TAG clause is dispatched to the tag evaluator, not word-tokenized into body terms + +# ── M2b — TEXT + NUMERIC combo intersects (same generic fix) ────────────── +Scenario: TEXT and NUMERIC clauses intersect + Given docs where @body:"phone" matches {1,2,3} and @price in [10 20] matches {2,3,9} + When FT.SEARCH runs query "@body:phone @price:[10 20]" + Then the matched doc set is {2,3} + And the NUMERIC filter is evaluated as a range, not tokenized into body terms + +# ── M3 — grouping scopes a union under an AND ───────────────────────────── +Scenario: parentheses scope a union within an intersection + Given docs where "red" matches {1,2,3}, "blue" matches {3,4}, "car" matches {2,3,4,5} + When FT.SEARCH runs query "car (red | blue)" + Then the matched doc set is {2,3,4} # car ∩ (red ∪ blue) = {2,3,4,5} ∩ {1,2,3,4} = {2,3,4} + And the result is identical to the explicit "car red | car blue" union + +# ── M4 — precedence: AND binds tighter than OR (DIALECT 2) ──────────────── +Scenario: unparenthesized mixed query groups AND tighter than OR + Given docs where "a" matches {1,2}, "b" matches {2,3}, "c" matches {7,8}, "d" matches {8,9} + When FT.SEARCH runs query "a b | c d" + Then the parse is (a AND b) OR (c AND d) and the matched set is {2} ∪ {8} = {2,8} + And it is NOT {2,3,7} (which the wrong "a AND (b|c) AND d" or "(a b|c) d" precedence would give) + +# ── M5 — no regression on already-correct shapes ────────────────────────── +Scenario: existing single-term, AND, fuzzy/prefix, and single-field queries are unchanged + Given the existing text-store corpus and the current 144 text-unit + 15 store-search tests + When the new parser handles "alpha", "alpha beta", "%alpa%", "al*", and "@body:alpha" + Then each returns a matched set + ordering byte-identical to the pre-change implementation + And the full pre-existing FT text test suite stays green + +# ── M6 — matched set + stable ordering is the contract (scoring best-effort) +Scenario: OR result set and ordering are deterministic and correct + Given the M1 corpus + When "alpha | beta" runs twice + Then both runs return the SAME ordered key list (stable, deterministic) + And BM25 scores are present and monotonic with relevance (exact union-score formula NOT asserted) + +# ── M7 — malformed input never panics ───────────────────────────────────── +Scenario: a malformed query returns an error frame, the server stays up + Given a running server + When FT.SEARCH runs query "alpha | (beta" # unbalanced paren + Then the reply is an error frame coded "syntax_error" + And the server process does not panic and serves the next command normally + +# ── Reject — named error codes (each leaves state unchanged: read-only path) ── +Scenario: unbalanced bracket/brace/paren is a syntax error + When FT.SEARCH runs "@tag:{bar" (or "a [10 20" or "a )") + Then the reply is error "syntax_error" + And no documents are returned and the index is unchanged + +Scenario: empty query or empty group is rejected + When FT.SEARCH runs "" (or "()") + Then the reply is error "empty_query" + And the index is unchanged + +Scenario: a field not in the schema is rejected + Given an index whose schema has no field "nope" + When FT.SEARCH runs "@nope:foo" + Then the reply is error "unknown_field" + And the index is unchanged + +Scenario: an invalid numeric filter is rejected + When FT.SEARCH runs "@price:[20 10]" (min>max) or "@price:[x y]" (non-numeric) + Then the reply is error "numeric_filter_invalid" + And the index is unchanged + +Scenario: an empty tag filter is rejected + When FT.SEARCH runs "@tag:{}" (or "@tag:{ }") + Then the reply is error "tag_filter_invalid" + And the index is unchanged + +Scenario: a term matching nothing is a valid empty result, not an error + Given a corpus where "zzz" appears in no document + When FT.SEARCH runs "zzz" and "alpha zzz" and "alpha | zzz" + Then "zzz" and "alpha zzz" return an empty result set (count 0), NO error + And "alpha | zzz" returns exactly alpha's matched set (zzz contributes ∅ to the union) +``` + + + + + +--- + +## 3 · CONTRACT — freeze the shape ▸ docs/05-step-3-contract.md + +``` +COMMAND FT.SEARCH [LIMIT off cnt] [other opts unchanged] + query is parsed by a NEW recursive-descent parser over this FROZEN grammar (RediSearch subset, + DIALECT-2 precedence). Bytes in, AST out, matched doc-id SET out. + +GRAMMAR (EBNF — the frozen accepted subset; anything outside -> "syntax_error"): + query = union + union = intersect ( '|' intersect )* # OR — lowest precedence + intersect = factor ( WS+ factor )* # implicit AND — binds tighter than '|' + factor = group | field_clause | term + group = '(' union ')' + field_clause = '@' field ':' ( tag_filter | numeric_filter | group | term+ ) + tag_filter = '{' tag_value ( '|' tag_value )* '}' # values OR-union within the tag field + numeric_filter = ('['|'(') num WS+ num (']'|')') # REUSE existing pre_parse_numeric_range: + # '(' = exclusive bound, +inf/-inf supported + term = WORD modifier? # WORD = analyzer token(s) + modifier = '*'(prefix) | '%'…'%'(fuzzy) # existing term modifiers, term-level leaves + # OUT of v1 (left exactly as today, never silently dropped): negation '-', optional '~', + # phrase quotes "…", SLOP/INORDER. Reaching them -> unchanged-or-syntax_error (never a panic). + +AST (FROZEN node type — src/text/query/ast.rs; fts-search-count-semantics consumes THIS): + enum QueryNode { + Term { field: Option, token: Bytes, modifier: TermModifier }, + And (Vec), // intersect children + Or (Vec), // union children + Tag { field: FieldIdx, values: Vec }, // values OR-unioned (per-value search_tag) + Numeric{ field: FieldIdx, min: f64, max: f64, min_excl: bool, max_excl: bool }, // reuse range eval + Empty, // a node that matches ∅ (absent term) — NOT an error + } + fn parse_query(input: &[u8], schema: &IndexSchema) -> Result + enum QueryError { Syntax, EmptyQuery, UnknownField(Bytes), NumericInvalid, TagInvalid } + // maps 1:1 to the wire error codes below; carries NO panic path (M7). + +EVALUATOR (src/text/query/eval.rs — the SET contract): + fn eval_set(node:&QueryNode, idx:&TextIndex, dfs:Option<&Dfs>) -> RoaringBitmap // matched doc-ids + Term/field-term -> reuse search_field / search_field_or leaf -> collect its result doc_ids + Tag -> reuse search_tag(field,value) per value (-> Vec) -> OR-union into a bitmap + Numeric-> reuse search_numeric_range(field,min,max,min_excl,max_excl) (-> Vec) -> bitmap + And(xs) -> fold ∩ (RoaringBitmap &=) · Or(xs) -> fold ∪ (|=) · Empty -> ∅ + # leaves already return doc-ids in the SHARED id space (ensure_doc_id); Vec -> RoaringBitmap + # collect is the only adaptation. & / | compose directly. (A set-only fast path that skips BM25 + # scoring on pure-filter leaves is a permitted optimization, NOT required for v1 correctness.) + +WIRE REPLY: + success -> existing FT.SEARCH array reply over eval_set(root): docs ordered by BM25 score DESC, + tie-break doc_id ASC (STABLE, deterministic). BM25 score across OR branches = best-effort + (sum of matched-leaf contributions); the exact union-score formula is NOT frozen here. + error -> Frame::Error, code ∈ { "syntax_error" | "empty_query" | "unknown_field" + | "numeric_filter_invalid" | "tag_filter_invalid" }. Index unchanged (read-only path). + +FROZEN BOUNDARY for fts-search-count-semantics (depends-on this task): + the "total matched" it must report == eval_set(root).len() (cardinality of the matched doc-id set, + BEFORE LIMIT/paging). This task freezes eval_set; the count task reads its cardinality. + +Schema / code surface (new module, additive): + + src/text/query/{mod.rs, ast.rs, parse.rs, eval.rs} (new recursive-descent parser + evaluator) + ~ src/command/vector_search/ft_text_search.rs (dispatch: route pure-text FT.SEARCH through + parse_query -> eval_set -> score/reply; + retire pipe-dropping tokenizer + first-colon slice) + + fuzz/fuzz_targets/fts_query_parse.rs (parser fuzz target — never panics; CLAUDE.md) + reuse (unchanged): TextIndex::search_field / search_field_or / search_tag, numeric range, RoaringBitmap. + HYBRID / vector / SPARSE dispatch: UNTOUCHED. +``` + +Status: FROZEN @ v1 — approved by Tin Dang (2026-06-16). Build SPLIT (freeze decision): this task = +**2a (parser + AST)** — `parse_query` → `QueryNode`/`QueryError` + grammar + parser fuzz target; the +new task **`fts-query-eval-dispatch` = 2b** (eval_set + ft_text_search dispatch + wire reply) depends-on +this one and INHERITS this frozen contract. The §3 shape below is the shared contract both halves honor. +Least-sure flag surfaced at freeze: [spec] FULL-REPLACEMENT SCOPE (§1 A1) — swapping the ad-hoc FT +text parser for a new recursive-descent parser+AST risks silently regressing an exotic already-working +shape that routes through an old path (e.g. quoted phrase, HYBRID-adjacent text); cost if wrong = a +compatibility shim + re-baseline. Mitigated by M5 byte-identical regression scenarios + the existing +144 text-unit / 15 store-search tests as the no-regression net, and by leaving HYBRID/vector/SPARSE +dispatch untouched. [contract] the SET semantics (eval_set cardinality) are what fts-search-count- +semantics inherits — if the union/intersect node algebra is wrong here, the count task re-opens this +contract. (Precedence A2 was the prior top risk; now VERIFIED against RediSearch DIALECT 2, so it +drops out.) [size] this is the largest v3-1 task: new module + dispatch rewrite + fuzz target — scoped +as ONE task per your "full query-AST parser" decision; decomposable if you'd rather split parser/eval. + + +--- + +## 4 · TESTS — failing-first suite (red) ▸ docs/06-step-4-tests.md + +Coverage target: 90% of parse_query branches (every grammar production + every QueryError code). +SCOPE = 2a PARSER ONLY — pure `parse_query(bytes, schema) -> Result` unit tests, +no index/server needed. The end-to-end matched-SET scenarios (M1/M2/M3 result sets, M5 no-regression, +M7 server-stays-up) are 2b's tests (`fts-query-eval-dispatch`); here we assert the PARSE TREE + errors. +Notation: per the frozen §3 AST there is NO separate Field node — a field scopes its terms by being +pushed onto each leaf `Term{field:Some(idx), ...}`. So `Field(body,[foo])` below is shorthand for +`Term{field:body, token:foo}`, and `@f:(a|b)` parses to `Or[Term{f,a}, Term{f,b}]` (field pushed into +the group). `@f:t1 t2` scopes BOTH terms to f (`term+`, Moon-compatible — keeps M5 safe; RediSearch's +stricter one-token binding is a deferred refinement, noted not silently adopted). +Plan (asserting the AST/error, not internals): + + - test_or_parses_as_or_node: "alpha | beta" -> Or[Term(alpha), Term(beta)] (M1 at parse level) + - test_pipe_never_discarded: assert '|' produces an Or node, never dropped (regression vs the bug) + - test_multiclause_parses_distinct_clauses: "@body:foo @tag:{bar}" -> And[Field(body,[foo]), Tag(tag,[bar])] + — the tag clause is a Tag node, NOT word-tokens "tag","bar" in body (M2 at parse level) + - test_text_numeric_clause: "@body:phone @price:[10 20]" -> And[Field(body,[phone]), Numeric(price,10,20,F,F)] + - test_grouping_scopes_union: "car (red | blue)" -> And[Term(car), Or[Term(red),Term(blue)]] (M3) + - test_field_scoped_group: "@f:(a | b)" -> Field(f, Or[Term(a),Term(b)]) + - test_precedence_and_binds_tighter: "a b | c d" -> Or[And[a,b], And[c,d]] (M4, DIALECT 2) + - test_modifiers_preserved: "%alpa%" / "al*" -> Term with Fuzzy / Prefix modifier (M5 parse level) + - test_numeric_exclusive_and_inf: "@price:[(10 +inf]" -> Numeric(price,10,+inf,min_excl=true,max_excl=false) + (reuse pre_parse_numeric_range — must NOT regress existing capability) + - test_multi_tag_values: "@tag:{a|b}" -> Tag(tag, [a, b]) + - REJECTS (each asserts the exact QueryError -> wire code, NO panic): + test_unbalanced_paren_is_syntax: "alpha | (beta" -> Err(Syntax) + test_unbalanced_brace_bracket: "@tag:{bar" / "@price:[10 20" -> Err(Syntax) + test_empty_query: "" / "()" -> Err(EmptyQuery) + test_unknown_field: "@nope:foo" (schema lacks "nope") -> Err(UnknownField) + test_numeric_invalid: "@price:[20 10]" (min>max) / "@price:[x y]" -> Err(NumericInvalid) + test_tag_invalid: "@tag:{}" / "@tag:{ }" -> Err(TagInvalid) + test_absent_term_is_not_error: "zzz" parses OK to Term(zzz) (emptiness is an EVAL outcome, not a parse error) + - test_parser_never_panics: fuzz-style table of malformed inputs -> all return Err, none panic (M7) + + +Tests live in: `tests/fts_query_parse.rs` · MUST run red (missing implementation) before Build. +Also add fuzz target `fuzz/fuzz_targets/fts_query_parse.rs` (parse_query never panics; CLAUDE.md). + + + + +--- + +## 5 · BUILD — AI writes code ▸ docs/07-step-5-build.md + +Safety rule (feature-specific): parse_query is on the malformed-input boundary — NO `unwrap`/`expect`/ +`panic!`/indexing-that-can-panic; every error path returns a `QueryError`. The recursive-descent must be +depth-bounded (reject pathological nesting with `Syntax`) so a deep `(((…)))` cannot blow the stack. +SCOPE = 2a PARSER ONLY. Build: `src/text/query/{mod.rs, ast.rs, parse.rs}` — the `QueryNode`/`TermModifier`/ +`QueryError` types + `parse_query(bytes, schema) -> Result` honoring the frozen §3 +grammar/precedence + `fuzz/fuzz_targets/fts_query_parse.rs`. Do NOT wire dispatch or write `eval_set` here +— that is 2b (`fts-query-eval-dispatch`). Reuse the existing `pre_parse_numeric_range` helper for numeric +bounds rather than re-implementing it. +Code lives in: `src/text/query/` +Constraints: do NOT change any test or the frozen contract; reuse existing crates only (roaring, bytes — +no new deps); ask if unclear. + + + +--- + +## 6 · VERIFY — evidence + non-functional review ▸ docs/08-step-6-verify.md + +SCOPE = 2a PARSER. (The matched-SET / no-regression / server-stays-up scenarios are 2b's verify.) +- [x] all tests pass — 20/20 `tests/fts_query_parse.rs` (incl. error-code conformance). Full lib + 3584/0; `text::` 144/0 (M5 no-regression net — 2a is purely additive). fmt --check clean; + `cargo clippy -- -D warnings` clean (no query/ warnings); fuzz target type-checks warning-free + on nightly (`cargo +nightly check --manifest-path fuzz/Cargo.toml --bin fts_query_parse`). +- [x] coverage did not decrease — net +20 parser tests + 1 fuzz target on a brand-new module; the + grammar productions + all five reject codes are exercised. +- [x] no test or contract was altered during build — §3 FROZEN @ v1 untouched; red suite went green + via new src only. (The error-code conformance test was ADDED in verify — a STRENGTHENING test + that locks the frozen wire codes, never a weakening.) +- [x] concurrency / timing safe — `parse_query` is a PURE function: no shared state, no async, no + locks, no interior mutability. Called once per FT.SEARCH command (per-query); 2b will invoke it + single-threaded on the owning shard's event loop. Nothing to race. No residue. +- [x] no exposed secrets, injection openings, or unexpected dependencies — parser sits on the + untrusted-input boundary and is hardened: never panics (test table + fuzz target), depth-bounded + (MAX_DEPTH=64, no stack blow-up on `(((…)))`), no unwrap/expect, raw-byte safe (handles \xff\xfe, + no UTF-8 assumption on tokens), bounded per-command allocation. It produces an AST — executes + nothing — so no injection surface. No new runtime crate: only ENABLED the existing `text-index` + feature in the fuzz crate (matches how `graph` is enabled there for cypher_parse). No finding. +- [x] layering & dependencies follow CONVENTIONS.md — new module is in the `text/` layer (below + `command/`); it imports only same-layer items (`text::store::{TermModifier, TextIndex}`) and has + NO upward dependency on `command/` — numeric-bound parsing was re-implemented locally precisely + to avoid importing the command-layer helper. File sizes: parse.rs ~440 / ast.rs ~85 / mod.rs ~15 + (all « 1500). No hot-path allocation concern (per-command, not per-key). +- [ ] a person reviewed and approved the change + +### Deep checks — do not skim (fill the path that applies; the resolver judges which) +- [x] WIRING (code) — `parse_query`/`QueryNode`/`QuerySchema`/`QueryError` are referenced by the 20 + conformance tests + the fuzz target; `QueryError::code()` is referenced by + test_error_codes_match_contract; `QuerySchema::from_names` by tests/fuzz, `from_index` is the + production constructor 2b calls. ⚠ THE FLAG: this is task 2a of a deliberate 2-way split — the + parser is NOT yet called from production dispatch; **`fts-query-eval-dispatch` (2b, depends-on + this task, recorded in state.json) is the imminent production consumer**. Every public symbol is + exercised by tests/fuzz, so nothing is orphaned — but production wiring is intentionally deferred + to 2b. Surfaced, not buried. +- [x] DEAD-CODE (code) — no orphaned symbol: the closed item above (code() now tested) was the only + gap. `FieldRef` (private) is used by resolve + the field-clause match. `push_field` is used for + field-scoped groups. clippy reports no dead code. No existing code was deleted (2a is additive; + the old parser is retired by 2b, not here). +- [ ] SEMANTIC (prose / non-code) — n/a (code change). + +### GATE RECORD +Proposed outcome: PASS — but GATE DEFERRED. Per the human decision at the 2a verify gate (Tin Dang, +2026-06-16), 2a is NOT gated standalone: build 2b (`fts-query-eval-dispatch`) on top, then present ONE +combined verify gate for the parser+evaluator+dispatch wired end-to-end. 2a's evidence stands (20/20, +lib 3584/0, fmt/clippy/fuzz clean, no residue); the combined gate will PASS 2a and 2b together. +Lowest-confidence item (carried to the combined gate): the full-replacement integration in 2b — does +the new parse→eval→reply path regress any already-working FT.SEARCH shape (M5)? +Outcome: **PASS** — resolved at the combined 2a+2b gate (2026-06-16). 2b wired the frozen parser +end-to-end; the M5 full-replacement question was answered green (lib 3584/0, e2e 12/12, tokio FT 5/5, +OR/combinator correct over the wire on shards 1 AND 4). No regression to any already-working shape. +If RISK-ACCEPTED -> owner: · ticket: · expires: (never for a security gap) +Reviewed by: Tin Dang (combined 2a+2b human gate, "PASS both") · 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-query-eval-dispatch/TASK.md b/.add/tasks/fts-query-eval-dispatch/TASK.md new file mode 100644 index 000000000..2fa0fba03 --- /dev/null +++ b/.add/tasks/fts-query-eval-dispatch/TASK.md @@ -0,0 +1,341 @@ +# TASK: Evaluate the query AST to matched sets + wire FT.SEARCH dispatch (2b) + +slug: fts-query-eval-dispatch · created: 2026-06-16 · stage: production · risk: high · autonomy: conservative +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: Evaluate the parsed `QueryNode` AST (from 2a) to a matched doc-id set + wire it into the +FT.SEARCH text dispatch — replacing the old inline parser with `parse_query → eval_query → +build_text_response` so OR unions, multi-`@clause` intersections, and grouping return correct results. +Framings weighed: ONE centralized kernel `eval_query(idx,&QueryNode,dfs,top_k)->Vec` +called by every handler text branch (chosen — removes the 4-way inline-parser duplication) · per-handler +kernel-internal splice keeping inline copies (rejected — leaves duplication) · single-shard-only first +cut (rejected — would silently leave multi-shard ranking on the buggy path) +INHERITED CONTRACT: this task does NOT re-freeze. It builds against fts-query-combinators §3 FROZEN @ v1 +(grammar · QueryNode · eval_set set-semantics · 5 wire error codes). See that task's §3. +Must: + + - E1 — eval_set(node, idx) -> RoaringBitmap: And = ∩ (fold &=), Or = ∪ (fold |=), Empty = ∅; leaves + reuse search_field/search_field_or (text) · search_tag per value OR-unioned · search_numeric_range. + Shared doc-id space (ensure_doc_id) so set-ops compose directly. + - E2 — eval_query wraps eval_set + best-effort scoring: TEXT leaves contribute BM25 (summed across OR + branches); docs matched only by TAG/NUMERIC score 0.0. Final order = score DESC, doc_id ASC (stable). + Pure single-field BM25 and pure-filter paths stay byte-identical to today (score + order). + - E3 — DISPATCH: every FT.SEARCH text branch (handler_sharded/monoio/single + ft_text_search() + + spsc) routes raw query bytes through `parse_query(bytes, QuerySchema::from_index(idx))` → on Ok + `eval_query` → `build_text_response(results, offset, count)`; on Err return `Frame::Error` with the + QueryError.code(). The old pre_parse_field_filter / parse_text_query / parse_field_targeted_query / + tokenize_with_modifiers calls on the text path are retired. + - E4 — UNTOUCHED: HYBRID / vector-KNN / SPARSE / SESSION / RANGE dispatch (caught before is_text_query) + stay byte-identical. The off-event-loop Yield path (dense-KNN only) is not touched. + - E5 — multi-shard DFS path (execute_text_search_with_global_idf, global_df/global_n) is preserved: + eval_query accepts Option<&global_df>/Option and forwards to the text leaves as today. + - E6 — server NEVER panics on a malformed query: parse error → Frame::Error, next command served. + +Reject: + + - malformed query (any QueryError) -> Frame::Error coded per §3 (syntax_error | empty_query | + unknown_field | numeric_filter_invalid | tag_filter_invalid). Index unchanged (read path). + - query with no matches -> empty result reply (count 0), NOT an error. + +After: + + - OR returns unions (~2,072 not ~10); TEXT+TAG / TEXT+NUMERIC combos intersect (~253 not 0); grouping + works; the duplicated inline parsers are gone; fts-search-count-semantics can count eval_set(root). + +Assumptions — lowest-confidence first: + + ⚠ A1 — M5 NO-REGRESSION across the full-replacement: routing all handler text branches through the + new path must keep every already-working shape (single term, AND, fuzzy/prefix, single @field:term, + bare @field:{tag}/@field:[num] fast-path) byte-identical. Lowest confidence because the change spans + 4 runtime handler files + the DFS path, and subtle reply differences (sort order, score, count) would + regress silently. If wrong: a per-shape compat fix. → Guarded by the existing 144 text-unit + 15 + store-search tests + new e2e tests asserting result sets AND that HYBRID/KNN replies are unchanged. + - [ ] A2 — best-effort scoring (text BM25 summed across OR; filters 0.0; order score desc / doc_id asc) + is acceptable; exact union-score formula is NOT frozen (per the 2a freeze decision). (high) + - [ ] A3 — the single branch point is is_text_query() (HYBRID/KNN/sparse caught before it), so only the + text branch changes; verified in the dispatch trace. (high) + - [ ] A4 — build_text_response is the one reply builder; feeding it the eval result Vec is byte-format + identical to today. (high — verified ft_text_search.rs:1807) + + + + +--- + +## 2 · SCENARIOS — pass/fail cases ▸ docs/04-step-2-scenarios.md + + + +```gherkin +# These are the END-TO-END result-set scenarios (the parse-tree level is 2a's suite). +# E1/E2 — OR union via the wired path +Scenario: OR query returns the union end-to-end + Given an index where "alpha" matches {a,b,c} and "beta" matches {c,d,e} in field body + When FT.SEARCH "alpha | beta" runs through eval_query + Then the matched key set is {a,b,c,d,e} (union), NOT {c} (the old AND bug) + And each result carries a __bm25_score field (best-effort scoring) + +# E1/E2 — TEXT+TAG combo intersect +Scenario: TEXT and TAG clauses intersect end-to-end + Given docs where @body:foo matches {1,2,3,4} and @tag:{bar} matches {3,4,5} + When FT.SEARCH "@body:foo @tag:{bar}" runs + Then the matched set is {3,4}, NOT empty (the old 0-result bug) + +# E1 — TEXT+NUMERIC combo intersect +Scenario: TEXT and NUMERIC clauses intersect + Given docs where @body:phone matches {1,2,3} and @price in [10 20] matches {2,3,9} + When FT.SEARCH "@body:phone @price:[10 20]" runs + Then the matched set is {2,3} + +# E1 — grouping +Scenario: grouping scopes a union under an AND end-to-end + Given red={1,2,3} blue={3,4} car={2,3,4,5} + When FT.SEARCH "car (red | blue)" runs + Then the matched set is {2,3,4} + +# E2 — best-effort scoring order +Scenario: OR results are ordered score desc, doc_id asc, deterministically + When "alpha | beta" runs twice + Then both runs return the SAME ordered key list + +# E3/E6 — malformed query never panics, returns coded error +Scenario: a malformed query returns a coded error and the server stays up + When FT.SEARCH "alpha | (beta" runs + Then the reply is Frame::Error coded "syntax_error" + And the very next FT.SEARCH command succeeds (server did not panic) + +Scenario: unknown field returns unknown_field + When FT.SEARCH "@nope:foo" runs against an index without field "nope" + Then the reply is Frame::Error coded "unknown_field" + +# E2/A1 — no regression on already-correct shapes +Scenario: single-term / AND / single @field / bare tag-filter are byte-identical to before + Given the existing text corpus + the 144 text-unit and 15 store-search tests + When "alpha", "alpha beta", "@body:alpha", "@tag:{bar}", "@price:[10 20]" run + Then result sets, ordering, scores and total counts match the pre-change implementation + And the full pre-existing FT text test suite stays green + +# E4 — other dispatch paths untouched +Scenario: HYBRID / vector-KNN / SPARSE replies are unchanged + When a KNN / HYBRID / SPARSE query runs + Then its reply is byte-identical to before (only the is_text_query branch changed) +``` + + + + + +--- + +## 3 · CONTRACT — freeze the shape ▸ docs/05-step-3-contract.md + +``` +INHERITED CONTRACT — fts-query-combinators §3 FROZEN @ v1 (grammar · QueryNode · eval_set set- +semantics · wire error codes). 2b does NOT re-freeze; it IMPLEMENTS that contract. The only new +symbol 2b adds is the evaluator kernel (an internal API, not a frozen wire contract): + + // src/text/query/eval.rs + fn eval_set(node: &QueryNode, idx: &TextIndex) -> RoaringBitmap // the frozen set-semantics + pub fn eval_query( + idx: &TextIndex, + node: &QueryNode, + global_df: Option<&HashMap>, // DFS path preserved (E5) + global_n: Option, + top_k: usize, + ) -> Vec // eval_set + best-effort BM25 scoring (E2) + +DISPATCH (E3): each FT.SEARCH text branch -> + match parse_query(raw, &QuerySchema::from_index(idx)) { + Ok(node) => build_text_response(&eval_query(idx,&node,gdf,gn,top_k), offset, count), + Err(e) => Frame::Error(""), // 5 frozen codes, never panic (E6) + } +WIRE REPLY: unchanged — build_text_response(results, offset, count) (ft_text_search.rs:1807): + Array[ Integer(total), (BulkString(key), Array[BulkString("__bm25_score"), BulkString(score)])* ]. + Order: score DESC, doc_id ASC. Pure-filter docs score 0.0 (matches today). HYBRID/KNN/SPARSE: UNTOUCHED. +Code surface: + src/text/query/eval.rs · ~ ft_text_search.rs (retire old parser on text path; route to + eval_query) · ~ handler_sharded/ft.rs · handler_monoio/ft.rs · handler_single.rs · spsc_handler.rs + (text branch -> parse_query→eval_query). reuse: search_field/_or/_tag/_numeric_range, build_text_response. +``` + +Status: INHERITED — fts-query-combinators §3 FROZEN @ v1 (approved Tin Dang 2026-06-16). No re-freeze. +Least-sure flag surfaced at freeze: [spec] M5 full-replacement regression across 4 runtime handler +files + the DFS path (A1) — guarded by the existing 144+15 tests + new e2e; [contract] eval_set node +algebra IS the inherited frozen set-semantics — a bug here is a contract violation, caught by the +combinator e2e scenarios. Combined 2a+2b human verify gate per the 2a-gate decision. + + +--- + +## 4 · TESTS — failing-first suite (red) ▸ docs/06-step-4-tests.md + +Coverage target: every E-rule via an in-process e2e test (build TextIndex, index docs, run search, +assert key set + order + error). Mirror the existing harness (ft_text_search.rs:2683 build pattern + +extract_hits). Use DISTINCT corpora so union≠intersection is observable. +Plan (asserting result sets / errors, not internals): + + - test_or_union_e2e: alpha={a,b,c} beta={c,d,e}; "alpha | beta" -> keys {a,b,c,d,e} (E1) + - test_text_tag_intersect_e2e: @body:foo={1,2,3,4} @tag:{bar}={3,4,5}; "@body:foo @tag:{bar}" -> {3,4} (E1) + - test_text_numeric_intersect_e2e: "@body:phone @price:[10 20]" -> {2,3} (E1) + - test_grouping_e2e: "car (red | blue)" -> {2,3,4} (E1) + - test_or_scoring_order_deterministic: "alpha | beta" twice -> identical ordered key list (E2) + - test_malformed_returns_coded_error: "alpha | (beta" -> Frame::Error "syntax_error"; next search ok (E3/E6) + - test_unknown_field_error: "@nope:foo" -> Frame::Error "unknown_field" (E3) + - test_no_regression_single_and_filter: "alpha" / "alpha beta" / "@body:alpha" / "@tag:{bar}" / + "@price:[10 20]" -> same sets+order+score as a captured pre-change baseline (E2/A1) + - test_empty_result_not_error: "zzz" (absent) -> empty reply count 0, NOT an error + - (regression net, not new files) the existing 144 text-unit + 15 store-search tests stay green; a + KNN/HYBRID smoke asserts that path is unchanged (E4). + + +Tests live in: `tests/fts_query_eval_e2e.rs` · MUST run red (eval_query missing) before Build. + + + + +--- + +## 5 · BUILD — AI writes code ▸ docs/07-step-5-build.md + +Safety rule (feature-specific): the dispatch path runs on untrusted query bytes — a parse Err MUST +become Frame::Error (never unwrap/expect/panic; E6). Only the is_text_query() branch may change; +HYBRID/KNN/SPARSE/SESSION/RANGE code stays byte-identical (E4). eval_set uses RoaringBitmap & / | for +the set algebra; leaf Vec -> bitmap collect. Preserve the DFS global_df/global_n forwarding (E5). +Build: + src/text/query/eval.rs (eval_set + eval_query) wired into src/text/query/mod.rs; then retire +the old text-path parser and route each handler text branch through parse_query → eval_query → +build_text_response (ft_text_search.rs, handler_sharded/ft.rs, handler_monoio/ft.rs, handler_single.rs, +spsc_handler.rs). Keep dead old-parser fns only if still used by a non-text path; else remove. +Code lives in: `src/text/query/`, `src/command/vector_search/`, `src/server/conn/`, `src/shard/` +Constraints: do NOT change any test or the inherited frozen contract; no new deps; ask if unclear. + + + +--- + +## 6 · VERIFY — evidence + non-functional review ▸ docs/08-step-6-verify.md + +- [x] all tests pass — see EVIDENCE below (lib 3584/0; e2e 12/12; tokio FT integration 5/5; + over-the-wire smoke + numeric x-shard probe all correct on shards 1 AND 4) +- [x] coverage did not decrease — +12 e2e tests (incl. 2 N-invariant guards); no test deleted. The + one suite that did NOT run (numeric shard-consistency) is a pre-existing stale `#[ignore]` + test — diagnosed, not regressed (FLAG-1). +- [x] no test or contract was altered during build — §3 INHERITED from 2a, untouched; the stale + numeric test was NOT modified (its intent reproduced manually instead — FLAG-1). +- [x] concurrency / timing of the risky operation is safe — the cross-shard DFS scatter (the + high-risk surface) is verified end-to-end: Phase-1 df-scatter N-invariant holds (one N per + shard, guarded by test_df_field_terms_single_entry_invariant), Phase-2 raw-query eval is + deterministic; 1-shard vs 4-shard returns IDENTICAL matched sets across 6 numeric/OR/combinator + queries. No new locks, no `.await`-held locks, no new `unsafe`. +- [x] no exposed secrets, injection openings, or unexpected dependencies — query bytes flow as + opaque `Bytes` re-parsed per shard via the frozen 2a parser (defensive: parse Err → coded + Frame::Error, never panic); no new deps. +- [x] layering & dependencies follow CONVENTIONS.md — `eval.rs` lives in `src/text/` using only + `TextIndex` methods (no upward command/ dep); dispatch wrappers in `command/vector_search/`. +- [ ] a person reviewed and approved the change — PENDING the combined 2a+2b human gate. + +### Deep checks — do not skim (fill the path that applies; the resolver judges which) +- [x] WIRING (code) — every new symbol is referenced. `eval_query`/`eval_set` ← run_text_query_on_index + (single-shard local + Phase-2 local + spsc_handler remote); `collect_df_field_terms` ← coordinator + Phase-1 scatter; `collect_highlight_terms` ← coordinator + spsc_handler HIGHLIGHT; `run_text_query` + ← handler_single / handler_sharded / handler_monoio. Confirmed: default clippy clean AND tokio + (no-text-index) clippy clean — both feature sets compile with zero unused-symbol warnings. +- [x] DEAD-CODE (code) — one KNOWN orphan: `scatter_text_search_filter` (coordinator ~1834-1933) + is now dead (the new raw-query scatter replaces it). Left in place ON PURPOSE: its removal + cascades into the InvertedSearch wire-protocol enum = scope creep beyond 2b. Recorded as a + deferred follow-up (FLAG-2), not silently shipped. +- [x] SEMANTIC (prose / non-code) — n/a (code change). + +### EVIDENCE (2026-06-16, macOS dev host; production magnitudes N/A — correctness only) +- **default clippy**: clean (`cargo clippy -- -D warnings`). +- **tokio clippy** (no-text-index path): clean (`--no-default-features --features runtime-tokio,jemalloc`). +- **lib**: `test result: ok. 3584 passed; 0 failed; 0 ignored` (244.82s, default features). +- **release build**: clean, `Finished release in 6m01s`. +- **e2e `fts_query_eval_e2e`**: 12/12 pass (OR-union, text∩tag, text∩numeric, grouping, OR-scoring + order, malformed→coded error, unknown-field, no-regression single AND-filter, empty-not-error, + kernel-direct, + 2 N-invariant df guards). +- **tokio FT integration** (`runtime-tokio,text-index,graph`): `hybrid_filter_tag` 1/1, + `ft_search_as_of_filter` 1/1, `txn_ft_search_snapshot` 3/3 — rewired tag-filter, as-of, and + **sharded txn-snapshot scatter** paths all green. +- **over-the-wire redis-cli smoke, shards 1 AND 4** (the OR/combinator fix end-to-end): + `alpha | beta`→3, `alpha beta`→1, `@body:alpha @tag:{bar}`→1, `@body:beta @price:[15 25]`→1, + `alpha | (beta`→syntax_error. Identical matched SET + ordering on both shard counts (only BM25 + magnitudes differ — within the frozen "scoring best-effort" boundary). +- **numeric 1-shard vs 4-shard identity probe** (replaces stale numeric test — FLAG-1): 6/6 + ALL-IDENTICAL and semantically correct (`@score:[5 10]`, exclusive `[(5 (10]`, `[-inf 4]`, + `[15 +inf]`, `@score+@status` combinator, `@status:{closed} | @score:[18 19]` OR-union = 8 keys ✓). + +### FLAGS (carried to the combined gate) +- **FLAG-1 (resolved, non-blocking)** — `tests/inverted_search_numeric_shard_consistency.rs` is a + pre-existing stale `#[ignore]` manual test: its `seed()` panics at line 73 because `hset_multiple` + emits HMSET (reply `+OK`) but the test annotates `let _: i64`, so it fails parsing "OK" as int — + in `seed()`, before any FT.SEARCH assertion. Independent of 2b (diff touches zero HSET code). + Intent reproduced manually (numeric x-shard probe above, 6/6 pass). Follow-up: fix the annotation + + per-test index isolation, then un-ignore. +- **FLAG-2 (deferred follow-up)** — dead `scatter_text_search_filter` in coordinator.rs; remove in a + cleanup task once the InvertedSearch wire-protocol simplification is in scope. +- **FLAG-3 (approximation, by design)** — `collect_df_field_terms` returns at-most-one entry to keep + the Phase-1 N-sentinel invariant (one N per shard); when text leaves span multiple fields the + field_hint collapses to None and df uses the cross-field path. Correctness preserved (set/ordering + identical x-shard, proven above); only BM25 IDF magnitude is approximate for multi-field OR. + +### GATE RECORD +Proposed outcome: **PASS** — but GATE DEFERRED to the combined 2a+2b human gate (per Tin Dang's +decision "build 2b first, gate together"). All functional evidence is green; the sole non-pass +(numeric shard-consistency) is a diagnosed pre-existing stale test (FLAG-1), not a 2b regression, +and its intent is independently re-verified. No security findings. No frozen-contract or test +weakening. Lowest-confidence item for the human: the full-replacement of all three FT.SEARCH +dispatch paths + the cross-shard DFS payload change (raw `Bytes`) — does any already-working +FT.SEARCH shape regress? (Mitigated by: lib 3584/0, tokio FT 5/5, smoke + x-shard probe on both +shard counts.) +If RISK-ACCEPTED -> owner: · ticket: · expires: (never for a security gap) +Outcome: **PASS** (combined 2a+2b gate). +Reviewed by: Tin Dang (combined 2a+2b human gate, "PASS both") · date: 2026-06-16 + + + +--- + +## 7 · OBSERVE — feed the next loop ▸ docs/09-the-loop.md + +Watch (reuse scenarios as monitors): OR/combinator result-set correctness — the e2e suite +(`tests/fts_query_eval_e2e.rs`) + the x-shard identity probe ARE the monitors. Re-run after any +FT.SEARCH dispatch or scatter change; a divergence between shards=1 and shards=4 matched sets is +the canary for a scatter-path regression. +Spec delta for the next loop: the cross-shard FT.SEARCH payload is now opaque `Bytes` re-parsed per +shard — `fts-search-count-semantics` (consumes `eval_set(root).len()`) and `fts-query-routing-robustness` +build on this; `fts-upsert-incremental` is independent of the query path. + +### Competency deltas +- [TDD · open] Test SELECTION must be validated to actually execute, not just compile: 4 of my chosen + FT integration tests ran 0 cases (`#![cfg(feature="runtime-tokio")]`-gated under monoio default; + one `#[ignore]`'d) and a green `cargo test --release` hid it. Evidence: first verify pass reported + "0 passed; 0 failed" yet I nearly read it as PASS. Lesson: assert a nonzero ran-count per suite, or + the suite is silent coverage. Mitigation already applied: ran them under the correct feature set. +- [TDD · open] A pre-existing `#[ignore]`'d manual test (`inverted_search_numeric_shard_consistency`) + is stale-broken at its seed (`let _: i64` vs HMSET `+OK`), so it cannot guard the scatter path it + was written for. Evidence: panics at line 73 before any assertion. Follow-up task: fix annotation + + per-test index isolation + un-ignore (a real cross-shard numeric guard is valuable post-2b). +- [SDD · open] A flat dispatch payload `(field_idx, Vec)` silently constrained the wire to + AND-only — the OR bug was unfixable at the leaf without a payload redesign. Evidence: OR returned + AND on multi-shard until `TextSearchPayload` carried raw `Bytes`. Lesson: when a cross-shard payload + can't REPRESENT the contract's algebra, that's a contract-blocking design smell to surface at freeze. +- [ADD · open] Building 2b on 2a's frozen contract before gating either ("gate together") worked: it + let the inherited eval_set algebra be validated by real end-to-end result sets rather than parser + unit tests alone. Evidence: 12/12 e2e + x-shard identity. Keep "split parser/dispatch, gate the + pair" as a pattern for contract-then-wire features. +- [ADD · open] Deferred-cleanup honesty: `scatter_text_search_filter` left dead-but-flagged (FLAG-2) + rather than removed-with-scope-creep into the InvertedSearch protocol. Folds at milestone close. diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ac405695..60775d747 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,41 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed — FT.SEARCH `OR` and multi-clause queries return correct result sets (PR #190) + +`FT.SEARCH` query combinators were silently broken: `OR` (`alpha | beta`) +collapsed to an intersection (returning only docs matching *both* terms), and a +multi-clause query such as `@body:foo @tag:{bar}` returned **zero** results +instead of the intersection. A code trace showed the root cause was a missing +parser layer — four runtime handlers each re-parsed the query inline with a +flat, AND-only shape that could not represent an `OR`/grouped AST, and the +cross-shard scatter carried that same flat shape so `OR` was unfixable at the +leaf. This lands a recursive-descent query parser producing a frozen +`QueryNode` AST and one centralized evaluator (`eval_set` folds the AST to a +`RoaringBitmap` — `AND` = intersection, `OR` = union — over the shared doc-id +space; `eval_query` adds best-effort BM25 with deterministic score-desc / +doc-id-asc ordering). All four FT.SEARCH text branches plus the cross-shard +Phase-2 scatter now route raw query bytes through `parse_query → eval_query → +build_text_response`; the cross-shard payload carries the opaque query bytes and +each shard re-parses, so `OR`/grouping/`TEXT+TAG`/`TEXT+NUMERIC` work in every +shard configuration. Malformed queries return a coded `Frame::Error` (one of +five frozen codes) and never panic the server. Verified end-to-end over the wire +on 1- and 4-shard servers (`alpha | beta` → union, `@body:foo @tag:{bar}` → +intersection, 1-shard vs 4-shard result sets identical). `HYBRID`/vector-KNN/ +`SPARSE`/`SESSION`/`RANGE` dispatch is untouched. + +### Performance — high-DF FT.SEARCH term queries no longer O(M²) (PR #190) + +The per-document term-frequency lookup on the BM25 path was an O(N) +`.position(|id| id == doc_id)` linear scan over a posting list, so a query on a +high-document-frequency term degraded to O(M²) (a ~5%-of-corpus term took +~419 ms on a 100K-doc index). `PostingList` now keeps `term_freqs`/`positions` +in sorted-doc-id (rank) order aligned with the doc-id `RoaringBitmap`, and +`tf()`/`positions_for()` resolve via `RoaringBitmap::rank` (sub-linear). The +rank alignment also fixed a latent BM25 corruption where re-indexing an updated +low-id document pushed its term frequency out of position, silently misaligning +scores. + ### Performance — monoio FT.SEARCH yield is now cost-free; brute-force knee raised to 1024 (PR #189) PR #179's monoio cooperative yield reaped the io_uring completion queue by diff --git a/fuzz/Cargo.lock b/fuzz/Cargo.lock index 4a02ee327..899ac5d8b 100644 --- a/fuzz/Cargo.lock +++ b/fuzz/Cargo.lock @@ -144,12 +144,6 @@ version = "0.22.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "72b3254f16251a8381aa12e40e3c4d2f0199f8c6508fbecb9d91f575e0fbb8c6" -[[package]] -name = "beef" -version = "0.5.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3a8241f3ebb85c056b509d4327ad0358fbbba6ffb340bf388f26350aeda225b1" - [[package]] name = "bitflags" version = "2.11.0" @@ -515,6 +509,15 @@ version = "1.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "42703706b716c37f96a77aea830392ad231f44c9e9a67872fa5548707e11b11c" +[[package]] +name = "fst" +version = "0.4.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7ab85b9b05e3978cc9a9cf8fea7f01b494e1a09ed3037e16ba39edc7a29eb61a" +dependencies = [ + "utf8-ranges", +] + [[package]] name = "futures" version = "0.3.32" @@ -875,6 +878,15 @@ version = "0.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "09edd9e8b54e49e587e4f6295a7d29c3ea94d469cb40ab8ca70b288248a81db2" +[[package]] +name = "levenshtein_automata" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0c2cdeb66e45e9f36bfad5bbdb4d2384e70936afbee843c6f6543f0c551ebb25" +dependencies = [ + "fst", +] + [[package]] name = "libc" version = "0.2.184" @@ -924,33 +936,32 @@ checksum = "5e5032e24019045c762d3c0f28f5b6b8bbf38563a65908389bf7978758920897" [[package]] name = "logos" -version = "0.14.4" +version = "0.16.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7251356ef8cb7aec833ddf598c6cb24d17b689d20b993f9d11a3d764e34e6458" +checksum = "eb2c55a318a87600ea870ff8c2012148b44bf18b74fad48d0f835c38c7d07c5f" dependencies = [ "logos-derive", ] [[package]] name = "logos-codegen" -version = "0.14.4" +version = "0.16.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "59f80069600c0d66734f5ff52cc42f2dabd6b29d205f333d61fd7832e9e9963f" +checksum = "58b3ffaa284e1350d017a57d04ada118c4583cf260c8fb01e0fe28a2e9cf8970" dependencies = [ - "beef", "fnv", - "lazy_static", "proc-macro2", "quote", + "regex-automata", "regex-syntax", "syn", ] [[package]] name = "logos-derive" -version = "0.14.4" +version = "0.16.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "24fb722b06a9dc12adb0963ed585f19fc61dc5413e6a9be9422ef92c091e731d" +checksum = "52d3a9855747c17eaf4383823f135220716ab49bea5fbea7dd42cc9a92f8aa31" dependencies = [ "logos-codegen", ] @@ -1114,7 +1125,7 @@ dependencies = [ [[package]] name = "moon" -version = "0.2.0" +version = "0.3.0" dependencies = [ "anyhow", "arc-swap", @@ -1133,6 +1144,7 @@ dependencies = [ "ctrlc", "dashmap", "flume", + "fst", "futures", "hex", "http-body-util", @@ -1140,6 +1152,7 @@ dependencies = [ "hyper-util", "io-uring", "itoa", + "levenshtein_automata", "libc", "logos", "lz4_flex", @@ -1157,6 +1170,7 @@ dependencies = [ "rand 0.10.0", "ringbuf", "roaring", + "rust-stemmers", "rustls", "rustls-pemfile", "serde", @@ -1166,6 +1180,7 @@ dependencies = [ "slotmap", "smallvec", "socket2", + "stop-words", "thiserror 2.0.18", "tikv-jemalloc-ctl", "tikv-jemallocator", @@ -1174,6 +1189,8 @@ dependencies = [ "tokio-util", "tracing", "tracing-subscriber", + "unicode-normalization", + "unicode-segmentation", "uuid", "xxhash-rust", ] @@ -1579,6 +1596,16 @@ dependencies = [ "byteorder", ] +[[package]] +name = "rust-stemmers" +version = "1.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e46a2036019fdb888131db7a4c847a1063a7493f971ed94ea82c67eada63ca54" +dependencies = [ + "serde", + "serde_derive", +] + [[package]] name = "rustc-hash" version = "2.1.2" @@ -1807,6 +1834,15 @@ dependencies = [ "lock_api", ] +[[package]] +name = "stop-words" +version = "0.10.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d68df56303396bcfb639455b3c166804aeb7994005010aab5e9e8a1277b8871d" +dependencies = [ + "serde_json", +] + [[package]] name = "strsim" version = "0.11.1" @@ -1923,6 +1959,21 @@ dependencies = [ "tikv-jemalloc-sys", ] +[[package]] +name = "tinyvec" +version = "1.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3e61e67053d25a4e82c844e8424039d9745781b3fc4f32b8d55ed50f5f667ef3" +dependencies = [ + "tinyvec_macros", +] + +[[package]] +name = "tinyvec_macros" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1f3ccbac311fea05f86f61904b462b55fb3df8837a366dfc601a0161d0532f20" + [[package]] name = "tokio" version = "1.51.1" @@ -2064,6 +2115,21 @@ version = "1.0.24" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e6e4313cd5fcd3dad5cafa179702e2b244f760991f45397d14d4ebf38247da75" +[[package]] +name = "unicode-normalization" +version = "0.1.25" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5fd4f6878c9cb28d874b009da9e8d183b5abc80117c40bbd187a1fde336be6e8" +dependencies = [ + "tinyvec", +] + +[[package]] +name = "unicode-segmentation" +version = "1.13.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c6f5d3c3b1bf09027a88a6bc961fc00497d651009560b5463668dc81b0fa87a8" + [[package]] name = "unicode-xid" version = "0.2.6" @@ -2082,6 +2148,12 @@ version = "0.9.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8ecb6da28b8a351d773b68d5825ac39017e680750f980f3a1a85cd8dd28a47c1" +[[package]] +name = "utf8-ranges" +version = "1.0.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7fcfc827f90e53a02eaef5e535ee14266c1d569214c6aa70133a624d8a3164ba" + [[package]] name = "utf8parse" version = "0.2.2" diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml index f3427024e..0c88582b5 100644 --- a/fuzz/Cargo.toml +++ b/fuzz/Cargo.toml @@ -14,7 +14,7 @@ bytes = "1" [dependencies.moon] path = ".." default-features = false -features = ["runtime-tokio", "jemalloc", "graph"] +features = ["runtime-tokio", "jemalloc", "graph", "text-index"] [dependencies.tempfile] version = "3" @@ -71,5 +71,10 @@ name = "csr_from_bytes" path = "fuzz_targets/csr_from_bytes.rs" doc = false +[[bin]] +name = "fts_query_parse" +path = "fuzz_targets/fts_query_parse.rs" +doc = false + [workspace] members = ["."] diff --git a/fuzz/fuzz_targets/fts_query_parse.rs b/fuzz/fuzz_targets/fts_query_parse.rs new file mode 100644 index 000000000..b2b99e26e --- /dev/null +++ b/fuzz/fuzz_targets/fts_query_parse.rs @@ -0,0 +1,14 @@ +#![no_main] +use libfuzzer_sys::fuzz_target; + +use moon::text::query::{parse_query, QuerySchema}; + +// Fuzz the FT.SEARCH query parser (task fts-query-combinators 2a). +// +// `parse_query` runs on untrusted client query strings and MUST never panic / hang / blow the +// stack — every malformed shape has to resolve to a `QueryError` (contract M7). The schema mixes +// the three field kinds so tag / numeric / text clause paths are all exercised by the input. +fuzz_target!(|data: &[u8]| { + let schema = QuerySchema::from_names(&["body", "title"], &["tag", "color"], &["price"]); + let _ = parse_query(data, &schema); +}); diff --git a/src/command/vector_search/ft_text_search.rs b/src/command/vector_search/ft_text_search.rs index 039b63f46..e2c62aa2f 100644 --- a/src/command/vector_search/ft_text_search.rs +++ b/src/command/vector_search/ft_text_search.rs @@ -1428,57 +1428,39 @@ pub fn ft_text_search(text_store: &TextStore, args: &[Frame]) -> Frame { None => return Frame::Error(Bytes::from_static(b"ERR invalid query")), }; - let text_index = match text_store.get_index(index_name.as_ref()) { - Some(idx) => idx, - None => { - return Frame::Error(Bytes::from_static(b"ERR no such index")); - } - }; - // Parse LIMIT clause (offset, count) with defaults (0, usize::MAX). let (limit_offset, limit_count) = parse_limit_clause(args); // Determine top_k: search for offset + count results so we can paginate. + // usize::MAX/2 for unlimited (avoids overflow in saturating arithmetic). let top_k = if limit_count == usize::MAX { - usize::MAX / 2 // large but not overflow-prone + usize::MAX / 2 } else { limit_offset.saturating_add(limit_count) }; let top_k = top_k.max(1); - // B-01 SITE 1 FIX (Plan 152-06): FieldFilter short-circuit BEFORE analyzer - // lookup. If the query is `@field:{value}` (or Plan 07 `@field:[min max]`), - // dispatch through the inverted-index path — TAG-only indexes (zero TEXT - // fields) and indexes whose analyzer would strip the tag value as a - // stopword (UAT Gap 2) work without touching the analyzer. + // Route through the centralized AST-based wrapper (fts-query-eval-dispatch 2b). + // run_text_query handles index lookup, parse, eval, tag/numeric filters, OR, + // multi-@clause combinators, and unknown-field errors — replacing the old + // pre_parse_field_filter / parse_text_query / execute_query_on_index dance. + // No HIGHLIGHT in this entry point (dispatch_vector_command callers have no db). #[cfg(feature = "text-index")] - match pre_parse_field_filter(query_bytes.as_ref()) { - Ok(Some(clause)) => { - let results = execute_query_on_index(text_index, &clause, None, None, top_k); - return build_text_response(&results, limit_offset, limit_count); - } - Ok(None) => { /* fall through to BM25 path */ } - Err(e) => return Frame::Error(Bytes::copy_from_slice(e.as_bytes())), + { + run_text_query( + text_store, + index_name.as_ref(), + query_bytes.as_ref(), + top_k, + limit_offset, + limit_count, + ) + } + #[cfg(not(feature = "text-index"))] + { + let _ = (text_store, index_name, query_bytes, top_k); + Frame::Error(Bytes::from_static(b"ERR text-index feature not enabled")) } - - // Use the first field's analyzer for query parsing (per D-03: all fields share same language). - let analyzer = match text_index.field_analyzers.first() { - Some(a) => a, - None => { - return Frame::Error(Bytes::from_static(b"ERR index has no TEXT fields")); - } - }; - - // Parse the query into (field_name, terms). - let clause = match parse_text_query(query_bytes.as_ref(), analyzer) { - Ok(c) => c, - Err(e) => return Frame::Error(Bytes::copy_from_slice(e.as_bytes())), - }; - - // Execute the search (cross-field or field-targeted). - let results = execute_query_on_index(text_index, &clause, None, None, top_k); - - build_text_response(&results, limit_offset, limit_count) } /// Execute text search on local shard without global IDF override (single-shard path). @@ -1552,6 +1534,59 @@ pub fn execute_text_search_with_global_idf( build_text_response(&results, offset, count) } +// ─── Centralized AST dispatch (fts-query-eval-dispatch 2b) ───────────────────── + +/// Centralized FT.SEARCH text dispatch: parse the query with the recursive-descent parser, evaluate +/// the AST to a matched + BM25-scored result set, and build the FT.SEARCH reply. This is the single +/// path that replaces the old `pre_parse_field_filter` / `parse_text_query` / +/// `execute_text_search_local` dance on the text branch, so OR unions, multi-`@clause` intersections, +/// and grouping return correct results. A parse error becomes a coded `Frame::Error` and NEVER +/// panics (E6). HYBRID / vector-KNN / SPARSE are caught before this and never reach it (E4). +/// +/// `global_df`/`global_n` inject the DFS global-IDF weights (multi-shard Phase 2); pass `None` on the +/// single-shard / local path (E5). The reply format is unchanged — `build_text_response`. +#[cfg(feature = "text-index")] +pub fn run_text_query_on_index( + text_index: &TextIndex, + query: &[u8], + global_df: Option<&HashMap>, + global_n: Option, + top_k: usize, + offset: usize, + count: usize, +) -> Frame { + use crate::text::query::{QuerySchema, eval_query, 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) + } + // 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())), + } +} + +/// Store-level wrapper around [`run_text_query_on_index`] — resolves the index, then dispatches on +/// the local (no global-IDF) path. Returns `ERR no such index` for an unknown index. Used by the +/// single-shard handlers, the per-handler co-located fast paths, and the SPSC text entry. +#[cfg(feature = "text-index")] +pub fn run_text_query( + text_store: &TextStore, + index_name: &[u8], + query: &[u8], + top_k: usize, + offset: usize, + count: usize, +) -> Frame { + match text_store.get_index(index_name) { + Some(text_index) => { + run_text_query_on_index(text_index, query, None, None, top_k, offset, count) + } + None => Frame::Error(Bytes::from_static(b"ERR no such index")), + } +} + // ─── Cross-field accumulation ───────────────────────────────────────────────── /// Execute cross-field search and accumulate BM25 scores per document. diff --git a/src/command/vector_search/mod.rs b/src/command/vector_search/mod.rs index e0ac5a633..52d5e4122 100644 --- a/src/command/vector_search/mod.rs +++ b/src/command/vector_search/mod.rs @@ -45,7 +45,9 @@ pub use ft_search::{ parse_session_clause, search_local, search_local_filtered, }; #[cfg(feature = "text-index")] -pub use ft_text_search::{FieldFilter, pre_parse_field_filter}; +pub use ft_text_search::{ + FieldFilter, pre_parse_field_filter, run_text_query, run_text_query_on_index, +}; 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, diff --git a/src/server/conn/handler_monoio/ft.rs b/src/server/conn/handler_monoio/ft.rs index 182fb2522..4d55ee7af 100644 --- a/src/server/conn/handler_monoio/ft.rs +++ b/src/server/conn/handler_monoio/ft.rs @@ -170,105 +170,12 @@ pub(super) async fn try_handle_ft_command( #[allow(clippy::unwrap_used)] let query_str = query_bytes.unwrap(); - // B-01 SITE 2 FIX (Plan 152-06): FieldFilter short-circuit BEFORE - // the analyzer-first parse_result block. TAG queries (and Plan 07 - // NumericRange) route through the InvertedSearch fan-out — no - // analyzer touched, no field_idx resolution (the filter carries - // its own field name; search_tag resolves against tag_fields). - #[cfg(feature = "text-index")] - { - match crate::command::vector_search::pre_parse_field_filter(query_str.as_ref()) - { - Ok(Some(clause)) => { - if let Some(filter) = clause.filter { - let (offset, count) = - crate::command::vector_search::parse_limit_clause(cmd_args); - let top_k = if count == usize::MAX { - 10000 - } else { - offset.saturating_add(count) - } - .max(1); - let response = - crate::shard::coordinator::scatter_text_search_filter( - index_name, - filter, - top_k, - offset, - count, - ctx.shard_id, - ctx.num_shards, - &ctx.shard_databases, - &ctx.dispatch_tx, - &ctx.spsc_notifiers, - ) - .await; - let mut response = response; - if let Some(ws_id) = conn.workspace_id.as_ref() { - strip_workspace_prefix_from_response(ws_id, cmd, &mut response); - } - responses.push(response); - return true; - } - } - Ok(None) => { /* fall through to BM25 path */ } - Err(e) => { - responses.push(Frame::Error(Bytes::from(e.to_owned()))); - return true; - } - } - } - - // Parse query and resolve field_idx inside a block scope so the - // MutexGuard from text_store() is dropped BEFORE .await. - // We use the TextIndex's own field_analyzers (same pipeline used at index time). - type ParseResult = - Result<(Vec, Option), String>; - // Parse query via thread-local slice (text_store is !Send, no lock needed). - let parse_result: ParseResult = crate::shard::slice::with_shard(|s| { - match s.text_store.get_index(&index_name) { - None => Err("ERR no such index".to_owned()), - Some(text_index) => match text_index.field_analyzers.first() { - None => Err("ERR index has no TEXT fields".to_owned()), - Some(analyzer) => { - let parsed = crate::command::vector_search::parse_text_query( - &query_str, analyzer, - ); - match parsed { - Err(e) => Err(e.to_owned()), - Ok(clause) => { - let field_idx = match &clause.field_name { - None => Ok(None), - Some(field_name) => { - match text_index.text_fields.iter().position(|f| { - f.field_name - .as_ref() - .eq_ignore_ascii_case(field_name.as_ref()) - }) { - Some(idx) => Ok(Some(idx)), - None => Err(format!( - "ERR unknown field '{}'", - String::from_utf8_lossy(field_name) - )), - } - } - }; - field_idx.map(|idx| (clause.terms, idx)) - } - } - } - }, - } - }); // with_shard borrow released here - - let (query_terms, field_idx) = match parse_result { - Ok(t) => t, - Err(e) => { - responses.push(Frame::Error(Bytes::from(e))); - return true; - } - }; - + // fts-query-eval-dispatch 2b: pass raw query bytes to scatter_text_search. + // The coordinator parses once (for Phase-1 df terms), then each shard + // re-parses with the recursive-descent AST evaluator. This correctly + // handles OR unions, multi-@clause intersections, tag/numeric filters, + // and grouping — replacing the old pre_parse_field_filter / parse_text_query + // pre-pass and the scatter_text_search_filter split path. let (offset, count) = crate::command::vector_search::parse_limit_clause(cmd_args); let top_k = if count == usize::MAX { 10000 @@ -277,7 +184,6 @@ pub(super) async fn try_handle_ft_command( } .max(1); - // Parse optional HIGHLIGHT/SUMMARIZE clauses from args. let highlight_opts = crate::command::vector_search::parse_highlight_clause(cmd_args); let summarize_opts = @@ -285,8 +191,7 @@ pub(super) async fn try_handle_ft_command( let response = crate::shard::coordinator::scatter_text_search( index_name, - query_terms, - field_idx, + query_str, top_k, offset, count, @@ -543,61 +448,11 @@ pub(super) async fn try_handle_ft_command( return true; } }; - // B-01 SITE 2 FIX (single-shard 151-03 fast path, Plan 152-06): - // FieldFilter short-circuit BEFORE the analyzer lookup and - // BEFORE the text_fields.is_empty() bail. - #[cfg(feature = "text-index")] - match crate::command::vector_search::pre_parse_field_filter( - query_bytes.as_ref(), - ) { - Ok(Some(clause)) => { - if clause.filter.is_some() { - let (offset, count) = - crate::command::vector_search::parse_limit_clause( - cmd_args, - ); - let top_k = if count == usize::MAX { - 10000 - } else { - offset.saturating_add(count) - } - .max(1); - let response = crate::shard::slice::with_shard(|s| match s - .text_store - .get_index(&index_name) - { - None => Frame::Error(Bytes::from_static( - b"ERR no such index", - )), - Some(text_index) => { - let results = crate::command::vector_search::ft_text_search::execute_query_on_index( - text_index, &clause, None, None, top_k, - ); - crate::command::vector_search::ft_text_search::build_text_response( - &results, offset, count, - ) - } - }); - let mut response = response; - if let Some(ws_id) = conn.workspace_id.as_ref() { - strip_workspace_prefix_from_response( - ws_id, - cmd, - &mut response, - ); - } - responses.push(response); - return true; - } - } - Ok(None) => { /* fall through */ } - Err(e) => { - responses - .push(Frame::Error(Bytes::copy_from_slice(e.as_bytes()))); - return true; - } - } - // Step 7: LIMIT parsing + top_k cap (T-151-03-02). + // fts-query-eval-dispatch 2b: single-shard local fast path. + // run_text_query handles index lookup, parse, eval, tag/numeric + // filters, OR, multi-@clause combinators in one call. + // HIGHLIGHT/SUMMARIZE: re-parse inside the same with_shard closure + // so text_index and databases[0] are borrowed disjointly off `s`. let (offset, count) = crate::command::vector_search::parse_limit_clause(cmd_args); let top_k = if count == usize::MAX { @@ -606,108 +461,56 @@ pub(super) async fn try_handle_ft_command( offset.saturating_add(count) } .max(1); - // Step 8: HIGHLIGHT / SUMMARIZE options. let highlight_opts = crate::command::vector_search::parse_highlight_clause(cmd_args); let summarize_opts = crate::command::vector_search::parse_summarize_clause(cmd_args); - let needs_db = highlight_opts.is_some() || summarize_opts.is_some(); - // Steps 2-10: acquire stores and execute via ShardSlice. - let text_search_result: Result = - crate::shard::slice::with_shard(|s| { - // Step 2-3: resolve index from text_store. - let text_index = match s.text_store.get_index(&index_name) { - Some(idx) => idx, - None => { - return Err(Frame::Error(Bytes::from_static( - b"ERR no such index", - ))); - } - }; - // Step 4: ensure TEXT fields. - if text_index.text_fields.is_empty() { - return Err(Frame::Error(Bytes::from_static( - b"ERR index has no TEXT fields", - ))); - } - // Step 5: parse query. - let analyzer = match text_index.field_analyzers.first() { - Some(a) => a, - None => { - return Err(Frame::Error(Bytes::from_static( - b"ERR index has no TEXT fields", - ))); - } - }; - let clause = - match crate::command::vector_search::parse_text_query( - query_bytes.as_ref(), - analyzer, - ) { - Ok(c) => c, - Err(msg) => { - return Err(Frame::Error(Bytes::copy_from_slice( - msg.as_bytes(), - ))); - } - }; - // Step 5b: resolve field_idx. - let field_idx = match &clause.field_name { - None => None, - Some(field_name) => { - match text_index.text_fields.iter().position(|f| { - f.field_name - .as_ref() - .eq_ignore_ascii_case(field_name.as_ref()) - }) { - Some(idx) => Some(idx), - None => { - let bad = field_name.clone(); - return Err(Frame::Error(Bytes::from( - format!( - "ERR unknown field '{}'", - String::from_utf8_lossy(&bad) - ), - ))); - } + let need_hl = highlight_opts.is_some() || summarize_opts.is_some(); + + let mut response = crate::shard::slice::with_shard(|s| { + #[cfg(feature = "text-index")] + { + let mut r = crate::command::vector_search::run_text_query( + &s.text_store, + &index_name, + query_bytes.as_ref(), + top_k, + offset, + count, + ); + if need_hl { + if let Some(text_index) = + s.text_store.get_index(&index_name) + { + if let Ok(node) = crate::text::query::parse_query( + query_bytes.as_ref(), + &crate::text::query::QuerySchema::from_index( + text_index, + ), + ) { + let terms = + crate::text::query::collect_highlight_terms( + &node, text_index, + ); + let db = &s.databases[0]; + crate::command::vector_search::apply_post_processing( + &mut r, + &terms, + text_index, + db, + highlight_opts.as_ref(), + summarize_opts.as_ref(), + ); } } - }; - let query_terms = clause.terms; - // Step 10: execute. - let mut response = - crate::command::vector_search::execute_text_search_local( - &s.text_store, - &index_name, - field_idx, - &query_terms, - top_k, - offset, - count, - ); - // Step 9+10b: optional post-processing with db access. - if needs_db { - let db = &s.databases[0]; - let term_strings: Vec = - query_terms.iter().map(|qt| qt.text.clone()).collect(); - crate::command::vector_search::apply_post_processing( - &mut response, - &term_strings, - text_index, - db, - highlight_opts.as_ref(), - summarize_opts.as_ref(), - ); } - Ok(response) - }); - let mut response = match text_search_result { - Ok(r) => r, - Err(e) => { - responses.push(e); - return true; + r } - }; + #[cfg(not(feature = "text-index"))] + Frame::Error(Bytes::from_static( + b"ERR text-index feature not enabled", + )) + }); if let Some(ws_id) = conn.workspace_id.as_ref() { strip_workspace_prefix_from_response(ws_id, cmd, &mut response); } diff --git a/src/server/conn/handler_sharded/ft.rs b/src/server/conn/handler_sharded/ft.rs index d2d64a976..d0e9ca22f 100644 --- a/src/server/conn/handler_sharded/ft.rs +++ b/src/server/conn/handler_sharded/ft.rs @@ -172,107 +172,12 @@ pub(super) async fn try_handle_ft_command( #[allow(clippy::unwrap_used)] // query_bytes is Some when is_text is true let query_str = query_bytes.unwrap(); - // B-01 SITE 3 FIX (Plan 152-06): FieldFilter short-circuit - // BEFORE the analyzer-first parse_result block. Symmetric with - // handler_monoio. TAG queries route through the InvertedSearch - // fan-out -- no analyzer touched, no field_idx resolution. - #[cfg(feature = "text-index")] - { - match crate::command::vector_search::pre_parse_field_filter(query_str.as_ref()) - { - Ok(Some(clause)) => { - if let Some(filter) = clause.filter { - let (offset, count) = - crate::command::vector_search::parse_limit_clause(cmd_args); - let top_k = if count == usize::MAX { - 10000 - } else { - offset.saturating_add(count) - } - .max(1); - let response = - crate::shard::coordinator::scatter_text_search_filter( - index_name, - filter, - top_k, - offset, - count, - ctx.shard_id, - ctx.num_shards, - &ctx.shard_databases, - &ctx.dispatch_tx, - &ctx.spsc_notifiers, - ) - .await; - let mut response = response; - if let Some(ws_id) = conn.workspace_id.as_ref() { - strip_workspace_prefix_from_response(ws_id, cmd, &mut response); - } - responses.push(response); - return true; - } - } - Ok(None) => { /* fall through */ } - Err(e) => { - responses.push(Frame::Error(Bytes::from(e.to_owned()))); - return true; - } - } - } - - // Parse query and resolve field_idx inside a block scope so the - // MutexGuard from text_store() is dropped BEFORE .await. - // We use the TextIndex's own field_analyzers (same pipeline used at index time). - type ParseResult = - Result<(Vec, Option), String>; - // The inner closure scans the TextIndex via text_store. - let parse_body = |ts: &crate::text::store::TextStore| -> ParseResult { - match ts.get_index(&index_name) { - None => Err("ERR no such index".to_owned()), - Some(text_index) => match text_index.field_analyzers.first() { - None => Err("ERR index has no TEXT fields".to_owned()), - Some(analyzer) => { - let parsed = crate::command::vector_search::parse_text_query( - &query_str, analyzer, - ); - match parsed { - Err(e) => Err(e.to_owned()), - Ok(clause) => { - let field_idx = match &clause.field_name { - None => Ok(None), - Some(field_name) => { - match text_index.text_fields.iter().position(|f| { - f.field_name - .as_ref() - .eq_ignore_ascii_case(field_name.as_ref()) - }) { - Some(idx) => Ok(Some(idx)), - None => Err(format!( - "ERR unknown field '{}'", - String::from_utf8_lossy(field_name) - )), - } - } - }; - field_idx.map(|idx| (clause.terms, idx)) - } - } - } - }, - } - }; - // Unconditional slice path: ShardSlice is always initialized. - let parse_result: ParseResult = - crate::shard::slice::with_shard(|s| parse_body(&s.text_store)); - - let (query_terms, field_idx) = match parse_result { - Ok(t) => t, - Err(e) => { - responses.push(Frame::Error(Bytes::from(e))); - return true; - } - }; - + // fts-query-eval-dispatch 2b: pass raw query bytes to scatter_text_search. + // The coordinator parses once (for Phase-1 df terms), then each shard + // re-parses with the recursive-descent AST evaluator. This correctly + // handles OR unions, multi-@clause intersections, tag/numeric filters, + // and grouping — replacing the old pre_parse_field_filter / parse_text_query + // pre-pass and the scatter_text_search_filter split path. let (offset, count) = crate::command::vector_search::parse_limit_clause(cmd_args); let top_k = if count == usize::MAX { 10000 @@ -281,7 +186,6 @@ pub(super) async fn try_handle_ft_command( } .max(1); - // Parse optional HIGHLIGHT/SUMMARIZE clauses from args. let highlight_opts = crate::command::vector_search::parse_highlight_clause(cmd_args); let summarize_opts = @@ -289,8 +193,7 @@ pub(super) async fn try_handle_ft_command( let mut response = crate::shard::coordinator::scatter_text_search( index_name, - query_terms, - field_idx, + query_str, top_k, offset, count, @@ -445,64 +348,11 @@ pub(super) async fn try_handle_ft_command( return true; } }; - // B-01 SITE 3 FIX (single-shard 151-03 fast path, - // Plan 152-06): FieldFilter short-circuit BEFORE - // text_fields.is_empty() bail. - #[cfg(feature = "text-index")] - match crate::command::vector_search::pre_parse_field_filter( - query_bytes.as_ref(), - ) { - Ok(Some(clause)) => { - if clause.filter.is_some() { - let (offset, count) = - crate::command::vector_search::parse_limit_clause(cmd_args); - let top_k = if count == usize::MAX { - 10000 - } else { - offset.saturating_add(count) - } - .max(1); - let lookup_body = - |ts: &crate::text::store::TextStore| -> Frame { - match ts.get_index(&index_name) { - None => Frame::Error(Bytes::from_static( - b"ERR no such index", - )), - Some(text_index) => { - let results = crate::command::vector_search::ft_text_search::execute_query_on_index( - text_index, &clause, None, None, top_k, - ); - crate::command::vector_search::ft_text_search::build_text_response( - &results, offset, count, - ) - } - } - }; - // Unconditional slice path: ShardSlice is always initialized. - let response = crate::shard::slice::with_shard(|s| { - lookup_body(&s.text_store) - }); - let mut response = response; - if let Some(ws_id) = conn.workspace_id.as_ref() { - strip_workspace_prefix_from_response( - ws_id, - cmd, - &mut response, - ); - } - responses.push(response); - return true; - } - } - Ok(None) => { /* fall through */ } - Err(e) => { - responses.push(Frame::Error(Bytes::copy_from_slice(e.as_bytes()))); - return true; - } - } - // text_store + databases[0] accessed in ONE with_shard closure - // (multi-resource arm) — text_index borrows from text_store and is - // also passed to apply_post_processing alongside &Database. + // fts-query-eval-dispatch 2b: single-shard local fast path. + // run_text_query handles index lookup, parse, eval, tag/numeric + // filters, OR, multi-@clause combinators in one call. + // HIGHLIGHT/SUMMARIZE: re-parse inside the same with_shard closure + // so text_index and databases[0] are borrowed disjointly off `s`. let (offset, count) = crate::command::vector_search::parse_limit_clause(cmd_args); let top_k = if count == usize::MAX { @@ -515,90 +365,46 @@ pub(super) async fn try_handle_ft_command( crate::command::vector_search::parse_highlight_clause(cmd_args); let summarize_opts = crate::command::vector_search::parse_summarize_clause(cmd_args); - let need_db = highlight_opts.is_some() || summarize_opts.is_some(); + let need_hl = highlight_opts.is_some() || summarize_opts.is_some(); - // The closure body returns the response Frame; early errors - // are encoded as Frame::Error returns. - let text_search_body = |ts: &crate::text::store::TextStore, - db_opt: Option<&crate::storage::db::Database>| - -> Frame { - let text_index = match ts.get_index(&index_name) { - Some(idx) => idx, - None => { - return Frame::Error(Bytes::from_static(b"ERR no such index")); - } - }; - if text_index.text_fields.is_empty() { - return Frame::Error(Bytes::from_static( - b"ERR index has no TEXT fields", - )); - } - let analyzer = match text_index.field_analyzers.first() { - Some(a) => a, - None => { - return Frame::Error(Bytes::from_static( - b"ERR index has no TEXT fields", - )); - } - }; - let clause = match crate::command::vector_search::parse_text_query( - query_bytes.as_ref(), - analyzer, - ) { - Ok(c) => c, - Err(msg) => { - return Frame::Error(Bytes::copy_from_slice(msg.as_bytes())); - } - }; - let field_idx = match &clause.field_name { - None => None, - Some(field_name) => { - match text_index.text_fields.iter().position(|f| { - f.field_name - .as_ref() - .eq_ignore_ascii_case(field_name.as_ref()) - }) { - Some(idx) => Some(idx), - None => { - return Frame::Error(Bytes::from(format!( - "ERR unknown field '{}'", - String::from_utf8_lossy(field_name) - ))); - } - } - } - }; - let query_terms = clause.terms; - let mut response = - crate::command::vector_search::execute_text_search_local( - ts, + let response = crate::shard::slice::with_shard(|s| { + #[cfg(feature = "text-index")] + { + let mut r = crate::command::vector_search::run_text_query( + &s.text_store, &index_name, - field_idx, - &query_terms, + query_bytes.as_ref(), top_k, offset, count, ); - if let Some(db) = db_opt { - let term_strings: Vec = - query_terms.iter().map(|qt| qt.text.clone()).collect(); - crate::command::vector_search::apply_post_processing( - &mut response, - &term_strings, - text_index, - db, - highlight_opts.as_ref(), - summarize_opts.as_ref(), - ); + if need_hl { + if let Some(text_index) = s.text_store.get_index(&index_name) { + if let Ok(node) = crate::text::query::parse_query( + query_bytes.as_ref(), + &crate::text::query::QuerySchema::from_index( + text_index, + ), + ) { + let terms = crate::text::query::collect_highlight_terms( + &node, text_index, + ); + let db = &s.databases[0]; + crate::command::vector_search::apply_post_processing( + &mut r, + &terms, + text_index, + db, + highlight_opts.as_ref(), + summarize_opts.as_ref(), + ); + } + } + } + r } - response - }; - - // Unconditional slice path: ShardSlice is always initialized. - let response = crate::shard::slice::with_shard(|s| { - let db_opt: Option<&crate::storage::db::Database> = - if need_db { Some(&s.databases[0]) } else { None }; - text_search_body(&s.text_store, db_opt) + #[cfg(not(feature = "text-index"))] + Frame::Error(Bytes::from_static(b"ERR text-index feature not enabled")) }); let mut response = response; if let Some(ws_id) = conn.workspace_id.as_ref() { diff --git a/src/server/conn/handler_single.rs b/src/server/conn/handler_single.rs index bbd604b52..a977d4ac7 100644 --- a/src/server/conn/handler_single.rs +++ b/src/server/conn/handler_single.rs @@ -1346,160 +1346,63 @@ pub async fn handle_connection( continue; } }; - // B-01 FIX (single-shard handler_single, Plan 152-06): - // FieldFilter short-circuit BEFORE text_fields.is_empty() bail. - #[cfg(feature = "text-index")] - match crate::command::vector_search::pre_parse_field_filter(query_bytes.as_ref()) { - Ok(Some(clause)) => { - if clause.filter.is_some() { - let (offset, count) = crate::command::vector_search::parse_limit_clause(cmd_args); - let top_k = if count == usize::MAX { 10000 } else { offset.saturating_add(count) }.max(1); - let response = match ts_mut.get_index(&index_name) { - None => crate::protocol::Frame::Error(bytes::Bytes::from_static(b"ERR no such index")), - Some(text_index) => { - let results = crate::command::vector_search::ft_text_search::execute_query_on_index( - text_index, &clause, None, None, top_k, - ); - crate::command::vector_search::ft_text_search::build_text_response( - &results, offset, count, - ) - } - }; - responses.push(response); - continue; - } - } - Ok(None) => { /* fall through */ } - Err(e) => { - responses.push(crate::protocol::Frame::Error(bytes::Bytes::copy_from_slice(e.as_bytes()))); - continue; - } - } - // Step 2: TextStore is already borrowed via ts_mut (no extra guard to drop). - // When text_store is None, ts_mut points at the empty fallback — - // get_index() returns None → Step 3 emits "ERR no such index" (correct). - // Step 3: index lookup. - let text_index = match ts_mut.get_index(&index_name) { - Some(idx) => idx, - None => { - responses.push(crate::protocol::Frame::Error(bytes::Bytes::from_static( - b"ERR no such index", - ))); - continue; - } - }; - // Step 4: ensure at least one TEXT field. - if text_index.text_fields.is_empty() { - responses.push(crate::protocol::Frame::Error(bytes::Bytes::from_static( - b"ERR index has no TEXT fields", - ))); - continue; - } - // Step 5: parse via first analyzer. - let analyzer = match text_index.field_analyzers.first() { - Some(a) => a, - None => { - responses.push(crate::protocol::Frame::Error(bytes::Bytes::from_static( - b"ERR index has no TEXT fields", - ))); - continue; - } - }; - let clause = match crate::command::vector_search::parse_text_query( - query_bytes.as_ref(), - analyzer, - ) { - Ok(c) => c, - Err(msg) => { - responses.push(crate::protocol::Frame::Error( - bytes::Bytes::copy_from_slice(msg.as_bytes()), - )); - continue; - } - }; - // Step 5b: resolve field_idx. - let field_idx = match &clause.field_name { - None => None, - Some(field_name) => match text_index - .text_fields - .iter() - .position(|f| { - f.field_name.as_ref().eq_ignore_ascii_case( - field_name.as_ref(), - ) - }) { - Some(idx) => Some(idx), - None => { - let bad_name = field_name.clone(); - responses.push(crate::protocol::Frame::Error(bytes::Bytes::from( - format!( - "ERR unknown field '{}'", - String::from_utf8_lossy(&bad_name) - ), - ))); - continue; - } - }, - }; - // Step 6: query_terms. - let query_terms = clause.terms; - // Step 7: LIMIT + top_k cap (T-151-03-02). + // fts-query-eval-dispatch 2b: single-shard (non-sharded) fast path. + // run_text_query handles index lookup, parse, eval, tag/numeric + // filters, OR, multi-@clause combinators in one call. + // ts_mut is already borrowed (parking_lot Mutex, non-reentrant); + // ts_mut points to the empty fallback when text_store is None — + // get_index() returns None → "ERR no such index" (correct). let (offset, count) = - crate::command::vector_search::parse_limit_clause( - cmd_args, - ); + crate::command::vector_search::parse_limit_clause(cmd_args); let top_k = if count == usize::MAX { 10000 } else { offset.saturating_add(count) } .max(1); - // Step 8: HIGHLIGHT / SUMMARIZE. let highlight_opts = - crate::command::vector_search::parse_highlight_clause( - cmd_args, - ); + crate::command::vector_search::parse_highlight_clause(cmd_args); let summarize_opts = - crate::command::vector_search::parse_summarize_clause( - cmd_args, - ); - // Step 9: DB read guard iff post-processing needed. - let db_guard_opt = if highlight_opts.is_some() - || summarize_opts.is_some() + crate::command::vector_search::parse_summarize_clause(cmd_args); + #[cfg(feature = "text-index")] { - Some(db[conn.selected_db].read()) - } else { - None - }; - // Step 10: execute + optional post-processing. - let mut response = - crate::command::vector_search::execute_text_search_local( + let mut response = crate::command::vector_search::run_text_query( &*ts_mut, &index_name, - field_idx, - &query_terms, + query_bytes.as_ref(), top_k, offset, count, ); - if let Some(ref db_guard) = db_guard_opt { - let term_strings: Vec = query_terms - .iter() - .map(|qt| qt.text.clone()) - .collect(); - crate::command::vector_search::apply_post_processing( - &mut response, - &term_strings, - text_index, - db_guard, - highlight_opts.as_ref(), - summarize_opts.as_ref(), - ); + if highlight_opts.is_some() || summarize_opts.is_some() { + if let Some(text_index) = ts_mut.get_index(&index_name) { + if let Ok(node) = crate::text::query::parse_query( + query_bytes.as_ref(), + &crate::text::query::QuerySchema::from_index(text_index), + ) { + let terms = crate::text::query::collect_highlight_terms(&node, text_index); + let db_guard = db[conn.selected_db].read(); + crate::command::vector_search::apply_post_processing( + &mut response, + &terms, + text_index, + &db_guard, + highlight_opts.as_ref(), + summarize_opts.as_ref(), + ); + } + } + } + responses.push(response); + continue; + } + #[cfg(not(feature = "text-index"))] + { + responses.push(crate::protocol::Frame::Error( + bytes::Bytes::from_static(b"ERR text-index feature not enabled"), + )); + continue; } - // Explicit drop of db_guard (inner); ts_mut / ts_guard drop at scope end. - drop(db_guard_opt); - responses.push(response); - continue; } } } diff --git a/src/shard/coordinator.rs b/src/shard/coordinator.rs index 2c957719c..a3222687d 100644 --- a/src/shard/coordinator.rs +++ b/src/shard/coordinator.rs @@ -1568,8 +1568,7 @@ pub async fn scatter_invalidate_range( /// **before** any `.await` point — required by RESEARCH Pitfall 2. pub async fn scatter_text_search( index_name: Bytes, - query_terms: Vec, - field_idx: Option, + query: Bytes, top_k: usize, offset: usize, count: usize, @@ -1582,31 +1581,59 @@ pub async fn scatter_text_search( summarize_opts: Option, ) -> Frame { let _ = shard_databases; // E2 removes - // Extract plain term strings for DocFreq phase (only needs term text, not modifiers). - let term_strings: Vec = query_terms.iter().map(|qt| qt.text.clone()).collect(); + + // ── Parse once for Phase-1 df terms + highlight terms (fts-query-eval-dispatch 2b) ── + // Parse inside a with_shard block so we have the index schema, then move owned + // values out. with_shard releases the borrow before any .await. + #[cfg(feature = "text-index")] + let (field_queries, term_strings) = { + use crate::text::query::{QuerySchema, collect_df_field_terms, collect_highlight_terms}; + let parse_result: Result< + (Vec<(Option, Vec)>, Vec), + crate::protocol::Frame, + > = crate::shard::slice::with_shard(|s| match s.text_store.get_index(&index_name) { + None => Err(Frame::Error(Bytes::from_static(b"ERR no such index"))), + Some(text_index) => { + let schema = QuerySchema::from_index(text_index); + match crate::text::query::parse_query(&query, &schema) { + Err(e) => Err(Frame::Error(Bytes::copy_from_slice(e.code().as_bytes()))), + Ok(node) => { + let fq = collect_df_field_terms(&node, text_index); + let ts = collect_highlight_terms(&node, text_index); + Ok((fq, ts)) + } + } + } + }); + match parse_result { + Err(err_frame) => return err_frame, + Ok(pair) => pair, + } + }; + #[cfg(not(feature = "text-index"))] + let (field_queries, _term_strings): (Vec<(Option, Vec)>, Vec) = + (Vec::new(), Vec::new()); // ── Single-shard fast path (per D-06) ──────────────────────────────────── if num_shards == 1 { // Local IDF is globally accurate with one shard — skip DFS pre-pass. - // Apply HIGHLIGHT/SUMMARIZE post-processing after local search. + // Use run_text_query_on_index with no global IDF (single-shard path). // // text_store + databases[0] accessed simultaneously via a single // `with_shard` call to avoid a reentrant `with_shard*` panic. let result = crate::shard::slice::with_shard(|s| { let ts = &s.text_store; - let mut r = crate::command::vector_search::ft_text_search::execute_text_search_local( - ts, - &index_name, - field_idx, - &query_terms, - top_k, - offset, - count, - ); - if highlight_opts.is_some() || summarize_opts.is_some() { - // Get the text_index reference, then borrow databases[0] - // disjointly. Both fields are tracked independently by rustc. - if let Some(text_index) = ts.get_index(&index_name) { + let text_index = match ts.get_index(&index_name) { + Some(idx) => idx, + None => return Frame::Error(Bytes::from_static(b"ERR no such index")), + }; + #[cfg(feature = "text-index")] + { + let mut r = crate::command::vector_search::ft_text_search::run_text_query_on_index( + text_index, &query, None, None, top_k, offset, count, + ); + if highlight_opts.is_some() || summarize_opts.is_some() { + // databases[0] borrowed disjointly from text_store — both live on `s`. if let Some(db) = s.databases.get_mut(0) { crate::command::vector_search::ft_text_search::apply_post_processing( &mut r, @@ -1618,16 +1645,20 @@ pub async fn scatter_text_search( ); } } + r + } + #[cfg(not(feature = "text-index"))] + { + let _ = text_index; + Frame::Error(Bytes::from_static(b"ERR text-index feature not enabled")) } - r }); return result; } // ── Phase 1: scatter DocFreq to all shards ──────────────────────────────── // Collect (term, df, N) from each shard to build global IDF weights. - // DocFreq only needs term strings (not modifiers) for df lookup. - let field_queries = vec![(field_idx, term_strings.clone())]; + // field_queries comes from collect_df_field_terms (above); shape unchanged. let mut doc_freq_receivers: Vec> = Vec::with_capacity(num_shards.saturating_sub(1)); let mut local_doc_freq: Option = None; @@ -1692,36 +1723,42 @@ pub async fn scatter_text_search( for shard_id in 0..num_shards { if shard_id == my_shard { - // Local: execute with global IDF directly. + // Local: execute with global IDF via run_text_query_on_index. // text_store + databases[0] folded into a single `with_shard` to // avoid reentrant `with_shard*` panic. Slice released before .await. let response = crate::shard::slice::with_shard(|s| { match s.text_store.get_index(&index_name) { Some(text_index) => { - let mut r = - crate::command::vector_search::ft_text_search::execute_text_search_with_global_idf( + #[cfg(feature = "text-index")] + { + let mut r = crate::command::vector_search::ft_text_search::run_text_query_on_index( text_index, - field_idx, - &query_terms, - &global_df, - global_n, + &query, + Some(&global_df), + Some(global_n), top_k, 0, // each shard returns top_k; coordinator applies final offset top_k, ); - if highlight_opts.is_some() || summarize_opts.is_some() { - if let Some(db) = s.databases.get_mut(0) { - crate::command::vector_search::ft_text_search::apply_post_processing( - &mut r, - &term_strings, - text_index, - db, - highlight_opts.as_ref(), - summarize_opts.as_ref(), - ); + if highlight_opts.is_some() || summarize_opts.is_some() { + if let Some(db) = s.databases.get_mut(0) { + crate::command::vector_search::ft_text_search::apply_post_processing( + &mut r, + &term_strings, + text_index, + db, + highlight_opts.as_ref(), + summarize_opts.as_ref(), + ); + } } + r + } + #[cfg(not(feature = "text-index"))] + { + let _ = text_index; + Frame::Error(Bytes::from_static(b"ERR text-index feature not enabled")) } - r } None => Frame::Error(Bytes::from_static(b"ERR unknown index")), } @@ -1732,9 +1769,8 @@ pub async fn scatter_text_search( let msg = ShardMessage::TextSearch(Box::new(crate::shard::dispatch::TextSearchPayload { index_name: index_name.clone(), - field_idx, - // Send full QueryTerm so remote shard applies the same expansion. - query_terms: query_terms.clone(), + // Send raw query bytes; each remote shard re-parses with the full AST. + query: query.clone(), global_df: global_df.clone(), global_n, top_k, @@ -2308,12 +2344,7 @@ mod tests { let result = scatter_text_search( Bytes::from_static(b"nonexistent_index"), - vec![crate::command::vector_search::QueryTerm { - text: "machine".to_owned(), - #[cfg(feature = "text-index")] - modifier: crate::text::store::TermModifier::Exact, - }], - None, // cross-field + Bytes::from_static(b"machine"), // raw query bytes (fts-query-eval-dispatch 2b) 10, 0, 10, @@ -2327,7 +2358,7 @@ mod tests { ) .await; - // Should be "ERR no such index" (local execute_text_search_local path), + // Should be "ERR no such index" (single-shard run_text_query_on_index path), // NOT a channel error. This proves the DFS pre-pass was skipped entirely. match &result { Frame::Error(e) => { diff --git a/src/shard/dispatch.rs b/src/shard/dispatch.rs index 06e2a18d3..9ee6e132c 100644 --- a/src/shard/dispatch.rs +++ b/src/shard/dispatch.rs @@ -275,8 +275,11 @@ pub struct FtHybridPayload { /// lines when the hot slotted variants are enqueued. pub struct TextSearchPayload { pub index_name: Bytes, - pub field_idx: Option, - pub query_terms: Vec, + /// Raw FT.SEARCH query bytes (fts-query-eval-dispatch 2b). Each shard + /// re-parses this with the recursive-descent parser so the full AST + /// (OR, multi-@clause, grouping) is evaluated correctly — replacing the + /// old `field_idx`/`query_terms` pre-parsed flat representation. + pub query: Bytes, pub global_df: std::collections::HashMap, pub global_n: u32, pub top_k: usize, diff --git a/src/shard/spsc_handler.rs b/src/shard/spsc_handler.rs index 52ffd4d83..051ed750c 100644 --- a/src/shard/spsc_handler.rs +++ b/src/shard/spsc_handler.rs @@ -1334,61 +1334,77 @@ pub(crate) fn handle_shard_message_shared( let _ = reply_tx.send(response); } ShardMessage::TextSearch(payload) => { - let crate::shard::dispatch::TextSearchPayload { - index_name, - field_idx, - query_terms, - global_df, - global_n, - top_k, - offset, - count, - highlight_opts, - summarize_opts, - reply_tx, - } = *payload; - // DFS Phase 2: execute BM25 text search with global IDF injected by coordinator. - // After scoring, apply HIGHLIGHT/SUMMARIZE post-processing if requested. - // Each shard applies post-processing to its own results using its local hash store - // (direct access — no cross-shard reads needed, no .await — safe to hold guards). - // - // query_terms is Vec — fuzzy/prefix terms use the OR-union expansion path. - // Extract plain strings for HIGHLIGHT/SUMMARIZE (needs analyzed term text only). - let term_strings: Vec = query_terms.iter().map(|qt| qt.text.clone()).collect(); - // text_store and read_db(0) accessed in one with_shard closure (multi-resource). - let response = { - crate::shard::slice::with_shard(|s| match s.text_store.get_index(&index_name) { - Some(text_index) => { - let mut result = - crate::command::vector_search::ft_text_search::execute_text_search_with_global_idf( + // Non-text-index build: only reply_tx is needed (the BM25 path is feature-gated, so the + // other payload fields would be unused). `..` ignores them. + #[cfg(not(feature = "text-index"))] + { + let crate::shard::dispatch::TextSearchPayload { reply_tx, .. } = *payload; + let _ = reply_tx.send(crate::protocol::Frame::Error(bytes::Bytes::from_static( + b"ERR text-index feature not enabled", + ))); + } + #[cfg(feature = "text-index")] + { + let crate::shard::dispatch::TextSearchPayload { + index_name, + query, + global_df, + global_n, + top_k, + offset, + count, + highlight_opts, + summarize_opts, + reply_tx, + } = *payload; + // DFS Phase 2: execute BM25 text search with global IDF injected by coordinator. + // The raw query bytes are re-parsed here with the recursive-descent parser so the + // full AST (OR, multi-@clause, grouping) is evaluated correctly on this shard. + // After scoring, apply HIGHLIGHT/SUMMARIZE post-processing if requested. Each shard + // post-processes against its own local hash store (no cross-shard read, no .await — + // safe to hold guards). text_store + databases[0] in one with_shard (multi-resource). + let response = { + crate::shard::slice::with_shard(|s| match s.text_store.get_index(&index_name) { + Some(text_index) => { + let mut result = + crate::command::vector_search::ft_text_search::run_text_query_on_index( text_index, - field_idx, - &query_terms, - &global_df, - global_n, + &query, + Some(&global_df), + Some(global_n), top_k, offset, count, ); - if highlight_opts.is_some() || summarize_opts.is_some() { - let db = &s.databases[0]; - crate::command::vector_search::ft_text_search::apply_post_processing( - &mut result, - &term_strings, - text_index, - db, - highlight_opts.as_ref(), - summarize_opts.as_ref(), - ); + if highlight_opts.is_some() || summarize_opts.is_some() { + // Re-parse to extract highlight terms for post-processing. + let term_strings = crate::text::query::parse_query( + &query, + &crate::text::query::QuerySchema::from_index(text_index), + ) + .map(|n| { + crate::text::query::collect_highlight_terms(&n, text_index) + }) + .unwrap_or_default(); + let db = &s.databases[0]; + crate::command::vector_search::ft_text_search::apply_post_processing( + &mut result, + &term_strings, + text_index, + db, + highlight_opts.as_ref(), + summarize_opts.as_ref(), + ); + } + result } - result - } - None => crate::protocol::Frame::Error(bytes::Bytes::from_static( - b"ERR unknown index", - )), - }) - }; - let _ = reply_tx.send(response); + None => crate::protocol::Frame::Error(bytes::Bytes::from_static( + b"ERR unknown index", + )), + }) + }; + let _ = reply_tx.send(response); + } } ShardMessage::VectorCommand { command, reply_tx } => { // All slice fields (vector_store, text_store, graph_store, databases) diff --git a/src/text/mod.rs b/src/text/mod.rs index 690761a7a..47fb68fa6 100644 --- a/src/text/mod.rs +++ b/src/text/mod.rs @@ -18,6 +18,8 @@ pub mod bm25; pub mod fst_dict; pub mod index_persist; pub mod posting; +#[cfg(feature = "text-index")] +pub mod query; pub mod store; pub mod term_dict; pub mod types; diff --git a/src/text/posting.rs b/src/text/posting.rs index fccc9ff97..34b07f3c7 100644 --- a/src/text/posting.rs +++ b/src/text/posting.rs @@ -42,6 +42,47 @@ impl PostingList { positions: None, } } + + /// 0-based index of `doc_id` within the rank-aligned parallel arrays. + /// + /// `RoaringBitmap::rank(d)` is the count of stored ids `<= d`, so for a present + /// `doc_id` it is the 1-based sorted position; subtract one for the array index. + /// Sub-linear (container-stride + popcount), unlike `iter().position()` (O(N)). + #[inline] + fn rank_index(&self, doc_id: u32) -> usize { + (self.doc_ids.rank(doc_id) as usize).saturating_sub(1) + } + + /// Term frequency of `doc_id` in this posting list. + /// + /// Returns the rank-aligned `term_freqs` entry when the doc is present, else `0` + /// (the `tf_absent` default — BM25 treats the term as not occurring). Never panics: + /// the rank-alignment invariant guarantees the index is valid, and a defensive + /// `get` degrades to `0` rather than indexing out of bounds. + #[inline] + pub fn tf(&self, doc_id: u32) -> u32 { + if !self.doc_ids.contains(doc_id) { + return 0; + } + self.term_freqs + .get(self.rank_index(doc_id)) + .copied() + .unwrap_or(0) + } + + /// Position list for `doc_id` (rank-aligned), or `None` when positions are not + /// tracked or the doc is absent. + #[inline] + pub fn positions_for(&self, doc_id: u32) -> Option<&[u32]> { + if !self.doc_ids.contains(doc_id) { + return None; + } + let idx = self.rank_index(doc_id); + self.positions + .as_ref() + .and_then(|p| p.get(idx)) + .map(Vec::as_slice) + } } /// Per-field inverted index storing term_id -> PostingList. @@ -75,42 +116,37 @@ impl PostingStore { }); if posting.doc_ids.contains(doc_id) { - // Upsert: find the index of this doc_id - let idx = posting.doc_ids.iter().position(|id| id == doc_id); - if let Some(idx) = idx { - posting.term_freqs[idx] += 1; - // Append positions if provided - if let Some(pos) = &positions { - if let Some(pos_list) = &mut posting.positions { - pos_list[idx].extend_from_slice(pos); - } else { - // Upgrade: create position tracking - let mut pos_list = vec![Vec::new(); posting.term_freqs.len()]; - pos_list[idx] = pos.clone(); - posting.positions = Some(pos_list); - } + // Existing doc: increment at the rank-aligned index. + let idx = posting.rank_index(doc_id); + posting.term_freqs[idx] += 1; + // Append positions if provided. + if let Some(pos) = &positions { + if let Some(pos_list) = &mut posting.positions { + pos_list[idx].extend_from_slice(pos); + } else { + // Upgrade: create position tracking, aligned to current docs. + let mut pos_list = vec![Vec::new(); posting.term_freqs.len()]; + pos_list[idx] = pos.clone(); + posting.positions = Some(pos_list); } } } else { - // New document + // New document: insert into the bitmap, then insert tf/positions AT THE RANK + // INDEX (not push) so term_freqs/positions stay rank-aligned with doc_ids — correct + // even when doc_id is not the current maximum (the document-update re-add path). posting.doc_ids.insert(doc_id); - posting.term_freqs.push(1); + let idx = posting.rank_index(doc_id); + posting.term_freqs.insert(idx, 1); match (&mut posting.positions, &positions) { - (Some(pos_list), Some(pos)) => { - pos_list.push(pos.clone()); - } - (Some(pos_list), None) => { - pos_list.push(Vec::new()); - } + (Some(pos_list), Some(pos)) => pos_list.insert(idx, pos.clone()), + (Some(pos_list), None) => pos_list.insert(idx, Vec::new()), (None, Some(pos)) => { - // Upgrade: create position tracking for all existing docs - let mut pos_list = vec![Vec::new(); posting.term_freqs.len() - 1]; - pos_list.push(pos.clone()); + // Upgrade: track positions for all docs; this doc's positions at idx. + let mut pos_list = vec![Vec::new(); posting.term_freqs.len()]; + pos_list[idx] = pos.clone(); posting.positions = Some(pos_list); } - (None, None) => { - // No positions, no change - } + (None, None) => {} } } } @@ -140,7 +176,9 @@ impl PostingStore { let mut removed = Vec::new(); for (&term_id, posting) in &mut self.postings { if posting.doc_ids.contains(doc_id) { - if let Some(idx) = posting.doc_ids.iter().position(|id| id == 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 { diff --git a/src/text/query/ast.rs b/src/text/query/ast.rs new file mode 100644 index 000000000..8fe09a69c --- /dev/null +++ b/src/text/query/ast.rs @@ -0,0 +1,74 @@ +//! FT.SEARCH query AST — the frozen `QueryNode` shape (task fts-query-combinators §3 @ v1). +//! +//! `parse_query` (see [`super::parse`]) turns a query string into a `QueryNode`; the evaluator +//! (task `fts-query-eval-dispatch`) folds it to a matched doc-id `RoaringBitmap`. Tokens are RAW +//! (un-analyzed) — analysis/stemming happens at eval time so the parser stays a pure function. + +use crate::text::store::TermModifier; +use bytes::Bytes; + +/// One node of the parsed FT.SEARCH query tree. +/// +/// `And`/`Or` children are evaluated by intersecting / unioning their matched doc-id sets. +/// A single-child `And`/`Or` is normalized away by the parser, so those vectors always hold ≥2 +/// children when present. `Empty` matches no document and is NOT an error (it is how an +/// intentionally-empty leaf — e.g. a stripped-to-nothing term — folds into set algebra). +#[derive(Debug, Clone, PartialEq)] +pub enum QueryNode { + /// A single term. `field` is the TEXT field index (into the index's text fields) when the + /// term was `@field:`-scoped, else `None` (default / all text fields). `token` is the RAW + /// term bytes (analyzed at eval time); `modifier` selects exact / fuzzy / prefix matching. + Term { + field: Option, + token: Bytes, + modifier: TermModifier, + }, + /// Intersection (implicit-AND / juxtaposition). Always ≥2 children. + And(Vec), + /// Union (the `|` operator). Always ≥2 children. + Or(Vec), + /// TAG membership filter `@field:{a|b}` — `values` are OR-unioned within the tag field. + Tag { field: Bytes, values: Vec }, + /// NUMERIC range filter `@field:[min max]`, inclusive unless the matching bound is exclusive. + Numeric { + field: Bytes, + min: f64, + max: f64, + min_excl: bool, + max_excl: bool, + }, + /// Matches ∅. Not an error — a valid empty leaf in the set algebra. + Empty, +} + +/// Parse failure — maps 1:1 to the wire error codes the dispatch layer emits as `Frame::Error`. +/// +/// The parser NEVER panics on malformed input (task M7): every malformed shape resolves to one +/// of these. The string forms are the contracted, RediSearch-adjacent error codes. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum QueryError { + /// Unbalanced `()` / `{}` / `[]`, or otherwise unparseable structure. + Syntax, + /// Empty query, or an empty group `()` with no terms. + EmptyQuery, + /// `@name:` where `name` is not a field in the index schema. Carries the offending name. + UnknownField(Bytes), + /// NUMERIC filter whose bounds are non-numeric, or `min > max`. + NumericInvalid, + /// TAG filter with no values (`{}` / `{ }`). + TagInvalid, +} + +impl QueryError { + /// The contracted wire error code (the byte string returned in `Frame::Error`). + #[inline] + pub fn code(&self) -> &'static str { + match self { + QueryError::Syntax => "syntax_error", + QueryError::EmptyQuery => "empty_query", + QueryError::UnknownField(_) => "unknown_field", + QueryError::NumericInvalid => "numeric_filter_invalid", + QueryError::TagInvalid => "tag_filter_invalid", + } + } +} diff --git a/src/text/query/eval.rs b/src/text/query/eval.rs new file mode 100644 index 000000000..e105570ef --- /dev/null +++ b/src/text/query/eval.rs @@ -0,0 +1,427 @@ +//! FT.SEARCH query evaluator — folds a parsed [`QueryNode`] AST to a matched doc-id set and a +//! best-effort BM25-scored result list (task `fts-query-eval-dispatch` 2b; contract inherited +//! from `fts-query-combinators` §3, FROZEN @ v1). +//! +//! Two layers: +//! * [`eval_set`] — the authoritative MEMBERSHIP. `And` = ∩, `Or` = ∪, `Empty` = ∅; leaves reuse +//! the index's own search machinery (`search_field` / `search_field_or` / `search_tag` / +//! `search_numeric_range`) and collect their doc-ids into a [`RoaringBitmap`]. Pure — set +//! membership is document-frequency-independent, so DFS weights do not enter here. +//! `eval_set(root).len()` is the "total matched" cardinality (the FROZEN boundary that +//! `fts-search-count-semantics` consumes) — kept `pub` for that task. +//! * [`eval_query`] — `eval_set` + best-effort scoring. TEXT leaves contribute BM25, summed +//! across OR branches and across leaves; docs matched only by TAG/NUMERIC score `0.0`. The +//! final order is score DESC, doc_id ASC (stable, deterministic). +//! +//! **Why per-leaf score summation is regression-safe.** BM25 is additive across query terms, and +//! `search_field` computes exactly Σ-over-terms. So for a single-field query the sum of per-leaf +//! BM25 contributions equals the old combined `search_field(&[t1, t2, …])` score — byte-identical +//! to the pre-2b path. Across multiple fields the sum is the RediSearch-correct cross-field score. +//! +//! Tokens in the AST are RAW (un-analyzed); analysis happens here so the parser stays pure. Analysis +//! reuses the index's per-field [`AnalyzerPipeline`] (the same one indexing used), so query and +//! document terms agree regardless of stemming. + +#![cfg(feature = "text-index")] + +use std::collections::HashMap; + +use bytes::Bytes; +use roaring::RoaringBitmap; + +use super::ast::QueryNode; +use crate::text::store::{TermModifier, TextIndex, TextSearchResult}; + +/// Fold the AST to the matched doc-id set (frozen §3 set-semantics). +/// +/// `And(xs)` intersects children (fold `&=`), `Or(xs)` unions them (fold `|=`), `Empty` is `∅`. +/// Leaves reuse the index's search machinery and collect doc-ids into the shared id space +/// (`ensure_doc_id`), so the set operations compose directly. +pub fn eval_set(node: &QueryNode, idx: &TextIndex) -> RoaringBitmap { + match node { + QueryNode::Empty => RoaringBitmap::new(), + + QueryNode::Term { + field, + token, + modifier, + } => term_results(idx, *field, token, modifier, None, None, usize::MAX) + .into_iter() + .map(|r| r.doc_id) + .collect(), + + QueryNode::Tag { field, values } => { + let mut bm = RoaringBitmap::new(); + for value in values { + for doc_id in idx.search_tag(field, value) { + bm.insert(doc_id); + } + } + bm + } + + QueryNode::Numeric { + field, + min, + max, + min_excl, + max_excl, + } => { + let mut bm = RoaringBitmap::new(); + for doc_id in idx.search_numeric_range(field, *min, *max, *min_excl, *max_excl) { + bm.insert(doc_id); + } + bm + } + + QueryNode::And(children) => { + let mut iter = children.iter(); + let Some(first) = iter.next() else { + return RoaringBitmap::new(); + }; + let mut acc = eval_set(first, idx); + for child in iter { + if acc.is_empty() { + break; // ∩ with anything stays empty — short-circuit. + } + acc &= &eval_set(child, idx); + } + acc + } + + QueryNode::Or(children) => { + let mut acc = RoaringBitmap::new(); + for child in children { + acc |= &eval_set(child, idx); + } + acc + } + } +} + +/// Evaluate the AST to a best-effort BM25-scored, ordered result list. +/// +/// 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`. +pub fn eval_query( + idx: &TextIndex, + node: &QueryNode, + global_df: Option<&HashMap>, + global_n: Option, + top_k: usize, +) -> Vec { + // 1. Authoritative membership (complete — no truncation). + let set = eval_set(node, idx); + if set.is_empty() { + return Vec::new(); + } + + // 2. Best-effort scores from TEXT leaves only. usize::MAX so every matched doc gets its true + // BM25 (search_field caps capacity by candidate count, not top_k — see store.rs), avoiding + // truncation-induced 0.0 scores for docs that are in the set but below a small top_k. + let mut scores: HashMap = HashMap::new(); + accumulate_text_scores(node, idx, global_df, global_n, &mut scores); + + // 3. Assemble results for docs in the set; pure-filter docs score 0.0. + let mut results: Vec = set + .iter() + .filter_map(|doc_id| { + idx.doc_id_to_key.get(&doc_id).map(|key| TextSearchResult { + doc_id, + key: key.clone(), + score: scores.get(&doc_id).copied().unwrap_or(0.0), + }) + }) + .collect(); + + // 4. Order: score DESC, doc_id ASC (stable, deterministic tie-break). + results.sort_by(|a, b| { + b.score + .partial_cmp(&a.score) + .unwrap_or(std::cmp::Ordering::Equal) + .then(a.doc_id.cmp(&b.doc_id)) + }); + results.truncate(top_k); + results +} + +/// Walk the AST, accumulating each TEXT leaf's BM25 contribution per doc into `scores`. TAG / +/// NUMERIC / Empty leaves contribute nothing (they score `0.0` by absence). Scores sum across OR +/// branches and across leaves — the additive-BM25 property keeps single-field queries identical to +/// the pre-2b combined score. +fn accumulate_text_scores( + node: &QueryNode, + idx: &TextIndex, + global_df: Option<&HashMap>, + global_n: Option, + scores: &mut HashMap, +) { + match node { + QueryNode::Empty | QueryNode::Tag { .. } | QueryNode::Numeric { .. } => {} + + QueryNode::Term { + field, + token, + modifier, + } => { + for r in term_results( + idx, + *field, + token, + modifier, + global_df, + global_n, + usize::MAX, + ) { + *scores.entry(r.doc_id).or_insert(0.0) += r.score; + } + } + + QueryNode::And(children) | QueryNode::Or(children) => { + for child in children { + accumulate_text_scores(child, idx, global_df, global_n, scores); + } + } + } +} + +/// Evaluate a single TEXT term leaf to scored results, reusing the index's search machinery. +/// +/// `field = Some(idx)` restricts to one text field; `None` searches all non-NOINDEX text fields +/// and sums per-doc scores across them (cross-field union — matches the pre-2b +/// `accumulate_cross_field` behaviour). Invalid UTF-8 in the raw token yields no matches (the +/// analyzer operates on `&str`); this never panics. +fn term_results( + idx: &TextIndex, + field: Option, + raw: &Bytes, + modifier: &TermModifier, + global_df: Option<&HashMap>, + global_n: Option, + top_k: usize, +) -> Vec { + let Ok(raw_str) = std::str::from_utf8(raw) else { + return Vec::new(); + }; + + match field { + Some(fidx) => { + term_results_in_field(idx, fidx, raw_str, modifier, global_df, global_n, top_k) + } + None => { + // Cross-field: union over non-NOINDEX text fields, summing per-doc scores. + let mut acc: HashMap = HashMap::new(); + for fidx in 0..idx.text_fields.len() { + if idx.text_fields[fidx].noindex { + continue; + } + for r in + term_results_in_field(idx, fidx, raw_str, modifier, global_df, global_n, top_k) + { + let entry = acc.entry(r.doc_id).or_insert((0.0, r.key.clone())); + entry.0 += r.score; + } + } + acc.into_iter() + .map(|(doc_id, (score, key))| TextSearchResult { doc_id, key, score }) + .collect() + } + } +} + +/// Evaluate a term leaf within a single text field. Exact terms run the field's full analyzer +/// (lowercase + NFKD + stem + stop-words) and AND-match via `search_field`; fuzzy/prefix terms are +/// lowercased + NFKD only (no stemming, per D-06/D-07), expanded via `expand_terms`, then OR-matched +/// via `search_field_or`. +fn term_results_in_field( + idx: &TextIndex, + fidx: usize, + raw_str: &str, + modifier: &TermModifier, + global_df: Option<&HashMap>, + global_n: Option, + top_k: usize, +) -> Vec { + match modifier { + TermModifier::Exact => { + // A single raw token may analyze to 0 terms (stop word) or >1 (rare); search_field + // AND-matches them, consistent with the pre-2b exact path. + let terms = analyze_raw(idx, fidx, raw_str, modifier); + if terms.is_empty() { + return Vec::new(); + } + idx.search_field(fidx, &terms, global_df, global_n, top_k) + } + + TermModifier::Fuzzy(_) | TermModifier::Prefix => { + let normalized = analyze_raw(idx, fidx, raw_str, modifier); + let Some(normalized) = normalized.first() else { + return Vec::new(); + }; + let ids = idx.expand_terms(fidx, normalized, modifier); + if ids.is_empty() { + return Vec::new(); + } + idx.search_field_or(fidx, &ids, global_df, global_n, top_k) + } + } +} + +/// Analyze a raw query token against one field, returning the analyzed term string(s). +/// +/// `Exact` runs the field's full analyzer (lowercase + NFKD + stem + stop-words) and may yield 0 +/// terms (stop word) or >1; fuzzy/prefix terms are lowercased + NFKD only (no stemming, D-06/D-07) +/// and yield a single normalized string. Centralizes the query-time analysis reused by the leaf +/// evaluator and the DFS / HIGHLIGHT term collectors so they cannot drift. +fn analyze_raw( + idx: &TextIndex, + fidx: usize, + raw_str: &str, + modifier: &TermModifier, +) -> Vec { + match modifier { + TermModifier::Exact => match idx.field_analyzers.get(fidx) { + Some(analyzer) => analyzer + .tokenize_with_positions(raw_str) + .into_iter() + .map(|(term, _pos)| term) + .collect(), + None => Vec::new(), + }, + TermModifier::Fuzzy(_) | TermModifier::Prefix => { + use unicode_normalization::UnicodeNormalization; + let normalized: String = raw_str.to_lowercase().nfkd().collect(); + if normalized.is_empty() { + Vec::new() + } else { + vec![normalized] + } + } + } +} + +/// The fields a term leaf scores against: its scoped field, or every non-NOINDEX text field. +fn leaf_fields(idx: &TextIndex, field: Option) -> Vec { + match field { + Some(fidx) => vec![fidx], + None => (0..idx.text_fields.len()) + .filter(|&i| !idx.text_fields[i].noindex) + .collect(), + } +} + +/// Collect the analyzed text-term strings a query matches on, for HIGHLIGHT / SUMMARIZE +/// post-processing (which highlights the matched terms in document bodies). Walks every TEXT leaf +/// (TAG / NUMERIC contribute no highlightable text), analyzes each against its field(s), and returns +/// the de-duplicated term strings. Invalid-UTF8 tokens yield nothing (never panics). +pub fn collect_highlight_terms(node: &QueryNode, idx: &TextIndex) -> Vec { + let mut out: Vec = Vec::new(); + collect_highlight_terms_inner(node, idx, &mut out); + out.sort_unstable(); + out.dedup(); + out +} + +fn collect_highlight_terms_inner(node: &QueryNode, idx: &TextIndex, out: &mut Vec) { + match node { + QueryNode::Empty | QueryNode::Tag { .. } | QueryNode::Numeric { .. } => {} + QueryNode::Term { + field, + token, + modifier, + } => { + let Ok(raw_str) = std::str::from_utf8(token) else { + return; + }; + for fidx in leaf_fields(idx, *field) { + out.extend(analyze_raw(idx, fidx, raw_str, modifier)); + } + } + QueryNode::And(children) | QueryNode::Or(children) => { + for child in children { + collect_highlight_terms_inner(child, idx, out); + } + } + } +} + +/// Collect the analyzed EXACT terms a query needs document frequencies for, for the DFS Phase-1 +/// scatter (`doc_freq_for_terms`). +/// +/// Returns **at most one** `(field_hint, terms)` entry. This is a hard invariant: each shard emits +/// one `"N"` (total-doc-count) sentinel **per entry**, and `aggregate_doc_freq` SUMS the `"N"`s +/// across shards — so more than one entry inflates the global N (wrong IDF), and zero entries when +/// the query has text leaves zeroes it (broken IDF). Therefore: +/// * No text leaf at all (pure TAG/NUMERIC/Empty) → `[]` — no text scoring, so no N is needed. +/// * Any text leaf (exact OR fuzzy/prefix) → exactly one entry, so N is gathered once. `terms` +/// holds only `Exact` analyzed terms (fuzzy/prefix score via `search_field_or`, which uses LOCAL +/// df — excluded, matching pre-2b). `field_hint` is the common field when every text leaf is the +/// same `@field`, else `None` (df counted against field 0 — the pre-2b cross-field behaviour). +pub fn collect_df_field_terms( + node: &QueryNode, + idx: &TextIndex, +) -> Vec<(Option, Vec)> { + let mut acc = DfAcc { + has_text: false, + single_field: None, + consistent: true, + terms: Vec::new(), + }; + collect_df_terms_inner(node, idx, &mut acc); + if !acc.has_text { + return Vec::new(); + } + // field_hint = the single shared field, only if every text leaf agreed on it. + let field_hint = if acc.consistent { + acc.single_field + } else { + None + }; + acc.terms.sort_unstable(); + acc.terms.dedup(); + vec![(field_hint, acc.terms)] +} + +/// Accumulator enforcing the single-entry invariant of [`collect_df_field_terms`]. +struct DfAcc { + has_text: bool, + single_field: Option, + consistent: bool, + terms: Vec, +} + +fn collect_df_terms_inner(node: &QueryNode, idx: &TextIndex, acc: &mut DfAcc) { + match node { + QueryNode::Empty | QueryNode::Tag { .. } | QueryNode::Numeric { .. } => {} + QueryNode::Term { + field, + token, + modifier, + } => { + acc.has_text = true; + // Track field consistency for the single N-gathering hint. + match field { + None => acc.consistent = false, // cross-field leaf → hint must be None (field 0) + Some(f) => match acc.single_field { + None => acc.single_field = Some(*f), + Some(prev) if prev != *f => acc.consistent = false, // mixed fields → None + _ => {} + }, + } + // Only Exact terms use global IDF (fuzzy/prefix → local df via search_field_or). Their + // presence still counts for `has_text` (so N is gathered) but they add no df terms. + if matches!(modifier, TermModifier::Exact) { + if let Ok(raw_str) = std::str::from_utf8(token) { + acc.terms + .extend(analyze_raw(idx, field.unwrap_or(0), raw_str, modifier)); + } + } + } + QueryNode::And(children) | QueryNode::Or(children) => { + for child in children { + collect_df_terms_inner(child, idx, acc); + } + } + } +} diff --git a/src/text/query/mod.rs b/src/text/query/mod.rs new file mode 100644 index 000000000..e6f5a884e --- /dev/null +++ b/src/text/query/mod.rs @@ -0,0 +1,17 @@ +//! FT.SEARCH query parsing — the recursive-descent parser + AST (task fts-query-combinators 2a). +//! +//! `parse_query(bytes, schema)` turns a query string into a [`QueryNode`] tree over the frozen +//! RediSearch subset (terms+modifiers · implicit-AND · OR `|` · grouping · `@field:` clauses · +//! TAG `{a|b}` · NUMERIC `[min max]`), with DIALECT-2 precedence and five named error codes. +//! The companion evaluator (`fts-query-eval-dispatch`, 2b) folds a `QueryNode` to a matched +//! doc-id `RoaringBitmap`; `fts-search-count-semantics` then counts that set's cardinality. +//! +//! The parser is a pure function (no index/analyzer): tokens are RAW and analyzed at eval time. + +mod ast; +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 parse::{QuerySchema, parse_query}; diff --git a/src/text/query/parse.rs b/src/text/query/parse.rs new file mode 100644 index 000000000..0094412c7 --- /dev/null +++ b/src/text/query/parse.rs @@ -0,0 +1,455 @@ +//! Recursive-descent parser for the FT.SEARCH query subset (task fts-query-combinators 2a). +//! +//! Grammar (frozen §3 @ v1, RediSearch DIALECT-2 precedence — AND > modifiers > OR): +//! ```text +//! query = union (empty -> EmptyQuery) +//! union = intersect ( '|' intersect )* (OR — lowest precedence) +//! intersect = factor+ (implicit AND by juxtaposition) +//! factor = group | field_clause | term +//! group = '(' union ')' +//! field_clause = '@' name ':' ( tag | numeric | group | term+ ) (field pushed onto leaf Terms) +//! tag = '{' value ( '|' value )* '}' +//! numeric = '[' bound bound ']' (bound may be '(' -prefixed exclusive; ±inf ok) +//! term = WORD ( '*' prefix | wrapped in %..% fuzzy )? +//! ``` +//! Pure: bytes + schema -> AST. Tokens are RAW; analysis happens in the evaluator (2b). The parser +//! NEVER panics (M7): every malformed shape resolves to a [`QueryError`]. Numeric-bound parsing is +//! re-implemented locally (≈ the command-layer `parse_numeric_bound`) so `text/` keeps NO upward +//! dependency on `command/` — same exclusive/±inf semantics, asserted by `test_numeric_exclusive_and_inf`. + +use super::ast::{QueryError, QueryNode}; +use crate::text::store::TermModifier; +use bytes::Bytes; + +/// Max parenthesis-nesting depth — guards the recursive descent against stack exhaustion on +/// pathological input like `((((((…))))))`. Exceeding it is a `Syntax` error, never a crash. +const MAX_DEPTH: usize = 64; + +/// Resolved kind of an `@field` reference. +#[derive(Clone, Copy)] +enum FieldRef { + /// A TEXT field at this index (into the index's text fields) — needed by `search_field`. + Text(usize), + Tag, + Numeric, +} + +/// Field-name → kind resolver for the parser. Built from an index (`from_index`) in production or +/// from plain names (`from_names`) in tests. Resolution is ASCII-case-insensitive, matching the +/// rest of the text engine (`@Status` resolves `status`). +pub struct QuerySchema { + text: Vec, + tag: Vec, + numeric: Vec, +} + +impl QuerySchema { + /// Build from field-name string slices (test/general construction). + pub fn from_names(text: &[&str], tag: &[&str], numeric: &[&str]) -> Self { + let conv = |xs: &[&str]| { + xs.iter() + .map(|s| Bytes::copy_from_slice(s.as_bytes())) + .collect() + }; + Self { + text: conv(text), + tag: conv(tag), + numeric: conv(numeric), + } + } + + /// Build from a live `TextIndex`'s declared field defs. + pub fn from_index(idx: &crate::text::store::TextIndex) -> Self { + Self { + text: idx + .text_fields + .iter() + .map(|f| f.field_name.clone()) + .collect(), + tag: idx + .tag_fields + .iter() + .map(|f| f.field_name.clone()) + .collect(), + numeric: idx + .numeric_fields + .iter() + .map(|f| f.field_name.clone()) + .collect(), + } + } + + fn resolve(&self, name: &[u8]) -> Option { + if let Some(i) = self + .text + .iter() + .position(|f| f.as_ref().eq_ignore_ascii_case(name)) + { + return Some(FieldRef::Text(i)); + } + if self + .tag + .iter() + .any(|f| f.as_ref().eq_ignore_ascii_case(name)) + { + return Some(FieldRef::Tag); + } + if self + .numeric + .iter() + .any(|f| f.as_ref().eq_ignore_ascii_case(name)) + { + return Some(FieldRef::Numeric); + } + None + } +} + +/// Parse an FT.SEARCH query string into a [`QueryNode`] AST. +/// +/// Returns `Err(QueryError::EmptyQuery)` for blank input, and the matching named error for any +/// malformed structure. Never panics. +pub fn parse_query(input: &[u8], schema: &QuerySchema) -> Result { + let mut p = Parser { + input, + pos: 0, + schema, + }; + p.skip_ws(); + if p.at_end() { + return Err(QueryError::EmptyQuery); + } + let node = p.parse_union(0)?; + p.skip_ws(); + if !p.at_end() { + // Trailing junk (e.g. a stray `)`) — unbalanced structure. + return Err(QueryError::Syntax); + } + Ok(node) +} + +struct Parser<'a> { + input: &'a [u8], + pos: usize, + schema: &'a QuerySchema, +} + +#[inline] +fn is_term_byte(b: u8) -> bool { + !b.is_ascii_whitespace() && !matches!(b, b'|' | b'(' | b')' | b'{' | b'}' | b'[' | b']' | b'@') +} + +impl Parser<'_> { + #[inline] + fn peek(&self) -> Option { + self.input.get(self.pos).copied() + } + + #[inline] + fn at_end(&self) -> bool { + self.pos >= self.input.len() + } + + #[inline] + fn skip_ws(&mut self) { + while let Some(b) = self.peek() { + if b.is_ascii_whitespace() { + self.pos += 1; + } else { + break; + } + } + } + + /// union = intersect ( '|' intersect )* + fn parse_union(&mut self, depth: usize) -> Result { + let mut branches = vec![self.parse_intersect(depth)?]; + loop { + self.skip_ws(); + if self.peek() == Some(b'|') { + self.pos += 1; + branches.push(self.parse_intersect(depth)?); + } else { + break; + } + } + Ok(if branches.len() == 1 { + branches.pop().unwrap_or(QueryNode::Empty) + } else { + QueryNode::Or(branches) + }) + } + + /// intersect = factor+ (at least one factor required) + fn parse_intersect(&mut self, depth: usize) -> Result { + let mut factors = Vec::new(); + loop { + self.skip_ws(); + match self.parse_factor(depth)? { + Some(node) => factors.push(node), + None => break, + } + } + match factors.len() { + 0 => Err(QueryError::Syntax), // empty branch (e.g. "a |", "| b") + 1 => Ok(factors.pop().unwrap_or(QueryNode::Empty)), + _ => Ok(QueryNode::And(factors)), + } + } + + /// factor = group | field_clause | term ; returns None at a boundary ('|', ')', end). + fn parse_factor(&mut self, depth: usize) -> Result, QueryError> { + self.skip_ws(); + match self.peek() { + None => Ok(None), + Some(b'|') | Some(b')') => Ok(None), + Some(b'(') => { + let node = self.parse_group(depth)?; + Ok(Some(node)) + } + Some(b'@') => Ok(Some(self.parse_field_clause(depth)?)), + // A tag/numeric delimiter or stray closer with no field in front is malformed. + Some(b'{') | Some(b'}') | Some(b'[') | Some(b']') => Err(QueryError::Syntax), + Some(_) => Ok(Some(self.parse_term(None)?)), + } + } + + /// group = '(' union ')' — an empty group "()" is EmptyQuery. + fn parse_group(&mut self, depth: usize) -> Result { + if depth + 1 >= MAX_DEPTH { + return Err(QueryError::Syntax); + } + self.pos += 1; // consume '(' + self.skip_ws(); + if self.peek() == Some(b')') { + self.pos += 1; + return Err(QueryError::EmptyQuery); + } + let node = self.parse_union(depth + 1)?; + self.skip_ws(); + if self.peek() != Some(b')') { + return Err(QueryError::Syntax); + } + self.pos += 1; // consume ')' + Ok(node) + } + + /// field_clause = '@' name ':' ( tag | numeric | group | term+ ) + fn parse_field_clause(&mut self, depth: usize) -> Result { + self.pos += 1; // consume '@' + let start = self.pos; + while let Some(b) = self.peek() { + if is_term_byte(b) && b != b':' { + self.pos += 1; + } else { + break; + } + } + let name = &self.input[start..self.pos]; + if name.is_empty() || self.peek() != Some(b':') { + return Err(QueryError::Syntax); + } + self.pos += 1; // consume ':' + let field = match self.schema.resolve(name) { + Some(f) => f, + None => return Err(QueryError::UnknownField(Bytes::copy_from_slice(name))), + }; + self.skip_ws(); + match (self.peek(), field) { + (Some(b'{'), FieldRef::Tag) => self.parse_tag(name), + (Some(b'['), FieldRef::Numeric) => self.parse_numeric(name), + (Some(b'('), FieldRef::Text(idx)) => { + let mut node = self.parse_group(depth)?; + push_field(&mut node, idx); + Ok(node) + } + (Some(b), FieldRef::Text(idx)) if is_term_byte(b) => { + // term+ : consecutive bare terms all scoped to this text field. + let mut terms = vec![self.parse_term(Some(idx))?]; + loop { + self.skip_ws(); + match self.peek() { + Some(b) if is_term_byte(b) => terms.push(self.parse_term(Some(idx))?), + _ => break, + } + } + Ok(if terms.len() == 1 { + terms.pop().unwrap_or(QueryNode::Empty) + } else { + QueryNode::And(terms) + }) + } + // wrong delimiter for the field's kind, or nothing after ':'. + _ => Err(QueryError::Syntax), + } + } + + /// tag = '{' value ( '|' value )* '}' — empty / blank values -> TagInvalid. + fn parse_tag(&mut self, field: &[u8]) -> Result { + self.pos += 1; // consume '{' + let start = self.pos; + while let Some(b) = self.peek() { + if b == b'}' { + break; + } + self.pos += 1; + } + if self.peek() != Some(b'}') { + return Err(QueryError::Syntax); // unbalanced + } + let body = &self.input[start..self.pos]; + self.pos += 1; // consume '}' + let mut values = Vec::new(); + for raw in body.split(|&b| b == b'|') { + let v = trim(raw); + if v.is_empty() { + return Err(QueryError::TagInvalid); + } + values.push(Bytes::copy_from_slice(v)); + } + if values.is_empty() { + return Err(QueryError::TagInvalid); + } + Ok(QueryNode::Tag { + field: Bytes::copy_from_slice(field), + values, + }) + } + + /// numeric = '[' bound bound ']' — exactly two bounds; non-numeric / min>max -> NumericInvalid. + fn parse_numeric(&mut self, field: &[u8]) -> Result { + self.pos += 1; // consume '[' + let start = self.pos; + while let Some(b) = self.peek() { + if b == b']' { + break; + } + self.pos += 1; + } + if self.peek() != Some(b']') { + return Err(QueryError::Syntax); // unbalanced + } + let body = &self.input[start..self.pos]; + self.pos += 1; // consume ']' + let parts: Vec<&[u8]> = body + .split(|b| b.is_ascii_whitespace()) + .filter(|s| !s.is_empty()) + .collect(); + if parts.len() != 2 { + return Err(QueryError::NumericInvalid); + } + let (min, min_excl) = parse_bound(parts[0]).ok_or(QueryError::NumericInvalid)?; + let (max, max_excl) = parse_bound(parts[1]).ok_or(QueryError::NumericInvalid)?; + if min > max { + return Err(QueryError::NumericInvalid); + } + Ok(QueryNode::Numeric { + field: Bytes::copy_from_slice(field), + min, + max, + min_excl, + max_excl, + }) + } + + /// term = WORD with an optional prefix '*' or %..% fuzzy wrapper. `field` scopes it. + fn parse_term(&mut self, field: Option) -> Result { + self.skip_ws(); + let start = self.pos; + while let Some(b) = self.peek() { + if is_term_byte(b) { + self.pos += 1; + } else { + break; + } + } + let word = &self.input[start..self.pos]; + if word.is_empty() { + return Err(QueryError::Syntax); + } + let (token, modifier) = classify_term(word)?; + Ok(QueryNode::Term { + field, + token: Bytes::copy_from_slice(token), + modifier, + }) + } +} + +/// Push `idx` onto every still-unscoped `Term` inside `node` (field-scoped group `@f:(…)`). +fn push_field(node: &mut QueryNode, idx: usize) { + match node { + QueryNode::Term { field, .. } => { + if field.is_none() { + *field = Some(idx); + } + } + QueryNode::And(children) | QueryNode::Or(children) => { + for c in children { + push_field(c, idx); + } + } + QueryNode::Tag { .. } | QueryNode::Numeric { .. } | QueryNode::Empty => {} + } +} + +/// Split a raw WORD into (raw token, modifier). Fuzzy `%t%`/`%%t%%`/`%%%t%%%` (distance = % count), +/// prefix `t*`, else exact. Empty/malformed -> Syntax. +fn classify_term(word: &[u8]) -> Result<(&[u8], TermModifier), QueryError> { + // Fuzzy: symmetric leading/trailing '%' (1..=3). + if word.first() == Some(&b'%') { + let lead = word.iter().take_while(|&&b| b == b'%').count(); + let trail = word.iter().rev().take_while(|&&b| b == b'%').count(); + if lead == trail && (1..=3).contains(&lead) && word.len() > 2 * lead { + let inner = &word[lead..word.len() - lead]; + if inner.iter().all(|&b| b != b'%') { + return Ok((inner, TermModifier::Fuzzy(lead as u8))); + } + } + return Err(QueryError::Syntax); + } + // Prefix: trailing '*'. + if word.last() == Some(&b'*') { + let inner = &word[..word.len() - 1]; + if inner.is_empty() || inner.contains(&b'*') { + return Err(QueryError::Syntax); + } + return Ok((inner, TermModifier::Prefix)); + } + Ok((word, TermModifier::Exact)) +} + +/// Parse a numeric bound: optional leading `(` (exclusive), then a finite number or ±inf. +/// Returns `None` on a non-numeric / NaN bound. Mirrors the command-layer `parse_numeric_bound` +/// semantics without depending on it (layering: `text/` must not import from `command/`). +fn parse_bound(s: &[u8]) -> Option<(f64, bool)> { + let (excl, rest) = if s.first() == Some(&b'(') { + (true, &s[1..]) + } else { + (false, s) + }; + let t = std::str::from_utf8(rest).ok()?.trim(); + let lower = t.to_ascii_lowercase(); + let v = match lower.as_str() { + "inf" | "+inf" | "infinity" | "+infinity" => f64::INFINITY, + "-inf" | "-infinity" => f64::NEG_INFINITY, + _ => t.parse::().ok()?, + }; + if v.is_nan() { + return None; + } + Some((v, excl)) +} + +#[inline] +fn trim(s: &[u8]) -> &[u8] { + let mut a = 0; + let mut b = s.len(); + while a < b && s[a].is_ascii_whitespace() { + a += 1; + } + while b > a && s[b - 1].is_ascii_whitespace() { + b -= 1; + } + &s[a..b] +} diff --git a/src/text/store.rs b/src/text/store.rs index 3b82902dd..b255a83f8 100644 --- a/src/text/store.rs +++ b/src/text/store.rs @@ -499,14 +499,11 @@ impl TextIndex { .get_posting(*term_id) .expect("posting exists: checked above"); - // Per RESEARCH Pitfall 1: use linear scan (not rank()) for correct TF lookup. - // term_freqs is in insertion order, NOT sorted doc_id order. - let tf = posting - .doc_ids - .iter() - .position(|id| id == doc_id) - .map(|idx| posting.term_freqs[idx] as f32) - .unwrap_or(0.0); + // 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 + // old linear scan is gone — see PostingList::tf. (Supersedes the former + // "RESEARCH Pitfall 1" note, which assumed insertion-order term_freqs.) + let tf = posting.tf(doc_id) as f32; // Use global_df if provided (DFS path), else local doc frequency. let df = global_df @@ -766,13 +763,8 @@ impl TextIndex { continue; } - // Linear scan TF lookup (same as search_field — insertion order, not rank). - let tf = posting - .doc_ids - .iter() - .position(|id| id == doc_id) - .map(|idx| posting.term_freqs[idx] as f32) - .unwrap_or(0.0); + // Rank-aligned TF lookup (fts-posting-rank-tf) — same as search_field. + let tf = posting.tf(doc_id) as f32; // Use local posting list df for expanded term IDs. let df = posting.doc_ids.len() as u32; diff --git a/tests/fts_posting_rank_tf.rs b/tests/fts_posting_rank_tf.rs new file mode 100644 index 000000000..dfe822469 --- /dev/null +++ b/tests/fts_posting_rank_tf.rs @@ -0,0 +1,218 @@ +//! RED tests for `fts-posting-rank-tf` (milestone v3-1-fts-hardening). +//! +//! Contract FROZEN @ v1 (`.add/tasks/fts-posting-rank-tf/TASK.md` §3): `PostingList`'s +//! `term_freqs` / `positions` are RANK-aligned to the sorted `doc_ids` bitmap, and +//! `PostingList::tf(doc_id)` is a sub-linear, correct term-frequency lookup that survives +//! document updates. +//! +//! TDD — these MUST be RED before build: +//! * unit tests are red because `tf()` / `positions_for()` don't exist yet, AND because a +//! naive `position()`-over-insertion-order lookup returns wrong values (6,3,1 vs 5,2,3); +//! * integration tests are red because the current push-to-end write (posting.rs:96-97) +//! misaligns `term_freqs` after a document update, corrupting BM25 ranking (TASK.md §1 A1). +//! +//! The bug only manifests with DISTINCT term frequencies — equal tfs mask the misalignment, +//! so every scenario below uses distinct counts. +#![cfg(feature = "text-index")] + +use bytes::Bytes; +use moon::protocol::Frame; +use moon::text::posting::PostingStore; +use moon::text::store::TextIndex; +use moon::text::types::{BM25Config, TextFieldDef}; + +// ─────────────────────────── unit: rank-aligned TF contract ─────────────────────────── + +/// Add `n` occurrences of `term_id` for `doc_id` (simulates first-index tf=n). +fn add_n(store: &mut PostingStore, term_id: u32, doc_id: u32, n: u32) { + for _ in 0..n { + store.add_term_occurrence(term_id, doc_id, None); + } +} + +/// M1 / A1 — the headline correctness fix. +#[test] +fn test_tf_correct_after_low_id_update() { + // term 1: doc0 ×1, doc1 ×2, doc2 ×3 — ascending insert (aligned today). + let mut store = PostingStore::new(); + add_n(&mut store, 1, 0, 1); + add_n(&mut store, 1, 1, 2); + add_n(&mut store, 1, 2, 3); + // Update the LOWEST-id doc: remove then re-index with ×5 (the misaligning path). + store.remove_doc(0); + add_n(&mut store, 1, 0, 5); + + let p = store.get_posting(1).expect("posting exists"); + // CONTRACT: tf(doc_id) is the TRUE term frequency, regardless of insert/update order. + // A naive position()-over-insertion-order lookup reads (6, 3, 1) here — the bug. + assert_eq!( + p.tf(0), + 5, + "updated low-id doc must read its own tf, not a neighbour's" + ); + assert_eq!( + p.tf(1), + 2, + "doc1 tf must be untouched by doc0's re-insertion" + ); + assert_eq!( + p.tf(2), + 3, + "doc2 tf must be untouched by doc0's re-insertion" + ); +} + +/// Reject — `tf_absent` defined default. +#[test] +fn test_tf_absent_is_zero() { + let mut store = PostingStore::new(); + add_n(&mut store, 1, 0, 2); + let p = store.get_posting(1).expect("posting exists"); + assert_eq!( + p.tf(999), + 0, + "absent doc_id -> tf 0 (no panic, no neighbour leak)" + ); +} + +/// M5 — positions stay aligned with TF after an update. +#[test] +fn test_positions_aligned_after_update() { + let mut store = PostingStore::new(); + store.add_term_occurrence(1, 0, Some(vec![0])); + store.add_term_occurrence(1, 1, Some(vec![0])); + store.add_term_occurrence(1, 2, Some(vec![0])); + // Update low-id doc0 with 5 occurrences at distinct positions. + store.remove_doc(0); + store.add_term_occurrence(1, 0, Some(vec![0, 1, 2, 3, 4])); + + let p = store.get_posting(1).expect("posting exists"); + let pos0 = p.positions_for(0).expect("positions tracked for doc0"); + assert_eq!( + pos0, + &[0, 1, 2, 3, 4][..], + "doc0 positions must belong to doc0 after update" + ); +} + +/// M2 — TF lookup is sub-linear: many lookups on a large posting must not be O(N) each. +#[test] +fn test_high_df_tf_lookup_is_sublinear() { + // One term in 50_000 docs (ascending). A linear .position() lookup is O(N) per call; + // rank() is sub-linear. Best-of-K guard (runner-noise tolerant, like perf_v0112). + const N: u32 = 50_000; + let mut store = PostingStore::new(); + for d in 0..N { + store.add_term_occurrence(1, d, None); + } + let p = store.get_posting(1).expect("posting exists"); + + // Sample lookups spread across the posting; assert correctness AND a sub-linear budget. + let mut best = std::time::Duration::MAX; + for _ in 0..5 { + let t = std::time::Instant::now(); + let mut acc: u64 = 0; + let mut d = 0u32; + while d < N { + acc += p.tf(d) as u64; // each tf is 1 here + d += 7; // ~7_143 lookups + } + std::hint::black_box(acc); + best = best.min(t.elapsed()); + if best < std::time::Duration::from_millis(50) { + break; + } + } + // ~7k rank-lookups over 50k docs must finish well under a linear-scan budget. + assert!( + best < std::time::Duration::from_millis(50), + "high-DF TF lookups took {best:?} — expected sub-linear (rank), not O(N) per call" + ); +} + +// ─────────────────────── integration: BM25 ranking through the store ─────────────────────── + +fn make_index() -> TextIndex { + let field = TextFieldDef::new(Bytes::from_static(b"body")); + TextIndex::new( + Bytes::from_static(b"idx"), + Vec::new(), + vec![field], + BM25Config::default(), + ) +} + +fn idx_doc(idx: &mut TextIndex, key_hash: u64, key: &str, text: &str) { + let args = vec![ + Frame::BulkString(Bytes::from_static(b"body")), + Frame::BulkString(Bytes::copy_from_slice(text.as_bytes())), + ]; + idx.index_document(key_hash, key.as_bytes(), &args); +} + +fn result_keys(results: &[moon::text::store::TextSearchResult]) -> Vec { + results + .iter() + .map(|r| String::from_utf8_lossy(r.key.as_ref()).into_owned()) + .collect() +} + +/// M3 (search_field / AND path) + M1 end-to-end — ranking must reflect the TRUE tfs after an update. +#[test] +fn test_search_field_ranking_correct_after_update() { + // alpha counts: a=1 (doc_id 0), b=2 (1), c=3 (2). Then update a -> 5. + let mut idx = make_index(); + idx_doc(&mut idx, 0, "a", "alpha"); + idx_doc(&mut idx, 1, "b", "alpha alpha"); + idx_doc(&mut idx, 2, "c", "alpha alpha alpha"); + idx_doc(&mut idx, 0, "a", "alpha alpha alpha alpha alpha"); // update a -> tf 5 + + let results = idx.search_field(0, &["alpha".to_string()], None, None, 10); + // TRUE BM25 order (len-normalised): a(tf5,dl5) > c(tf3,dl3) > b(tf2,dl2) -> [a, c, b]. + // The misalignment bug reads tf (6,3,1) -> order [b, a, c]. Either way, a must rank first. + let keys = result_keys(&results); + assert_eq!( + keys, + vec!["a", "c", "b"], + "ranking must reflect TRUE tfs (a=5,c=3,b=2)" + ); +} + +/// M3 (search_field_or / OR-expanded path) — the second read site uses the same correct lookup. +#[test] +fn test_search_field_or_ranking_correct_after_update() { + let mut idx = make_index(); + idx_doc(&mut idx, 0, "a", "alpha"); + idx_doc(&mut idx, 1, "b", "alpha alpha"); + idx_doc(&mut idx, 2, "c", "alpha alpha alpha"); + idx_doc(&mut idx, 0, "a", "alpha alpha alpha alpha alpha"); + + // Resolve the indexed term_id for "alpha" (whatever stem the analyzer produced). + let term_id = idx.field_term_dicts[0] + .get("alpha") + .expect("term 'alpha' indexed"); + let results = idx.search_field_or(0, &[term_id], None, None, 10); + let keys = result_keys(&results); + assert_eq!( + keys.first().map(String::as_str), + Some("a"), + "OR path: a (tf=5) must rank first" + ); +} + +/// M4 — no regression on the already-correct ascending-insert path (must STAY green). +#[test] +fn test_ascending_insert_ranking_unchanged() { + let mut idx = make_index(); + idx_doc(&mut idx, 0, "a", "alpha alpha alpha"); // tf 3 + idx_doc(&mut idx, 1, "b", "alpha alpha"); // tf 2 + idx_doc(&mut idx, 2, "c", "alpha"); // tf 1 + // No updates — the path the old code already handled correctly. + let results = idx.search_field(0, &["alpha".to_string()], None, None, 10); + let keys = result_keys(&results); + assert_eq!( + keys, + vec!["a", "b", "c"], + "ascending-insert ranking by tf must be stable" + ); +} diff --git a/tests/fts_query_eval_e2e.rs b/tests/fts_query_eval_e2e.rs new file mode 100644 index 000000000..6accea3fc --- /dev/null +++ b/tests/fts_query_eval_e2e.rs @@ -0,0 +1,496 @@ +//! RED end-to-end tests for `fts-query-eval-dispatch` task 2b — evaluating the parsed +//! `QueryNode` AST (from 2a) to a matched doc-id SET and wiring it into FT.SEARCH dispatch. +//! +//! Contract INHERITED from `fts-query-combinators` §3 (FROZEN @ v1): the dispatch path runs +//! `parse_query → eval_query → build_text_response`, so OR unions, multi-`@clause` intersections, +//! and grouping return correct result sets. This suite asserts RESULT SETS + ordering + coded +//! errors at the wire boundary (the parse-tree level is 2a's `tests/fts_query_parse.rs`). +//! +//! Two new symbols are exercised (both absent until 2b builds them → this suite is RED by +//! compile-failure, the correct TDD red state): +//! * `moon::text::query::eval_query(idx, node, gdf, gn, top_k) -> Vec` +//! — the centralized evaluator kernel (frozen §3 set-semantics + best-effort BM25). +//! * `moon::command::vector_search::run_text_query(store, index, raw, top_k, off, count) -> Frame` +//! — the dispatch wrapper every handler text branch routes through (parse + eval + respond, +//! Err → Frame::Error with the frozen wire code). +//! +//! Distinct corpora are used so union ≠ intersection is observable (the old AND/0-result bugs). +#![cfg(feature = "text-index")] + +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::store::{TextIndex, TextStore}; +use moon::text::types::{BM25Config, NumericFieldDef, TagFieldDef, TextFieldDef}; + +// ── corpus construction ───────────────────────────────────────────────────── + +/// A schema with TEXT body(0)+title(1), TAG "tag", NUMERIC "price". +fn empty_index() -> TextIndex { + TextIndex::new_with_schema( + Bytes::from_static(b"idx"), + Vec::new(), + vec![ + TextFieldDef::new(Bytes::from_static(b"body")), + TextFieldDef::new(Bytes::from_static(b"title")), + ], + vec![TagFieldDef::new(Bytes::from_static(b"tag"))], + vec![NumericFieldDef::new(Bytes::from_static(b"price"))], + BM25Config::default(), + ) +} + +/// Flatten `[(field,value)]` into HSET-style Frame args. +fn args(pairs: &[(&str, &str)]) -> Vec { + let mut v = Vec::with_capacity(pairs.len() * 2); + for (f, val) in pairs { + v.push(Frame::BulkString(Bytes::copy_from_slice(f.as_bytes()))); + v.push(Frame::BulkString(Bytes::copy_from_slice(val.as_bytes()))); + } + v +} + +/// Index one doc. All three index paths share `hash`+`key` so they collapse to one doc_id +/// (ensure_doc_id), giving TEXT/TAG/NUMERIC a shared doc-id space — the property set algebra +/// relies on. +fn add_doc( + idx: &mut TextIndex, + hash: u64, + key: &str, + text: &[(&str, &str)], + tags: &[(&str, &str)], + nums: &[(&str, &str)], +) { + let kb = key.as_bytes(); + if !text.is_empty() { + idx.index_document(hash, kb, &args(text)); + } + if !tags.is_empty() { + idx.tag_index_document(hash, kb, &args(tags)); + } + if !nums.is_empty() { + idx.numeric_index_document(hash, kb, &args(nums)); + } +} + +fn store_of(idx: TextIndex) -> TextStore { + let mut ts = TextStore::new(); + ts.create_index(Bytes::from_static(b"idx"), idx) + .expect("create_index ok"); + ts +} + +// ── frame helpers (mirror ft_text_search.rs extract_hits) ─────────────────── + +/// FT.SEARCH reply is `[total, key, fields, key, fields, ...]`; keys are the odd indices. +fn keys_set(frame: &Frame) -> BTreeSet> { + match frame { + Frame::Array(items) => items + .iter() + .skip(1) + .step_by(2) + .filter_map(|f| match f { + Frame::BulkString(b) => Some(b.to_vec()), + _ => None, + }) + .collect(), + other => panic!("expected Frame::Array, got {other:?}"), + } +} + +fn keys_ordered(frame: &Frame) -> Vec> { + match frame { + Frame::Array(items) => items + .iter() + .skip(1) + .step_by(2) + .filter_map(|f| match f { + Frame::BulkString(b) => Some(b.to_vec()), + _ => None, + }) + .collect(), + other => panic!("expected Frame::Array, got {other:?}"), + } +} + +fn total(frame: &Frame) -> i64 { + match frame { + Frame::Array(items) => match items.first() { + Some(Frame::Integer(n)) => *n, + _ => -1, + }, + other => panic!("expected Frame::Array, got {other:?}"), + } +} + +fn err_bytes(frame: &Frame) -> Vec { + match frame { + Frame::Error(b) => b.to_vec(), + other => panic!("expected Frame::Error, got {other:?}"), + } +} + +fn contains(hay: &[u8], needle: &[u8]) -> bool { + hay.windows(needle.len()).any(|w| w == needle) +} + +fn set_of(keys: &[&str]) -> BTreeSet> { + keys.iter().map(|k| k.as_bytes().to_vec()).collect() +} + +/// Run a full FT.SEARCH text query through the centralized dispatch wrapper, all results. +fn search(ts: &TextStore, q: &str) -> Frame { + run_text_query(ts, b"idx", q.as_bytes(), 1000, 0, usize::MAX) +} + +// ─────────────────────────── E1 — OR is a union ──────────────────────────── +#[test] +fn test_or_union_e2e() { + // alpha matches {a,b,c}; beta matches {c,d,e}. The old bug stripped `|` → AND → {c}. + let mut idx = empty_index(); + add_doc(&mut idx, 1, "a", &[("body", "alpha")], &[], &[]); + add_doc(&mut idx, 2, "b", &[("body", "alpha")], &[], &[]); + add_doc(&mut idx, 3, "c", &[("body", "alpha beta")], &[], &[]); + add_doc(&mut idx, 4, "d", &[("body", "beta")], &[], &[]); + add_doc(&mut idx, 5, "e", &[("body", "beta")], &[], &[]); + idx.build_fst(); + let ts = store_of(idx); + + let r = search(&ts, "alpha | beta"); + assert_eq!( + keys_set(&r), + set_of(&["a", "b", "c", "d", "e"]), + "OR must union" + ); + assert_eq!(total(&r), 5); + + // sanity: AND still narrows to the overlap, single terms unchanged. + assert_eq!(keys_set(&search(&ts, "alpha beta")), set_of(&["c"])); + assert_eq!(keys_set(&search(&ts, "alpha")), set_of(&["a", "b", "c"])); + assert_eq!(keys_set(&search(&ts, "beta")), set_of(&["c", "d", "e"])); +} + +// ─────────────────── E1 — TEXT + TAG clauses intersect ────────────────────── +#[test] +fn test_text_tag_intersect_e2e() { + // @body:foo = {1,2,3,4}; @tag:{bar} = {3,4,5}. Old bug word-tokenized the tag → 0 results. + let mut idx = empty_index(); + add_doc(&mut idx, 1, "1", &[("body", "foo")], &[("tag", "x")], &[]); + add_doc(&mut idx, 2, "2", &[("body", "foo")], &[("tag", "x")], &[]); + add_doc(&mut idx, 3, "3", &[("body", "foo")], &[("tag", "bar")], &[]); + add_doc(&mut idx, 4, "4", &[("body", "foo")], &[("tag", "bar")], &[]); + add_doc( + &mut idx, + 5, + "5", + &[("body", "other")], + &[("tag", "bar")], + &[], + ); + idx.build_fst(); + let ts = store_of(idx); + + assert_eq!( + keys_set(&search(&ts, "@body:foo @tag:{bar}")), + set_of(&["3", "4"]), + "TEXT∩TAG must intersect, not 0" + ); +} + +// ─────────────────── E1 — TEXT + NUMERIC clauses intersect ────────────────── +#[test] +fn test_text_numeric_intersect_e2e() { + // @body:phone = {1,2,3}; @price:[10 20] = {2,3,9}. Intersection = {2,3}. + let mut idx = empty_index(); + add_doc( + &mut idx, + 1, + "1", + &[("body", "phone")], + &[], + &[("price", "5")], + ); + add_doc( + &mut idx, + 2, + "2", + &[("body", "phone")], + &[], + &[("price", "10")], + ); + add_doc( + &mut idx, + 3, + "3", + &[("body", "phone")], + &[], + &[("price", "20")], + ); + add_doc( + &mut idx, + 9, + "9", + &[("body", "tablet")], + &[], + &[("price", "15")], + ); + idx.build_fst(); + let ts = store_of(idx); + + assert_eq!( + keys_set(&search(&ts, "@body:phone @price:[10 20]")), + set_of(&["2", "3"]), + "TEXT∩NUMERIC must intersect" + ); +} + +// ─────────────────────────── E1 — grouping ───────────────────────────────── +#[test] +fn test_grouping_e2e() { + // red={1,2,3} blue={3,4} car={2,3,4,5}; car (red | blue) = car ∩ (red∪blue) = {2,3,4}. + let mut idx = empty_index(); + add_doc(&mut idx, 1, "1", &[("body", "red")], &[], &[]); + add_doc(&mut idx, 2, "2", &[("body", "red car")], &[], &[]); + add_doc(&mut idx, 3, "3", &[("body", "red blue car")], &[], &[]); + add_doc(&mut idx, 4, "4", &[("body", "blue car")], &[], &[]); + add_doc(&mut idx, 5, "5", &[("body", "car")], &[], &[]); + idx.build_fst(); + let ts = store_of(idx); + + assert_eq!( + keys_set(&search(&ts, "car (red | blue)")), + set_of(&["2", "3", "4"]), + "grouping must scope the union under the AND" + ); +} + +// ──────────────────── E2 — deterministic scored ordering ──────────────────── +#[test] +fn test_or_scoring_order_deterministic() { + let mut idx = empty_index(); + add_doc(&mut idx, 1, "a", &[("body", "alpha")], &[], &[]); + add_doc(&mut idx, 2, "b", &[("body", "alpha alpha")], &[], &[]); + add_doc(&mut idx, 3, "c", &[("body", "beta")], &[], &[]); + idx.build_fst(); + let ts = store_of(idx); + + let first = keys_ordered(&search(&ts, "alpha | beta")); + let second = keys_ordered(&search(&ts, "alpha | beta")); + assert_eq!(first, second, "ordering must be deterministic across runs"); + assert_eq!( + first.iter().collect::>().len(), + first.len(), + "no duplicate keys in the union result" + ); +} + +// ────────────────── E3/E6 — malformed query → coded error ─────────────────── +#[test] +fn test_malformed_returns_coded_error() { + let mut idx = empty_index(); + add_doc(&mut idx, 1, "a", &[("body", "alpha")], &[], &[]); + idx.build_fst(); + let ts = store_of(idx); + + let bad = search(&ts, "alpha | (beta"); // unbalanced paren + assert!( + contains(&err_bytes(&bad), b"syntax_error"), + "malformed query must reply with the frozen `syntax_error` code" + ); + + // E6 — the server did not panic; the very next query still works. + assert_eq!(keys_set(&search(&ts, "alpha")), set_of(&["a"])); +} + +#[test] +fn test_unknown_field_error() { + let mut idx = empty_index(); + add_doc(&mut idx, 1, "a", &[("body", "alpha")], &[], &[]); + idx.build_fst(); + let ts = store_of(idx); + + let r = search(&ts, "@nope:foo"); + assert!( + contains(&err_bytes(&r), b"unknown_field"), + "querying an undeclared field must reply with `unknown_field`" + ); +} + +// ─────────────── E2/A1 — no regression on already-correct shapes ──────────── +#[test] +fn test_no_regression_single_and_filter() { + // Each of these shapes worked before 2b; routing them through the new path must keep the + // same result SETS. (Exact BM25 scores are held by the existing store/text unit suites, + // which eval_query reuses verbatim — see §4 regression-net note.) + let mut idx = empty_index(); + add_doc( + &mut idx, + 1, + "1", + &[("body", "alpha beta")], + &[("tag", "bar")], + &[("price", "10")], + ); + add_doc( + &mut idx, + 2, + "2", + &[("body", "alpha")], + &[("tag", "bar")], + &[("price", "15")], + ); + add_doc( + &mut idx, + 3, + "3", + &[("body", "beta")], + &[("tag", "baz")], + &[("price", "99")], + ); + idx.build_fst(); + let ts = store_of(idx); + + assert_eq!( + keys_set(&search(&ts, "alpha")), + set_of(&["1", "2"]), + "single term" + ); + assert_eq!( + keys_set(&search(&ts, "alpha beta")), + set_of(&["1"]), + "implicit AND" + ); + assert_eq!( + keys_set(&search(&ts, "@body:alpha")), + set_of(&["1", "2"]), + "single @field" + ); + assert_eq!( + keys_set(&search(&ts, "@tag:{bar}")), + set_of(&["1", "2"]), + "bare tag filter" + ); + assert_eq!( + keys_set(&search(&ts, "@price:[10 20]")), + set_of(&["1", "2"]), + "bare numeric filter" + ); + + // pure-filter docs score 0.0; the reply is still well-formed (count matches set size). + assert_eq!(total(&search(&ts, "@tag:{bar}")), 2); +} + +// ───────────────── absent term → empty result, NOT an error ───────────────── +#[test] +fn test_empty_result_not_error() { + let mut idx = empty_index(); + add_doc(&mut idx, 1, "a", &[("body", "alpha")], &[], &[]); + idx.build_fst(); + let ts = store_of(idx); + + let r = search(&ts, "zzz"); // term present nowhere + assert!(!matches!(r, Frame::Error(_)), "no-match is not an error"); + assert_eq!(total(&r), 0, "absent term → 0 results"); + assert!(keys_set(&r).is_empty()); +} + +// ─────────── frozen kernel symbol — eval_query gets a direct test ─────────── +#[test] +fn test_eval_query_kernel_direct() { + // The contract names eval_query as THE evaluator kernel; exercise it directly (no store / + // dispatch wrapper) so a regression in the kernel surfaces independently of the wiring. + let mut idx = empty_index(); + add_doc(&mut idx, 1, "a", &[("body", "alpha")], &[], &[]); + add_doc(&mut idx, 2, "b", &[("body", "alpha")], &[], &[]); + add_doc(&mut idx, 3, "c", &[("body", "beta")], &[], &[]); + idx.build_fst(); + + let schema = QuerySchema::from_index(&idx); + let node = parse_query(b"alpha | beta", &schema).expect("parse ok"); + let results = eval_query(&idx, &node, None, None, 1000); + + let got: BTreeSet> = results.iter().map(|r| r.key.to_vec()).collect(); + assert_eq!( + got, + set_of(&["a", "b", "c"]), + "kernel must union the OR branches" + ); + // text leaves carry a BM25 score; the union is non-empty and finite. + assert!(results.iter().all(|r| r.score.is_finite())); +} + +// ─── DFS Phase-1 N-invariant: collect_df_field_terms returns AT MOST ONE entry ─── +// aggregate_doc_freq SUMS one "N" sentinel per (field,terms) entry across shards. >1 entry inflates +// the global N; 0 entries for a query WITH text leaves zeroes it — both corrupt multi-shard BM25 IDF. +#[test] +fn test_df_field_terms_single_entry_invariant() { + let mut idx = empty_index(); + add_doc( + &mut idx, + 1, + "1", + &[("body", "alpha"), ("title", "beta")], + &[("tag", "bar")], + &[("price", "10")], + ); + idx.build_fst(); + let schema = QuerySchema::from_index(&idx); + + // OR across fields + a tag filter: still exactly one df entry (so N is gathered exactly once). + let node = parse_query(b"alpha | @title:beta @tag:{bar}", &schema).expect("parse ok"); + let fq = collect_df_field_terms(&node, &idx); + assert!( + fq.len() <= 1, + "must emit at most one (field,terms) df entry, got {}", + fq.len() + ); + assert_eq!( + fq.len(), + 1, + "a query with text leaves must emit one entry so N is gathered" + ); + + // Single-field query → hint is that field (pre-2b parity). + let single = parse_query(b"@body:alpha", &schema).expect("parse ok"); + let fq1 = collect_df_field_terms(&single, &idx); + assert_eq!(fq1.len(), 1); + assert_eq!(fq1[0].0, Some(0), "single @body query → field-0 hint"); +} + +#[test] +fn test_df_field_terms_pure_filter_vs_fuzzy() { + let mut idx = empty_index(); + add_doc( + &mut idx, + 1, + "1", + &[("body", "alpha")], + &[("tag", "bar")], + &[], + ); + idx.build_fst(); + let schema = QuerySchema::from_index(&idx); + + // Pure TAG filter → NO text leaf → no entry (no text scoring → N not needed). + let tag_only = parse_query(b"@tag:{bar}", &schema).expect("parse ok"); + assert!( + collect_df_field_terms(&tag_only, &idx).is_empty(), + "pure filter → no df entry" + ); + + // Pure fuzzy → has a text leaf but no Exact term → still ONE entry (empty terms) so N is gathered. + let fuzzy = parse_query(b"%alph%", &schema).expect("parse ok"); + let fq = collect_df_field_terms(&fuzzy, &idx); + assert_eq!( + fq.len(), + 1, + "fuzzy text leaf must still yield one entry so global N is gathered" + ); + assert!( + fq[0].1.is_empty(), + "fuzzy terms use LOCAL df → no exact df terms collected" + ); +} diff --git a/tests/fts_query_parse.rs b/tests/fts_query_parse.rs new file mode 100644 index 000000000..e43e3c012 --- /dev/null +++ b/tests/fts_query_parse.rs @@ -0,0 +1,302 @@ +//! RED tests for `fts-query-combinators` task 2a — the FT.SEARCH query PARSER. +//! +//! Contract FROZEN @ v1 (`.add/tasks/fts-query-combinators/TASK.md` §3): a recursive-descent +//! parser turns an FT.SEARCH query string into a `QueryNode` AST over the RediSearch subset +//! (terms+modifiers · implicit-AND · OR `|` · grouping `()` · `@field:` clauses · TAG `{a|b}` · +//! NUMERIC `[min max]`), with DIALECT-2 precedence (AND > modifiers > OR) and 5 named error codes. +//! +//! These are PURE parser tests — `parse_query(bytes, schema) -> Result`, +//! no index/server. The AST holds RAW (un-analyzed) tokens; analysis happens in 2b (eval). The +//! end-to-end matched-SET behaviour (M1/M2/M3 result sets, M5 no-regression, M7 server-stays-up) +//! is `fts-query-eval-dispatch`'s suite, not here. +//! +//! TDD — RED before build: `moon::text::query` does not exist yet, so the suite fails to compile; +//! once 2a lands `parse_query`/`QueryNode`/`QueryError`/`QuerySchema`, these go green unchanged. +#![cfg(feature = "text-index")] + +use bytes::Bytes; +use moon::text::query::{QueryError, QueryNode, QuerySchema, parse_query}; +use moon::text::store::TermModifier; + +/// Schema: text fields body(0), title(1); tag field "tag"; numeric field "price". +fn schema() -> QuerySchema { + QuerySchema::from_names(&["body", "title"], &["tag"], &["price"]) +} + +// ── small constructors so expected trees read cleanly ────────────────────── +fn ex(field: Option, tok: &str) -> QueryNode { + QueryNode::Term { + field, + token: Bytes::copy_from_slice(tok.as_bytes()), + modifier: TermModifier::Exact, + } +} +fn tag(field: &str, values: &[&str]) -> QueryNode { + QueryNode::Tag { + field: Bytes::copy_from_slice(field.as_bytes()), + values: values + .iter() + .map(|v| Bytes::copy_from_slice(v.as_bytes())) + .collect(), + } +} +fn p(q: &str) -> QueryNode { + parse_query(q.as_bytes(), &schema()).expect("parse ok") +} + +// ─────────────────────────── M1 — OR is a union node ─────────────────────── +#[test] +fn test_or_parses_as_or_node() { + assert_eq!( + p("alpha | beta"), + QueryNode::Or(vec![ex(None, "alpha"), ex(None, "beta")]) + ); +} + +/// The headline regression: `|` must NEVER be silently dropped (the old bug turned it into AND). +#[test] +fn test_pipe_never_discarded() { + match p("alpha | beta") { + QueryNode::Or(children) => assert_eq!(children.len(), 2, "OR must keep both branches"), + other => panic!("`|` must parse to an Or node, got {other:?}"), + } +} + +// ──────────────── M2 — multi-clause: distinct typed clauses, AND-joined ───── +#[test] +fn test_multiclause_parses_distinct_clauses() { + // @body:foo @tag:{bar} -> And[ Term{body,foo}, Tag(tag,[bar]) ] + // The tag clause is a Tag NODE, never word-tokens "tag","bar" scoped to body (the old bug). + assert_eq!( + p("@body:foo @tag:{bar}"), + QueryNode::And(vec![ex(Some(0), "foo"), tag("tag", &["bar"])]), + ); +} + +#[test] +fn test_text_numeric_clause() { + // @body:phone @price:[10 20] -> And[ Term{body,phone}, Numeric(price,10,20,false,false) ] + assert_eq!( + p("@body:phone @price:[10 20]"), + QueryNode::And(vec![ + ex(Some(0), "phone"), + QueryNode::Numeric { + field: Bytes::from_static(b"price"), + min: 10.0, + max: 20.0, + min_excl: false, + max_excl: false, + }, + ]), + ); +} + +#[test] +fn test_multi_tag_values() { + assert_eq!(p("@tag:{a|b}"), tag("tag", &["a", "b"])); +} + +// ─────────────────────────── M3 — grouping ───────────────────────────────── +#[test] +fn test_grouping_scopes_union() { + // car (red | blue) -> And[ Term(car), Or[Term(red),Term(blue)] ] + assert_eq!( + p("car (red | blue)"), + QueryNode::And(vec![ + ex(None, "car"), + QueryNode::Or(vec![ex(None, "red"), ex(None, "blue")]), + ]), + ); +} + +#[test] +fn test_field_scoped_group() { + // @body:(a | b) -> Or[ Term{body,a}, Term{body,b} ] (field pushed into the group) + assert_eq!( + p("@body:(a | b)"), + QueryNode::Or(vec![ex(Some(0), "a"), ex(Some(0), "b")]), + ); +} + +// ─────────────── M4 — precedence: AND binds tighter than OR (DIALECT 2) ───── +#[test] +fn test_precedence_and_binds_tighter() { + // a b | c d == (a AND b) OR (c AND d) + assert_eq!( + p("a b | c d"), + QueryNode::Or(vec![ + QueryNode::And(vec![ex(None, "a"), ex(None, "b")]), + QueryNode::And(vec![ex(None, "c"), ex(None, "d")]), + ]), + ); +} + +// ─────────────── M5 (parse level) — modifiers + numeric capability preserved ─ +#[test] +fn test_modifiers_preserved() { + // %alpa% -> fuzzy distance 1 on raw "alpa"; al* -> prefix on raw "al". + match p("%alpa%") { + QueryNode::Term { + token, modifier, .. + } => { + assert_eq!(token, Bytes::from_static(b"alpa")); + assert_eq!(modifier, TermModifier::Fuzzy(1)); + } + other => panic!("expected fuzzy Term, got {other:?}"), + } + match p("al*") { + QueryNode::Term { + token, modifier, .. + } => { + assert_eq!(token, Bytes::from_static(b"al")); + assert_eq!(modifier, TermModifier::Prefix); + } + other => panic!("expected prefix Term, got {other:?}"), + } +} + +#[test] +fn test_numeric_exclusive_and_inf() { + // @price:[(10 +inf] -> exclusive min 10, inclusive max +inf (must NOT regress existing capability) + match p("@price:[(10 +inf]") { + QueryNode::Numeric { + field, + min, + max, + min_excl, + max_excl, + } => { + assert_eq!(field, Bytes::from_static(b"price")); + assert_eq!(min, 10.0); + assert!(max.is_infinite() && max.is_sign_positive()); + assert!(min_excl, "(10 -> exclusive min"); + assert!(!max_excl); + } + other => panic!("expected Numeric, got {other:?}"), + } +} + +#[test] +fn test_single_field_term_unchanged() { + // @body:alpha -> Term{body, alpha} (the already-correct single-clause path) + assert_eq!(p("@body:alpha"), ex(Some(0), "alpha")); +} + +// ─────────────── Rejects — each a named QueryError, never a panic ─────────── +#[test] +fn test_unbalanced_paren_is_syntax() { + assert_eq!( + parse_query(b"alpha | (beta", &schema()), + Err(QueryError::Syntax) + ); +} + +#[test] +fn test_unbalanced_brace_bracket() { + assert_eq!( + parse_query(b"@tag:{bar", &schema()), + Err(QueryError::Syntax) + ); + assert_eq!( + parse_query(b"@price:[10 20", &schema()), + Err(QueryError::Syntax) + ); +} + +#[test] +fn test_empty_query() { + assert_eq!(parse_query(b"", &schema()), Err(QueryError::EmptyQuery)); + assert_eq!(parse_query(b" ", &schema()), Err(QueryError::EmptyQuery)); + assert_eq!(parse_query(b"()", &schema()), Err(QueryError::EmptyQuery)); +} + +#[test] +fn test_unknown_field() { + assert_eq!( + parse_query(b"@nope:foo", &schema()), + Err(QueryError::UnknownField(Bytes::from_static(b"nope"))), + ); +} + +#[test] +fn test_numeric_invalid() { + assert_eq!( + parse_query(b"@price:[20 10]", &schema()), + Err(QueryError::NumericInvalid) + ); // min>max + assert_eq!( + parse_query(b"@price:[x y]", &schema()), + Err(QueryError::NumericInvalid) + ); // non-numeric +} + +#[test] +fn test_tag_invalid() { + assert_eq!( + parse_query(b"@tag:{}", &schema()), + Err(QueryError::TagInvalid) + ); + assert_eq!( + parse_query(b"@tag:{ }", &schema()), + Err(QueryError::TagInvalid) + ); +} + +/// A term matching nothing is NOT a parse error — emptiness is an EVAL outcome. +#[test] +fn test_absent_term_is_not_error() { + assert_eq!(p("zzz"), ex(None, "zzz")); + assert_eq!( + p("alpha zzz"), + QueryNode::And(vec![ex(None, "alpha"), ex(None, "zzz")]), + ); +} + +/// M7 — a table of malformed inputs must each return Err, never panic / hang. +#[test] +fn test_parser_never_panics() { + let malformed: &[&[u8]] = &[ + b"|", + b"| |", + b"(", + b")", + b"(((", + b"@", + b"@:", + b"@:foo", + b"@tag:", + b"@tag:{", + b"[", + b"]", + b"@price:[", + b"@price:]", + b"{}", + b"@@@", + b":::", + b"a | | b", + b"((a)", + b"@tag:{a|}", + b"@price:[1]", + b"@price:[1 2 3]", + b"\xff\xfe", + b"a (b | c", + ]; + for q in malformed { + // The only contract is: returns a Result, never panics. (Err or Ok both fine here.) + let _ = parse_query(q, &schema()); + } +} + +/// The frozen §3 wire error codes — QueryError::code() must map 1:1 (the dispatch layer in 2b +/// emits these as Frame::Error). Locking them here keeps 2b from drifting off the contract. +#[test] +fn test_error_codes_match_contract() { + assert_eq!(QueryError::Syntax.code(), "syntax_error"); + assert_eq!(QueryError::EmptyQuery.code(), "empty_query"); + assert_eq!( + QueryError::UnknownField(Bytes::new()).code(), + "unknown_field" + ); + assert_eq!(QueryError::NumericInvalid.code(), "numeric_filter_invalid"); + assert_eq!(QueryError::TagInvalid.code(), "tag_filter_invalid"); +}