Skip to content

Commit 8a5c32d

Browse files
committed
fix(temporal): address Codex P2s — DepClosure::default ready + HLC fallback to lance_version
Two real foot-guns codex caught on #468; both fixed + regression-tested (15/15 green). P2 #1 — DepClosure::default() blocked every row. The derived Default produced satisfied: false, so any consumer using ..Default::default() (or default()) would silently make deinterlace drop every contemporary row via Classification::dispatchable. The trivial/empty case IS the ready case (matches DepClosure::ready + NoDeps). Replaced derive with a manual Default that delegates to ready(); regression: dep_closure_default_is_ready_not_blocking. P2 #2 — unwrap_or(0) on hlc_tick forced every missing-HLC row ahead of all HLC rows. The doc says "falls back to lance_version" but the code used 0. During partial cross-server rollout (mixed HLC + legacy) this returned a non-causal projection that clustered all legacy rows at the start regardless of their version. Changed to unwrap_or_else(|| r.lance_version()) so single-server / legacy rows sort by their own version; cross-server rows sort by HLC; mixed inputs interleave on the unified scale. Regression: deinterlace_mixed_hlc_falls_back_to_lance_version. https://claude.ai/code/session_01VysoWJ6vsyg3wEGc5v7T5v
1 parent db9249a commit 8a5c32d

1 file changed

Lines changed: 46 additions & 2 deletions

File tree

crates/lance-graph-planner/src/temporal.rs

Lines changed: 46 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,7 @@ pub struct DepEdge {
183183
/// A subject's data-dependency closure at a reference, plus whether it is
184184
/// satisfied. The `DependsClosure` impl owns the satisfaction logic; this module
185185
/// only consumes [`satisfied`](Self::satisfied).
186-
#[derive(Debug, Clone, Default)]
186+
#[derive(Debug, Clone)]
187187
pub struct DepClosure {
188188
/// The edges that must hold for the subject to be data-causally ready.
189189
pub edges: Vec<DepEdge>,
@@ -201,6 +201,16 @@ impl DepClosure {
201201
}
202202
}
203203

204+
/// The trivial/empty closure IS the ready case — `Default` matches
205+
/// [`DepClosure::ready`] so `..Default::default()` and the `derive(Default)`
206+
/// reflex don't silently produce `satisfied: false` (which would make
207+
/// [`deinterlace`] drop every otherwise-contemporary row). Codex P2 on #468.
208+
impl Default for DepClosure {
209+
fn default() -> Self {
210+
Self::ready()
211+
}
212+
}
213+
204214
/// The DATA-causal axis seam. Returns the `depends_on` closure for `subject` at
205215
/// `v_ref`. Implemented by the consumer against whatever SPO source it has
206216
/// (in-memory ndjson, the lance SPO store, an HTTP query) — Rubicon's `Depends`
@@ -301,7 +311,11 @@ where
301311
})
302312
.cloned()
303313
.collect();
304-
out.sort_by_key(|r| (r.hlc_tick().unwrap_or(0), r.lance_version()));
314+
// Falls back to the row's own lance_version when no HLC tick is present
315+
// (single-server / legacy rows) — NOT to 0, which would force every
316+
// missing-HLC row ahead of all HLC rows regardless of its version (Codex
317+
// P2 on #468; honors the documented "fallback to lance_version").
318+
out.sort_by_key(|r| (r.hlc_tick().unwrap_or_else(|| r.lance_version()), r.lance_version()));
305319
out
306320
}
307321

@@ -443,4 +457,34 @@ mod tests {
443457
assert_eq!(q.hlc_tick, None);
444458
assert_eq!(q.mode, EpistemicMode::Strict);
445459
}
460+
461+
/// Codex P2 on #468: `DepClosure::default()` must be ready (matching
462+
/// [`DepClosure::ready`] + [`NoDeps`]), not `satisfied: false` — otherwise
463+
/// any consumer using `..Default::default()` silently blocks every row.
464+
#[test]
465+
fn dep_closure_default_is_ready_not_blocking() {
466+
let d = DepClosure::default();
467+
assert!(d.satisfied, "Default DepClosure must be ready (satisfied: true)");
468+
assert!(d.edges.is_empty());
469+
}
470+
471+
/// Codex P2 on #468: a row without an HLC tick must fall back to its own
472+
/// `lance_version` as the deinterlace key — NOT to 0 (which would force
473+
/// legacy rows ahead of all HLC rows during partial cross-server rollout).
474+
#[test]
475+
fn deinterlace_mixed_hlc_falls_back_to_lance_version() {
476+
let v_ref = QueryReference { ref_version: 1000, ..QueryReference::default() };
477+
// Two HLC-bearing rows + one legacy row (no HLC, lance_version 750).
478+
// With the old `unwrap_or(0)` the legacy row would sort first; with
479+
// the fallback to lance_version it sorts between HLCs 500 and 900.
480+
let rows = vec![
481+
Row::new(100, 1, Some(500)),
482+
Row::new(750, 1, None),
483+
Row::new(900, 1, Some(900)),
484+
];
485+
let out = deinterlace(&rows, &v_ref, &NoDeps);
486+
let order: Vec<u64> =
487+
out.iter().map(|r| r.hlc.unwrap_or(r.v)).collect();
488+
assert_eq!(order, vec![500, 750, 900], "legacy row must sort by its lance_version, not 0");
489+
}
446490
}

0 commit comments

Comments
 (0)