diff --git a/crates/rvf/rvf-runtime/src/options.rs b/crates/rvf/rvf-runtime/src/options.rs index f39e7b8a2..ad9477d91 100644 --- a/crates/rvf/rvf-runtime/src/options.rs +++ b/crates/rvf/rvf-runtime/src/options.rs @@ -19,6 +19,35 @@ pub enum DistanceMetric { Cosine, } +impl DistanceMetric { + /// Encode this metric as a single byte for manifest persistence. + /// + /// Encoding: 0 = L2 (default / backward-compatible), 1 = InnerProduct, 2 = Cosine. + /// Old manifests written before this field existed have 0x00 at that byte + /// (it was a reserved zero), so they boot correctly as L2. + pub(crate) fn to_id(self) -> u8 { + match self { + DistanceMetric::L2 => 0, + DistanceMetric::InnerProduct => 1, + DistanceMetric::Cosine => 2, + } + } + + /// Decode a metric from a manifest byte. + /// + /// Unknown values fall back to L2 for forward-compatibility: a store + /// written by a newer version with an unknown metric ID is treated as + /// L2-distance, which is at least type-safe even if not semantically + /// correct. + pub(crate) fn from_id(id: u8) -> Self { + match id { + 1 => DistanceMetric::InnerProduct, + 2 => DistanceMetric::Cosine, + _ => DistanceMetric::L2, + } + } +} + /// Compression profile for stored vectors. #[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] pub enum CompressionProfile { diff --git a/crates/rvf/rvf-runtime/src/read_path.rs b/crates/rvf/rvf-runtime/src/read_path.rs index 8dc7c6a62..679a2881a 100644 --- a/crates/rvf/rvf-runtime/src/read_path.rs +++ b/crates/rvf/rvf-runtime/src/read_path.rs @@ -6,6 +6,7 @@ //! 3. Background: parse Level 1 -> full segment directory //! 4. On-demand: load cold segments as queries need them +use crate::options::DistanceMetric; use rvf_types::{FileIdentity, SegmentHeader, SegmentType, SEGMENT_HEADER_SIZE, SEGMENT_MAGIC}; use std::io::{self, Read, Seek, SeekFrom}; @@ -31,6 +32,11 @@ pub(crate) struct ParsedManifest { pub dimension: u16, pub total_vectors: u64, pub profile_id: u8, + /// Distance metric decoded from byte [19] of the manifest header. + /// + /// Stores written before this field existed have 0x00 there (reserved), + /// which decodes as `DistanceMetric::L2` — the backward-compatible default. + pub metric: DistanceMetric, pub segment_dir: Vec, pub deleted_ids: Vec, pub file_identity: Option, @@ -159,6 +165,9 @@ fn parse_manifest_payload(payload: &[u8]) -> Option { ]); let seg_count = u32::from_le_bytes([payload[14], payload[15], payload[16], payload[17]]); let profile_id = payload[18]; + // Byte [19] encodes the distance metric (was reserved zero in older stores). + // DistanceMetric::from_id(0) == L2, so old stores boot correctly. + let metric = DistanceMetric::from_id(payload[19]); let mut offset = 22; // past header (4+2+8+4+1+3) @@ -269,6 +278,7 @@ fn parse_manifest_payload(payload: &[u8]) -> Option { dimension, total_vectors, profile_id, + metric, segment_dir, deleted_ids, file_identity, diff --git a/crates/rvf/rvf-runtime/src/store.rs b/crates/rvf/rvf-runtime/src/store.rs index f9f9f5ee5..da8890add 100644 --- a/crates/rvf/rvf-runtime/src/store.rs +++ b/crates/rvf/rvf-runtime/src/store.rs @@ -1387,6 +1387,7 @@ impl RvfStore { self.options.dimension, total_vectors, self.options.profile, + self.options.metric.to_id(), &new_segment_dir, &empty_dels, fi, @@ -2467,6 +2468,13 @@ impl RvfStore { self.epoch = manifest.epoch; self.options.dimension = manifest.dimension; self.options.profile = manifest.profile_id; + // Restore the distance metric persisted in the manifest header (byte + // [19], previously a reserved zero). Old stores read 0x00 there and + // boot as L2 — the correct backward-compatible default. Without this + // restore, COW dual-graph queries open the parent via open_readonly() + // which goes through boot() and was silently resetting the metric to + // L2, breaking cosine queries (recall@10 ≈ 0.10 → ≈ 1.0 after fix). + self.options.metric = manifest.metric; // Pre-size the slab from the manifest so the cold-open load does a // single allocation instead of growing through repeated doublings. self.vectors = VectorData::with_capacity( @@ -2577,6 +2585,7 @@ impl RvfStore { self.options.dimension, total_vectors, self.options.profile, + self.options.metric.to_id(), &self.segment_dir, &deleted_ids, fi, diff --git a/crates/rvf/rvf-runtime/src/write_path.rs b/crates/rvf/rvf-runtime/src/write_path.rs index b9e05bf9e..66273bbeb 100644 --- a/crates/rvf/rvf-runtime/src/write_path.rs +++ b/crates/rvf/rvf-runtime/src/write_path.rs @@ -137,7 +137,7 @@ impl SegmentWriter { /// Write a minimal MANIFEST_SEG recording current state. /// /// This is a simplified manifest that stores: - /// - epoch, dimension, total_vectors, total_segments, profile_id + /// - epoch, dimension, total_vectors, total_segments, profile_id, metric_id /// - segment directory entries (seg_id, offset, length, type) /// - deletion bitmap (vector IDs as simple packed u64 array) /// - file identity (68 bytes, appended for lineage provenance) @@ -149,6 +149,7 @@ impl SegmentWriter { dimension: u16, total_vectors: u64, profile_id: u8, + metric_id: u8, segment_dir: &[(u64, u64, u64, u8)], // (seg_id, offset, payload_len, seg_type) deleted_ids: &[u64], ) -> io::Result<(u64, u64)> { @@ -158,6 +159,7 @@ impl SegmentWriter { dimension, total_vectors, profile_id, + metric_id, segment_dir, deleted_ids, None, @@ -165,6 +167,10 @@ impl SegmentWriter { } /// Write a MANIFEST_SEG with optional FileIdentity appended. + /// + /// The `metric_id` is encoded into header byte [19] (previously a reserved + /// zero byte): 0 = L2, 1 = InnerProduct, 2 = Cosine. Old stores that + /// wrote 0x00 at that position boot correctly as L2 (backward-compatible). #[allow(clippy::too_many_arguments)] pub(crate) fn write_manifest_seg_with_identity( &mut self, @@ -173,6 +179,7 @@ impl SegmentWriter { dimension: u16, total_vectors: u64, profile_id: u8, + metric_id: u8, segment_dir: &[(u64, u64, u64, u8)], deleted_ids: &[u64], file_identity: Option<&rvf_types::FileIdentity>, @@ -190,12 +197,15 @@ impl SegmentWriter { let mut payload = Vec::with_capacity(payload_size); // Manifest header. + // Layout: epoch[0..4] | dim[4..6] | total_vecs[6..14] | seg_count[14..18] + // | profile_id[18] | metric_id[19] | reserved[20..22] payload.extend_from_slice(&epoch.to_le_bytes()); payload.extend_from_slice(&dimension.to_le_bytes()); payload.extend_from_slice(&total_vectors.to_le_bytes()); payload.extend_from_slice(&seg_count.to_le_bytes()); payload.push(profile_id); - payload.extend_from_slice(&[0u8; 3]); // reserved + payload.push(metric_id); // byte [19]: distance metric identifier + payload.extend_from_slice(&[0u8; 2]); // bytes [20..22]: reserved // Segment directory. for &(sid, off, plen, stype) in segment_dir { diff --git a/crates/rvf/tests/rvf-integration/tests/cow_ann_recall.rs b/crates/rvf/tests/rvf-integration/tests/cow_ann_recall.rs index 788950452..4e804f925 100644 --- a/crates/rvf/tests/rvf-integration/tests/cow_ann_recall.rs +++ b/crates/rvf/tests/rvf-integration/tests/cow_ann_recall.rs @@ -205,6 +205,187 @@ fn cow_ann_recall_vs_exact() { println!("PASS: cow_ann_recall_vs_exact (recall@{K} = {recall:.4})"); } +// =========================================================================== +// TEST 5: cow_ann_recall_vs_exact_cosine +// =========================================================================== + +/// Cosine-metric COW recall regression test. +/// +/// This is the primary regression test for the native COW dual-graph cosine +/// bug (fixed in this PR): before the fix the parent store was re-opened via +/// `open_readonly()` which went through `boot()` without restoring the metric, +/// so the parent defaulted to L2. The parent HNSW was built with L2 distance +/// and returned L2-ordered candidates that were then merged with the child's +/// cosine distances — completely breaking the ordering. +/// +/// Before fix: cosine recall@10 ≈ 0.10 (bug reproducible here). +/// After fix : cosine recall@10 ≥ 0.95 (metric persisted in manifest). +/// +/// Design mirrors `cow_ann_recall_vs_exact` (L2) with: +/// - `metric: DistanceMetric::Cosine` +/// - ground-truth computed via cosine distance (1 − cos_sim) +/// - same child-edit mix (60 new, 20 override, 10 tombstone) +fn make_cosine_opts(dim: u16) -> RvfOptions { + RvfOptions { + dimension: dim, + metric: DistanceMetric::Cosine, + ..Default::default() + } +} + +/// Cosine distance: 1 − dot(a,b)/(‖a‖·‖b‖). +fn cosine_dist(a: &[f32], b: &[f32]) -> f32 { + let mut dot = 0.0f32; + let mut na = 0.0f32; + let mut nb = 0.0f32; + for (x, y) in a.iter().zip(b.iter()) { + dot += x * y; + na += x * x; + nb += y * y; + } + let denom = (na * nb).sqrt(); + if denom < f32::EPSILON { + 1.0 + } else { + 1.0 - dot / denom + } +} + +/// Exact brute-force k-NN over a slice of (id, vector) pairs using cosine +/// distance. +fn exact_knn_cosine(query: &[f32], corpus: &[(u64, Vec)], k: usize) -> Vec { + let mut dists: Vec<(u64, f32)> = corpus + .iter() + .map(|(id, v)| (*id, cosine_dist(query, v))) + .collect(); + dists.sort_by(|a, b| a.1.total_cmp(&b.1).then_with(|| a.0.cmp(&b.0))); + dists.iter().take(k).map(|(id, _)| *id).collect() +} + +#[test] +fn cow_ann_recall_vs_exact_cosine() { + let dir = TempDir::new().unwrap(); + let base_path = dir.path().join("base_cos.rvf"); + let child_path = dir.path().join("child_cos.rvf"); + // Use the same dim/count as the L2 test so the parent slab is large enough + // for HNSW to kick in on both arms. + const DIM: u16 = 32; + const BASE_N: usize = 1_200; + const K: usize = 10; + + // ── Build base store (cosine metric) ───────────────────────────────── + let mut base = RvfStore::create(&base_path, make_cosine_opts(DIM)).unwrap(); + let base_vecs: Vec> = (0..BASE_N) + .map(|i| lcg_vector(DIM as usize, i as u64 + 20_000)) + .collect(); + let base_refs: Vec<&[f32]> = base_vecs.iter().map(|v| v.as_slice()).collect(); + let base_ids: Vec = (0..BASE_N as u64).collect(); + base.ingest_batch(&base_refs, &base_ids, None).unwrap(); + base.close().unwrap(); + + // ── Branch ─────────────────────────────────────────────────────────── + let mut base = RvfStore::open(&base_path).unwrap(); + // Verify the metric was persisted correctly (sanity check). + assert_eq!( + base.metric(), + DistanceMetric::Cosine, + "base store metric must survive close()+open() round-trip" + ); + let mut child = base.branch(&child_path).unwrap(); + base.close().unwrap(); + + // ── Child edits ─────────────────────────────────────────────────────── + // (a) 60 new vectors (IDs 5000..5059) + const NEW_START: u64 = 5_000; + const NEW_COUNT: usize = 60; + let new_vecs: Vec> = (0..NEW_COUNT) + .map(|i| lcg_vector(DIM as usize, 29_000 + i as u64)) + .collect(); + let new_refs: Vec<&[f32]> = new_vecs.iter().map(|v| v.as_slice()).collect(); + let new_ids: Vec = (NEW_START..NEW_START + NEW_COUNT as u64).collect(); + child.ingest_batch(&new_refs, &new_ids, None).unwrap(); + + // (b) Override 20 parent vectors (IDs 0..19). + const OVERRIDE_COUNT: usize = 20; + let override_vecs: Vec> = (0..OVERRIDE_COUNT) + .map(|i| lcg_vector(DIM as usize, 99_000 + i as u64)) + .collect(); + let override_refs: Vec<&[f32]> = override_vecs.iter().map(|v| v.as_slice()).collect(); + let override_ids: Vec = (0..OVERRIDE_COUNT as u64).collect(); + child + .ingest_batch(&override_refs, &override_ids, None) + .unwrap(); + + // (c) Tombstone 10 parent vectors (IDs 100..109). + const TOMBSTONE_START: u64 = 100; + const TOMBSTONE_COUNT: usize = 10; + let tombstone_ids: Vec = + (TOMBSTONE_START..TOMBSTONE_START + TOMBSTONE_COUNT as u64).collect(); + child.delete(&tombstone_ids).unwrap(); + + // ── Build cosine ground-truth corpus visible from child ─────────────── + let mut ground_truth_corpus: Vec<(u64, Vec)> = Vec::new(); + + let override_set: std::collections::HashSet = override_ids.iter().copied().collect(); + let tombstone_set: std::collections::HashSet = tombstone_ids.iter().copied().collect(); + for (i, v) in base_vecs.iter().enumerate() { + let id = i as u64; + if override_set.contains(&id) || tombstone_set.contains(&id) { + continue; + } + ground_truth_corpus.push((id, v.clone())); + } + for (i, v) in override_vecs.iter().enumerate() { + ground_truth_corpus.push((override_ids[i], v.clone())); + } + for (i, v) in new_vecs.iter().enumerate() { + ground_truth_corpus.push((new_ids[i], v.clone())); + } + + // ── Query ───────────────────────────────────────────────────────────── + // Use a query vector near parent vector 500 (not overridden, not tombstoned). + let query = lcg_vector(DIM as usize, 500 + 20_000); + + // Exact cosine ground truth. + let exact_top_k = exact_knn_cosine(&query, &ground_truth_corpus, K); + assert_eq!( + exact_top_k.len(), + K, + "ground truth must return K={K} results" + ); + + // COW ANN via dual-graph merge (the path that was broken before the fix). + let ann_opts = QueryOptions { + ef_search: 300, + ..Default::default() + }; + let ann_results = child.query(&query, K, &ann_opts).unwrap(); + assert_eq!(ann_results.len(), K, "ANN query must return K={K} results"); + let ann_ids: Vec = ann_results.iter().map(|r| r.id).collect(); + + let recall = recall_at_k(&ann_ids, &exact_top_k); + println!( + "cow_ann_recall_vs_exact_cosine: recall@{K} = {:.4} (ANN top-{K}: {:?})", + recall, ann_ids + ); + + // Before the fix this assertion fired with recall ≈ 0.10. + // After the fix (metric persisted in manifest → parent re-opened with + // the correct Cosine metric) recall@10 must be ≥ 0.95. + assert!( + recall >= 0.95, + "recall@{K} {:.4} is below the 0.95 contract — \ + possible metric-persistence regression (ANN={:?}, exact={:?})", + recall, + ann_ids, + exact_top_k + ); + + child.close().unwrap(); + + println!("PASS: cow_ann_recall_vs_exact_cosine (recall@{K} = {recall:.4})"); +} + // =========================================================================== // TEST 2: cow_ann_override_correctness // ===========================================================================