feat(text): v3-1 FTS hardening — upsert / count / routing (final 3 tasks)#192
Merged
Conversation
…al scan `PostingStore::remove_doc` scanned every term's posting list to clear a single document, so each HSET upsert / bulk re-index cost O(total-vocabulary) and the indexing rate collapsed as the corpus grew (~376 vs ~18,052 docs/s). Add a reverse `doc_id -> SmallVec<[term_id; 8]>` index, recorded exactly once per (doc, term) edge on the new-doc branch of `add_term_occurrence`; `remove_doc` now visits only the terms a document actually contributed — O(terms-in-doc), independent of V — removing each at its rank-aligned index and erasing the reverse entry. Search output is byte-identical: the inherited rank-aligned PostingList contract (term_freqs/positions sorted-doc-id aligned with the doc_ids bitmap) is preserved, so matched doc sets, BM25 scores, doc_freq, num_docs and avgdl are unchanged. Defensive on desync: an absent doc is a no-op returning an empty Vec, and a stale reverse entry (posting missing or no longer holding the doc) is skipped — no unwrap/expect/panic on the removal path. Tests (src/text/mod.rs): 6 unit tests covering reverse-map population (unique terms), removal via the reverse map, absent-doc no-op, stale-entry skip, memory reclamation across repeated upserts, and byte-identical posting state after index->upsert->delete vs a fresh build of the final state; plus a #[ignore]'d scaling proof (V~40000 removal < 20x V~20; old impl ~2000x). Lib regression 3590 passed / 0 failed; clippy clean on default + runtime-tokio,jemalloc. ADD: v3-1-fts-hardening / fts-upsert-incremental — verify gate PASS. author: Tin Dang
…semantics) The integer match count in an FT.SEARCH reply was capped at the LIMIT/top_k page size. `build_text_response` reported `results.len()`, but the result vector had already been truncated to `top_k = offset + count` by `eval_query`, so `FT.SEARCH idx "term" LIMIT 0 5` over 100 matches replied `5` instead of `100`. The multi-shard DFS coordinator compounded it: `merge_text_results` counted the merged-and-truncated returned docs, discarding each shard's true matched count. Surface the true count from the evaluator (it already builds the full matched set before truncating) WITHOUT editing the frozen fts-query-eval-dispatch contract: add `eval_query_counted` returning (top_k page, pre-truncation matched-and- resolvable count); `eval_query` is now its `.0` wrapper. Add `build_text_response_with_total` carrying an explicit reply[0]; the 3-arg `build_text_response` delegates with `results.len()` (legacy behavior for the off-live-path callers). `run_text_query_on_index` threads the true total; `merge_text_results` sums each shard's items[0] (keys partition to one shard, so the sum is exact) and tolerates errored/odd frames (contribute 0, never panic). A matched doc with no doc_id_to_key entry is unreturnable, so it is excluded from both the page and the total (count = matched AND resolvable). Tests: 4 unit (ft_text_search) for the response builder + merge sum + errored- shard tolerance; 3 e2e (fts_query_eval_e2e) for eval_query_counted, single-shard LIMIT, and no-LIMIT no-regression. Over-the-wire probe: 1-shard and 4-shard servers both report total=30 under LIMIT 0 5 (returned=5), parity confirmed. Full lib regression 3594 passed / 0 failed; clippy clean on default + tokio,jemalloc. ADD: v3-1-fts-hardening / fts-search-count-semantics — verify gate PASS. author: Tin Dang
…one SPARSE, no AND-path panic
Three routing/panic-safety fixes on the FT.SEARCH classification path. No
result, score, key, or ordering change on any query that already worked.
R1 (panic-safety): TextStore::search_field carried three
`.expect("posting exists: checked above")` on the BM25 AND + scoring path.
Replace each with defensive control flow — a missing AND-term posting returns
an empty result set; a missing posting during scoring skips that term's
contribution. The upstream is_none guard keeps these branches unreachable
today, so output is byte-identical; this removes a latent server panic if that
guard is ever refactored away.
R2 (text mis-routed to vector): is_text_query uppercased the query then matched
the bare substring "KNN ", so `FT.SEARCH idx "knn tutorial"` was classified
non-text and fell through to the case-sensitive KNN parser, returning
`ERR invalid KNN query syntax`. Key on the canonical `[KNN` bracket instead:
prose containing "knn" is text; `*=>[KNN ...]` stays non-text and routes to the
vector engine. The one unit assertion encoding the old over-aggressive
detection (`!is_text_query(b"knn 10")`) is updated to the corrected semantics.
R3 (vector mis-routed to text, user-approved scope): is_text_query only sees
args[1], so a standalone `SPARSE @f $p` clause with a text-looking query string
took the text fast-path and silently dropped the sparse retriever. Add
`has_sparse_clause` and AND `!has_sparse_clause(args)` into all six text-route
gates (monoio/sharded multi-shard + single-shard re-check, single-threaded,
spsc) so any SPARSE-carrying query defers to ft_search. SPARSE-inside-HYBRID was
already deferred by parse_hybrid_modifier and is unchanged.
Tests: 3 new unit tests (is_text_query prose-knn is text, has_sparse_clause
detects standalone, the route predicate defers SPARSE); the canonical-KNN and
bare-`*` non-text cases are retained. search_field no-expect/unwrap confirmed by
source audit; output-identity by the full FT regression. Wire probe:
`FT.SEARCH idx "knn"` -> 2, `"knn tutorial"` -> 1 (AND), neither errors as KNN;
canonical `*=>[KNN ...]` still routes to the vector engine. Full lib regression
3597 passed / 0 failed / 1 ignored; clippy clean on default and
runtime-tokio,jemalloc.
ADD: v3-1-fts-hardening / fts-query-routing-robustness — verify gate PASS.
author: Tin Dang
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughThree FT.SEARCH improvements are shipped together: (1) routing robustness — ChangesFT.SEARCH routing robustness, count semantics, and incremental upsert
Sequence Diagram(s)sequenceDiagram
participant Client
rect rgba(70, 130, 180, 0.5)
note over Client,merge_text_results: Multi-shard FT.SEARCH text query
end
participant handler as handler_sharded/handler_monoio
participant spsc as spsc_handler
participant eval as eval_query_counted
participant builder as build_text_response_with_total
participant merge as merge_text_results
Client->>handler: FT.SEARCH index query [LIMIT ...]
handler->>handler: is_text_query(query) && !has_sparse_clause(args)
handler->>spsc: scatter text search to each shard
spsc->>eval: eval_query_counted(idx, node, top_k)
eval-->>spsc: (results[0..top_k], total_matched)
spsc->>builder: build_text_response_with_total(results, total_matched, offset, count)
builder-->>spsc: Frame reply[0]=total_matched
spsc-->>handler: per-shard Frame
handler->>merge: merge_text_results(shard_frames)
merge->>merge: sum shard items[0] (errored shards = 0)
merge-->>Client: Frame reply[0]=sum_total_matched
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/command/vector_search/ft_text_search.rs (1)
1886-1917:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
build_text_response_with_totalkeeps forbidden clone-heavy construction on the command dispatch path.This new path still performs
clone()-based response assembly insrc/command/, which violates the dispatch allocation policy and preserves avoidable hot-path overhead. Please switch this builder to an ownership/move-based path (or equivalent zero/low-copy strategy) for keys/fields.As per coding guidelines,
No Box::new(), Vec::new(), String::new(), Arc::new(), clone(), format!(), or to_string() in command dispatch (src/command/), protocol parsing, shard event loops, or I/O drivers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/command/vector_search/ft_text_search.rs` around lines 1886 - 1917, The build_text_response_with_total function violates the dispatch allocation policy by using clone()-based construction (specifically cloning r.key with .clone()) and heap allocations (String::with_capacity, vec! macro) in the command dispatch path. Refactor this function to use ownership/move-based semantics when building Frame responses for keys and fields, eliminating the r.key.clone() call and avoiding unnecessary String and Vec allocations. Consider restructuring how the score buffer and fields array are constructed to minimize heap allocations on this hot path while maintaining the same response format.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.add/tasks/fts-query-routing-robustness/TASK.md:
- Line 151: Fix the markdownlint violations in the TASK.md file at two
locations. At line 151, add a language identifier (text) to the opening fence of
the code block by changing the opening triple backticks from ``` to ```text to
satisfy the fenced code language requirement. At line 302, remove any spaces
that appear inside the inline code span delimiters to satisfy MD038, ensuring
that backticks directly wrap the code content without intervening whitespace.
In @.add/tasks/fts-upsert-incremental/TASK.md:
- Around line 141-176: The markdown fenced code block in the PostingStore
contract specification is missing a language identifier, which violates
markdownlint rules. Add a language specifier (for example, text) to the opening
fence marks (```text instead of ```) to satisfy the MD040 rule. Apply the same
fence-style fix to the analogous contract block in the related task file that
also contains unspecified fenced code blocks.
In `@src/text/posting.rs`:
- Around line 92-97: The estimated_bytes() method in PostingStore is not
accounting for memory used by the newly added doc_terms HashMap field, causing
memory telemetry and limits to be systematically under-reported. Update the
estimated_bytes() method to include memory estimates for the doc_terms field in
addition to the existing postings estimate. The estimate should account for both
the HashMap structure overhead and the SmallVec contents it stores for each
document.
---
Outside diff comments:
In `@src/command/vector_search/ft_text_search.rs`:
- Around line 1886-1917: The build_text_response_with_total function violates
the dispatch allocation policy by using clone()-based construction (specifically
cloning r.key with .clone()) and heap allocations (String::with_capacity, vec!
macro) in the command dispatch path. Refactor this function to use
ownership/move-based semantics when building Frame responses for keys and
fields, eliminating the r.key.clone() call and avoiding unnecessary String and
Vec allocations. Consider restructuring how the score buffer and fields array
are constructed to minimize heap allocations on this hot path while maintaining
the same response format.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ffa54eef-0912-40b0-a58b-d9b052ae1550
📒 Files selected for processing (17)
.add/state.json.add/tasks/fts-query-routing-robustness/TASK.md.add/tasks/fts-search-count-semantics/TASK.md.add/tasks/fts-upsert-incremental/TASK.mdCHANGELOG.mdsrc/command/vector_search/ft_text_search.rssrc/command/vector_search/mod.rssrc/server/conn/handler_monoio/ft.rssrc/server/conn/handler_sharded/ft.rssrc/server/conn/handler_single.rssrc/shard/spsc_handler.rssrc/text/mod.rssrc/text/posting.rssrc/text/query/eval.rssrc/text/query/mod.rssrc/text/store.rstests/fts_query_eval_e2e.rs
|
|
||
| ## 3 · CONTRACT — freeze the shape ▸ docs/05-step-3-contract.md | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Fix markdownlint violations to keep task docs lint-clean.
Line 151 should declare a fenced code language (e.g., text), and Line 302 should remove spaces inside the inline code span to satisfy MD038.
Suggested patch
-```
+```text
FT.SEARCH ROUTING + BM25-AND panic-hardening. No wire/RESP shape change; one NEW pub helper.
...
-```
+```- AND-ed into all SIX routing gates: handler_monoio/ft.rs:68 + :442, handler_sharded/ft.rs:68 + :342,
- handler_single.rs:1338, spsc_handler.rs:1845. `is_text_query` keys on `[KNN` (the bare `KNN ` scan is
+ AND-ed into all SIX routing gates: handler_monoio/ft.rs:68 + :442, handler_sharded/ft.rs:68 + :342,
+ handler_single.rs:1338, spsc_handler.rs:1845. `is_text_query` keys on `[KNN` (the bare `KNN` scan is
gone). All 3 `expect("posting exists")` removed from search_field (grep: none). Confirmed via grep +
green build.Also applies to: 302-302
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 151-151: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.add/tasks/fts-query-routing-robustness/TASK.md at line 151, Fix the
markdownlint violations in the TASK.md file at two locations. At line 151, add a
language identifier (text) to the opening fence of the code block by changing
the opening triple backticks from ``` to ```text to satisfy the fenced code
language requirement. At line 302, remove any spaces that appear inside the
inline code span delimiters to satisfy MD038, ensuring that backticks directly
wrap the code content without intervening whitespace.
Source: Linters/SAST tools
Comment on lines
+141
to
+176
| ``` | ||
| PostingStore (src/text/posting.rs) — INTERNAL data-structure change. No wire/RESP change. | ||
|
|
||
| STATE (added): | ||
| doc_terms: HashMap<u32 /*doc_id*/, SmallVec<[u32 /*term_id*/; 8]>> | ||
| INVARIANT: doc_terms[d] == { t : postings[t].doc_ids.contains(d) } (a set — no duplicates) | ||
|
|
||
| fn add_term_occurrence(&mut self, term_id: u32, doc_id: u32, positions: Option<Vec<u32>>) | ||
| SIGNATURE UNCHANGED. On the NEW-doc branch (`!posting.doc_ids.contains(doc_id)`) ALSO record | ||
| term_id into doc_terms[doc_id] (push; this branch fires once per (doc,term) so no dup). On the | ||
| existing-doc branch (tf increment) the reverse map is untouched. Rank-aligned tf/positions | ||
| insert is unchanged. | ||
|
|
||
| fn remove_doc(&mut self, doc_id: u32) -> Vec<(u32 /*term_id*/, u32 /*old_tf*/)> | ||
| SIGNATURE + RETURN SEMANTICS UNCHANGED (callers sum it for stats; order is unspecified). New body: | ||
| let Some(terms) = self.doc_terms.remove(&doc_id) else { return Vec::new() }; // absent_doc_noop | ||
| for term_id in terms: | ||
| let Some(posting) = self.postings.get_mut(&term_id) else { continue }; // stale_reverse_entry_skip | ||
| if !posting.doc_ids.contains(doc_id) { continue }; // stale_reverse_entry_skip | ||
| let idx = posting.rank_index(doc_id); // BEFORE bitmap removal (rank-aligned) | ||
| old_tf = posting.term_freqs.remove(idx); posting.doc_ids.remove(doc_id); | ||
| if let Some(pos) = &mut posting.positions { if idx < pos.len() { pos.remove(idx); } } | ||
| removed.push((term_id, old_tf)); | ||
| Visits EXACTLY |terms| postings — NOT all of self.postings. O(terms-in-doc × rank). | ||
|
|
||
| REJECT responses (internal — no error frames; these are defensive control-flow outcomes): | ||
| absent_doc_noop -> doc_terms.remove == None -> return Vec::new(); postings + stats untouched. | ||
| stale_reverse_entry_skip -> get_mut == None OR !doc_ids.contains -> `continue`; never unwrap/expect/panic. | ||
|
|
||
| INHERITED & PRESERVED (fts-posting-rank-tf frozen contract — UNCHANGED): | ||
| term_freqs/positions kept sorted-doc_id (rank) aligned with doc_ids; tf()/positions_for() via rank. | ||
| RESULT-AFFECTING OUTPUTS UNCHANGED: doc_freq, get_posting, store num_docs/avgdl, FT.SEARCH output. | ||
|
|
||
| Schema/access: IN-MEMORY ONLY. No on-disk format change — postings (and the reverse map) are | ||
| rebuilt from AOF/WAL replay at load, so NO migration. Per-shard; no cross-shard state. | ||
| ``` |
There was a problem hiding this comment.
Specify a language for this fenced code block (MD040).
Add a fence language (for example, text) to satisfy markdownlint and keep docs lint-clean. The same fence-style fix should also be applied to the analogous contract block in .add/tasks/fts-search-count-semantics/TASK.md.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 141-141: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.add/tasks/fts-upsert-incremental/TASK.md around lines 141 - 176, The
markdown fenced code block in the PostingStore contract specification is missing
a language identifier, which violates markdownlint rules. Add a language
specifier (for example, text) to the opening fence marks (```text instead of
```) to satisfy the MD040 rule. Apply the same fence-style fix to the analogous
contract block in the related task file that also contains unspecified fenced
code blocks.
Source: Linters/SAST tools
Comment on lines
+92
to
+97
| /// Reverse index: doc_id -> the term_ids that document contributed (a set, no duplicates). | ||
| /// Lets `remove_doc` visit only a document's own terms instead of scanning every posting, | ||
| /// making per-doc removal O(terms-in-doc) instead of O(total vocabulary) — the upsert/bulk | ||
| /// re-index cliff (fts-upsert-incremental). Kept in sync with `postings`: `add_term_occurrence` | ||
| /// records the edge on the new-doc branch; `remove_doc` erases the doc's entry. | ||
| doc_terms: HashMap<u32, SmallVec<[u32; 8]>>, |
There was a problem hiding this comment.
estimated_bytes() now under-reports memory after introducing doc_terms.
PostingStore gained a reverse doc_terms map, but the memory estimator still accounts only for postings. That makes memory telemetry/limits systematically low after this change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/text/posting.rs` around lines 92 - 97, The estimated_bytes() method in
PostingStore is not accounting for memory used by the newly added doc_terms
HashMap field, causing memory telemetry and limits to be systematically
under-reported. Update the estimated_bytes() method to include memory estimates
for the doc_terms field in addition to the existing postings estimate. The
estimate should account for both the HashMap structure overhead and the SmallVec
contents it stores for each document.
TinDang97
pushed a commit
that referenced
this pull request
Jun 16, 2026
Close the ADD loop for the v3-1-fts-hardening milestone (6/6 tasks PASS, PR #192 merged to main). Consolidate the 9 confirmed competency deltas into the versioned foundation and mark the milestone done. Foundation (bump foundation-version 4 → 5): - PROJECT.md §Spec (SDD ×2): re-derive task scope from the code in Specify — a "perf-only" framing hid a correctness bug, and routing's real defects were unlike its original framing; a cross-shard payload that cannot REPRESENT the contract's algebra is a freeze-time design smell. - CONVENTIONS.md (TDD ×3): distinct/asymmetric index fixtures; assert a nonzero per-suite ran-count (test selection must execute, not just compile); a stale #[ignore]'d test can guard nothing. - CONVENTIONS.md (ADD ×4): A/B a tuning knee on each target arch (x86_64 breached the aarch64-green 5% bound → knee coarsened 512→1024); risk:high ⇒ conservative human gate is load-bearing; split-parser/dispatch, gate-the-pair; deferred- cleanup honesty. - PROJECT.md §Key Decisions: one CLOSE + fold row. Bookkeeping: - All 9 deltas flipped open → folded (fts-posting-rank-tf ×3, fts-query-eval-dispatch ×5, ft-yield-costfree-monoio ×1). - MILESTONE.md: 5 exit criteria + 6 task boxes checked. - state.json: milestone status → done; RETRO.md exit report written. Carried follow-ups (candidates for the next milestone, not blockers): un-ignore the stale numeric consistency test, remove the dead scatter_text_search_filter path, tighten the multi-field-OR IDF magnitude approximation. author: Tin Dang
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
v3-1 FTS hardening — final 3 tasks (upsert / count / routing)
Completes the v3-1-fts-hardening milestone. Stacked work #189/#190 already
merged to
main, so this branches cleanly offmain(no stack).All five milestone exit criteria are now delivered (high-DF O(M²) cliff and the
OR/combinator/count fixes landed in #190; this PR closes the remaining three):
8e6488dHSETupsert / bulk pass) calledremove_doc, which scanned every term's posting — O(total-vocabulary) per doc (the ~376 vs ~18,052 docs/s indexing cliff). Adds a reversedoc_id → term_idsindex soremove_docvisits only the doc's own terms — O(terms-in-doc), independent of V. Search output byte-identical.93f0adaLIMIT/top_kpage size, and the multi-shard coordinator counted merged-and-truncated docs. Now reports the true matched, key-resolvable count (RediSearch semantics): evaluator surfaces the count before truncation; multi-shard merge sums each shard's local matched count (keys partition to one shard → exact; errored shards contribute 0). Returned pages unchanged.4de1d6cTextStore::search_field's 3×.expect("posting exists")on the BM25 AND/scoring path → defensivelet-else(no server panic); (R2)is_text_querymatched the bare substringKNN, soFT.SEARCH idx "knn tutorial"mis-routed to the KNN parser →ERR invalid KNN query syntax— now keys on the canonical[KNNbracket, so prose with "knn" searches as text while*=>[KNN …]still routes to the vector engine; (R3) a standaloneSPARSE @field $paramclause with a text-looking query string was silently dropped onto the text path — ahas_sparse_clauseguard now defers it to the vector engine at all six text-route gates. Output byte-identical for existing queries.Method (ADD)
Each task ran the full specification bundle (spec → scenarios → frozen contract →
red tests → build → verify gate PASS). Tasks 2 and 3 use additive symbols
(
eval_query_counted,build_text_response_with_total,has_sparse_clause) sothey never edit the frozen
eval_query/build_text_response/is_text_querycontracts from the combinators work.
Verification
--no-default-features --features runtime-tokio,jemalloc): green.runtime-tokio,jemalloc;cargo fmt --checkclean.has_sparse_clausedetects standalone,route predicate defers SPARSE); count-semantics unit + e2e tests over a corpus
larger than the page; upsert incremental-removal tests.
FT.SEARCH idx "knn"→ textresults (not a KNN error); canonical
*=>[KNN …]still routes to the vector engine.Carried follow-ups (open ADD deltas — not blockers)
scatter_text_search_filterpath, and a multi-field-ORIDF approximation are tracked as open deltas for the milestone-close fold.
Closes the v3-1-fts-hardening milestone.
Summary by CodeRabbit
Bug Fixes
Performance