Skip to content

Commit 3fc22ec

Browse files
committed
fix(sprint-13/codex-p2): D-CSV-14 gen=0 + D-CSV-16 cam_pq_search semantics
Two codex P2 findings on the merged sprint-13 W-I2 / W-I3 PRs (#395, #396). Both bugs are silent semantic regressions; both fixes are surgical. ## #395 / D-CSV-14 — splat_ops::splat_gaussian deprecated shim, gen=0 case Old free fn wrote `SplatField::generation = generation` verbatim. The deprecation shim seeds `Think::from_field(field, generation.saturating_sub(1))` then calls `splat_gaussian`, which advances cycle and writes `generation = self.cycle`. For `generation >= 1` this matches v1; for `generation == 0` the saturating sub stays at 0 and the post-advance cycle becomes 1, producing `generation = 1` instead of 0. Fix: after the delegated call, walk `think.splat_field` and overwrite the touched splat (matched by `mean`) with the caller-supplied `generation`. Both push-new and merge-existing paths are covered. Test: `deprecated_splat_gaussian_preserves_generation_zero` asserts verbatim preservation for both push and merge paths. ## #396 / D-CSV-16 — WitnessCorpus::cam_pq_search exact-match semantics Old (sprint-12) `cam_pq_search` was exact SPO top-k via HashMap. W-I3's patch added a `#[cfg(feature = "with-cam-pq")]` early-return that routed through `WitnessIndexCamPq::cam_pq_search`, which is distance-ranked ANN over the whole corpus. Two semantic regressions follow when CAM-PQ is enabled: - Missing SPO: v1 returns empty; v2 returns k unrelated neighbours. - Existing SPO with <k entries: v1 returns just the matches; v2 pads with unrelated nearest neighbours. The distance-ranked API was already added as `query_similar` (returns `Option<Vec<(&WitnessEntry, f32)>>`), so callers wanting ANN have it. `cam_pq_search` should keep its exact-match contract. Fix: remove the `with-cam-pq` early-return block; the HashMap path (always present) handles both feature states identically. Test: `cam_pq_search_exact_match_when_cam_pq_enabled` (cfg with-cam-pq) asserts a missing SPO returns empty and an existing SPO returns only its own entries — never unrelated neighbours. ## Validation - `cargo test --manifest-path crates/thinking-engine/Cargo.toml splat_ops` → 5/5 pass (including new gen=0 test) - `cargo check -p lance-graph --features with-cam-pq --tests` → clean - `cargo check -p lance-graph --tests` (feature OFF) → clean - `cargo test -p lance-graph --features with-cam-pq` linking fails with `ld terminated with signal 7 [Bus error]` — environmental linker constraint, same one W-I3 originally hit; not a code defect. https://claude.ai/code/session_01UwJuKqP828qyX1VkLgGJFS
1 parent 8d321ff commit 3fc22ec

2 files changed

Lines changed: 68 additions & 12 deletions

File tree

crates/lance-graph/src/graph/arigraph/witness_corpus.rs

Lines changed: 37 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -409,20 +409,12 @@ impl WitnessCorpus {
409409
evicted
410410
}
411411

412-
/// Top-k query by SPO.
412+
/// Exact-match top-k query by SPO.
413413
///
414-
/// With `with-cam-pq` ON and cam_pq_state Some: distance-ranked via CAM-PQ ADC.
415-
/// Otherwise: HashMap chain-order fallback.
414+
/// Always returns entries whose `spo` exactly equals the query; never
415+
/// returns unrelated neighbours. For distance-ranked nearest-neighbour
416+
/// search, use `query_similar` (feature `with-cam-pq`).
416417
pub fn cam_pq_search(&self, spo: u64, k: usize) -> Vec<&WitnessEntry> {
417-
#[cfg(feature = "with-cam-pq")]
418-
if let Some(state) = self.cam_pq_state.as_ref() {
419-
return state
420-
.index
421-
.cam_pq_search(spo, k)
422-
.into_iter()
423-
.filter_map(|(pos, _dist)| self.entries.get(pos))
424-
.collect();
425-
}
426418
self.cam_pq_index
427419
.lookup(spo)
428420
.iter()
@@ -979,6 +971,39 @@ mod tests {
979971
assert_eq!(idx.len(), 0);
980972
}
981973

974+
// Codex P2 (PR #396): cam_pq_search must remain exact-match even when
975+
// CAM-PQ is enabled. ANN search lives on query_similar. Asserts that a
976+
// missing SPO yields an empty result (not k unrelated neighbours) and
977+
// that an existing SPO yields only its own entries.
978+
#[cfg(feature = "with-cam-pq")]
979+
#[test]
980+
fn cam_pq_search_exact_match_when_cam_pq_enabled() {
981+
let mut corpus = WitnessCorpus::new();
982+
let spo_a = 0xAAAA_u64;
983+
let spo_b = 0xBBBB_u64;
984+
for ts in [100u64, 200, 300] {
985+
corpus.insert(make_entry(spo_a, ts));
986+
}
987+
for ts in [10u64, 20, 30] {
988+
corpus.insert(make_entry(spo_b, ts));
989+
}
990+
corpus.enable_cam_pq(tiny_codebook(), 0xCAFE_BABE);
991+
992+
let hits_a = corpus.cam_pq_search(spo_a, 5);
993+
assert_eq!(hits_a.len(), 3, "must return exactly the 3 spo_a entries");
994+
assert!(
995+
hits_a.iter().all(|e| e.spo == spo_a),
996+
"no unrelated neighbours allowed"
997+
);
998+
999+
let missing = corpus.cam_pq_search(0xCCCC_u64, 5);
1000+
assert!(
1001+
missing.is_empty(),
1002+
"missing SPO must return empty, not k nearest neighbours; got {} entries",
1003+
missing.len()
1004+
);
1005+
}
1006+
9821007
// T12: feature OFF smoke — WitnessCorpus::new() and query() work.
9831008
// (Compiles and runs under both feature flags; tests only HashMap path.)
9841009
#[test]

crates/thinking-engine/src/splat_ops.rs

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,15 @@ pub fn splat_gaussian(
5353
) {
5454
let mut think = Think::from_field(std::mem::take(field), generation.saturating_sub(1));
5555
think.splat_gaussian(mean, variance, energy);
56+
// For generation == 0, saturating_sub(1) underflows to 0 and the method's
57+
// advance_cycle() then writes generation=1, diverging from v1 verbatim
58+
// semantics. Restore by overwriting the touched splat (matched by `mean`).
59+
for s in think.splat_field.iter_mut() {
60+
if s.mean == mean {
61+
s.generation = generation;
62+
break;
63+
}
64+
}
5665
*field = think.splat_field;
5766
}
5867

@@ -169,6 +178,28 @@ mod tests {
169178
assert_eq!(field_shim[0].generation, think.splat_field[0].generation);
170179
}
171180

181+
#[test]
182+
#[allow(deprecated)]
183+
fn deprecated_splat_gaussian_preserves_generation_zero() {
184+
// Codex P2 (PR #395): caller passing generation=0 must see the v1
185+
// verbatim semantics (stored generation == 0), not the off-by-one
186+
// produced by Think::splat_gaussian advancing cycle from 0 → 1.
187+
let mut field_push: Vec<SplatField> = Vec::new();
188+
splat_gaussian(&mut field_push, 7, 1.0, 1.0, 0);
189+
assert_eq!(field_push.len(), 1);
190+
assert_eq!(field_push[0].generation, 0, "push path: gen=0 verbatim");
191+
192+
let mut field_merge: Vec<SplatField> = vec![SplatField {
193+
mean: 9,
194+
variance: 1.0,
195+
energy: 2.0,
196+
generation: 42,
197+
}];
198+
splat_gaussian(&mut field_merge, 9, 1.0, 3.0, 0);
199+
assert_eq!(field_merge.len(), 1);
200+
assert_eq!(field_merge[0].generation, 0, "merge path: gen=0 verbatim");
201+
}
202+
172203
#[test]
173204
#[allow(deprecated)]
174205
fn deprecated_score_hole_closure_forwards_to_method() {

0 commit comments

Comments
 (0)