feat(vector): FastScan SIMD scan — NEON TBL kernel + live mutable-path integration#248
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? |
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (46)
💤 Files with no reviewable changes (12)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (18)
📝 WalkthroughWalkthroughAdds vector and graph tuning defaults, persists per-index search knobs, threads tuning through search execution, updates exact-beam and L2 scoring paths, expands FastScan kernels and LUT construction, adds a mutable-segment FastScan pre-filter, and updates docs, benchmarks, and changelog entries. ChangesVector Search Runtime Tuning and FastScan Improvements
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 1
🧹 Nitpick comments (1)
BENCHMARK.md (1)
883-888: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the benchmark images.
redis/redis-stack-server:latestandqdrant/qdrant:latestmake these numbers drift as upstream images move. Please capture the exact tags/digests used for the run so the published tables stay reproducible.🤖 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 `@BENCHMARK.md` around lines 883 - 888, The benchmark environment description uses floating container tags, which makes the reported results non-reproducible. Update the benchmark entry in BENCHMARK.md to replace the RedisSearch and Qdrant image references with the exact tags or digests used for this run, keeping the rest of the environment details unchanged. Use the existing environment section near the RediSearch and Qdrant mentions to locate the edit.
🤖 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 `@docs/vector-search-guide.md`:
- Around line 49-51: Update the `EF_RUNTIME` documentation entry in the vector
search guide so it matches the actual auto-beam logic used by the search path.
The current text in the table describes `0/omit = auto` as `max(k×15, 200)`, but
the implementation resolves auto EF through the search behavior that uses
`max(k*20, 200)` with the dimension boost. Revise the wording in that
`EF_RUNTIME` row to reflect the real formula and keep the runtime-set/restore
note intact.
---
Nitpick comments:
In `@BENCHMARK.md`:
- Around line 883-888: The benchmark environment description uses floating
container tags, which makes the reported results non-reproducible. Update the
benchmark entry in BENCHMARK.md to replace the RedisSearch and Qdrant image
references with the exact tags or digests used for this run, keeping the rest of
the environment details unchanged. Use the existing environment section near the
RediSearch and Qdrant mentions to locate the edit.
🪄 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: 25dfb6d2-db87-4cfd-913b-6675a0b2c6e4
📒 Files selected for processing (13)
BENCHMARK.mdCHANGELOG.mdCargo.tomlbenches/fastscan_bench.rsdocs/guides/tuning.mddocs/vector-search-guide.mdsrc/command/vector_search/ft_config.rssrc/command/vector_search/ft_create.rssrc/command/vector_search/tests.rssrc/server/conn/handler_monoio/ft.rssrc/vector/distance/fastscan.rssrc/vector/segment/ivf.rssrc/vector/segment/mutable.rs
Comment on lines
+49
to
+51
| | `EF_RUNTIME` | auto | 10-4096 | Search beam width. 0/omit = auto: max(k×15, 200). Higher = better recall, lower QPS. Tunable at runtime: `FT.CONFIG SET <idx> EF_RUNTIME <n>` (0 = restore auto) — applies to the next search, no rebuild | | ||
| | `COMPACT_THRESHOLD` | 1000 | 100-100000 | Min vectors before auto-compaction. Higher = fewer larger HNSW graphs | | ||
| | `QUANTIZATION` | TQ4 | TQ1-TQ4, SQ8 | Compression level. TQ4 = 4-bit (best compression), SQ8 = 8-bit (higher recall) | | ||
| | `QUANTIZATION` | TQ4 | TQ1-TQ4, SQ8 | Compression level. TQ4 = 4-bit (best compression, **unit-sphere metrics COSINE/IP only** — collapses on unnormalized L2), SQ8 = 8-bit (higher recall, all metrics) | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the auto-beam formula.
This entry says EF_RUNTIME = 0 uses max(k×15, 200), but the search path still resolves auto EF as max(k*20, 200) with the dimension boost. That mismatch will mislead tuning.
♻️ Suggested text update
-| `EF_RUNTIME` | auto | 10-4096 | Search beam width. 0/omit = auto: max(k×15, 200). Higher = better recall, lower QPS. Tunable at runtime: `FT.CONFIG SET <idx> EF_RUNTIME <n>` (0 = restore auto) — applies to the next search, no rebuild |
+| `EF_RUNTIME` | auto | 10-4096 | Search beam width. 0/omit = auto: max(k×20, 200) with the same dimension boost used by the search path. Higher = better recall, lower QPS. Tunable at runtime: `FT.CONFIG SET <idx> EF_RUNTIME <n>` (0 = restore auto) — applies to the next search, no rebuild |📝 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
| | `EF_RUNTIME` | auto | 10-4096 | Search beam width. 0/omit = auto: max(k×15, 200). Higher = better recall, lower QPS. Tunable at runtime: `FT.CONFIG SET <idx> EF_RUNTIME <n>` (0 = restore auto) — applies to the next search, no rebuild | | |
| | `COMPACT_THRESHOLD` | 1000 | 100-100000 | Min vectors before auto-compaction. Higher = fewer larger HNSW graphs | | |
| | `QUANTIZATION` | TQ4 | TQ1-TQ4, SQ8 | Compression level. TQ4 = 4-bit (best compression), SQ8 = 8-bit (higher recall) | | |
| | `QUANTIZATION` | TQ4 | TQ1-TQ4, SQ8 | Compression level. TQ4 = 4-bit (best compression, **unit-sphere metrics COSINE/IP only** — collapses on unnormalized L2), SQ8 = 8-bit (higher recall, all metrics) | | |
| | `EF_RUNTIME` | auto | 10-4096 | Search beam width. 0/omit = auto: max(k×20, 200) with the same dimension boost used by the search path. Higher = better recall, lower QPS. Tunable at runtime: `FT.CONFIG SET <idx> EF_RUNTIME <n>` (0 = restore auto) — applies to the next search, no rebuild | | |
| | `COMPACT_THRESHOLD` | 1000 | 100-100000 | Min vectors before auto-compaction. Higher = fewer larger HNSW graphs | | |
| | `QUANTIZATION` | TQ4 | TQ1-T4, SQ8 | Compression level. TQ4 = 4-bit (best compression, **unit-sphere metrics COSINE/IP only** — collapses on unnormalized L2), SQ8 = 8-bit (higher recall, all metrics) | |
🤖 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 `@docs/vector-search-guide.md` around lines 49 - 51, Update the `EF_RUNTIME`
documentation entry in the vector search guide so it matches the actual
auto-beam logic used by the search path. The current text in the table describes
`0/omit = auto` as `max(k×15, 200)`, but the implementation resolves auto EF
through the search behavior that uses `max(k*20, 200)` with the dimension boost.
Revise the wording in that `EF_RUNTIME` row to reflect the real formula and keep
the runtime-set/restore note intact.
TinDang97
added a commit
that referenced
this pull request
Jul 8, 2026
Two per-index runtime knobs to close the recall gap to exact-beam engines (Qdrant reaches ~1.0 R@10 at ef 256 because its beam navigates full-precision vectors; Moon's beam ranked candidates by quantized ADC): - RERANK_MULT <1-64> (default 4): deepens the HQ-1 exact-rerank stage — the top mult*k beam candidates are re-scored with true f16 sidecar distances before top-k truncation, recovering true neighbors the ADC ranking dropped below the old fixed 4*k cut. Cost ~mult*k*dim f16 decodes per segment. - EXACT_BEAM ON|OFF (default OFF): the HNSW beam itself navigates with exact f16 sidecar distances instead of quantized ADC — candidate SELECTION becomes exact, recall goes graph-limited. QPS cost grows with dimension; sidecar-less segments silently keep the ADC beam. Implementation: - SearchTuning carrier threaded IndexMeta -> MvccContext/SearchSnapshot -> all immutable-segment search paths (sync, yielding, worker-pool fan-out, FT.RECOMMEND, hybrid) via *_with_tuning variants; the knob-free entry points keep the defaults, so no behavior change unless a knob is set. - hnsw_search_filtered gains an exact_f16 override checked at the head of both distance closures (one predicted branch when off); distance conventions match rerank_exact so cross-segment merge stays consistent. rerank_exact is skipped under EXACT_BEAM (distances are already true metric values). - Index-meta sidecar v4: appends rerank_mult + exact_beam per index; v1-v3 sidecars load with defaults; out-of-range persisted mult clamps back to 4. FT.CONFIG SET broadcasts to all shards (monoio fix from the EF_RUNTIME wave) and persists via save_index_meta_sidecar. Tests: FT.CONFIG set/get/validation for both knobs; sidecar v4 roundtrip + v1 default-migration + corrupt-mult clamp; functional red/green — RERANK_MULT monotone recall (mult 16 strictly beats mult 1 on seeded TQ4), EXACT_BEAM strictly beats the ADC beam at narrow ef on varying-norm L2 data with exact returned distances, and sidecar-less fallback. Full lib suite 3962 green; fmt + clippy clean on default and tokio+jemalloc feature sets. refs PR #248 author: Tin Dang
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/vector/segment/immutable.rs (1)
1085-1096: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider reusing the shared L2 helper instead of re-deriving it inline.
The L2 branch of
tq_finis identical tol2_true_from_sphereintq_adc.rs(both take the pre-scaledd̂²·na²value and computediff² + (q_norm/na)·vwith the samena <= 0guard). Three copies of this subtle correction now exist (finish_tq,tq_fin,l2_true_from_sphere); a silent divergence would corrupt recall on one path only. Exposing the helper aspub(crate)and calling it here removes one copy.♻️ Proposed reuse
+ use crate::vector::turbo_quant::tq_adc::l2_true_from_sphere; let l2_adjust = self.collection_meta.metric == crate::vector::types::DistanceMetric::L2; let tq_fin = |v: f32, na: f32| -> f32 { if !l2_adjust { return v; } - let diff = na - q_norm; - if na <= 0.0 { - return diff * diff; - } - diff * diff + (q_norm / na) * v + l2_true_from_sphere(v, na, q_norm) };(requires changing
fn l2_true_from_spheretopub(crate) fnintq_adc.rs.)🤖 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/vector/segment/immutable.rs` around lines 1085 - 1096, The L2 correction in the tq_fin closure is duplicated from l2_true_from_sphere in tq_adc.rs and should be reused instead of re-derived inline. Make l2_true_from_sphere pub(crate) and call it from immutable.rs, keeping the same na <= 0 guard and diff² + (q_norm/na)·v behavior so finish_tq, tq_fin, and l2_true_from_sphere stay in sync.src/command/vector_search/ft_search/execute.rs (1)
103-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a helper to avoid the repeated
SearchTuningconstruction.The exact literal
SearchTuning { rerank_mult: idx.meta.rerank_mult, exact_beam: idx.meta.exact_beam }is now duplicated here and insearch_local_filtered(Lines 287-290),dispatch.rs,hybrid.rs(×2), andrecommend.rs. A single constructor keeps the mapping in one place, so a future field onSearchTuningcan't be silently dropped from one call site.♻️ Suggested helper (on
IndexMetaorSearchTuning)impl SearchTuning { #[inline] pub fn from_meta(meta: &crate::vector::store::IndexMeta) -> Self { Self { rerank_mult: meta.rerank_mult, exact_beam: meta.exact_beam } } }Then each call site becomes
let tuning = SearchTuning::from_meta(&idx.meta);.🤖 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_search/execute.rs` around lines 103 - 106, The repeated SearchTuning literal construction should be centralized so the idx.meta field mapping lives in one place. Add a helper such as SearchTuning::from_meta (or an equivalent IndexMeta method) that builds SearchTuning from IndexMeta, then replace the duplicated constructions in execute, search_local_filtered, dispatch, hybrid, and recommend with that helper to keep future fields from being missed.src/vector/segment/mutable.rs (1)
173-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffFile exceeds the 1500-line module limit.
This file is already ≥2394 lines and this PR adds further FastScan/L2 logic to it. Consider extracting the FastScan shadow helpers (
push_fastscan_shadow,maintain_fastscan_shadow,fastscan_shadow_consistent) and/or the MVCC scan paths into submodules undersrc/vector/segment/mutable/.As per coding guidelines: "No single
.rsfile should exceed 1500 lines; split larger modules into submodules."🤖 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/vector/segment/mutable.rs` around lines 173 - 178, The mutable segment module is already over the file-size limit, and the new FastScan/L2 additions should be split out of the main mutable.rs module. Move the FastScan shadow helpers (push_fastscan_shadow, maintain_fastscan_shadow, fastscan_shadow_consistent) and/or the MVCC scan logic into dedicated submodules under src/vector/segment/mutable/ so the parent file stays under the 1500-line guideline, then wire them back through the existing mutable segment types and methods.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.
Nitpick comments:
In `@src/command/vector_search/ft_search/execute.rs`:
- Around line 103-106: The repeated SearchTuning literal construction should be
centralized so the idx.meta field mapping lives in one place. Add a helper such
as SearchTuning::from_meta (or an equivalent IndexMeta method) that builds
SearchTuning from IndexMeta, then replace the duplicated constructions in
execute, search_local_filtered, dispatch, hybrid, and recommend with that helper
to keep future fields from being missed.
In `@src/vector/segment/immutable.rs`:
- Around line 1085-1096: The L2 correction in the tq_fin closure is duplicated
from l2_true_from_sphere in tq_adc.rs and should be reused instead of re-derived
inline. Make l2_true_from_sphere pub(crate) and call it from immutable.rs,
keeping the same na <= 0 guard and diff² + (q_norm/na)·v behavior so finish_tq,
tq_fin, and l2_true_from_sphere stay in sync.
In `@src/vector/segment/mutable.rs`:
- Around line 173-178: The mutable segment module is already over the file-size
limit, and the new FastScan/L2 additions should be split out of the main
mutable.rs module. Move the FastScan shadow helpers (push_fastscan_shadow,
maintain_fastscan_shadow, fastscan_shadow_consistent) and/or the MVCC scan logic
into dedicated submodules under src/vector/segment/mutable/ so the parent file
stays under the 1500-line guideline, then wire them back through the existing
mutable segment types and methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c530e266-b6cb-4e95-8e28-28046141e54a
📒 Files selected for processing (30)
BENCHMARK.mdCHANGELOG.mddocs/guides/tuning.mddocs/vector-search-guide.mdsrc/command/vector_search/ft_config.rssrc/command/vector_search/ft_create.rssrc/command/vector_search/ft_search/dispatch.rssrc/command/vector_search/ft_search/execute.rssrc/command/vector_search/hybrid.rssrc/command/vector_search/recommend.rssrc/command/vector_search/tests.rssrc/vector/hnsw/search.rssrc/vector/index_persist.rssrc/vector/persistence/recover_v2.rssrc/vector/persistence/warm_search.rssrc/vector/search_pool.rssrc/vector/segment/holder.rssrc/vector/segment/immutable.rssrc/vector/segment/mutable.rssrc/vector/store.rssrc/vector/turbo_quant/tq_adc.rssrc/vector/types.rstests/moonstore_warm_e2e.rstests/per_index_compaction_weight.rstests/quickwins_red.rstests/vector_edge_cases.rstests/vector_exact_rerank.rstests/vector_insert_bench.rstests/vector_segment_merge.rstests/vector_stress.rs
✅ Files skipped from review due to trivial changes (4)
- src/vector/persistence/recover_v2.rs
- CHANGELOG.md
- tests/per_index_compaction_weight.rs
- docs/vector-search-guide.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/command/vector_search/ft_config.rs
- BENCHMARK.md
…h integration Implements the FAISS FastScan technique (Andre et al., VLDB'15) end to end and wires it into the serving path for the first time (the existing FastScan skeleton was AVX2/scalar-only and reachable only from tests). Kernel wave (src/vector/distance/fastscan.rs): - NEON TBL kernel (vqtbl1q_u8) for aarch64, the primary platform, which previously fell back to scalar: 32 distances/block at ~2 ns/candidate (65.6 ns/block @128d, 262 ns @512d, 524 ns @1024d), 17-21x faster than the per-candidate scalar ADC path. - AVX2 overflow fix: nibble-pair distances were added as u8 before widening, wrapping for LUT pairs summing past 255 (the old parity test masked LUTs to 0x7F to hide it). All kernels now widen to u16 before adding and use saturating accumulation — scalar/NEON/AVX2 bit-identical over the full u8 LUT range. - Adaptive quantized LUT builder (build_quantized_lut): FAISS-style per-coordinate bias + global scale bounded for both u8 entries and the u16 accumulator, with f32 reconstruction (acc/scale + bias). Replaces precompute_lut, which hardcoded the legacy v1 1/sqrt(768) CENTROIDS table (encode/search codebook asymmetry) and a fixed LUT_SCALE that could overflow both domains. Live-path integration (src/vector/segment/mutable.rs): - Mutable segments maintain a FAISS-interleaved shadow of the TQ4 codes (32-vector blocks, +padded_dim/2 B/vector) written at append time. - The MVCC brute-force scan screens candidates with the SIMD kernel and exact-rescores only those whose sound lower bound (approx - 0.5*padded_dim/scale) beats the current heap worst. Results are bit-identical to the plain scan; kernel saturation only under-estimates, which routes the candidate to the exact rescore. - Measured (Apple Silicon): top-10 over 20K x 128d 1.25 ms -> 96 us (~13x), 5K x 768d 1.66 ms -> 423 us (3.9x). Engages above 64 entries; SQ8/A2 and TQ-prod scoring keep their existing paths. Tests & benches: - Full-range kernel parity tests (NEON/AVX2/dispatch vs scalar), saturation tests, LUT reconstruction-accuracy and ranking tests. - Shadow-layout test + FastScan-vs-plain equality tests covering full scans, 64-entry chunked scans, MVCC snapshot visibility, and bitmap filters. - New criterion bench benches/fastscan_bench.rs (block kernels, LUT build, end-to-end mutable scan A/B). author: Tin Dang
…roadcast fix FT.CONFIG SET <idx> EF_RUNTIME <n> now adjusts the HNSW search beam width at runtime (RediSearch parity: recall/QPS trade-off without rebuilding the index). Range matches FT.CREATE (10-4096); 0 restores the auto heuristic. The FT.SEARCH path already reads meta.hnsw_ef_runtime per query, so the change applies to the next search immediately. Value persists via the index meta sidecar. FT.CONFIG GET EF_RUNTIME added alongside. Also fixes a latent multi-shard bug in the monoio handler: FT.CONFIG SET was dispatched to the connection's local shard only, silently leaving the other N-1 shard partitions on the old value (AUTOCOMPACT, COMPACTION_WEIGHT and MERGE_RECALL_TOLERANCE were equally affected). SET now routes through broadcast_vector_command like FT.CREATE/FT.DROPINDEX; GET stays local since all shards agree after a broadcast SET. The tokio sharded handler already broadcast FT.CONFIG via its catch-all. Motivation: benchmarking recall/QPS curves previously required one full index build per ef point (EF_RUNTIME was FT.CREATE-time only) — a 3-point sweep on glove-200 (1.18M vectors) tripled ingest cost and write-stalled under the 3-index compaction backlog. footer: benchmark campaign Moon vs Qdrant/RediSearch/turbovec author: Tin Dang
… + tuning recipes Default change: FT.CREATE with DISTANCE_METRIC L2 and no explicit QUANTIZATION now defaults to SQ8 instead of TQ4. TQ's norm-scaled ADC estimator assumes unit-sphere metrics (COSINE/IP); on unnormalized L2 data it collapses — recall 0.002-0.003 measured on gist-960-euclidean (1M x 960d, GCE t2a-standard-8). SQ8's per-vector affine quantization is metric-faithful on raw L2. COSINE/IP defaults unchanged (TQ4). Explicit TQ* + L2 is honored with a tracing warning (data may be unit-normalized, where L2 is monotonic with cosine). BENCHMARK.md §10.10: first campaign on standard ANN-benchmarks datasets with bundled ground truth, full recall/QPS curves via the new runtime FT.CONFIG EF_RUNTIME. Headlines (glove-200-angular 1.18M, ARM, iso-recall): Moon 1.7x/3.2x RediSearch qps at 0.83 recall, 2.9x/3.1x Qdrant at 0.887, >10x turbovec (flat scan collapses at 1.18M); time-to-green 277s vs RediSearch 1498s. turbovec's native 100K config: Moon wins both axes at k=10, wins concurrency 1.7x at k=64. Tuning guide + vector-search guide: runtime EF_RUNTIME recipe, bulk-load --max-unflushed-immutable-segments 0 (MA1 guard counts TOTAL segments and throttles 1M+ loads 24x), VACUUM VECTOR settle recipe (segments multiply query cost: 4-5x qps after merge; local-shard caveat), TQ4 metric caveat, merged-vs-unmerged recall-ceiling note. footer: benchmark campaign 2026-07-08, Moon vs Qdrant/RediSearch/turbovec author: Tin Dang
…iso-recall bands gist-960-euclidean final rows: Moon SQ8 settled 0.816@995/2077 ef16, 0.962@528/1085 ef64, 0.994@216/472 ef256 vs Qdrant (max 0.965@134/314) and RediSearch (max 0.927@246/270, ingest 2871s vs Moon 396s). Instances stopped after campaign. author: Tin Dang
Root cause of the gist-960 recall collapse (0.002-0.003 at every ef): TQ
encodes unit DIRECTIONS plus a norm trailer, and every scoring path
ranked L2 queries by sphere_dist * ||a||^2 — the unit-sphere distance
between the normalized query and the decoded direction, scaled by the
document norm. That ranking is only metric-valid on the unit sphere
(COSINE/IP, where the encode side normalizes too). On unnormalized L2
data a small-norm vector scores near-zero regardless of direction, so
the beam fills with the smallest-norm vectors and the exact rerank
sidecar cannot rescue candidates that never entered the beam.
Fix: reconstruct the true metric from the stored document norm and the
query norm computed at prepare time:
||a - q||^2 = (||a|| - ||q||)^2 + ||a||*||q|| * d_sphere^2
applied at every TQ scoring exit when the collection metric is L2:
- HNSW beam search dist_bfs (subcent + standard LUT arms) and the
budgeted variant (budget inverted through the transform so the
early-exit check on the raw sphere sum stays valid; zero-norm vectors
now return ||q||^2 instead of 0.0)
- mutable brute-force scan (plain + FastScan-filtered chunks; the
pre-filter LOWER BOUND transforms through the same affine map, which
has non-negative slope, so soundness is preserved) and the legacy
brute-force path (also fixes TQ4A2 decoded-L2)
- immutable flat_scan and the multibit/4-bit brute-force scans
COSINE/IP scoring is byte-for-byte unchanged (the transform is gated on
metric == L2). For normalized data the transform degenerates to the old
formula (diff ~ 0, nq/na ~ 1), so existing L2-normalized tests pass
unchanged.
Red/green: new tests pin recall on varying-norm unnormalized data
against exact f32 ground truth at two levels — mutable brute-force scan
(was 4/10, now >= 7/10) and HNSW beam search (now >= 6/10). FT.CREATE's
TQ+L2 warning softened to a recommendation (SQ8 stays the L2 default:
still equal-or-better recall and no FWHT cost).
footer: gist-960-euclidean benchmark campaign follow-up
author: Tin Dang
Explicit TQ4 post-fix (14a0702, same t2a-standard-8 + harness): recall@10 0.784/0.942/0.986 at ef 16/64/256 vs 0.002-0.003 pre-fix — ~350x recovery, within ~0.03 of SQ8. Recall-only run (unsettled index, --skip-settle). Instances stopped after verification. author: Tin Dang
Two per-index runtime knobs to close the recall gap to exact-beam engines (Qdrant reaches ~1.0 R@10 at ef 256 because its beam navigates full-precision vectors; Moon's beam ranked candidates by quantized ADC): - RERANK_MULT <1-64> (default 4): deepens the HQ-1 exact-rerank stage — the top mult*k beam candidates are re-scored with true f16 sidecar distances before top-k truncation, recovering true neighbors the ADC ranking dropped below the old fixed 4*k cut. Cost ~mult*k*dim f16 decodes per segment. - EXACT_BEAM ON|OFF (default OFF): the HNSW beam itself navigates with exact f16 sidecar distances instead of quantized ADC — candidate SELECTION becomes exact, recall goes graph-limited. QPS cost grows with dimension; sidecar-less segments silently keep the ADC beam. Implementation: - SearchTuning carrier threaded IndexMeta -> MvccContext/SearchSnapshot -> all immutable-segment search paths (sync, yielding, worker-pool fan-out, FT.RECOMMEND, hybrid) via *_with_tuning variants; the knob-free entry points keep the defaults, so no behavior change unless a knob is set. - hnsw_search_filtered gains an exact_f16 override checked at the head of both distance closures (one predicted branch when off); distance conventions match rerank_exact so cross-segment merge stays consistent. rerank_exact is skipped under EXACT_BEAM (distances are already true metric values). - Index-meta sidecar v4: appends rerank_mult + exact_beam per index; v1-v3 sidecars load with defaults; out-of-range persisted mult clamps back to 4. FT.CONFIG SET broadcasts to all shards (monoio fix from the EF_RUNTIME wave) and persists via save_index_meta_sidecar. Tests: FT.CONFIG set/get/validation for both knobs; sidecar v4 roundtrip + v1 default-migration + corrupt-mult clamp; functional red/green — RERANK_MULT monotone recall (mult 16 strictly beats mult 1 on seeded TQ4), EXACT_BEAM strictly beats the ADC beam at narrow ef on varying-norm L2 data with exact returned distances, and sidecar-less fallback. Full lib suite 3962 green; fmt + clippy clean on default and tokio+jemalloc feature sets. refs PR #248 author: Tin Dang
Server-wide startup defaults for the new per-index recall knobs and the graph result cache, following the PR #249 config pattern: - --vector-ef-runtime (default 0 = heuristic): FT.CREATE EF_RUNTIME starting value when the schema omits it; explicit EF_RUNTIME wins. - --vector-rerank-mult (default 4, 1..=64): exact-rerank oversample depth seeded into new indexes. - --vector-exact-beam (bool, default off; registered in BOOL_FLAGS): beam navigation over the exact f16 sidecar for new indexes. - --graph-result-cache-entries (default 256) and --graph-result-cache-bytes (default 4 MiB): Cypher result cache limits applied at GraphStore construction. All five work identically as moon.conf keys. Values are installed at startup via first-write-wins OnceLock globals (set in main.rs and run_embedded) with unset-fallback to the compiled-in defaults, so unit tests and lib embedders keep pre-flag behavior. validate_tuning_defaults rejects out-of-range values at startup (fail-fast, before bind). Per-index FT.CONFIG SET still overrides any server default, and the knobs persist per index in the VMIX v5 sidecar as before. Defaults respect multi-db visibility scoping (db_index) unchanged. Tests: dedicated integration binary tests/vector_config_defaults.rs (OnceLock isolation from the shared lib-test process) covering default propagation into FT.CREATE, FT.CONFIG GET/SET override, explicit EF_RUNTIME precedence, and graph cache limit install; config parse + range-validation unit tests; BOOL_FLAGS registration test. Docs: configuration guide (vector table + new Graph section), tuning guide fleet-wide note, CHANGELOG. author: Tin Dang
…window
scripts/audit-unsafe.sh only scans the 3 lines immediately preceding an
`unsafe {` for the SAFETY: marker; the fastscan NEON block's comment was
6 lines long, putting the marker outside the window and failing the CI
Lint job. Compressed the comment to 3 lines — same invariants stated
(baseline NEON on aarch64, all reads bounded by the length asserts).
author: Tin Dang
…okio-only tests Two build fixes surfaced by CI and the pre-merge review: - src/command/vector_search/tests.rs: restore the 4-arg ft_create / ft_config call sites (trailing db_index) that the rebase onto main silently auto-merged back to the pre-WS5a 3-arg form (17 E0061s). - tests/txn_kv_wiring.rs, tests/workspace_integration.rs, tests/mq_integration.rs: add the five new ServerConfig fields (graph_result_cache_entries/bytes, vector_ef_runtime, vector_rerank_mult, vector_exact_beam) to full struct literals. These binaries are tokio-gated, so the default-features gate never compiled them locally. Review follow-ups from the pre-merge review wave: - index_persist.rs: doc comment now states v1..v5 support (was v1-v3). - ivf.rs: loud TODO marking the missing metric-faithful L2 correction in the IVF scorer — IVF segments are test-only today (holder.ivf is never populated in production), but the l2_adjust/tq_fin fix must land there before IVF is wired into compaction. author: Tin Dang
TinDang97
force-pushed
the
feat/vector-fastscan
branch
from
d002b0f to
be110a6
Compare
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 (2)
src/vector/store.rs (1)
70-170: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftFile far exceeds the 1500-line guideline.
src/vector/store.rsruns to roughly 4800+ lines in this review batch, well past the mandated limit forsrc/**/*.rs. This PR adds further fields/methods/tests to it. Consider splitting store/index-lifecycle management, WS5a db-scoping, background compact/merge, and warm/cold tier transition logic into submodules.As per coding guidelines, "No single
.rsfile should exceed 1500 lines; split larger modules into submodules."🤖 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/vector/store.rs` around lines 70 - 170, The store module is far over the allowed size, and this change adds more to src/vector/store.rs instead of reducing it. Split the responsibilities in store/index lifecycle code into submodules (for example db-scoping, compaction/merge, and warm/cold tier transitions) and move the related types/functions such as IndexMeta, VectorCreateDefaults, set_vector_create_defaults, and vector_create_defaults to appropriately named modules re-exported from store.rs.Source: Coding guidelines
src/vector/index_persist.rs (1)
465-483: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winProduction sidecar writer still serializes v4 —
rerank_mult/exact_beamare silently lost on every restart.
save_index_metadata_v3is the only writer actually invoked in production (viaVectorStore::save_index_meta_sidecar), and it callsserialize_index_metas_v4, notserialize_index_metas_v5. The v5 knob block (rerank_mult,exact_beam) this PR adds is therefore never written to disk in practice, even thoughserialize_index_metas()(the general entry point) correctly writes v5. AnyFT.CONFIG SET <idx> RERANK_MULT|EXACT_BEAM— and any non-default--vector-rerank-mult/--vector-exact-beambaked into an index at creation — will silently revert to the hard-coded defaults(4, false)after any restart or crash recovery, because the loader reads backVERSION_V4and falls into the pre-v5 default branch.Do you want me to add a regression test that saves via
save_index_metadata_v3with non-defaultrerank_mult/exact_beam, reloads, and asserts they round-trip (this would have caught the gap)?🐛 Proposed fix
/// Write all active index metadata **with compaction weights** to the sidecar file -/// (current on-disk format: v4, WS5a db_index; name kept as `_v3` for API stability -/// across the many existing call sites — see module docs for the v4 wire format). +/// (current on-disk format: v5, search-tuning knobs; name kept as `_v3` for API +/// stability across the many existing call sites — see module docs for the wire format). /// /// Called after FT.CREATE / FT.DROPINDEX / FT.CONFIG SET COMPACTION_WEIGHT. /// Atomically replaces the file via write-to-temp + rename. pub fn save_index_metadata_v3(shard_dir: &Path, pairs: &[(&IndexMeta, f32)]) -> io::Result<()> { let path = shard_dir.join("vector-indexes.meta"); let tmp_path = shard_dir.join(".vector-indexes.meta.tmp"); - let data = serialize_index_metas_v4(pairs); + let data = serialize_index_metas_v5(pairs);🤖 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/vector/index_persist.rs` around lines 465 - 483, The production sidecar writer is still emitting v4 metadata, so the new v5 fields like rerank_mult and exact_beam are never persisted and will reset on reload. Update save_index_metadata_v3 (and any caller like VectorStore::save_index_meta_sidecar if needed) to serialize with serialize_index_metas_v5 instead of serialize_index_metas_v4, keeping the atomic temp-file rename flow intact. Make sure the disk format written by the only production writer matches the loader’s v5 expectations so the new config knobs round-trip correctly.
🤖 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 `@docs/guides/tuning.md`:
- Around line 458-471: The settle recipe in the tuning guide needs to clearly
state that VACUUM VECTOR only affects the connection’s local shard and that the
“~8× more fresh connections” approach in the shard-coverage guidance is
best-effort rather than guaranteed. Update the wording in the settle/merge
section to either describe the replay loop as probabilistic and
repeat-until-stable, or replace it with an explicit per-shard iteration approach
if deterministic coverage is intended, keeping the guidance anchored around
VACUUM VECTOR, SO_REUSEPORT, and FT.INFO graph_segments.
In `@src/config.rs`:
- Around line 905-910: The error string in the graph result cache size
validation contains an accidental run of extra spaces, so clean up the message
in the `graph_result_cache_entries` check to use normal spacing only. Update the
`Err(...).to_string()` text in this validation block so the operator-facing
message reads naturally and contains no stray whitespace artifacts.
- Around line 882-920: Split the misplaced Rust doc block in ServerConfig so
validate_databases_bound keeps the `--databases`/MAX_DATABASES documentation and
validate_tuning_defaults only documents the tuning-default validation. Add a
separating blank line or move the `u8 db_index` comment back above
validate_databases_bound, then leave the tuning-related prose directly above
validate_tuning_defaults so each function has the correct standalone doc
comment.
---
Outside diff comments:
In `@src/vector/index_persist.rs`:
- Around line 465-483: The production sidecar writer is still emitting v4
metadata, so the new v5 fields like rerank_mult and exact_beam are never
persisted and will reset on reload. Update save_index_metadata_v3 (and any
caller like VectorStore::save_index_meta_sidecar if needed) to serialize with
serialize_index_metas_v5 instead of serialize_index_metas_v4, keeping the atomic
temp-file rename flow intact. Make sure the disk format written by the only
production writer matches the loader’s v5 expectations so the new config knobs
round-trip correctly.
In `@src/vector/store.rs`:
- Around line 70-170: The store module is far over the allowed size, and this
change adds more to src/vector/store.rs instead of reducing it. Split the
responsibilities in store/index lifecycle code into submodules (for example
db-scoping, compaction/merge, and warm/cold tier transitions) and move the
related types/functions such as IndexMeta, VectorCreateDefaults,
set_vector_create_defaults, and vector_create_defaults to appropriately named
modules re-exported from store.rs.
🪄 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: f56a0c3a-3f06-4f27-ae49-db37956770a8
📒 Files selected for processing (46)
BENCHMARK.mdCHANGELOG.mdCargo.tomlbenches/fastscan_bench.rsdocs/guides/configuration.mddocs/guides/tuning.mddocs/vector-search-guide.mdsrc/command/vector_search/ft_config.rssrc/command/vector_search/ft_create.rssrc/command/vector_search/ft_search/dispatch.rssrc/command/vector_search/ft_search/execute.rssrc/command/vector_search/hybrid.rssrc/command/vector_search/recommend.rssrc/command/vector_search/tests.rssrc/config.rssrc/config/conf_file.rssrc/graph/cypher/result_cache.rssrc/graph/store.rssrc/main.rssrc/server/conn/handler_monoio/ft.rssrc/server/embedded.rssrc/vector/distance/fastscan.rssrc/vector/hnsw/search.rssrc/vector/index_persist.rssrc/vector/persistence/recover_v2.rssrc/vector/persistence/warm_search.rssrc/vector/search_pool.rssrc/vector/segment/holder.rssrc/vector/segment/immutable.rssrc/vector/segment/ivf.rssrc/vector/segment/mutable.rssrc/vector/store.rssrc/vector/turbo_quant/tq_adc.rssrc/vector/types.rstests/moonstore_warm_e2e.rstests/mq_integration.rstests/per_index_compaction_weight.rstests/quickwins_red.rstests/txn_kv_wiring.rstests/vector_config_defaults.rstests/vector_edge_cases.rstests/vector_exact_rerank.rstests/vector_insert_bench.rstests/vector_segment_merge.rstests/vector_stress.rstests/workspace_integration.rs
💤 Files with no reviewable changes (12)
- tests/moonstore_warm_e2e.rs
- tests/quickwins_red.rs
- tests/vector_stress.rs
- tests/vector_config_defaults.rs
- tests/vector_edge_cases.rs
- tests/vector_segment_merge.rs
- tests/mq_integration.rs
- tests/workspace_integration.rs
- tests/vector_insert_bench.rs
- tests/txn_kv_wiring.rs
- tests/vector_exact_rerank.rs
- tests/per_index_compaction_weight.rs
✅ Files skipped from review due to trivial changes (4)
- Cargo.toml
- BENCHMARK.md
- docs/vector-search-guide.md
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (18)
- src/server/conn/handler_monoio/ft.rs
- src/vector/persistence/recover_v2.rs
- src/vector/types.rs
- src/command/vector_search/recommend.rs
- src/command/vector_search/ft_search/dispatch.rs
- src/command/vector_search/hybrid.rs
- src/vector/turbo_quant/tq_adc.rs
- src/vector/persistence/warm_search.rs
- src/command/vector_search/ft_search/execute.rs
- src/command/vector_search/ft_config.rs
- benches/fastscan_bench.rs
- src/vector/search_pool.rs
- src/vector/segment/ivf.rs
- src/vector/segment/holder.rs
- src/vector/segment/mutable.rs
- src/vector/hnsw/search.rs
- src/vector/segment/immutable.rs
- src/vector/distance/fastscan.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 (2)
src/vector/store.rs (1)
70-170: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftFile far exceeds the 1500-line guideline.
src/vector/store.rsruns to roughly 4800+ lines in this review batch, well past the mandated limit forsrc/**/*.rs. This PR adds further fields/methods/tests to it. Consider splitting store/index-lifecycle management, WS5a db-scoping, background compact/merge, and warm/cold tier transition logic into submodules.As per coding guidelines, "No single
.rsfile should exceed 1500 lines; split larger modules into submodules."🤖 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/vector/store.rs` around lines 70 - 170, The store module is far over the allowed size, and this change adds more to src/vector/store.rs instead of reducing it. Split the responsibilities in store/index lifecycle code into submodules (for example db-scoping, compaction/merge, and warm/cold tier transitions) and move the related types/functions such as IndexMeta, VectorCreateDefaults, set_vector_create_defaults, and vector_create_defaults to appropriately named modules re-exported from store.rs.Source: Coding guidelines
src/vector/index_persist.rs (1)
465-483: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winProduction sidecar writer still serializes v4 —
rerank_mult/exact_beamare silently lost on every restart.
save_index_metadata_v3is the only writer actually invoked in production (viaVectorStore::save_index_meta_sidecar), and it callsserialize_index_metas_v4, notserialize_index_metas_v5. The v5 knob block (rerank_mult,exact_beam) this PR adds is therefore never written to disk in practice, even thoughserialize_index_metas()(the general entry point) correctly writes v5. AnyFT.CONFIG SET <idx> RERANK_MULT|EXACT_BEAM— and any non-default--vector-rerank-mult/--vector-exact-beambaked into an index at creation — will silently revert to the hard-coded defaults(4, false)after any restart or crash recovery, because the loader reads backVERSION_V4and falls into the pre-v5 default branch.Do you want me to add a regression test that saves via
save_index_metadata_v3with non-defaultrerank_mult/exact_beam, reloads, and asserts they round-trip (this would have caught the gap)?🐛 Proposed fix
/// Write all active index metadata **with compaction weights** to the sidecar file -/// (current on-disk format: v4, WS5a db_index; name kept as `_v3` for API stability -/// across the many existing call sites — see module docs for the v4 wire format). +/// (current on-disk format: v5, search-tuning knobs; name kept as `_v3` for API +/// stability across the many existing call sites — see module docs for the wire format). /// /// Called after FT.CREATE / FT.DROPINDEX / FT.CONFIG SET COMPACTION_WEIGHT. /// Atomically replaces the file via write-to-temp + rename. pub fn save_index_metadata_v3(shard_dir: &Path, pairs: &[(&IndexMeta, f32)]) -> io::Result<()> { let path = shard_dir.join("vector-indexes.meta"); let tmp_path = shard_dir.join(".vector-indexes.meta.tmp"); - let data = serialize_index_metas_v4(pairs); + let data = serialize_index_metas_v5(pairs);🤖 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/vector/index_persist.rs` around lines 465 - 483, The production sidecar writer is still emitting v4 metadata, so the new v5 fields like rerank_mult and exact_beam are never persisted and will reset on reload. Update save_index_metadata_v3 (and any caller like VectorStore::save_index_meta_sidecar if needed) to serialize with serialize_index_metas_v5 instead of serialize_index_metas_v4, keeping the atomic temp-file rename flow intact. Make sure the disk format written by the only production writer matches the loader’s v5 expectations so the new config knobs round-trip correctly.
🤖 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 `@docs/guides/tuning.md`:
- Around line 458-471: The settle recipe in the tuning guide needs to clearly
state that VACUUM VECTOR only affects the connection’s local shard and that the
“~8× more fresh connections” approach in the shard-coverage guidance is
best-effort rather than guaranteed. Update the wording in the settle/merge
section to either describe the replay loop as probabilistic and
repeat-until-stable, or replace it with an explicit per-shard iteration approach
if deterministic coverage is intended, keeping the guidance anchored around
VACUUM VECTOR, SO_REUSEPORT, and FT.INFO graph_segments.
In `@src/config.rs`:
- Around line 905-910: The error string in the graph result cache size
validation contains an accidental run of extra spaces, so clean up the message
in the `graph_result_cache_entries` check to use normal spacing only. Update the
`Err(...).to_string()` text in this validation block so the operator-facing
message reads naturally and contains no stray whitespace artifacts.
- Around line 882-920: Split the misplaced Rust doc block in ServerConfig so
validate_databases_bound keeps the `--databases`/MAX_DATABASES documentation and
validate_tuning_defaults only documents the tuning-default validation. Add a
separating blank line or move the `u8 db_index` comment back above
validate_databases_bound, then leave the tuning-related prose directly above
validate_tuning_defaults so each function has the correct standalone doc
comment.
---
Outside diff comments:
In `@src/vector/index_persist.rs`:
- Around line 465-483: The production sidecar writer is still emitting v4
metadata, so the new v5 fields like rerank_mult and exact_beam are never
persisted and will reset on reload. Update save_index_metadata_v3 (and any
caller like VectorStore::save_index_meta_sidecar if needed) to serialize with
serialize_index_metas_v5 instead of serialize_index_metas_v4, keeping the atomic
temp-file rename flow intact. Make sure the disk format written by the only
production writer matches the loader’s v5 expectations so the new config knobs
round-trip correctly.
In `@src/vector/store.rs`:
- Around line 70-170: The store module is far over the allowed size, and this
change adds more to src/vector/store.rs instead of reducing it. Split the
responsibilities in store/index lifecycle code into submodules (for example
db-scoping, compaction/merge, and warm/cold tier transitions) and move the
related types/functions such as IndexMeta, VectorCreateDefaults,
set_vector_create_defaults, and vector_create_defaults to appropriately named
modules re-exported from store.rs.
🪄 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: f56a0c3a-3f06-4f27-ae49-db37956770a8
📒 Files selected for processing (46)
BENCHMARK.mdCHANGELOG.mdCargo.tomlbenches/fastscan_bench.rsdocs/guides/configuration.mddocs/guides/tuning.mddocs/vector-search-guide.mdsrc/command/vector_search/ft_config.rssrc/command/vector_search/ft_create.rssrc/command/vector_search/ft_search/dispatch.rssrc/command/vector_search/ft_search/execute.rssrc/command/vector_search/hybrid.rssrc/command/vector_search/recommend.rssrc/command/vector_search/tests.rssrc/config.rssrc/config/conf_file.rssrc/graph/cypher/result_cache.rssrc/graph/store.rssrc/main.rssrc/server/conn/handler_monoio/ft.rssrc/server/embedded.rssrc/vector/distance/fastscan.rssrc/vector/hnsw/search.rssrc/vector/index_persist.rssrc/vector/persistence/recover_v2.rssrc/vector/persistence/warm_search.rssrc/vector/search_pool.rssrc/vector/segment/holder.rssrc/vector/segment/immutable.rssrc/vector/segment/ivf.rssrc/vector/segment/mutable.rssrc/vector/store.rssrc/vector/turbo_quant/tq_adc.rssrc/vector/types.rstests/moonstore_warm_e2e.rstests/mq_integration.rstests/per_index_compaction_weight.rstests/quickwins_red.rstests/txn_kv_wiring.rstests/vector_config_defaults.rstests/vector_edge_cases.rstests/vector_exact_rerank.rstests/vector_insert_bench.rstests/vector_segment_merge.rstests/vector_stress.rstests/workspace_integration.rs
💤 Files with no reviewable changes (12)
- tests/moonstore_warm_e2e.rs
- tests/quickwins_red.rs
- tests/vector_stress.rs
- tests/vector_config_defaults.rs
- tests/vector_edge_cases.rs
- tests/vector_segment_merge.rs
- tests/mq_integration.rs
- tests/workspace_integration.rs
- tests/vector_insert_bench.rs
- tests/txn_kv_wiring.rs
- tests/vector_exact_rerank.rs
- tests/per_index_compaction_weight.rs
✅ Files skipped from review due to trivial changes (4)
- Cargo.toml
- BENCHMARK.md
- docs/vector-search-guide.md
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (18)
- src/server/conn/handler_monoio/ft.rs
- src/vector/persistence/recover_v2.rs
- src/vector/types.rs
- src/command/vector_search/recommend.rs
- src/command/vector_search/ft_search/dispatch.rs
- src/command/vector_search/hybrid.rs
- src/vector/turbo_quant/tq_adc.rs
- src/vector/persistence/warm_search.rs
- src/command/vector_search/ft_search/execute.rs
- src/command/vector_search/ft_config.rs
- benches/fastscan_bench.rs
- src/vector/search_pool.rs
- src/vector/segment/ivf.rs
- src/vector/segment/holder.rs
- src/vector/segment/mutable.rs
- src/vector/hnsw/search.rs
- src/vector/segment/immutable.rs
- src/vector/distance/fastscan.rs
🛑 Comments failed to post (3)
docs/guides/tuning.md (1)
458-471: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -n "VACUUM VECTOR|SO_REUSEPORT|graph_segments|local shard" src docsRepository: pilotspace/moon
Length of output: 13008
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1719,1775p' src/command/server_admin.rs printf '\n----\n' sed -n '400,470p' src/server/listener.rs printf '\n----\n' sed -n '272,320p' src/shard/event_loop.rs printf '\n----\n' sed -n '456,476p' docs/guides/tuning.mdRepository: pilotspace/moon
Length of output: 9883
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n "fresh connections|repeat until|per-shard|SO_REUSEPORT|VACUUM VECTOR" docs/guides/tuning.md docs/operations/reclamation-runbook.md src/command/server_admin.rs src/server/listener.rs src/shard/event_loop.rsRepository: pilotspace/moon
Length of output: 7050
Make the settle recipe explicit about shard coverage.
VACUUM VECTORruns on the connection’s local shard, and the SO_REUSEPORT path only sends each fresh connection to one shard probabilistically; the “~8× more fresh connections” loop is a heuristic, not a guarantee. Call that out as best-effort, or switch to an explicit per-shard replay loop if deterministic coverage is required.🤖 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 `@docs/guides/tuning.md` around lines 458 - 471, The settle recipe in the tuning guide needs to clearly state that VACUUM VECTOR only affects the connection’s local shard and that the “~8× more fresh connections” approach in the shard-coverage guidance is best-effort rather than guaranteed. Update the wording in the settle/merge section to either describe the replay loop as probabilistic and repeat-until-stable, or replace it with an explicit per-shard iteration approach if deterministic coverage is intended, keeping the guidance anchored around VACUUM VECTOR, SO_REUSEPORT, and FT.INFO graph_segments.src/config.rs (2)
882-920: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Misplaced doc comment:
validate_databases_bound's docs ended up onvalidate_tuning_defaults.Lines 883-887 (about
--databasesfitting theu8db_index tag) arevalidate_databases_bound's original doc comment, but they now sit directly abovevalidate_tuning_defaultswith no separating blank line, merging into one doc block. As a resultvalidate_databases_bound(defined at line 920) has no doc comment at all, andvalidate_tuning_defaults's doc is confusingly prefixed with unrelated--databases/MAX_DATABASESprose.📝 Proposed fix: split the doc comments back apart
impl ServerConfig { - /// Validate `--databases` fits the `u8` db_index tag used by vector/text - /// index scoping (WS5a round 2). Returns an error message (not a - /// `Frame`/`anyhow::Error` — kept plain so both `main.rs`'s early-boot - /// path and unit tests can format/assert on it directly) when the - /// configured count exceeds `MAX_DATABASES`. /// Validate the vector/graph tuning-default flags (startup error, not a /// silent clamp — a typo in a fleet-wide default must be loud). Mirrors /// the per-index FT.CONFIG ranges exactly so a value accepted here is /// accepted there and vice versa. pub fn validate_tuning_defaults(&self) -> Result<(), String> { ... } + /// Validate `--databases` fits the `u8` db_index tag used by vector/text + /// index scoping (WS5a round 2). Returns an error message (not a + /// `Frame`/`anyhow::Error` — kept plain so both `main.rs`'s early-boot + /// path and unit tests can format/assert on it directly) when the + /// configured count exceeds `MAX_DATABASES`. pub fn validate_databases_bound(&self) -> Result<(), String> { ... }📝 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.impl ServerConfig { /// Validate the vector/graph tuning-default flags (startup error, not a /// silent clamp — a typo in a fleet-wide default must be loud). Mirrors /// the per-index FT.CONFIG ranges exactly so a value accepted here is /// accepted there and vice versa. pub fn validate_tuning_defaults(&self) -> Result<(), String> { if self.vector_ef_runtime != 0 && !(10..=4096).contains(&self.vector_ef_runtime) { return Err(format!( "--vector-ef-runtime {} out of range (10-4096, or 0 for the auto heuristic)", self.vector_ef_runtime )); } if !(1..=64).contains(&self.vector_rerank_mult) { return Err(format!( "--vector-rerank-mult {} out of range (1-64)", self.vector_rerank_mult )); } if self.graph_result_cache_entries == 0 { return Err( "--graph-result-cache-entries must be >= 1 (the cache cannot be sized to zero entries; it is admission-gated, not disableable by size)" .to_string(), ); } if self.graph_result_cache_bytes < 4096 { return Err(format!( "--graph-result-cache-bytes {} too small (minimum 4096)", self.graph_result_cache_bytes )); } Ok(()) } /// Validate `--databases` fits the `u8` db_index tag used by vector/text /// index scoping (WS5a round 2). Returns an error message (not a /// `Frame`/`anyhow::Error` — kept plain so both `main.rs`'s early-boot /// path and unit tests can format/assert on it directly) when the /// configured count exceeds `MAX_DATABASES`. pub fn validate_databases_bound(&self) -> Result<(), String> {🤖 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/config.rs` around lines 882 - 920, Split the misplaced Rust doc block in ServerConfig so validate_databases_bound keeps the `--databases`/MAX_DATABASES documentation and validate_tuning_defaults only documents the tuning-default validation. Add a separating blank line or move the `u8 db_index` comment back above validate_databases_bound, then leave the tuning-related prose directly above validate_tuning_defaults so each function has the correct standalone doc comment.
905-910: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Typo: irregular whitespace gap in the error message.
"...sized to zero entries; it is admission-gated..."has a stray run of spaces from what looks like a line-wrap artifact. This string is shown directly to operators on a failed start.✏️ Proposed fix
- "--graph-result-cache-entries must be >= 1 (the cache cannot be sized to zero entries; it is admission-gated, not disableable by size)" + "--graph-result-cache-entries must be >= 1 (the cache cannot be sized to zero \ + entries; it is admission-gated, not disableable by size)"📝 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.if self.graph_result_cache_entries == 0 { return Err( "--graph-result-cache-entries must be >= 1 (the cache cannot be sized to zero \ entries; it is admission-gated, not disableable by size)" .to_string(), ); }🤖 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/config.rs` around lines 905 - 910, The error string in the graph result cache size validation contains an accidental run of extra spaces, so clean up the message in the `graph_result_cache_entries` check to use normal spacing only. Update the `Err(...).to_string()` text in this validation block so the operator-facing message reads naturally and contains no stray whitespace artifacts.
pilotspacex-byte
pushed a commit
that referenced
this pull request
Jul 8, 2026
Two per-index runtime knobs to close the recall gap to exact-beam engines (Qdrant reaches ~1.0 R@10 at ef 256 because its beam navigates full-precision vectors; Moon's beam ranked candidates by quantized ADC): - RERANK_MULT <1-64> (default 4): deepens the HQ-1 exact-rerank stage — the top mult*k beam candidates are re-scored with true f16 sidecar distances before top-k truncation, recovering true neighbors the ADC ranking dropped below the old fixed 4*k cut. Cost ~mult*k*dim f16 decodes per segment. - EXACT_BEAM ON|OFF (default OFF): the HNSW beam itself navigates with exact f16 sidecar distances instead of quantized ADC — candidate SELECTION becomes exact, recall goes graph-limited. QPS cost grows with dimension; sidecar-less segments silently keep the ADC beam. Implementation: - SearchTuning carrier threaded IndexMeta -> MvccContext/SearchSnapshot -> all immutable-segment search paths (sync, yielding, worker-pool fan-out, FT.RECOMMEND, hybrid) via *_with_tuning variants; the knob-free entry points keep the defaults, so no behavior change unless a knob is set. - hnsw_search_filtered gains an exact_f16 override checked at the head of both distance closures (one predicted branch when off); distance conventions match rerank_exact so cross-segment merge stays consistent. rerank_exact is skipped under EXACT_BEAM (distances are already true metric values). - Index-meta sidecar v4: appends rerank_mult + exact_beam per index; v1-v3 sidecars load with defaults; out-of-range persisted mult clamps back to 4. FT.CONFIG SET broadcasts to all shards (monoio fix from the EF_RUNTIME wave) and persists via save_index_meta_sidecar. Tests: FT.CONFIG set/get/validation for both knobs; sidecar v4 roundtrip + v1 default-migration + corrupt-mult clamp; functional red/green — RERANK_MULT monotone recall (mult 16 strictly beats mult 1 on seeded TQ4), EXACT_BEAM strictly beats the ADC beam at narrow ef on varying-norm L2 data with exact returned distances, and sidecar-less fallback. Full lib suite 3962 green; fmt + clippy clean on default and tokio+jemalloc feature sets. refs PR #248 author: Tin Dang
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.
Summary
Implements the FAISS FastScan technique (André et al., VLDB'15) end to end and wires it into the serving path for the first time. The pre-existing FastScan skeleton was dead code (IVF segments only built in tests), AVX2/scalar-only — aarch64, our primary platform, silently ran the scalar fallback — and carried three latent bugs.
Kernel wave (
src/vector/distance/fastscan.rs)vqtbl1q_u8): each coordinate's 16-entry u8 LUT fits exactly oneqregister; 32 candidate distances per interleaved block.0x7F, hiding it). All kernels now widen to u16 before adding and accumulate saturating; scalar/NEON/AVX2 are bit-identical over the full u8 range.build_quantized_lut): FAISS-style per-coordinate bias + global scale bounded for both u8 entries and the u16 accumulator, with f32 reconstruction. Replacesprecompute_lut, which hardcoded the legacy v11/√768codebook (encode/search asymmetry — the codebook-v2 recall bug class) and a fixed scale that could overflow both domains.Live-path integration (
src/vector/segment/mutable.rs)maintain_fastscan_shadow) from both insertion paths;clone_suffixrebuilds it with rebased lanes.approx − 0.5·padded_dim/scale, saturation only under-estimates) beats the current heap worst → results are bit-identical to the plain scan (equality tests cover full scans, chunked scans, MVCC snapshots, bitmap filters).Benchmarks (Linux VM, aarch64)
Per-query LUT build costs ~2–16 µs → the filter engages only above 64 entries. SQ8 / A2 / TQ-prod paths are untouched.
Validation
cargo clippy -D warningsclean on default andruntime-tokio,jemallocmatrices;cargo fmt --checkclean.mset_invalidates_every_second_arg_key(client-tracking suite) is flaky on this machine and fails identically on untouchedmain— pre-existing, unrelated.Follow-ups (future waves)
M0=32matches the block size, but codes are vector-major — needs a gather-transpose vs memory-layout decision.Summary by CodeRabbit
FT.CONFIG SET <idx> EF_RUNTIME(incl.0=auto), plus recall controlsRERANK_MULTandEXACT_BEAM.FT.CONFIG SETto apply consistently across all shards.VACUUM VECTOR“settle” workflow, and bulk-load/compaction guidance.