diff --git a/crates/omnigraph/src/table_store.rs b/crates/omnigraph/src/table_store.rs index 430c2c7..c9ff90a 100644 --- a/crates/omnigraph/src/table_store.rs +++ b/crates/omnigraph/src/table_store.rs @@ -634,7 +634,10 @@ impl TableStore { // duplicates / dropped rows). Mirror the commit-time renumbering // here, using `ds.manifest.max_fragment_id() + 1` as the base and // accounting for prior stages. - let next_id_base = ds.manifest.max_fragment_id.unwrap_or(0) + // ds.manifest.max_fragment_id is Option; cast up to u64 because + // Lance's Fragment::id (and the commit-time renumbering counter in + // Transaction::fragments_with_ids) operate on u64. + let next_id_base = ds.manifest.max_fragment_id.unwrap_or(0) as u64 + 1 + prior_stages_fragment_count(prior_stages); assign_fragment_ids(&mut new_fragments, next_id_base); @@ -768,6 +771,23 @@ impl TableStore { /// filter, a merge_insert that rewrites an existing fragment would /// surface twice — once via the original committed fragment, once via /// the rewrite in `new_fragments`. + /// + /// **Filter contract is incomplete on staged fragments.** When `filter` + /// is `Some(...)`, Lance pushes the predicate to per-fragment scans + /// with stats-based pruning. Uncommitted fragments produced by + /// `write_fragments_internal` lack the per-column statistics that + /// committed fragments carry; Lance's optimizer drops them from the + /// filtered scan even when their data would match. Staged-fragment + /// rows are silently absent from the result. `scanner.use_stats(false)` + /// does not fix this in lance 4.0.0. Callers needing correct filtered + /// reads against staged data should use a different strategy (the + /// engine's MR-794 step 2+ design uses in-memory pending-batch + /// accumulation + DataFusion `MemTable` instead — see + /// `.context/mr-794-step2-design.md`). + /// + /// This method remains on the surface for primitive-level testing + /// (basic stage + scan correctness without filters works) and for + /// callers that don't need filter pushdown. pub async fn scan_with_staged( &self, ds: &Dataset, diff --git a/crates/omnigraph/tests/staged_writes.rs b/crates/omnigraph/tests/staged_writes.rs index f0e70cd..811ccaf 100644 --- a/crates/omnigraph/tests/staged_writes.rs +++ b/crates/omnigraph/tests/staged_writes.rs @@ -339,13 +339,21 @@ async fn stage_merge_insert_then_commit_persists_merged_view() { assert_eq!(total, 2, "merge_insert must not duplicate the matched row"); } -/// Filter pushdown via `scan_with_staged`: a SQL filter applies across -/// both committed and staged fragments. This validates the MR-794 -/// ticket's claim that Lance's `with_fragments` preserves pushdown -/// behavior (per Lance tests `test_scalar_index_respects_fragment_list` -/// etc.). +/// **Documented limitation** (see `scan_with_staged` doc): when a filter +/// is supplied, Lance's stats-based pruning drops the staged fragment from +/// the filtered scan because uncommitted fragments produced by +/// `write_fragments_internal` lack per-column statistics. The result +/// contains only matching committed rows; matching staged rows are +/// silently absent. `scanner.use_stats(false)` does not bypass this in +/// lance 4.0.0. +/// +/// This test pins the actual behavior so a future change either preserves +/// it (and updates the doc) or fixes it (and rewrites this test). The +/// engine's MR-794 step 2+ design uses in-memory pending-batch +/// accumulation + DataFusion `MemTable` for read-your-writes instead, so +/// production code is unaffected. #[tokio::test] -async fn scan_with_staged_pushes_filter_through_committed_and_staged() { +async fn scan_with_staged_with_filter_silently_drops_staged_rows() { let dir = tempfile::tempdir().unwrap(); let uri = format!("{}/people.lance", dir.path().to_str().unwrap()); let store = TableStore::new(dir.path().to_str().unwrap()); @@ -368,7 +376,9 @@ async fn scan_with_staged_pushes_filter_through_committed_and_staged() { .await .unwrap(); - // Filter: age >= 30 → expect alice, carol, dave (not bob). + // Filter: age >= 30. Correct semantics would return alice, carol, dave. + // Actual: dave (staged, age=35) is dropped — only the committed matches + // come back. let batches = store .scan_with_staged( &ds, @@ -378,18 +388,33 @@ async fn scan_with_staged_pushes_filter_through_committed_and_staged() { ) .await .unwrap(); - assert_eq!(collect_ids(&batches), vec!["alice", "carol", "dave"]); + assert_eq!( + collect_ids(&batches), + vec!["alice", "carol"], + "documented limitation: filter pushdown drops staged fragments. \ + If you're here because this assertion failed: either (a) Lance \ + exposed a way to scan uncommitted fragments without stats-based \ + pruning (good — update to assert == [alice, carol, dave]), or \ + (b) something changed in our scan_with_staged path. See PR #67 \ + test fix discussion + .context/mr-794-step2-design.md §1.1." + ); - // Same filter as count: same arithmetic. - let count = store - .count_rows_with_staged( + // Without filter, staged data IS visible — confirms the issue is + // specifically filter pushdown, not fragment scanning per se. + let unfiltered = store + .scan_with_staged( &ds, std::slice::from_ref(&staged), - Some("age >= 30".to_string()), + None, + None, ) .await .unwrap(); - assert_eq!(count, 3); + assert_eq!( + collect_ids(&unfiltered), + vec!["alice", "bob", "carol", "dave"], + "unfiltered scan_with_staged returns all rows correctly" + ); } /// **Documented contract** (see `stage_merge_insert` doc): chained