feat(text): v3-1 FTS hardening — posting-rank TF + query combinators (OR/intersect fix)#190
Merged
Merged
Conversation
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? |
|
Warning Review limit reached
More reviews will be available in 28 minutes and 21 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughImplements two FTS hardening tasks: (1) rank-aligned ChangesFTS Hardening Implementation
Milestone and Task Planning Documents
Sequence DiagramsequenceDiagram
participant Client
participant Handler as FT.SEARCH Handler
participant RunTextQuery as run_text_query
participant RunOnIndex as run_text_query_on_index
participant Coordinator as scatter_text_search
participant SPSC as spsc_handler
Client->>Handler: FT.SEARCH <index> <raw query bytes>
alt single-shard fast path
Handler->>RunTextQuery: index_name, raw bytes, top_k
RunTextQuery->>RunOnIndex: TextIndex, raw bytes, global_df=None
RunOnIndex->>RunOnIndex: parse_query → eval_query → build_text_response
RunOnIndex-->>Handler: Frame (results or coded error)
else multi-shard
Handler->>Coordinator: scatter_text_search(raw bytes)
Coordinator->>Coordinator: parse_query, collect_df_field_terms
Note over Coordinator: Phase 1 — scatter DF requests
Coordinator->>SPSC: TextSearchPayload { query: Bytes, global_df=None }
SPSC-->>Coordinator: local DF counts
Coordinator->>Coordinator: aggregate global_df, global_n
Note over Coordinator: Phase 2 — scatter scored search
Coordinator->>RunOnIndex: run_text_query_on_index(global_df, global_n)
Coordinator->>SPSC: TextSearchPayload { query: Bytes, global_df }
SPSC->>RunOnIndex: run_text_query_on_index(global_df)
SPSC-->>Coordinator: shard Frame
Coordinator->>Coordinator: merge + highlight/summarize
Coordinator-->>Handler: merged Frame
end
Handler-->>Client: FT.SEARCH response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
…ank-tf Consolidate the 2026-06-16 4-feature deep-review + benchmark findings into a new-major v3 "secondary-engine correctness & parity" theme (chosen over GitHub issues), as 3 sub-milestones under .add/milestones/: - v3-1-fts-hardening (active): the benchmark's biggest gap vs RediSearch - v3-2-graph-correctness (planned) - v3-3-vector-kv-polish (planned) Open the first v3-1 task, fts-posting-rank-tf, through the ADD specification bundle (specify -> scenarios -> contract FROZEN @ v1). The code investigation upgraded it from a perf-only fix to perf + a latent BM25-correctness bug: the read path indexes term_freqs by the sorted RoaringBitmap rank, but writes push term_freqs in insertion order -- so re-indexing an updated low-id document misaligns term_freqs and corrupts BM25 for the whole term (store.rs:343,355 + posting.rs:96-97 vs store.rs:504-509). Frozen contract: rank-aligned parallel arrays -- term_freqs/positions kept in sorted doc_id order, PostingList::tf(doc_id) via rank (sub-linear, kills the O(M^2) high-DF cliff), insert-at-rank on add. risk: high, autonomy: conservative -> verify requires a human gate. author: Tin Dang
…O(M^2) cliff PostingList stored term_freqs/positions in insertion order while the BM25 read path indexed them by the sorted RoaringBitmap rank (doc_ids.iter() .position()). The two only agreed when documents were inserted in ascending doc_id order. Re-indexing an updated document (remove_doc + index_document reusing the same doc_id) re-inserts mid-bitmap but pushed the new tf to the end, so term_freqs/positions silently misaligned and corrupted BM25 scoring for the entire term. The bug was invisible whenever term frequencies were equal, which is why existing tests missed it. Fix: keep term_freqs/positions in sorted-doc_id (rank) order — the same order the read path already assumed. - add PostingList::rank_index/tf/positions_for: rank() is count(ids <= d), so rank-1 is the 0-based array index. Sub-linear (container-stride + popcount) vs the former O(N) linear scan, which also removes the high-DF O(M^2) ranking cliff seen in the 4-feature benchmark. - add_term_occurrence inserts tf/positions AT the rank index (not push) on a new doc, and increments at the rank index on an existing doc. - remove_doc removes at the rank index computed before the bitmap delete. - both store.rs read sites call posting.tf(doc_id); the stale "RESEARCH Pitfall 1 — use linear scan not rank()" note is superseded (it assumed insertion-order term_freqs, which no longer holds). tf(absent) returns 0 (BM25 treats the term as not occurring); never panics. No on-disk format change — postings are rebuilt from HSET replay on reload. Tests (tests/fts_posting_rank_tf.rs, red before this commit): - tf correct after low-id update (5,2,3 not the buggy 6,3,1) - tf(absent) = 0 - positions aligned after update - high-DF tf lookup is sub-linear (50K docs, best-of-K < 50ms) - search_field / search_field_or ranking correct after update ([a,c,b]) - ascending-insert ranking unchanged (no regression on the correct path) Evidence: 7/7 new tests green; full lib suite 3584 passed / 0 failed; text:: 144 passed; text::store::tests 15 passed; cargo fmt --check clean; cargo clippy -- -D warnings clean. ADD: fts-posting-rank-tf (v3-1-fts-hardening), contract FROZEN @ v1, risk: high / autonomy: conservative — build phase. author: Tin Dang
Record the ADD verify gate and observe-phase output for the rank-aligned posting TF fix (code shipped in the preceding commit). §6 VERIFY — human gate (risk: high · autonomy: conservative, so no auto-pass): - evidence: 7/7 new tests, full lib 3584/0, text:: 144/0, store 15/0, fmt + clippy + tokio/text-index parity all clean - concurrency: BM25 read + auto-index write both run single-threaded on the owning shard under exclusive &mut TextStore; off-event-loop yield is dense-KNN-only on an owned snapshot and never touches postings -> no residue - security: pure in-memory re-index, panic-free tf()/positions_for(), no new dep, no on-disk format change -> no finding - wiring: tf() at the only two BM25 read sites; rank_index() private + fully used; one surfaced flag (positions_for is frozen-contract forward API, referenced only by its conformance test until phrase queries land) — accepted - Outcome: PASS, reviewed by Tin Dang 2026-06-16 §7 OBSERVE — competency deltas (open, fold at milestone close): - [SDD] re-derive task scope from code in Specify, not the milestone's framing (a "perf-only" task hid a latent correctness bug on the same path) - [TDD] posting/index fixtures must use DISTINCT tfs; equal values mask positional misalignment (the bug survived because all prior tests used equal) - [ADD] risk:high + conservative correctly forced the human gate on an otherwise-green change with a real judgement call ADD: fts-posting-rank-tf -> phase done, gate PASS. Milestone v3-1-fts-hardening stays open (1/1 tasks done) until its goal (all combinators correct + true counts + no cliffs) is met by the remaining tasks. author: Tin Dang
Mark task 1/5 done (gate PASS, commits 45b3db8+a7e816d) and note the frozen rank-aligned PostingList TF contract that fts-upsert-incremental inherits. author: Tin Dang
A grounded code trace (component-tracer) upgraded this from two point-bugs to
ONE root cause: the FT.SEARCH query parser has no combinator layer.
- Defect 1 (OR ~10 vs 2,072): `|` is never parsed — tokenize_with_modifiers
hands it to the analyzer, unicode_words() strips it, so `a | b` becomes two
Exact AND terms (ft_text_search.rs:1095 -> search_field AND, store.rs:464).
- Defect 2 (TEXT+TAG combo 0 vs 253): parse_field_targeted_query slices terms
after the FIRST `:`, so `@tag:{bar}` is word-tokenized into spurious body
terms and the TAG filter is never dispatched (ft_text_search.rs:1273). Same
bug silently breaks @text+@num and drops the text clause in @tag-first order.
Froze a query GRAMMAR + AST (QueryNode) + eval_set set-semantics contract @ v1
(RediSearch DIALECT-2 precedence: AND > modifiers > OR, verified against
redis.io). Generic field-clause intersection (TEXT/TAG/NUMERIC), correct
matched SET + stable ordering (union scoring best-effort), 5 named error codes,
never-panic. fts-search-count-semantics now counts over eval_set(root).len().
Per the freeze decision, SPLIT the build into two tasks sharing this contract:
- fts-query-combinators (2a): recursive-descent parse_query -> QueryNode +
QueryError + parser fuzz target. Owns the frozen contract. (active, tests phase)
- fts-query-eval-dispatch (2b): eval_set (RoaringBitmap union/intersect) +
ft_text_search dispatch rewrite + wire reply. depends-on 2a; inherits contract.
author: Tin Dang
Add a recursive-descent parser that turns an FT.SEARCH query string into a
`QueryNode` AST over the frozen RediSearch subset (terms+modifiers ·
implicit-AND · OR `|` · grouping `()` · `@field:` clauses · TAG `{a|b}` ·
NUMERIC `[min max]`), with DIALECT-2 precedence (AND > modifiers > OR) and the
five contracted error codes. This is task 2a of the fts-query-combinators split
(contract FROZEN @ v1); the evaluator + dispatch wiring is 2b
(fts-query-eval-dispatch), which consumes this AST.
New module src/text/query/ (text-index gated):
- ast.rs: QueryNode { Term{field,token,modifier} · And · Or · Tag{field,values}
· Numeric{field,min,max,min_excl,max_excl} · Empty } + QueryError with a
code() -> wire string ("syntax_error" / "empty_query" / "unknown_field" /
"numeric_filter_invalid" / "tag_filter_invalid").
- parse.rs: parse_query(bytes, &QuerySchema) -> Result<QueryNode, QueryError>,
pure (no index/analyzer — tokens are RAW, analyzed at eval time). QuerySchema
resolves @field -> kind (case-insensitive) from names (tests) or a TextIndex.
Design / safety:
- Fixes the two root causes the trace found: `|` is now a first-class Or node
(never stripped by the analyzer), and a second `@field:` clause parses as its
own typed node (Tag/Numeric) instead of being word-tokenized into text terms.
- Never panics on malformed input (M7): every bad shape -> a QueryError. The
descent is depth-bounded (MAX_DEPTH) so deep `(((…)))` can't blow the stack.
- No unwrap/expect on the parse path; numeric-bound parsing is local (mirrors
the command-layer parse_numeric_bound: '(' exclusive, ±inf) so text/ keeps no
upward dependency on command/.
- Field clause uses `term+` (a field scopes its bare terms — Moon-compatible,
keeps M5 regression-safe); RediSearch's stricter one-token binding is deferred.
Tests (tests/fts_query_parse.rs, red before this commit) + fuzz target:
- 19 parser unit tests: OR-node, pipe-never-dropped, multi-clause typed nodes,
text+numeric, grouping, field-scoped group, AND-binds-tighter precedence,
modifiers, exclusive/±inf numeric, every reject -> exact QueryError, and a
never-panic table. All 19 green.
- fuzz/fuzz_targets/fts_query_parse.rs (+ text-index enabled in fuzz crate) —
parse_query never panics on arbitrary bytes (CLAUDE.md new-parser rule).
Evidence: 19/19 new green; full lib 3584/0; text:: 144/0 (M5 no-regression);
cargo fmt --check clean; cargo clippy -- -D warnings clean; fuzz target type-
checks warning-free on nightly.
ADD: fts-query-combinators 2a, build phase. parse_query/QueryNode are referenced
by tests + the fuzz target now; production dispatch wiring lands in 2b.
author: Tin Dang
…inators 2a) Add test_error_codes_match_contract asserting QueryError::code() maps 1:1 to the five frozen §3 wire codes (syntax_error / empty_query / unknown_field / numeric_filter_invalid / tag_filter_invalid). Locks the contract so 2b's dispatch can't drift, and wires code() (previously referenced only by 2b). author: Tin Dang
…rred to 2b) 2a (parser) verify §6 filled: 20/20 parser tests, full lib 3584/0, text 144/0 (M5 net), fmt/clippy/fuzz clean; pure function (no concurrency residue), parser hardened on the untrusted boundary (never-panic + depth-bound + raw-byte safe), clean text<-command layering. Per the human decision at the 2a gate, the gate is NOT taken standalone — build 2b (eval + dispatch) then present ONE combined verify gate for the wired end-to-end feature. author: Tin Dang
… (2b) Replace the four duplicated inline query parsers across the FT.SEARCH text dispatch with one centralized kernel. Every text branch now routes raw query bytes through parse_query -> eval_query -> build_text_response. - src/text/query/eval.rs (new): eval_set folds the QueryNode AST to a RoaringBitmap (And = intersection, Or = union, leaves reuse search_field/_or/_tag/_numeric_range over the shared doc-id space); eval_query adds best-effort BM25 (text leaves summed across OR branches, pure-filter docs score 0.0, order score DESC / doc_id ASC). Also collect_df_field_terms (Phase-1 df scatter, at-most-one entry to keep the per-shard N-sentinel invariant) and collect_highlight_terms. - Cross-shard payload: TextSearchPayload now carries opaque query Bytes re-parsed per shard instead of a flat (field_idx, Vec<QueryTerm>) that could not represent an OR/AST -- this fixes OR in every shard config. - Rewire handler_single / handler_sharded / handler_monoio + spsc_handler + coordinator scatter through run_text_query / run_text_query_on_index. - Malformed queries return Frame::Error with the frozen 5 codes; the server never panics on bad query bytes (defensive parse path). Fixes the two FTS combinator bugs: OR (|) silently degraded to AND, and multi-@clause queries (e.g. @Body:foo @tag:{bar}) returned 0 results. Removes the inline-parser duplication (net source reduction). HYBRID / vector-KNN / SPARSE / SESSION / RANGE dispatch stays byte-identical. Verified: lib 3584/0; e2e 12/12; tokio FT integration 5/5; redis-cli smoke + numeric 1-shard-vs-4-shard identity probe correct on shards 1 AND 4. Implements fts-query-eval-dispatch (2b) against the frozen fts-query-combinators (2a) §3 contract. author: Tin Dang
…uards (2b) End-to-end suite asserting matched key SETS (not parser internals) through the wired parse_query -> eval_query -> build_text_response path. Uses distinct corpora so union != intersection is observable. - OR union, TEXT+TAG and TEXT+NUMERIC intersect, grouping, deterministic OR-scoring order, malformed -> coded error (server stays up), unknown-field error, empty-result-not-error, no-regression single AND/filter, kernel-direct. - Two Phase-1 df-scatter guards: test_df_field_terms_single_entry_invariant and test_df_field_terms_pure_filter_vs_fuzzy -- pin the at-most-one-entry rule that keeps the per-shard N sentinel from being summed wrong across shards. 12/12 green. File-gated #![cfg(feature = "text-index")]. Covers fts-query-eval-dispatch (2b) §2 scenarios. author: Tin Dang
…gate PASS - fts-query-eval-dispatch §6: full verify evidence (lib 3584/0, e2e 12/12, tokio FT integration 5/5, over-the-wire smoke + numeric x-shard identity probe on shards 1 AND 4), deep WIRING/DEAD-CODE checks, and three carried flags. - §7 observe: competency deltas (TDD: assert nonzero ran-count per suite -- 4 feature-gated FT tests ran 0 cases under a green run; the stale #[ignore]'d numeric test is seed-broken; SDD: a flat cross-shard payload could not express the OR algebra; ADD: split-parser/wire-then-gate-the-pair pattern worked). - Both tasks gated PASS at the combined 2a+2b human gate (Tin Dang, "PASS both"); fts-query-combinators GATE RECORD resolved from deferred -> PASS. - state.json: 2a and 2b -> done / gate PASS. Deferred follow-ups recorded as flags: dead scatter_text_search_filter cleanup (FLAG-2, cascades into the InvertedSearch wire protocol); multi-field OR IDF approximation (FLAG-3, set/order correct, magnitude approximate); fix + un-ignore the stale inverted_search_numeric_shard_consistency test (FLAG-1). author: Tin Dang
TinDang97
force-pushed
the
feat/v3-1-fts-posting-rank-tf
branch
from
83241b8 to
98b3154
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/conn/handler_monoio/ft.rs (1)
428-512:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove the disabled-feature text error outside the
text-indexcfg block.The
#[cfg(not(feature = "text-index"))]arm on Lines 509-512 is nested inside the outer#[cfg(feature = "text-index")]FT.SEARCH block, so it is never compiled whentext-indexis disabled. In that build, single-shard text queries fall through to the vector parser instead of returningERR text-index feature not enabled.Proposed fix
+ #[cfg(not(feature = "text-index"))] + if cmd.eq_ignore_ascii_case(b"FT.SEARCH") { + if let Some(Frame::BulkString(query_bytes)) = cmd_args.get(1) { + if crate::command::vector_search::is_text_query(query_bytes.as_ref()) { + responses.push(Frame::Error(Bytes::from_static( + b"ERR text-index feature not enabled", + ))); + return true; + } + } + } + #[cfg(feature = "text-index")] if cmd.eq_ignore_ascii_case(b"FT.SEARCH") {🤖 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/server/conn/handler_monoio/ft.rs` around lines 428 - 512, The `#[cfg(not(feature = "text-index"))]` error branch returning "ERR text-index feature not enabled" is nested inside the outer `#[cfg(feature = "text-index")]` block, which means it never compiles when the text-index feature is disabled. Move the disabled-feature error handling outside the outer `#[cfg(feature = "text-index")]` conditional that wraps the entire FT.SEARCH command block. This error response should be returned when FT.SEARCH is called and the text-index feature is not enabled, preventing text queries from falling through to the vector parser when the feature is disabled.
🧹 Nitpick comments (2)
.add/tasks/fts-query-eval-dispatch/TASK.md (1)
256-290: ⚡ Quick winEnsure FLAG-1 (stale numeric test) intent coverage is tracked for follow-up.
FLAG-1 (lines 282–287) identifies a pre-existing stale test (
inverted_search_numeric_shard_consistency) that cannot run due to a seed annotation mismatch. The mitigation is that intent was "reproduced manually" via the numeric x-shard probe (lines 278–279). This is an acceptable short-term workaround, but the follow-up (fix annotation + per-test isolation + un-ignore) must be tracked as a real task to prevent the gap from becoming permanent.Recommendation: ensure this follow-up is added to the project backlog or linked to a milestone so the real cross-shard numeric guard is restored.
🤖 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-eval-dispatch/TASK.md around lines 256 - 290, Ensure FLAG-1 follow-up work is properly tracked in your project's backlog or issue tracker. The stale test inverted_search_numeric_shard_consistency (currently #[ignore]) has a known issue where seed() fails due to annotation mismatch (let _: i64 expects int but hset_multiple returns OK string), and while the intent was reproduced manually in the numeric x-shard probe (6/6 pass), the real follow-up tasks must be recorded: fix the annotation to properly handle the OK response, implement per-test index isolation, and remove the #[ignore] attribute to restore the automated guard. Add this as a tracked task or milestone item to prevent this testing gap from becoming permanent.src/server/conn/handler_single.rs (1)
1367-1405: 💤 Low valueRedundant inner feature gates — dead code in
#[cfg(not(feature = "text-index"))]branch.Lines 1367 and 1399–1404 are inside the outer
#[cfg(feature = "text-index")]block at line 1324. Whentext-indexis disabled, the outer gate excludes this entire path, so the inner#[cfg(not(feature = "text-index"))]block can never compile in. The error frame at lines 1401–1403 is unreachable dead code.The inner
#[cfg(feature = "text-index")]at line 1367 is similarly redundant but harmless.🔧 Suggested simplification
Remove the redundant inner feature gates since the outer gate at line 1324 already ensures
text-indexis enabled:- #[cfg(feature = "text-index")] { let mut response = crate::command::vector_search::run_text_query( &*ts_mut, // ... ); // ... highlight/summarize logic ... 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; - }🤖 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/server/conn/handler_single.rs` around lines 1367 - 1405, Remove the redundant inner feature gates since the outer gate at line 1324 already ensures text-index is enabled. Delete the `#[cfg(feature = "text-index")]` attribute guard at line 1367 and its closing brace, and remove the entire `#[cfg(not(feature = "text-index"))]` block at lines 1399-1404 (the dead code containing the error frame). Move the implementation code that calls `run_text_query` and the subsequent post-processing logic directly into the outer feature-gated context so it executes without redundant inner guards.
🤖 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 `@src/shard/spsc_handler.rs`:
- Around line 1337-1407: Split the oversized Rust files to comply with the
1500-line limit. At src/shard/spsc_handler.rs (lines 1337-1407), extract the
text search handling logic (the match statement processing TextSearchPayload
with feature gates, the run_text_query_on_index call, and post-processing with
apply_post_processing) into a focused submodule (e.g., text_search_handler),
leaving shared wiring and dispatch logic in mod.rs. At src/shard/dispatch.rs
(lines 278-282), extract message payload type definitions (including
TextSearchPayload and other message types) into dedicated payload modules
organized by category (text_payloads, graph_payloads, pubsub_payloads, etc.),
then re-export all types from mod.rs to preserve existing API paths and avoid
breaking imports throughout the codebase.
In `@src/text/query/eval.rs`:
- Around line 127-145: The issue is that results are truncated at line 145 with
results.truncate(top_k) before the full match cardinality is preserved, causing
build_text_response() to report only the page size instead of the actual total
matches. Save the cardinality of the results set (or eval_set) before truncating
to top_k, then either return this full count alongside the truncated results
from eval_query, or pass it explicitly to build_text_response() so it can report
the correct total match count independently of the page size returned.
- Around line 153-177: The `accumulate_text_scores()` function unconditionally
accumulates scores from all text leaves in every branch, regardless of whether
documents actually matched those branches. In the `QueryNode::And(children) |
QueryNode::Or(children)` match arm, modify the logic to first determine which
documents matched each child branch by calling `eval_set(child)`, then only
accumulate scores for documents present in that child's matched set. This
ensures documents only receive score contributions from branches they actually
matched, preventing incorrect reordering caused by scoring documents against
failed branches.
In `@tests/fts_posting_rank_tf.rs`:
- Around line 98-131: The test `test_high_df_tf_lookup_is_sublinear` contains a
hard wall-clock assertion on `best < std::time::Duration::from_millis(50)` that
is machine and load dependent, causing nondeterministic CI failures. Remove this
timing-based assertion from the default unit test flow. Instead, keep the
correctness validation of the TF lookups (the loop that accumulates tf values
via `p.tf(d)` and verifies the operation completes) as the core test, and either
conditionally compile the timing gate with a performance-test flag or
environment variable, or remove it entirely from the default test. The goal is
to preserve correctness checks that run in all test environments while isolating
performance assertions for dedicated perf testing only.
---
Outside diff comments:
In `@src/server/conn/handler_monoio/ft.rs`:
- Around line 428-512: The `#[cfg(not(feature = "text-index"))]` error branch
returning "ERR text-index feature not enabled" is nested inside the outer
`#[cfg(feature = "text-index")]` block, which means it never compiles when the
text-index feature is disabled. Move the disabled-feature error handling outside
the outer `#[cfg(feature = "text-index")]` conditional that wraps the entire
FT.SEARCH command block. This error response should be returned when FT.SEARCH
is called and the text-index feature is not enabled, preventing text queries
from falling through to the vector parser when the feature is disabled.
---
Nitpick comments:
In @.add/tasks/fts-query-eval-dispatch/TASK.md:
- Around line 256-290: Ensure FLAG-1 follow-up work is properly tracked in your
project's backlog or issue tracker. The stale test
inverted_search_numeric_shard_consistency (currently #[ignore]) has a known
issue where seed() fails due to annotation mismatch (let _: i64 expects int but
hset_multiple returns OK string), and while the intent was reproduced manually
in the numeric x-shard probe (6/6 pass), the real follow-up tasks must be
recorded: fix the annotation to properly handle the OK response, implement
per-test index isolation, and remove the #[ignore] attribute to restore the
automated guard. Add this as a tracked task or milestone item to prevent this
testing gap from becoming permanent.
In `@src/server/conn/handler_single.rs`:
- Around line 1367-1405: Remove the redundant inner feature gates since the
outer gate at line 1324 already ensures text-index is enabled. Delete the
`#[cfg(feature = "text-index")]` attribute guard at line 1367 and its closing
brace, and remove the entire `#[cfg(not(feature = "text-index"))]` block at
lines 1399-1404 (the dead code containing the error frame). Move the
implementation code that calls `run_text_query` and the subsequent
post-processing logic directly into the outer feature-gated context so it
executes without redundant inner guards.
🪄 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: b44fae3f-1131-4c82-a6c1-622ad6604cad
⛔ Files ignored due to path filters (1)
fuzz/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
.add/milestones/v3-1-fts-hardening/MILESTONE.md.add/milestones/v3-2-graph-correctness/MILESTONE.md.add/milestones/v3-3-vector-kv-polish/MILESTONE.md.add/state.json.add/tasks/fts-posting-rank-tf/TASK.md.add/tasks/fts-query-combinators/TASK.md.add/tasks/fts-query-eval-dispatch/TASK.mdfuzz/Cargo.tomlfuzz/fuzz_targets/fts_query_parse.rssrc/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/coordinator.rssrc/shard/dispatch.rssrc/shard/spsc_handler.rssrc/text/mod.rssrc/text/posting.rssrc/text/query/ast.rssrc/text/query/eval.rssrc/text/query/mod.rssrc/text/query/parse.rssrc/text/store.rstests/fts_posting_rank_tf.rstests/fts_query_eval_e2e.rstests/fts_query_parse.rs
Comment on lines
+1337
to
+1407
| // 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); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Split oversized shard modules to comply with the Rust file-size policy.
Both files are well over the 1500-line cap, which makes ongoing changes harder to review and increases risk in hot paths.
src/shard/spsc_handler.rs#L1337-L1407: Split into focused submodules (e.g., text_search handling, vector/hybrid handlers, WAL/AOF helpers) and keep shared wiring in a smallermod.rs.src/shard/dispatch.rs#L278-L282: Split message payload/type definitions into dedicated modules (e.g., text payloads, graph payloads, pubsub payloads) and re-export frommod.rsto preserve API paths.
As per coding guidelines, "No single .rs file should exceed 1500 lines. Split into submodules if approaching this limit."
📍 Affects 2 files
src/shard/spsc_handler.rs#L1337-L1407(this comment)src/shard/dispatch.rs#L278-L282
🤖 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/shard/spsc_handler.rs` around lines 1337 - 1407, Split the oversized Rust
files to comply with the 1500-line limit. At src/shard/spsc_handler.rs (lines
1337-1407), extract the text search handling logic (the match statement
processing TextSearchPayload with feature gates, the run_text_query_on_index
call, and post-processing with apply_post_processing) into a focused submodule
(e.g., text_search_handler), leaving shared wiring and dispatch logic in mod.rs.
At src/shard/dispatch.rs (lines 278-282), extract message payload type
definitions (including TextSearchPayload and other message types) into dedicated
payload modules organized by category (text_payloads, graph_payloads,
pubsub_payloads, etc.), then re-export all types from mod.rs to preserve
existing API paths and avoid breaking imports throughout the codebase.
Source: Coding guidelines
Comment on lines
+127
to
+145
| let mut results: Vec<TextSearchResult> = 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); |
There was a problem hiding this comment.
Preserve full match cardinality before truncating.
Line 145 truncates the only value returned by eval_query, and build_text_response() later reports results.len() as the FT.SEARCH total. With LIMIT 0 10, a query matching 1,000 docs reports 10, not the full match count. Return the eval_set cardinality alongside the page candidates, or pass an explicit total into response building before truncation.
🤖 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/query/eval.rs` around lines 127 - 145, The issue is that results are
truncated at line 145 with results.truncate(top_k) before the full match
cardinality is preserved, causing build_text_response() to report only the page
size instead of the actual total matches. Save the cardinality of the results
set (or eval_set) before truncating to top_k, then either return this full count
alongside the truncated results from eval_query, or pass it explicitly to
build_text_response() so it can report the correct total match count
independently of the page size returned.
Comment on lines
+153
to
+177
| fn accumulate_text_scores( | ||
| node: &QueryNode, | ||
| idx: &TextIndex, | ||
| global_df: Option<&HashMap<String, u32>>, | ||
| global_n: Option<u32>, | ||
| scores: &mut HashMap<u32, f32>, | ||
| ) { | ||
| 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); | ||
| } | ||
| } |
There was a problem hiding this comment.
Score only terms from branches that matched the document.
accumulate_text_scores() walks every TEXT leaf unconditionally. For (@tag:{red} alpha) | beta, a doc that matches beta and contains alpha but fails @tag:{red} still receives alpha’s score from the failed branch, which can reorder results incorrectly. Carry branch-local membership into scoring, e.g. score each child against eval_set(child) and only add contributions for docs in that child’s matched set.
🤖 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/query/eval.rs` around lines 153 - 177, The
`accumulate_text_scores()` function unconditionally accumulates scores from all
text leaves in every branch, regardless of whether documents actually matched
those branches. In the `QueryNode::And(children) | QueryNode::Or(children)`
match arm, modify the logic to first determine which documents matched each
child branch by calling `eval_set(child)`, then only accumulate scores for
documents present in that child's matched set. This ensures documents only
receive score contributions from branches they actually matched, preventing
incorrect reordering caused by scoring documents against failed branches.
Comment on lines
+98
to
+131
| /// 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" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Avoid hard wall-clock assertions in default unit test.
This < 50ms gate is machine/load dependent and can make CI fail nondeterministically even when lookup complexity is correct. Keep this as a perf-only assertion (ignored/env-gated) and preserve correctness checks in default test runs.
Suggested adjustment
#[test]
+#[ignore = "timing-dependent perf check; run in dedicated perf environment"]
fn test_high_df_tf_lookup_is_sublinear() {
@@
- assert!(
- best < std::time::Duration::from_millis(50),
- "high-DF TF lookups took {best:?} — expected sub-linear (rank), not O(N) per call"
- );
+ assert!(
+ best < std::time::Duration::from_millis(50),
+ "high-DF TF lookups took {best:?} — expected sub-linear (rank), not O(N) per call"
+ );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// 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" | |
| ); | |
| } | |
| /// M2 — TF lookup is sub-linear: many lookups on a large posting must not be O(N) each. | |
| #[test] | |
| #[ignore = "timing-dependent perf check; run in dedicated perf environment"] | |
| 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" | |
| ); | |
| } |
🤖 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 `@tests/fts_posting_rank_tf.rs` around lines 98 - 131, The test
`test_high_df_tf_lookup_is_sublinear` contains a hard wall-clock assertion on
`best < std::time::Duration::from_millis(50)` that is machine and load
dependent, causing nondeterministic CI failures. Remove this timing-based
assertion from the default unit test flow. Instead, keep the correctness
validation of the TF lookups (the loop that accumulates tf values via `p.tf(d)`
and verifies the operation completes) as the core test, and either conditionally
compile the timing gate with a performance-test flag or environment variable, or
remove it entirely from the default test. The goal is to preserve correctness
checks that run in all test environments while isolating performance assertions
for dedicated perf testing only.
CI Lint (cargo fmt --check) wanted line-wrapping on long add_doc()/assert! calls in the e2e test plus a few eval/coordinator/handler lines. Formatting-only, no logic change -- restores fmt-clean across the v3-1 diff. author: Tin Dang
Fixed: FT.SEARCH OR/multi-clause result-set correctness. Performance: high-DF term query O(M^2) cliff removed (rank-aligned posting TF). Satisfies the CI CHANGELOG check. author: Tin Dang
pilotspacex-byte
added a commit
that referenced
this pull request
Jun 16, 2026
…sks) (#192) Completes the v3-1-fts-hardening milestone (final 3 tasks; #189/#190 landed the rest). - fts-upsert-incremental (8e6488d): O(V)-per-doc re-index scan → reverse doc_id→term_ids index, O(terms-in-doc). Search output byte-identical. - fts-search-count-semantics (93f0ada): FT.SEARCH integer reply = true total-matched (pre-truncation), multi-shard = Σ per-shard count. RediSearch semantics. - fts-query-routing-robustness (4de1d6c): R1 search_field 3× expect→let-else (no panic); R2 is_text_query keys on canonical [KNN bracket (prose 'knn' searches as text); R3 has_sparse_clause defers standalone SPARSE to the vector engine at all 6 text-route gates. CI all green. Full lib regression 3597/0; tokio-runtime lib 2960/0.
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 — posting-rank TF + query combinators (parser + evaluator + dispatch)
Consolidates the biggest FTS correctness gaps the 2026-06-16 4-feature benchmark exposed vs
RediSearch. Three ADD tasks, each gated PASS:
1.
fts-posting-rank-tf— kill the BM25 corruption + O(M²) cliffPostingListnow keepsterm_freqs/positionsin sorted-doc_id (rank) order, looked up viaRoaringBitmap::rank(sub-linear) instead of a linear scan. Fixes a latent BM25 bug wherere-indexing an updated low-id doc pushed its TF to the end → misaligned scores, and removes the
high-DF O(M²) query cliff. (commits
45b3db8,a7e816d)2 + 3.
fts-query-combinators(2a parser) +fts-query-eval-dispatch(2b eval+dispatch)Fixes the two combinator bugs: OR (
|) silently degraded to AND, and multi-@clausequeries (
@body:foo @tag:{bar}) returned 0 results.QueryNodeAST (term/AND/OR/field-clause/group),with 5 wire error codes locked by tests. (commits
0cde883,750d8cd,7a4ac75)eval_querykernel replaces the four duplicated inline parsers acrossthe FT.SEARCH text dispatch.
eval_setfolds the AST to aRoaringBitmap(And = ∩, Or = ∪,leaves reuse
search_field/_or/_tag/_numeric_rangeover the shared doc-id space);eval_queryadds best-effort BM25 (text leaves summed across OR branches, pure-filter docs 0.0, order
score-DESC/doc_id-ASC). Cross-shard payload changed from a flat
(field_idx, Vec<QueryTerm>)— which could not represent an OR/AST — to opaque query
Bytesre-parsed per shard, fixing ORin every shard config. Malformed queries return
Frame::Errorwith a coded reason; theserver never panics on bad query bytes. HYBRID / vector-KNN / SPARSE / SESSION / RANGE dispatch
untouched. (commits
9023a92,87a13ac,83241b8)Verification (correctness only — production magnitudes are GCloud/VM, out of scope here)
cargo test --lib: 3584 passed, 0 failedfts_query_eval_e2e: 12/12 (incl. 2 Phase-1 N-invariant df-scatter guards)runtime-tokio,text-index,graph):hybrid_filter_tag1/1,ft_search_as_of_filter1/1,txn_ft_search_snapshot3/3alpha | beta→3,alpha beta→1,@body:alpha @tag:{bar}→1,@body:beta @price:[15 25]→1, malformed→syntax_errorKnown follow-ups (flagged, not silently shipped)
tests/inverted_search_numeric_shard_consistency.rsis a pre-existing stale#[ignore]'d manual test (seed panics onlet _: i64vs HMSET's+OK, before any assertion).Independent of this PR; intent reproduced manually. Follow-up: fix annotation + per-test index
isolation, then un-ignore.
scatter_text_search_filter(coordinator) left in place; removal cascades intothe InvertedSearch wire-protocol enum (separate cleanup).
per-shard N-sentinel invariant). Set/order correct (proven x-shard); only BM25 magnitude approx.
Unblocks
fts-search-count-semantics(consumeseval_set(root).len()as the true total-matched).Summary by CodeRabbit
New Features
Bug Fixes
Tests