fix(mem_wal): support wal flush interval on durable writes#7791
fix(mem_wal): support wal flush interval on durable writes#7791hamersaw wants to merge 20 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMemWAL separates WAL durability from index visibility through writer-global cursors, adopts count-based visibility, removes legacy indexed-write configuration, updates Java and Python bindings and statistics, and revises replay, flushing, scheduling, scanning, and close behavior. ChangesMemWAL cursor and writer flow
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…ndex `insert_batches_parallel` spawned one OS thread per index on every call via `std::thread::scope`. That is sized for flush-time batches; for a handful of rows the spawn costs more than the indexing itself. Merge it into `insert_batches`, which now picks the path by size: inline on the calling thread when there is a single index or the batch is at or below `PARALLEL_INDEX_MIN_ROWS` (64) rows, and threaded above it. Atomicity is unchanged — both paths run every task and keep only the first error. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
`check_poisoned()` guarded the three write paths but no read path, so a writer that had been fenced by a peer or had latched a persistence failure kept serving scans. Those scans can hand out rows that are not durable and that replay will not reproduce — a divergent snapshot from a shard that is already known to be dead. Recovery is evict and reopen; until then the shard should fail closed. Guard `scan()`, `active_memtable_ref()`, and `in_memory_memtable_refs()`, mirroring SlateDB's `check_closed()` at the top of every read. `memtable_stats()` is deliberately left unguarded: a poisoned writer is exactly when an operator — and the eviction path — most needs to read its state. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…t re-append After `replay_memtable_from_wal` rehydrated a memtable, the durability cursor stayed at "nothing flushed". The next WAL flush therefore re-covered `[0, end)`: it appended the already-durable rows to the WAL a second time *and* re-inserted every replayed row into the in-memory indexes. None of the three indexes is idempotent. HNSW mints fresh node ids for the same row, so KNN returns it twice and burns two of the `k` slots. FTS increments `doc_count`, `total_tokens`, and every term's `df` rather than recomputing them, corrupting BM25 for the whole memtable. BTree is a multiset whose second insert sets `pk_has_overrides` permanently, disabling HNSW plan selection and WAND pruning. A full scan kept looking healthy while every index-accelerated query silently returned duplicates — and it compounded, because the WAL now held those rows twice, so the next crash replayed both copies. Stamp the durability cursor at the end of replay: the batches came from the WAL, and `insert_batches` has just re-derived the indexes over them. The index cursor already advanced itself; only durability was missing. Regression test reproduces the original report: reopen + replay + one 2-row put grew the WAL from 8 rows to 18 instead of to 10. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…hard open An in-memory index configured on a column that cannot support it fails deterministically on every insert — including inserts replayed from the WAL. So once a row is durable the shard can never reopen: replay re-reads the same rows, hits the same error, and `open()` propagates it. That is a permanently down shard, and it is the failure mode that makes poison-and-replay non-terminating. Validate once at open, before a single row is accepted: FTS columns must be Utf8/LargeUtf8/Utf8View, HNSW columns must be FixedSizeList<Float32> with a non-zero dimension, every index column must exist, and composite primary-key columns must have an order-preserving key encoding. BTree needs only existence — its backend falls through to per-row `ScalarValue` extraction and accepts any type the schema can hold. Also close two related gaps: - FTS silently appended an empty batch when its column was missing from the batch, so a misconfigured index stayed empty forever while the shard reported healthy. It now errors, matching BTree and HNSW. - HNSW's runtime capacity-exhaustion errors were labelled `invalid_input`, which blames the caller for a well-formed batch. They mean the index was sized below what the memtable holds, so they are `internal`. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
`mark_wal_flushed` had no production callers. The two collections it maintained — `wal_batch_mapping` and `flushed_batch_positions` — were allocated per memtable and read only from tests, as was `MemTable::last_flushed_wal_entry_position` (production tracks that on `WriterState`, a different struct). `BatchStore:: is_wal_flush_complete` had no callers at all. What the L0 flush actually gates on is `MemTable::all_flushed_to_wal()`, which derives from the batch store's durability watermark and stays. The tests were calling `mark_wal_flushed` only to satisfy that precondition, so they now set the watermark directly — the same thing production does, instead of a parallel bookkeeping path that only tests could reach. The two tests that covered nothing but the deleted mapping are replaced by one that covers the surviving behaviour: `all_flushed_to_wal()` flips only once the watermark covers every committed batch. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
The durable watermark was meaningless across a memtable rotation. `WalFlusher` is built once per writer and its watch channel is never reset, but the targets put against it were *memtable-local* batch positions — and positions restart at 0 in every new memtable. So after memtable A flushed N batches (watermark = N), the first put into memtable B targeted position 1, saw N >= 1, and acked immediately with no WAL append at all. The first N puts into every post-rotation memtable were falsely acked as durable: `durable_write: true` silently degraded to non-durable. When B's append finally landed it sent a *smaller* value, walking the watermark backwards — so a watcher from A targeting N could miss N entirely and block until some later memtable climbed back to it. A hang, not just a stale read. Fix the coordinate system rather than the symptom: - `BatchStore` carries an immutable `global_offset`, stamped at freeze from the outgoing store's `global_end()`. It is a coordinate, not a cursor: it cannot restart and cannot move backwards. - The durability cursor moves onto `WalFlusher` as a writer-global exclusive count, and is the only thing the watch channel carries. It advances monotonically (`max`), so an out-of-order completion cannot walk it back. - `BatchStore::local_end(global_cursor)` is the single place the global-to-local subtraction is written. It saturates in both directions, and both are reachable: a cursor below a store's offset means "nothing here yet" — the ordinary state of a freshly rotated memtable — and open-coding the subtraction underflows there, which in release wraps to a huge end and makes the entire new memtable visible at once. - The per-memtable `max_flushed_batch_position` (inclusive, `usize::MAX`-sentinel) is deleted. `pending_wal_flush_*` and `all_flushed_to_wal` now derive from the global cursor. Two things fell out while wiring it up: - The put path captured `batch_store` *after* the freeze check that may rotate the memtable, pairing the new store with the old store's end position. Capture it before, next to the insert that produced those positions. - `flush_memtable` sampled the cursor before awaiting the WAL-append completion it depends on, so the L0 precondition saw a stale value. Sample it after. Regression test: with a local target, a post-rotation put acks at durable=2 while its own batch spans [2, 3) — acked, never appended. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
`max_visible_batch_position` was an inclusive position starting at 0, so a cursor of 0 meant *both* "nothing is visible" and "batch 0 is visible". There was no sentinel and no `Option` to tell them apart. The window is real, not theoretical. `BatchStore::append` publishes `committed_len` on the put path, under the state lock, before the WAL flush that indexes the batch is even triggered — and that flush is a ~100ms S3 PUT on another task. So batch 0 of every memtable sat committed and readable for a full round-trip before it was indexed or durable, and only through the arms backed by the batch store: the index-backed arms, reading the same cursor, saw nothing. The tiers actively disagreed. Express the cursor as an exclusive count. `0` now means nothing, the `+1` disappears, `i < count` replaces `i <= max`, and the off-by-one becomes inexpressible. `checked_sub(1)` conversions at the consumers fall away — every site got simpler, as did `bounded_in_memory_membership`, whose `batch_count == 0` case now falls out of the arithmetic instead of being special-cased. Rename it to `indexed_count` while we are here. It has only ever been an *indexed* cursor — it is advanced at the end of `insert_batches`, once every index insert for a batch completes, and never before. Five read sites treated it as a visibility cursor, which is a separate thing that belongs to the writer. Deriving visibility from it is the next change; this one just stops the name from lying. Two things the conversion turned up: - `point_lookup` did `indexed_count().min(len - 1)`, reading the cursor as an inclusive index. It had no "nothing visible" case at all. - `test_shard_writer_e2e_correctness` passed *only* because of this bug: it wrote with `durable_write: false` and scanned, and the rows appeared because the un-advanced cursor of 0 was misread as "batch 0 is visible". A non-durable put is not yet read-your-writes — the index apply is welded to the WAL flush — so the test now writes durably, which is what it meant to test. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Readers keyed off `indexed_count`, which the index insert advances itself. But the WAL append and the index apply run under one `tokio::join!`, and `join!` runs both arms to completion — it does not cancel the index arm when the append fails. So on a failed append the index arm still advanced the cursor every reader treats as visibility, publishing rows that are not in the WAL and that replay will not reproduce. `MemTableScanner::new` asserts the opposite in its own doc comment. Split the cursor in two. `indexed_count` stays the *apply* cursor: it tracks what the index layer has ingested, it is what HNSW's contiguity check needs, and it advances on a failed flush — which is fine, because indexes are derived state and replay rebuilds them. `visible_count` is new, and is the only thing readers snapshot: the writer advances it downstream of *both* arms succeeding, so `visible => durable` holds unconditionally. Also make an index-apply failure terminal. `insert_batches` joins every index thread unconditionally, so a failure leaves the others fully applied, and none of them can be rolled back: HNSW has no delete, FTS has already incremented its collection statistics, BTree has linked into an append-only skiplist. Both ways out are corrupt — retry re-covers the range and re-inserts into the indexes that did succeed; skip it and the failed index is permanently short rows the others have. So discard instead: poison, and let reopen rebuild the indexes from the WAL. The rollback primitive already exists and it is `open()`. Replay publishes explicitly: its batches are durable by construction and it bypasses the flush, so without it the recovered rows stay invisible. Regression tests pin both halves. With the index arm publishing, a row whose WAL append failed becomes readable (visible_count=1 where it must be 0). An index insert that fails deterministically now poisons the writer instead of leaving it to limp on with a corrupt index. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…the WAL flush The WAL append and the index apply were welded together under one `tokio::join!` on one schedule. They have nothing in common: one is an in-memory write measured in microseconds, the other a ~100ms S3 PUT billed per call. Batching exists to bound API cost, which an in-memory index apply does not incur — so the index was being dragged onto the WAL's schedule for no reason, and that is what made read-your-writes impossible without durability. Split them into two tasks with two channels, each a single sequential consumer: - The index-apply task is triggered per put, in both modes. Being a single consumer is the safety property: `HnswGraph::insert_batch` hard-rejects any range whose start is not its `indexed_len`, and a single consumer guarantees contiguous in-order ranges however many putters race behind it. Ordering comes from the task, not from a flush interval — so triggering per-put is exactly as safe as triggering on a timer, and a put becomes visible in milliseconds rather than waiting on an S3 round-trip. - The WAL-append task keeps the expensive work and is now append-only. The `join!` is gone, and with it the dirty read it caused: a failed append can no longer publish rows through an index arm that ran anyway. They need separate channels, not two message types on one: `TaskDispatcher::run` awaits `handle()` inline, so sharing would queue every index apply behind an S3 PUT. Visibility becomes derived rather than published. `WriterCursors` holds the writer-global `durable` count; each memtable's `IndexStore` holds its own `indexed` count; and `visible` is computed on demand as `min(indexed, durable)` under `durable_write`, or just `indexed` without it. Nothing caches it, which is deliberate: a cached `min` recomputed by two independent tasks is the classic store-buffer race — under Release/Acquire both tasks can read the other's pre-store value, both compute a minimum below the true one, and a max-clamped publish leaves it permanently short, hanging any put blocked on it. With nothing cached there is nothing to leave stale. The notify channel is a bare wake-up and every waiter recomputes. What this buys: `durable_write: false` now costs the caller durability *only*, not visibility. A non-durable put is read-your-writes through every arm, indexed ones included — previously the row sat unindexed in the batch store until some later flush happened along, so a full scan could find it while every index-accelerated query could not. Also: `close()` drains both tasks, `freeze` triggers the index apply for the outgoing store, and every memtable's `IndexStore` is bound to the cursors — including the empty one built when no indexes are configured, which would otherwise fall back to `visible == indexed` and publish before durability. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
`flush_interval_ms` was inert. It routed to a timer that was only ever evaluated *on the write path*, so it could add a redundant trigger but never delay or batch one — and since every durable put triggered its own append, tuning the knob did nothing at all. Give the WAL flusher a real ticker (`MessageHandler::tickers`, which the memtable flusher already used) and take the append off the put path. The append is the only thing on that schedule: it is an S3 PUT, billed per call, and bounding that cost is the entire reason a flush interval exists. The index apply is not on it — it is in-memory and free to batch, so it stays per-put on its own task. The tick names no store. `MessageFactory` is synchronous and cannot take the async state lock, and capturing an `Arc<BatchStore>` at handler construction would pin the first memtable forever. So `WalFlushSource::NextPending` is resolved when the message is *handled* — by walking the live stores oldest-first and taking the first that still owes an append. Oldest-first, not "the active memtable", and this is load-bearing. WAL entry positions are assigned in append-call order; replay walks them ascending; row positions follow; and primary-key recency is "newest visible row position wins". So append order *is* dedup order. A tick enqueued before a freeze is handled after it, and resolving to the active memtable would append the incoming memtable's batches ahead of the outgoing one's tail — silently handing the key to the stale row on the next replay. It survives the crash that caused it, and a full scan cannot see it. Selecting by cursor makes the target a function of what is durable rather than of when the timer fired. Two starvation fixes in the dispatcher, both of which this makes reachable: - The interval used the default `MissedTickBehavior::Burst`, which replays every tick missed while `handle()` ran. A WAL append easily outlasts its own interval, so missed ticks accumulate, the ticker arm is always ready, and — being `biased` — it starves the channel. Now `Delay`. - The `biased` select polled the ticker *before* `rx.recv()`. A tick only ever adds an append a real trigger would have made anyway, whereas a message may be a freeze's completion cell or `close()`'s final append, which nothing else delivers. Messages now win. `durable_write: true` with no ticker is rejected at open: the put path no longer triggers its own append, so such a writer would block forever on an append that never comes. Better a clear error than a deadlock. Accepted cost: single-client sequential *durable* throughput drops from ~10 writes/sec (one PUT round-trip) to roughly one per tick. That is the policy choice — the interval should mean what it says, and API cost should be bounded. Latency-sensitive callers want `durable_write: false`, which now costs them durability only, not visibility. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
A single memtable holds at most `max_memtable_batches` batches, but a WAL is unbounded. Replay stuffed every WAL entry into one memtable, so a shard whose WAL had grown past one memtable's capacity failed `open()` outright with "MemTable batch store is full" — permanently unopenable. Replay now rotates exactly as the live write path does, on the same trigger. When the active memtable reaches the flush threshold it is sealed and — because the data is already durable in the WAL — flushed straight to a Lance generation with the same `MemTableFlusher::flush` the live path uses, rather than held in memory until open finishes. That bounds resident memory to ~two memtables and truncates the WAL as it goes, so a later reopen replays only the unflushed tail. Rotation is at WAL-entry boundaries, so each sealed generation covers a clean range of complete entries and stamps the last as its `replay_after_wal_entry_position`. The flush trigger is now one predicate, `memtable_reached_flush_threshold`, shared by the live path (post-insert, "room for one more batch?") and replay (pre-insert, "room for the next entry?"). It carries both criteria — `max_memtable_size` bytes and batch-store capacity. The byte trigger matters beyond avoiding overflow: it is what keeps a memtable under `max_memtable_rows`, and therefore keeps the in-memory HNSW index (sized to `max_memtable_rows`) from exhausting its capacity when the final active memtable is indexed. A single predicate is what stops the two paths from drifting — the exact class of bug this change set is about. `MemTable::is_batch_store_full` is deleted: its one production caller now goes through the shared predicate, and the two remaining test callers use the public `batch_store().is_full()` directly. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
… cursor Making the durability cursor writer-global renamed `MemTableStats:: max_flushed_batch_position` (per-memtable, inclusive, Option) to `durable_batch_count` + `global_offset` (writer-global, exclusive), but the Python and Java bindings still referenced the old field, so neither excluded crate compiled. Expose the new fields: Python's stats dict gains `durable_batch_count` and `global_offset` in place of `max_flushed_batch_position`, and Java's `MemTableStats` replaces `maxFlushedBatchPosition()` (Optional<Long>) with `durableBatchCount()` and `globalOffset()` (primitive longs), with the JNI constructor updated to match. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
`sync_indexed_write`, `async_index_buffer_rows`, and `async_index_interval` had no behavioral consumers — they were only serialized into the config-defaults metadata map. They existed to configure async index-update buffering, which the index-apply-task split replaced with unconditional per-put indexing: a write is now read-your-writes through the index in every mode, which is exactly what `sync_indexed_write` promised. The knobs no longer control anything. Remove all three across every surface: the Rust core field, builders, defaults, and metadata serialization; the write-throughput bench's now-meaningless sync/async index axis; and the Python and Java binding parameters. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
2c51844 to
6a9ab9a
Compare
The public `insert_batches` doc linked to the private `PARALLEL_INDEX_MIN_ROWS` const, which `-D rustdoc::private-intra-doc-links` rejects. Demote the link to a plain code span; the surrounding prose already explains the threshold. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
hamersaw
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (13)
rust/lance/src/dataset/mem_wal/write.rs (3)
7688-7699: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale comment: contradicts this same PR's own read-your-writes redesign.
This new comment says a non-durable put "is not yet read-your-writes: the index apply is welded to the WAL flush." That was true before this PR; after it,
put_memtable_no_waittriggers the index apply unconditionally regardless ofdurable_write(seetest_non_durable_put_is_read_your_writes, added in this same file), so a non-durable put through this exactShardWriteris read-your-writes now. Keepingdurable_write(true)here is harmless, but the rationale as written will mislead the next reader.📝 Suggested comment fix
- // `durable_write(true)` so the put waits for its flush, which is what - // currently publishes the rows. A non-durable put is *not* yet - // read-your-writes: the index apply is welded to the WAL flush, so the - // rows stay invisible until the next flush. This test used to pass with - // `durable_write(false)` only because an un-advanced cursor of 0 was - // misread as "batch 0 is visible" — it was asserting the dirty read. + // `durable_write(true)` so the put also waits for its WAL flush, giving + // this test a fully-durable assertion. A non-durable put is read-your- + // writes too now (the index apply runs independently of the WAL flush — + // see `test_non_durable_put_is_read_your_writes`); this test used to pass + // with `durable_write(false)` only because an un-advanced cursor of 0 was + // misread as "batch 0 is visible" — it was asserting the dirty read. let new_config = ShardWriterConfig::new(new_shard_id).with_durable_write(true);🤖 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 `@rust/lance/src/dataset/mem_wal/write.rs` around lines 7688 - 7699, Update the comment above new_config in the recovery verification test to reflect that put_memtable_no_wait now applies the index regardless of durable_write, so non-durable ShardWriter puts are read-your-writes. Remove the outdated claim that non-durable writes remain invisible until WAL flush, while preserving the explanation for using durable_write(true) if still relevant.
869-977: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftRebuild replay indexes before flushing rotated memtables
rust/lance/src/dataset/mem_wal/write.rs:911-971inserts replayed batches withinsert_batches_only, then rebuildsactive’s indexes only once after the loop. Any memtable sealed mid-replay is flushed with an emptyIndexStore, so configured secondary indexes and the PK dedup sidecar can be lost for every generation except the last.🤖 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 `@rust/lance/src/dataset/mem_wal/write.rs` around lines 869 - 977, Update the replay loop around flush_replayed_memtable so each active memtable rebuilds its in-memory indexes after insert_batches_only and before rotation/flush. Apply the same index population and error handling currently used in the final active-memtable rebuild to every sealed generation, preserving configured secondary indexes and PK dedup metadata in flushed memtables; avoid relying solely on the post-loop rebuild.
1167-1273: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftWait for index apply before queuing the memtable flush
rust/lance/src/dataset/mem_wal/write.rs:1217-1270
freeze_memtable()enqueuesTriggerIndexApplyand then immediately hands the same memtable to the flush task.flush_memtable()only waits on WAL durability, whileapply_index_range()mutates the sharedArc<IndexStore>in place andflush_with_indexes()reads that same state to build the flushed-generation indexes. That can write a generation with an incomplete HNSW/FTS/PK index.close()already blocks on the index watcher for this path, so the auto-flush path needs the same guard.🤖 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 `@rust/lance/src/dataset/mem_wal/write.rs` around lines 1167 - 1273, Update freeze_memtable around the trigger_index_apply and memtable flush enqueue so the queued flush waits for the pending index-apply completion before reading IndexStore state. Reuse the existing index watcher/completion mechanism used by close(), and ensure the guard covers all index types, including PK indexes, before sending TriggerMemTableFlush.rust/lance/src/dataset/mem_wal/memtable/flush.rs (1)
216-234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the
durableparameter's writer-global semantics.
durableis easy to confuse with a memtable-local batch count (ascovered_wal_entry_positionis documented to be 1-based, this one deserves the same treatment). It must be the writer-global exclusive WAL-durable count (seeWalFlusher::durable()), not a count local tomemtable's own batch store — passing a local count here would silently satisfyall_flushed_to_walfor batches that were never actually appended.📝 Suggested doc addition
pub async fn flush( &self, memtable: &MemTable, epoch: u64, covered_wal_entry_position: u64, + // `durable`: writer-global exclusive count of WAL-durable batches + // (see `WalFlusher::durable()`), NOT a count local to `memtable`'s + // own batch store. Used only to gate `all_flushed_to_wal`. durable: usize, ) -> Result<FlushResult> {Apply the same doc note to
flush_with_indexes.Also applies to: 449-468
🤖 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 `@rust/lance/src/dataset/mem_wal/memtable/flush.rs` around lines 216 - 234, Document the durable parameter in both flush and flush_with_indexes as the writer-global exclusive WAL-durable count, sourced from WalFlusher::durable(), rather than a count local to the memtable’s batch store. Clarify its distinction from covered_wal_entry_position and state that passing a local count is invalid because it may mark unappended batches as durable.Source: Coding guidelines
rust/lance/src/dataset/mem_wal/memtable/batch_store.rs (2)
487-505: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReport the inclusive last pending batch position.
The range uses exclusive
end, butend_batch_positionis exposed as the last pending position. It currently reports one past the final batch.Proposed fix
let mut stats = PendingWalFlushStats { start_batch_position: Some(start), - end_batch_position: Some(end), + end_batch_position: Some(end - 1),🤖 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 `@rust/lance/src/dataset/mem_wal/memtable/batch_store.rs` around lines 487 - 505, Update pending_wal_flush_stats to report the inclusive final pending batch position by setting end_batch_position from the exclusive range end to the preceding batch position. Keep pending_wal_flush_range’s [start, end) semantics unchanged and preserve the empty/default behavior.
231-250: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftReject writer-global coordinate overflow.
global_offset + committed_lencan wrap, making a pending store appear older than the durability cursor and potentially skipping WAL persistence. Validateglobal_offset.checked_add(capacity)at construction with a contextual error, then keepglobal_end()within that validated bound.As per coding guidelines, “Use
checked_addandchecked_mulinstead ofwrapping_addandwrapping_mulfor counters and IDs, and return an error on overflow.” <coding_guidelines>Also applies to: 451-455
🤖 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 `@rust/lance/src/dataset/mem_wal/memtable/batch_store.rs` around lines 231 - 250, Update BatchStore::with_capacity_at to validate global_offset.checked_add(capacity) during construction and return a contextual error when the writer-global range overflows, adjusting callers as needed for the fallible constructor. Store or reuse the validated end bound so global_end() cannot exceed it, and ensure committed_len calculations remain within that checked range.Source: Coding guidelines
java/src/main/java/org/lance/memwal/MemTableStats.java (1)
35-54: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winPreserve the existing public API and use JavaBean getters.
Replacing the public constructor/getter breaks existing Java clients. Retain deprecated compatibility overloads/accessors, and expose the new fields as
getDurableBatchCount()andgetGlobalOffset().As per coding guidelines, “Never break public API signatures; deprecate old APIs and add a replacement instead” and “Use JavaBean-style
getXXX()getter methods instead of bare accessor-style names.” <coding_guidelines>Also applies to: 87-94
🤖 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 `@java/src/main/java/org/lance/memwal/MemTableStats.java` around lines 35 - 54, The MemTableStats API must preserve existing public constructor and accessor compatibility while exposing the new fields with JavaBean naming. Retain deprecated overloads for the previous constructor and accessors, add or rename accessors to getDurableBatchCount() and getGlobalOffset(), and keep the existing behavior for all fields.Source: Coding guidelines
rust/lance/src/dataset/mem_wal/memtable/scanner/exec.rs (1)
39-50: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the exclusive
visible_countboundary.Position
visible_countis already invisible, but>processes that batch. Althoughmax_visible_rowusually prevents inserting its rows, this still evaluates data outside the snapshot. Usebatch_position >= visible_countand cover counts 0 and 1.Proposed fix
- if batch_position > visible_count { + if batch_position >= visible_count {🤖 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 `@rust/lance/src/dataset/mem_wal/memtable/scanner/exec.rs` around lines 39 - 50, Update the batch boundary check in the visible-row scanning function to use an exclusive visible_count boundary: change the condition guarding current_row advancement from batch_position > visible_count to batch_position >= visible_count. Ensure counts 0 and 1 treat the corresponding batch positions as invisible, without changing handling of empty batches or visible positions.python/src/mem_wal.rs (2)
942-953: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve the deprecated Python stats key during migration.
Removing
max_flushed_batch_positionimmediately breaks clients indexing the existing stats dictionary. Retain a deprecated compatibility key—or version the stats API—and add assertions covering both the legacy and replacement fields.As per coding guidelines, “Never break public API signatures; deprecate old APIs and add a replacement instead,” and all Python features must include corresponding tests.
🤖 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 `@python/src/mem_wal.rs` around lines 942 - 953, Update memtable_stats_to_pydict to retain the deprecated max_flushed_batch_position key alongside max_buffered_batch_position, mapping it to the appropriate legacy value while preserving the replacement field. Add or update Python-facing tests and assertions to verify both stats keys remain available during migration.Source: Coding guidelines
1035-1047: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not reset writer-global counters when synthesizing closed stats.
durable_batch_countandglobal_offsetare writer-global coordinates, so resetting both to zero after a non-empty generation makes the reported cursor move backwards. Preserve the durable count and advance the offset by the closed memtable’s batch count; extend the close test to assert these fields.As per coding guidelines, all Python bug fixes and features must include corresponding tests.
🤖 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 `@python/src/mem_wal.rs` around lines 1035 - 1047, Update closed_memtable_stats to preserve stats_before_close.durable_batch_count and advance stats_before_close.global_offset by the closed memtable’s batch_count instead of resetting either writer-global counter; keep per-generation row and batch fields reset. Extend the relevant close test to assert both preserved/advanced values, and ensure the Python-facing fix has corresponding test coverage.Source: Coding guidelines
rust/lance/src/dataset/mem_wal/index.rs (1)
864-890: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEnforce the exclusive indexed/visible prefix end-to-end.
The cursor is an exclusive prefix count, but writers can skip positions while readers either admit position
countor silently clamp an invalid count. Together, these paths can publish batches that were never indexed.
rust/lance/src/dataset/mem_wal/index.rs#L864-L890: require a single tracked position to equal the current indexed count and use checked addition.rust/lance/src/dataset/mem_wal/index.rs#L1029-L1032: validate that batch positions form a contiguous range beginning at the current indexed count.rust/lance/src/dataset/mem_wal/index.rs#L1767-L1786: replace the gap-advancement expectation with contiguous advancement and gap-rejection coverage.rust/lance/src/dataset/mem_wal/memtable/scanner/exec.rs#L39-L50: excludebatch_position == visible_count.rust/lance/src/dataset/mem_wal/scanner/point_lookup.rs#L894-L902: reportvisible_count > lenas an internal invariant error rather than clamping it.As per coding guidelines, validate inputs at API boundaries, do not silently guard impossible conditions, and use checked arithmetic for counters.
🤖 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 `@rust/lance/src/dataset/mem_wal/index.rs` around lines 864 - 890, Enforce the exclusive indexed/visible prefix across all listed sites: in rust/lance/src/dataset/mem_wal/index.rs:864-890, update advance_indexed_count and its callers to require the tracked position equals the current indexed count and use checked addition; in rust/lance/src/dataset/mem_wal/index.rs:1029-1032, validate batch positions are contiguous from the current indexed count; in rust/lance/src/dataset/mem_wal/index.rs:1767-1786, replace gap-advancement expectations with contiguous advancement and gap-rejection coverage; in rust/lance/src/dataset/mem_wal/memtable/scanner/exec.rs:39-50, exclude batch_position equal to visible_count; and in rust/lance/src/dataset/mem_wal/scanner/point_lookup.rs:894-902, return an internal invariant error when visible_count exceeds the collection length instead of clamping it.Source: Coding guidelines
rust/lance/src/dataset/mem_wal/memtable/scanner/exec/brute_force_vector.rs (1)
204-248: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winOff-by-one in
compute_topk's batch skip check:>should be>=.
self.visible_countis an exclusive count (batch_position < visible_countis visible, confirmed bycompute_max_visible_rowin this same file and bypoint_lookup.rs's "cursor is an exclusive count" comment). This loop's skip condition still uses the old inclusive-semantics comparison:if batch_position > self.visible_count {This lets the batch at
batch_position == visible_count(which is not visible) through the outer skip. It's currently masked because the innerif pos > max_visible_row { break; }correctly drops all of that batch's rows — so results are correct today — but the column fetch, distance computation, and prefilter evaluation for that entire invisible batch run unnecessarily on every top-k query while a memtable straddles a rotation boundary.🐛 Proposed fix
- if batch_position > self.visible_count { + if batch_position >= self.visible_count {🤖 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 `@rust/lance/src/dataset/mem_wal/memtable/scanner/exec/brute_force_vector.rs` around lines 204 - 248, Update the batch skip condition in compute_topk to use the exclusive visible_count boundary, skipping batches when batch_position is greater than or equal to self.visible_count. Preserve the existing current_row advancement and ensure batches with positions below visible_count continue through candidate evaluation.rust/lance/benches/mem_wal/fts/mem_wal_fineweb_fts.rs (1)
412-437: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReject zero flush intervals for durable benchmark runs
Args::default()uses100ms, but--max-wal-flush-interval-ms 0is still reachable, andrun_readforcesdurable_write(true)on top of this helper. That now builds an invaliddurable_write=true/max_wal_flush_interval=Noneconfig and fails at writer open; reject or remap the zero case here instead.🤖 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 `@rust/lance/benches/mem_wal/fts/mem_wal_fineweb_fts.rs` around lines 412 - 437, Update shard_writer_config so a zero max_wal_flush_interval_ms cannot produce a durable configuration with max_wal_flush_interval set to None when durable writes are enabled, including the run_read path. Remap zero to a valid flush interval or reject it before constructing the ShardWriterConfig, while preserving the existing behavior for nonzero intervals.
🤖 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 `@rust/lance/src/dataset/mem_wal/index.rs`:
- Around line 1196-1209: Update the test helper create_sized_batch to build its
data through the project’s gen_batch() builder, replacing the manual
RecordBatch::try_new and Arrow array construction with the builder’s col() calls
and into_reader_rows() conversion. Preserve the existing schema, row count, IDs,
names, and descriptions.
- Around line 409-420: Remove the obsolete durability and visibility-watermark
wording from the documentation for indexed_count, including the corresponding
comment block around the additional occurrence. Keep only documentation
describing it as the exclusive count of fully indexed memtable batches, and
avoid implying WAL durability, scanner visibility, or publication semantics.
- Around line 190-207: Update the primary-key validation block in the
surrounding index construction logic to resolve every name in pk_columns with
schema.field_with_name and return the existing descriptive missing-column error
for any absent field. Keep the encodable-type validation conditional on
pk_columns.len() > 1, and add a test covering a single-column primary key whose
field is missing.
In `@rust/lance/src/dataset/mem_wal/scanner/point_lookup.rs`:
- Around line 1396-1397: Update the comment near the BTree setup in the
point-lookup scanner to state that the BTree exercises the indexed primary-key
path, rather than claiming it advances visibility or causes batches to appear.
Keep the explanation aligned with insert_with_batch_position advancing
indexed_count even without a secondary index.
In `@rust/lance/src/dataset/mem_wal/write.rs`:
- Around line 2720-2746: Restore index-update statistics by adding a
SharedWriteStats field to IndexApplyHandler and recording the apply duration and
row count around apply_index_range in handle(). Remove the obsolete has_indexes
and record_index_update logic from do_flush, since index application now occurs
exclusively through IndexApplyHandler.
---
Outside diff comments:
In `@java/src/main/java/org/lance/memwal/MemTableStats.java`:
- Around line 35-54: The MemTableStats API must preserve existing public
constructor and accessor compatibility while exposing the new fields with
JavaBean naming. Retain deprecated overloads for the previous constructor and
accessors, add or rename accessors to getDurableBatchCount() and
getGlobalOffset(), and keep the existing behavior for all fields.
In `@python/src/mem_wal.rs`:
- Around line 942-953: Update memtable_stats_to_pydict to retain the deprecated
max_flushed_batch_position key alongside max_buffered_batch_position, mapping it
to the appropriate legacy value while preserving the replacement field. Add or
update Python-facing tests and assertions to verify both stats keys remain
available during migration.
- Around line 1035-1047: Update closed_memtable_stats to preserve
stats_before_close.durable_batch_count and advance
stats_before_close.global_offset by the closed memtable’s batch_count instead of
resetting either writer-global counter; keep per-generation row and batch fields
reset. Extend the relevant close test to assert both preserved/advanced values,
and ensure the Python-facing fix has corresponding test coverage.
In `@rust/lance/benches/mem_wal/fts/mem_wal_fineweb_fts.rs`:
- Around line 412-437: Update shard_writer_config so a zero
max_wal_flush_interval_ms cannot produce a durable configuration with
max_wal_flush_interval set to None when durable writes are enabled, including
the run_read path. Remap zero to a valid flush interval or reject it before
constructing the ShardWriterConfig, while preserving the existing behavior for
nonzero intervals.
In `@rust/lance/src/dataset/mem_wal/index.rs`:
- Around line 864-890: Enforce the exclusive indexed/visible prefix across all
listed sites: in rust/lance/src/dataset/mem_wal/index.rs:864-890, update
advance_indexed_count and its callers to require the tracked position equals the
current indexed count and use checked addition; in
rust/lance/src/dataset/mem_wal/index.rs:1029-1032, validate batch positions are
contiguous from the current indexed count; in
rust/lance/src/dataset/mem_wal/index.rs:1767-1786, replace gap-advancement
expectations with contiguous advancement and gap-rejection coverage; in
rust/lance/src/dataset/mem_wal/memtable/scanner/exec.rs:39-50, exclude
batch_position equal to visible_count; and in
rust/lance/src/dataset/mem_wal/scanner/point_lookup.rs:894-902, return an
internal invariant error when visible_count exceeds the collection length
instead of clamping it.
In `@rust/lance/src/dataset/mem_wal/memtable/batch_store.rs`:
- Around line 487-505: Update pending_wal_flush_stats to report the inclusive
final pending batch position by setting end_batch_position from the exclusive
range end to the preceding batch position. Keep pending_wal_flush_range’s
[start, end) semantics unchanged and preserve the empty/default behavior.
- Around line 231-250: Update BatchStore::with_capacity_at to validate
global_offset.checked_add(capacity) during construction and return a contextual
error when the writer-global range overflows, adjusting callers as needed for
the fallible constructor. Store or reuse the validated end bound so global_end()
cannot exceed it, and ensure committed_len calculations remain within that
checked range.
In `@rust/lance/src/dataset/mem_wal/memtable/flush.rs`:
- Around line 216-234: Document the durable parameter in both flush and
flush_with_indexes as the writer-global exclusive WAL-durable count, sourced
from WalFlusher::durable(), rather than a count local to the memtable’s batch
store. Clarify its distinction from covered_wal_entry_position and state that
passing a local count is invalid because it may mark unappended batches as
durable.
In `@rust/lance/src/dataset/mem_wal/memtable/scanner/exec.rs`:
- Around line 39-50: Update the batch boundary check in the visible-row scanning
function to use an exclusive visible_count boundary: change the condition
guarding current_row advancement from batch_position > visible_count to
batch_position >= visible_count. Ensure counts 0 and 1 treat the corresponding
batch positions as invisible, without changing handling of empty batches or
visible positions.
In `@rust/lance/src/dataset/mem_wal/memtable/scanner/exec/brute_force_vector.rs`:
- Around line 204-248: Update the batch skip condition in compute_topk to use
the exclusive visible_count boundary, skipping batches when batch_position is
greater than or equal to self.visible_count. Preserve the existing current_row
advancement and ensure batches with positions below visible_count continue
through candidate evaluation.
In `@rust/lance/src/dataset/mem_wal/write.rs`:
- Around line 7688-7699: Update the comment above new_config in the recovery
verification test to reflect that put_memtable_no_wait now applies the index
regardless of durable_write, so non-durable ShardWriter puts are
read-your-writes. Remove the outdated claim that non-durable writes remain
invisible until WAL flush, while preserving the explanation for using
durable_write(true) if still relevant.
- Around line 869-977: Update the replay loop around flush_replayed_memtable so
each active memtable rebuilds its in-memory indexes after insert_batches_only
and before rotation/flush. Apply the same index population and error handling
currently used in the final active-memtable rebuild to every sealed generation,
preserving configured secondary indexes and PK dedup metadata in flushed
memtables; avoid relying solely on the post-loop rebuild.
- Around line 1167-1273: Update freeze_memtable around the trigger_index_apply
and memtable flush enqueue so the queued flush waits for the pending index-apply
completion before reading IndexStore state. Reuse the existing index
watcher/completion mechanism used by close(), and ensure the guard covers all
index types, including PK indexes, before sending TriggerMemTableFlush.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d9eb1f21-0977-44fd-bea5-20b38f93090d
📒 Files selected for processing (40)
java/lance-jni/src/mem_wal.rsjava/src/main/java/org/lance/memwal/MemTableStats.javajava/src/main/java/org/lance/memwal/ShardWriterConfig.javajava/src/test/java/org/lance/memwal/MemWalTest.javapython/python/lance/dataset.pypython/python/tests/test_mem_wal.pypython/src/dataset.rspython/src/mem_wal.rsrust/lance/benches/mem_wal/fts/mem_wal_fineweb_fts.rsrust/lance/benches/mem_wal/fts/mem_wal_fts_read_bench.rsrust/lance/benches/mem_wal/kv/mem_wal_kv_point_lookup.rsrust/lance/benches/mem_wal/point_lookup/mem_wal_point_lookup_bench.rsrust/lance/benches/mem_wal/vector/hnsw/mem_wal_recall_hnsw.rsrust/lance/benches/mem_wal/vector/mem_wal_index_micro.rsrust/lance/benches/mem_wal/vector/mem_wal_vector_bench.rsrust/lance/benches/mem_wal/write/mem_wal_replay.rsrust/lance/benches/mem_wal/write/mem_wal_shard_writer_backpressure.rsrust/lance/benches/mem_wal/write/mem_wal_write.rsrust/lance/src/dataset/mem_wal/api.rsrust/lance/src/dataset/mem_wal/hnsw/graph.rsrust/lance/src/dataset/mem_wal/hnsw/storage.rsrust/lance/src/dataset/mem_wal/index.rsrust/lance/src/dataset/mem_wal/index/fts.rsrust/lance/src/dataset/mem_wal/memtable.rsrust/lance/src/dataset/mem_wal/memtable/batch_store.rsrust/lance/src/dataset/mem_wal/memtable/flush.rsrust/lance/src/dataset/mem_wal/memtable/scanner/builder.rsrust/lance/src/dataset/mem_wal/memtable/scanner/exec.rsrust/lance/src/dataset/mem_wal/memtable/scanner/exec/brute_force_vector.rsrust/lance/src/dataset/mem_wal/memtable/scanner/exec/btree.rsrust/lance/src/dataset/mem_wal/memtable/scanner/exec/dedup_scan.rsrust/lance/src/dataset/mem_wal/memtable/scanner/exec/fts.rsrust/lance/src/dataset/mem_wal/memtable/scanner/exec/scan.rsrust/lance/src/dataset/mem_wal/memtable/scanner/exec/vector.rsrust/lance/src/dataset/mem_wal/scanner/block_list.rsrust/lance/src/dataset/mem_wal/scanner/exec/pk_block_filter.rsrust/lance/src/dataset/mem_wal/scanner/fts_search.rsrust/lance/src/dataset/mem_wal/scanner/point_lookup.rsrust/lance/src/dataset/mem_wal/wal.rsrust/lance/src/dataset/mem_wal/write.rs
💤 Files with no reviewable changes (10)
- java/src/test/java/org/lance/memwal/MemWalTest.java
- rust/lance/benches/mem_wal/write/mem_wal_replay.rs
- rust/lance/benches/mem_wal/kv/mem_wal_kv_point_lookup.rs
- rust/lance/benches/mem_wal/point_lookup/mem_wal_point_lookup_bench.rs
- rust/lance/benches/mem_wal/fts/mem_wal_fts_read_bench.rs
- rust/lance/src/dataset/mem_wal/api.rs
- rust/lance/benches/mem_wal/vector/mem_wal_vector_bench.rs
- python/src/dataset.rs
- java/src/main/java/org/lance/memwal/ShardWriterConfig.java
- python/python/lance/dataset.py
| fn create_sized_batch(schema: &ArrowSchema, start_id: i32, num_rows: usize) -> RecordBatch { | ||
| let ids: Vec<i32> = (0..num_rows as i32).map(|i| start_id + i).collect(); | ||
| let names: Vec<String> = ids.iter().map(|id| format!("name-{id}")).collect(); | ||
| let descriptions: Vec<String> = ids.iter().map(|id| format!("hello world {id}")).collect(); | ||
| RecordBatch::try_new( | ||
| Arc::new(schema.clone()), | ||
| vec![ | ||
| Arc::new(Int32Array::from(ids)), | ||
| Arc::new(StringArray::from(names)), | ||
| Arc::new(StringArray::from(descriptions)), | ||
| ], | ||
| ) | ||
| .unwrap() | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use the project’s gen_batch() test-data builder.
Replace the manual RecordBatch::try_new setup with .col() and .into_reader_rows().
As per coding guidelines, use the gen_batch() builder API for test data setup instead of manual Arrow construction.
🤖 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 `@rust/lance/src/dataset/mem_wal/index.rs` around lines 1196 - 1209, Update the
test helper create_sized_batch to build its data through the project’s
gen_batch() builder, replacing the manual RecordBatch::try_new and Arrow array
construction with the builder’s col() calls and into_reader_rows() conversion.
Preserve the existing schema, row count, IDs, names, and descriptions.
Source: Coding guidelines
Address CodeRabbit review feedback on the WAL visibility branch:
- validate_index_configs now rejects a single-column primary key on a
column absent from the schema, closing the gap where such a config
passed validation and then failed deterministically on every index
build and WAL replay. Existence is checked for all PK columns; the
order-preserving encodable-type check stays gated on composite keys.
- Restore the index-update statistics. Index application moved onto its
own task, leaving do_flush's record_index_update branch dead (gated on
a hardcoded false) so index_update_count/time/rows and
log_wal_breakdown() reported a permanent zero. apply_index_range now
returns IndexApplyStats { rows_indexed, duration } and
IndexApplyHandler records real applies, skipping coalesced no-ops. Drop
the dead do_flush gate and the vestigial index fields on WalFlushResult.
- Fix stale docs that still called indexed_count a durable visibility
watermark, and correct the point-lookup BTree test comment.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
rust/lance/src/dataset/mem_wal/write.rs (4)
1624-1640: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate configuration before claiming and fencing the shard.
claim_epochandwrite_fence_sentinelrun before these checks. An invalid flush interval or index configuration therefore fences the existing writer and then fails to open. Move all side-effect-free validation intoShardWriter::openbeforeclaim_epoch.As per coding guidelines, validate invalid configuration at the API boundary.
🤖 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 `@rust/lance/src/dataset/mem_wal/write.rs` around lines 1624 - 1640, Move the durable-write interval check and validate_index_configs call in ShardWriter::open so both execute before claim_epoch and write_fence_sentinel. Keep these validations side-effect-free and reject invalid configuration at the API boundary, ensuring no shard claim or fencing occurs when validation fails.Source: Coding guidelines
2598-2600: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winPropagate close-time flush failures.
All three completion results are discarded. A failed final WAL append or MemTable flush therefore lets
close()returnOk(()), falsely acknowledging persistence before the in-memory state is dropped. Match each completion result and return its typed error; treat a closed completion channel as an I/O error.Proposed WAL completion handling
- let _ = reader.await_value().await; + match reader.await_value().await { + Some(Ok(_)) => {} + Some(Err(failure)) => return Err(failure.into_error()), + None => { + return Err(Error::io( + "WAL flush handler exited before reporting close completion", + )); + } + }For MemTable watchers, call
durability.into_result()?, aswait_for_flush_drainalready does.As per coding guidelines, external-call failures that can corrupt data or falsely acknowledge writes must be propagated.
Also applies to: 2630-2632, 2656-2658
🤖 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 `@rust/lance/src/dataset/mem_wal/write.rs` around lines 2598 - 2600, Update the completion handling in the close path around the three watcher branches to propagate every final WAL append or MemTable flush result instead of discarding it. Match each awaited completion, return its typed error, use durability.into_result()? for MemTable watchers consistent with wait_for_flush_drain, and convert a closed completion channel into an I/O error so close() cannot return success before persistence completes.Source: Coding guidelines
1940-1946: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the watcher documentation to match cursor-based visibility.
These comments still say the watcher resolves a combined flush/watermark operation and that non-durable writes return
None. The implementation now always returns a watcher: it waits for indexing, plus durability only when enabled.As per coding guidelines, documentation examples and public API semantics must remain synchronized.
Also applies to: 1987-1992, 2057-2059
🤖 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 `@rust/lance/src/dataset/mem_wal/write.rs` around lines 1940 - 1946, Update the watcher documentation for the affected methods around Self::delete, Self::put_no_wait, and the additional referenced declarations to describe cursor-based visibility: the watcher always resolves indexing, and waits for durability only when durability is enabled. Remove statements describing a combined flush/watermark operation or None for non-durable writes, while preserving the documented in-memory visibility behavior.Source: Coding guidelines
1209-1238: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftDo not drop the outgoing memtable when rotation dispatch fails.
After
std::mem::replace, eithertrigger_index_apply(...)?ortrigger_flush(...)?can return beforeold_memtableis added tofrozen_memtables. The local value is then dropped, and the put path suppresses the error and returns success—losing the newly inserted rows.Make rotation transactional: retain the outgoing memtable before any fallible dispatch, and poison/return the failure instead of merely warning.
As per coding guidelines, external-call failures that can corrupt data must be handled, and cleanup failures must not be silently swallowed.
Also applies to: 2123-2125
🤖 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 `@rust/lance/src/dataset/mem_wal/write.rs` around lines 1209 - 1238, Make memtable rotation transactional in the rotation method containing old_memtable, trigger_index_apply, and trigger_flush: retain the outgoing memtable before either fallible dispatch, and ensure every dispatch failure poisons and returns the error rather than allowing the local memtable to drop or merely warning. Add the retained memtable to frozen_memtables on failure as needed, preserving the inserted rows and applying the same handling to the related cleanup path around the additionally noted code.Source: Coding guidelines
rust/lance/src/dataset/mem_wal/index.rs (1)
118-133: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate
field_idagainst the resolved column.validate_index_configsonly checksconfig.column(), so a config with a stale or mismatchedfield_idcan still pass and later be resolved under the wrong schema field byget_*_by_field_id. Reject mismatches and include the index name, column, configured field ID, and resolved field ID in the error.🤖 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 `@rust/lance/src/dataset/mem_wal/index.rs` around lines 118 - 133, Update validate_index_configs around the schema.field_with_name resolution to compare each config’s configured field_id with the resolved field.id. Reject mismatches using an invalid-input error that includes the index name, column, configured field ID, and resolved field ID; preserve the existing missing-column validation.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.
Inline comments:
In `@rust/lance/src/dataset/mem_wal/wal.rs`:
- Around line 401-405: Validate every position in [start, end_batch_position)
before either cursor advances, updating both the indexed path and
flush_from_batch_store to reject missing batches instead of silently filtering
them out. Include the missing position and relevant batch-store bounds in the
returned error, while preserving normal processing when all positions exist.
---
Outside diff comments:
In `@rust/lance/src/dataset/mem_wal/index.rs`:
- Around line 118-133: Update validate_index_configs around the
schema.field_with_name resolution to compare each config’s configured field_id
with the resolved field.id. Reject mismatches using an invalid-input error that
includes the index name, column, configured field ID, and resolved field ID;
preserve the existing missing-column validation.
In `@rust/lance/src/dataset/mem_wal/write.rs`:
- Around line 1624-1640: Move the durable-write interval check and
validate_index_configs call in ShardWriter::open so both execute before
claim_epoch and write_fence_sentinel. Keep these validations side-effect-free
and reject invalid configuration at the API boundary, ensuring no shard claim or
fencing occurs when validation fails.
- Around line 2598-2600: Update the completion handling in the close path around
the three watcher branches to propagate every final WAL append or MemTable flush
result instead of discarding it. Match each awaited completion, return its typed
error, use durability.into_result()? for MemTable watchers consistent with
wait_for_flush_drain, and convert a closed completion channel into an I/O error
so close() cannot return success before persistence completes.
- Around line 1940-1946: Update the watcher documentation for the affected
methods around Self::delete, Self::put_no_wait, and the additional referenced
declarations to describe cursor-based visibility: the watcher always resolves
indexing, and waits for durability only when durability is enabled. Remove
statements describing a combined flush/watermark operation or None for
non-durable writes, while preserving the documented in-memory visibility
behavior.
- Around line 1209-1238: Make memtable rotation transactional in the rotation
method containing old_memtable, trigger_index_apply, and trigger_flush: retain
the outgoing memtable before either fallible dispatch, and ensure every dispatch
failure poisons and returns the error rather than allowing the local memtable to
drop or merely warning. Add the retained memtable to frozen_memtables on failure
as needed, preserving the inserted rows and applying the same handling to the
related cleanup path around the additionally noted code.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 15642c97-9253-4cfb-ad2a-38323c15efa7
📒 Files selected for processing (4)
rust/lance/src/dataset/mem_wal/index.rsrust/lance/src/dataset/mem_wal/scanner/point_lookup.rsrust/lance/src/dataset/mem_wal/wal.rsrust/lance/src/dataset/mem_wal/write.rs
The public `WalFlushResult` doc linked to the private `apply_index_range` fn, which `-D rustdoc::private-intra-doc-links` rejects. Demote the link to a plain code span; the surrounding prose already explains the reference. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/lance/src/dataset/mem_wal/wal.rs (1)
163-167: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake reader visibility poison-aware.
visible_countis exposed throughIndexStore::visible_count, but it never checks the terminal poison state. A poisoned writer can still report already-indexed rows, especially in non-durable mode where this returnsindexed_countimmediately. Gate this path on poison or return an error so poisoned writers cannot serve reads.🤖 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 `@rust/lance/src/dataset/mem_wal/wal.rs` around lines 163 - 167, Update MemWAL::visible_count, including its non-durable early-return path, to check the terminal poison state before reporting any rows. Reuse the existing poison-state mechanism and ensure poisoned writers cannot expose indexed data through IndexStore::visible_count, either by returning zero or propagating the established error type.
🤖 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.
Outside diff comments:
In `@rust/lance/src/dataset/mem_wal/wal.rs`:
- Around line 163-167: Update MemWAL::visible_count, including its non-durable
early-return path, to check the terminal poison state before reporting any rows.
Reuse the existing poison-state mechanism and ensure poisoned writers cannot
expose indexed data through IndexStore::visible_count, either by returning zero
or propagating the established error type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a066d2f6-0bff-4f48-b0cc-d80fa5829a84
📒 Files selected for processing (1)
rust/lance/src/dataset/mem_wal/wal.rs
…or lock `check_poisoned` and `mark_terminal_failure` both `.unwrap()`ed the `terminal_error` lock, so a panic anywhere under it turned every later poison check into a panic of its own. Ignore the poisoning instead of reporting it. The guarded data cannot be torn: the sole writer builds the `WalFlushFailure` and assigns it whole under an `is_none()` check, so a panic mid-section leaves the slot exactly as it was. There is no invariant here for poisoning to protect — the flag is pure alarm. Mapping the poisoning to an error would be worse than the panic. This mutex exists to carry the reason a writer is fenced, and recovery is reopen -> replay driven by that `FenceReason`; answering "mutex was poisoned" buries it precisely when a caller needs it. It also has nowhere to go in `mark_terminal_failure`, which returns `()`. Test: latch a `PersistenceFailure`, poison the mutex from a panicking thread, then assert the typed reason and message still surface and the latch still keeps the first failure. It panics against the old code. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
| @@ -1104,7 +1232,6 @@ impl SharedWriterState { | |||
| self.wal_flusher.trigger_flush( | |||
There was a problem hiding this comment.
[P1] Retain the outgoing memtable before any fallible dispatch. state.memtable has already been replaced here. If this send, or the preceding index-apply send, fails, the question-mark return happens before old_memtable is retained in frozen_memtables, so accepted rows disappear from the read view. Reproduced on b239143: put 10 rows, stop the task executor, force_seal_active returns WAL flush channel closed, and the following scan returns 0 rows instead of 10. Please retain the outgoing table first, then dispatch, and poison/return the failure without dropping the table.
| ) -> Result<()> { | ||
| for config in configs { | ||
| let column = config.column(); | ||
| let field = schema.field_with_name(column).map_err(|_| { |
There was a problem hiding this comment.
[P1] Verify that the configured field ID identifies this resolved column. Resolving only config.column is insufficient because later index selection also uses config.field_id. A BTree configured for column name with the PK field ID for id passes this function and can be reused as the single-column PK index under the wrong identity. A ShardWriter::open regression with that exact mismatch succeeds on b239143; it should return InvalidInput. Please compare the configured and resolved field IDs and include both in the error.
| // single row is accepted. Such a config fails deterministically on every | ||
| // insert, including inserts replayed from the WAL — so once a row is | ||
| // durable the shard can never reopen. Fail the open instead. | ||
| validate_index_configs(index_configs, schema.as_ref(), &pk_columns)?; |
There was a problem hiding this comment.
[P1] Run all side-effect-free validation before claiming the epoch. This validation runs only after claim_epoch and, for a successor, write_fence_sentinel. An open that is guaranteed to fail therefore fences the healthy writer already serving the shard. Reproduced on b239143: writer B gets the expected invalid flush-interval error, then writer A fails check_fenced with PeerClaimedEpoch (local epoch 1, stored epoch 2). Please derive PK metadata and run interval/index validation before claim_epoch.
| batch_store, | ||
| indexes, | ||
| }, | ||
| source: WalFlushSource::BatchStore { batch_store }, |
There was a problem hiding this comment.
[P1] Propagate the completion result of this final flush. The await_value call immediately below discards both WalFlushFailure and channel closure, allowing close to continue toward Ok after this persistence operation failed. The same pattern remains for frozen-memtable and WAL-only completions. This overlaps #7769, so please land and rebase that PR first and preserve its typed error propagation here. Close must distinguish success, stored failure, and a closed completion channel for every requested final persistence operation.
u70b3
left a comment
u70b3
left a comment
There was a problem hiding this comment.
Review summary (current head b239143) The latest poisoned-terminal_error lock fix is focused and its regression test passes, but it does not change the P1 blockers in the inline comments. I recommend landing #7769 first and rebasing this PR on it because the close/error-propagation paths overlap; after that, the rotation, open-ordering, and index-identity regressions still need fixes. One additional close-path issue is outside this PR diff and cannot be attached inline: TaskExecutor::shutdown_all (write.rs:545-552) logs handler errors and JoinError panics, then always returns Ok. A regression using a real ShardWriter reaches an intentional handler panic but ShardWriter::close still returns Ok. Please retain and return the first task failure so shutdown cannot falsely report success.
Three fixes from PR review, each confirmed by reproducing the failure in a test that fails on the pre-fix tree: - open(): derive PK metadata and run the durable-write/flush-interval and index-config validation *before* claim_epoch. These checks ran after the epoch was claimed (and, for a successor, after the predecessor was fenced), so an open doomed by purely local input knocked the healthy incumbent off the shard with a PeerClaimedEpoch fence. - validate_index_configs(): reject a config whose field_id names a different column than its `column`. Index selection keys off field_id alone (a single-column PK reuses the BTree whose field_id matches), so a mismatch bound the wrong index under a valid-looking name -- stale reads plus the wrong column flushed into the durable PK sidecar. Coupled with open()'s validation move because the signature and its only caller change together. - freeze_memtable(): retain the outgoing memtable in the read view before the fallible index-apply/WAL-flush dispatches, and poison on a failed send. The active memtable was already replaced, so a failed dispatch dropped the table and its accepted rows silently vanished -- a zero-row scan with no error, on a branch whose whole point is to poison instead. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Resolved the ShardWriter::close conflict by keeping lance-format#7769's typed failure propagation (flush_final_wal / merge_close_stage) while passing this branch's append-only WalFlushSource::BatchStore { batch_store } -- index apply is a separate task here, so the final flush carries no indexes. Dropped the removed `sync_indexed_write` field from lance-format#7769's new close-failure test.
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
rust/lance/src/dataset/mem_wal/memtable/scanner/exec/brute_force_vector.rs (1)
239-247: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winSkip the first invisible batch with
>= visible_count.
visible_countis exclusive, but this condition processesbatch_position == visible_count. Those rows are eventually discarded bymax_visible_row, after unnecessary distance/filter evaluation that may also surface errors from data outside the scan snapshot.- if batch_position > self.visible_count { + if batch_position >= self.visible_count {🤖 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 `@rust/lance/src/dataset/mem_wal/memtable/scanner/exec/brute_force_vector.rs` around lines 239 - 247, Update the batch visibility check in the scanner loop to skip batches when batch_position is equal to or greater than self.visible_count, while preserving the current_row advancement for skipped batches. This prevents processing rows outside the exclusive visibility boundary before max_visible_row filtering.rust/lance/src/dataset/mem_wal/write.rs (3)
38-348: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftIntroduce a cross-language deprecation window for the removed MemWAL APIs. The PR removes public configuration and statistics surfaces outright, breaking Rust and Python consumers without a migration path.
rust/lance/src/dataset/mem_wal/write.rs#L38-L348: retain deprecated configuration fields/builders or equivalent compatibility methods.rust/lance/src/dataset/mem_wal/write.rs#L2786-L2801: retain a deprecated derived accessor formax_flushed_batch_position.python/python/lance/dataset.py#L5089-L5095: retain deprecatedinitialize_mem_walkeywords with migration warnings.python/python/lance/dataset.py#L5185-L5191: retain deprecatedmem_wal_writerkeywords with migration warnings.As per coding guidelines, public API signatures must not be broken; old APIs must be deprecated and provided with replacements.
🤖 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 `@rust/lance/src/dataset/mem_wal/write.rs` around lines 38 - 348, Introduce a deprecation window for the removed MemWAL APIs: in rust/lance/src/dataset/mem_wal/write.rs:38-348 retain the removed public configuration fields and builder methods, marking them deprecated and directing callers to replacements; in rust/lance/src/dataset/mem_wal/write.rs:2786-2801 retain a deprecated derived max_flushed_batch_position accessor; in python/python/lance/dataset.py:5089-5095 and python/python/lance/dataset.py:5185-5191 restore the removed initialize_mem_wal and mem_wal_writer keywords, emit migration warnings when used, and preserve the replacement API behavior.Source: Coding guidelines
2177-2185: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake every post-acceptance dispatch failure terminal. Once rows are inserted or a memtable is retained, a closed background channel must poison the writer and return an error; otherwise callers can observe ambiguous writes or wait forever.
rust/lance/src/dataset/mem_wal/write.rs#L2177-L2185: poison before propagating index-apply send failure.rust/lance/src/dataset/mem_wal/write.rs#L1263-L1282: apply the same handling to the subsequent MemTable-flush send.🤖 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 `@rust/lance/src/dataset/mem_wal/write.rs` around lines 2177 - 2185, The post-acceptance dispatches must poison the writer before returning failures from closed background channels. In rust/lance/src/dataset/mem_wal/write.rs:2177-2185, update the trigger_index_apply handling around writer_state.trigger_index_apply so send failure poisons the writer before propagation; apply the same poison-before-error handling to the subsequent MemTable-flush send at rust/lance/src/dataset/mem_wal/write.rs:1263-1282, preserving existing success behavior.
2684-2695: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not let index-drain errors bypass the remaining close stages.
The preceding
trigger_index_apply(...)?andwatcher.wait().await?return before the final WAL flush, MemTable drain, andshutdown_all(). Fold these operations intoclose_resultwithmerge_close_stage, just like the new WAL and freeze stages.🤖 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 `@rust/lance/src/dataset/mem_wal/write.rs` around lines 2684 - 2695, Update the close flow around trigger_index_apply and watcher.wait so their errors are merged into close_result via merge_close_stage instead of propagated with ?. Preserve execution of the final WAL flush, MemTable drain, and shutdown_all stages even when index draining fails, using descriptive stage names consistent with the existing WAL and freeze stages.
🟡 Other comments (1)
python/python/lance/dataset.py-5976-6012 (1)
5976-6012: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winImplement the validation promised by
DataOverlayFile.There is no
__post_init__, so descending, duplicate, negative, mixed-shape, or per-field-count-mismatched offsets are accepted despite the documentedValueErrorcontract. Validatedata_file, offset shape,u32range, strict ordering, and sparse-list count here, with parameterized tests.As per coding guidelines, validate inputs at API boundaries and reject invalid values with descriptive errors containing the relevant names, values, sizes, and types.
🤖 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 `@python/python/lance/dataset.py` around lines 5976 - 6012, Add a __post_init__ to DataOverlayFile that validates data_file, accepts either a flat integer-offset list or a sparse list of per-field integer lists, rejects mixed shapes and invalid types, enforces the u32 range and strict ascending order for every offset list, and requires sparse-list count to match data_file’s field count. Raise descriptive ValueError/TypeError messages containing the relevant parameter names, values, sizes, and types, and add parameterized tests covering each invalid case.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.
Inline comments:
In `@rust/lance/src/dataset/mem_wal/index.rs`:
- Around line 201-204: Update validate_index_configs at the
lance_schema.field(column) lookup to propagate a descriptive InvalidInput error
when the Arrow schema contains a column absent from the Lance schema, instead of
panicking via expect. Add a test covering mismatched Arrow and Lance schemas and
verify the public API returns the expected error.
---
Outside diff comments:
In `@rust/lance/src/dataset/mem_wal/memtable/scanner/exec/brute_force_vector.rs`:
- Around line 239-247: Update the batch visibility check in the scanner loop to
skip batches when batch_position is equal to or greater than self.visible_count,
while preserving the current_row advancement for skipped batches. This prevents
processing rows outside the exclusive visibility boundary before max_visible_row
filtering.
In `@rust/lance/src/dataset/mem_wal/write.rs`:
- Around line 38-348: Introduce a deprecation window for the removed MemWAL
APIs: in rust/lance/src/dataset/mem_wal/write.rs:38-348 retain the removed
public configuration fields and builder methods, marking them deprecated and
directing callers to replacements; in
rust/lance/src/dataset/mem_wal/write.rs:2786-2801 retain a deprecated derived
max_flushed_batch_position accessor; in python/python/lance/dataset.py:5089-5095
and python/python/lance/dataset.py:5185-5191 restore the removed
initialize_mem_wal and mem_wal_writer keywords, emit migration warnings when
used, and preserve the replacement API behavior.
- Around line 2177-2185: The post-acceptance dispatches must poison the writer
before returning failures from closed background channels. In
rust/lance/src/dataset/mem_wal/write.rs:2177-2185, update the
trigger_index_apply handling around writer_state.trigger_index_apply so send
failure poisons the writer before propagation; apply the same
poison-before-error handling to the subsequent MemTable-flush send at
rust/lance/src/dataset/mem_wal/write.rs:1263-1282, preserving existing success
behavior.
- Around line 2684-2695: Update the close flow around trigger_index_apply and
watcher.wait so their errors are merged into close_result via merge_close_stage
instead of propagated with ?. Preserve execution of the final WAL flush,
MemTable drain, and shutdown_all stages even when index draining fails, using
descriptive stage names consistent with the existing WAL and freeze stages.
---
Other comments:
In `@python/python/lance/dataset.py`:
- Around line 5976-6012: Add a __post_init__ to DataOverlayFile that validates
data_file, accepts either a flat integer-offset list or a sparse list of
per-field integer lists, rejects mixed shapes and invalid types, enforces the
u32 range and strict ascending order for every offset list, and requires
sparse-list count to match data_file’s field count. Raise descriptive
ValueError/TypeError messages containing the relevant parameter names, values,
sizes, and types, and add parameterized tests covering each invalid case.
🪄 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: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 67f8d9a9-451e-4e18-abb1-5b49b84ed1a4
📒 Files selected for processing (11)
python/python/lance/dataset.pyrust/lance/src/dataset/mem_wal/index.rsrust/lance/src/dataset/mem_wal/memtable/flush.rsrust/lance/src/dataset/mem_wal/memtable/scanner/exec/brute_force_vector.rsrust/lance/src/dataset/mem_wal/memtable/scanner/exec/btree.rsrust/lance/src/dataset/mem_wal/memtable/scanner/exec/dedup_scan.rsrust/lance/src/dataset/mem_wal/memtable/scanner/exec/fts.rsrust/lance/src/dataset/mem_wal/memtable/scanner/exec/scan.rsrust/lance/src/dataset/mem_wal/memtable/scanner/exec/vector.rsrust/lance/src/dataset/mem_wal/scanner/exec/pk_block_filter.rsrust/lance/src/dataset/mem_wal/write.rs
💤 Files with no reviewable changes (1)
- rust/lance/src/dataset/mem_wal/scanner/exec/pk_block_filter.rs
| let resolved_field_id = lance_schema | ||
| .field(column) | ||
| .expect("column resolved in the Arrow schema is present in the Lance schema") | ||
| .id; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Return an error when the Arrow and Lance schemas disagree.
validate_index_configs is public and receives both schemas independently. A column present only in schema reaches this .expect() and panics instead of returning InvalidInput.
Proposed fix
let resolved_field_id = lance_schema
.field(column)
- .expect("column resolved in the Arrow schema is present in the Lance schema")
+ .ok_or_else(|| {
+ Error::invalid_input(format!(
+ "index '{}' references column '{}' present in the Arrow schema but absent from the Lance schema",
+ config.name(),
+ column,
+ ))
+ })?
.id;Add a test with mismatched Arrow/Lance schemas. As per coding guidelines, validate inputs at API boundaries with descriptive errors and never use .expect() for fallible library operations.
📝 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.
| let resolved_field_id = lance_schema | |
| .field(column) | |
| .expect("column resolved in the Arrow schema is present in the Lance schema") | |
| .id; | |
| let resolved_field_id = lance_schema | |
| .field(column) | |
| .ok_or_else(|| { | |
| Error::invalid_input(format!( | |
| "index '{}' references column '{}' present in the Arrow schema but absent from the Lance schema", | |
| config.name(), | |
| column, | |
| )) | |
| })? | |
| .id; |
🤖 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 `@rust/lance/src/dataset/mem_wal/index.rs` around lines 201 - 204, Update
validate_index_configs at the lance_schema.field(column) lookup to propagate a
descriptive InvalidInput error when the Arrow schema contains a column absent
from the Lance schema, instead of panicking via expect. Add a test covering
mismatched Arrow and Lance schemas and verify the public API returns the
expected error.
Source: Coding guidelines
…kipping it `apply_index_range` and `flush_from_batch_store` walked `[start, end)` and silently dropped any position `BatchStore::get` returned `None` for. The store is append-only (`get` returns `None` only past `committed_len`), so a hole means the caller asked to cover a batch that was never committed. Skipping it while still advancing the cursor was silent corruption: the WAL path advanced durability to `end_batch_position` even though a batch was never appended (lost on replay), and the index path advanced `indexed_count` past a never-indexed batch (rows counted visible but absent from every index). Both now error on a missing position, naming the range and committed length. The flush path returns a terminal (writer_poisoned) error so `flush` poisons the writer before durability moves; the index path's error poisons via its handler. Reopen replays the WAL. Holes are impossible today, but this fails loudly the day eviction lands rather than diverging silently. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
2 participants
Summary
The MemWAL writer had a visibility model that didn't hold.
visible ⟹ durablewasasserted in
MemTableScanner's own doc comment but never enforced, and the two watermarksit was defined over were both broken. This series makes the invariant true, then rebuilds
durable_writeto mean what SlateDB'sawait_durablemeans: it controls whether thecaller blocks on durability, and nothing else.
Every bug below was verified by reintroducing it and watching a test fail, not by
inspection. Line references are to the pre-change tree.
The bugs
A poisoned writer served reads.
check_poisoned()guarded the three write paths and noread path, so a writer already known dead — fenced by a peer, or with a latched persistence
failure — kept handing out rows that were never durable and that replay would not
reproduce. A divergent snapshot from a corpse.
Replay re-appended and re-indexed everything it recovered. Nothing marked replayed
batches as already-durable, so the next WAL flush re-covered
[0, end): it appended thealready-durable rows a second time and re-inserted every replayed row into the indexes.
None of the three in-memory indexes is idempotent — HNSW mints fresh node ids for the same
row, FTS increments
doc_count/dfrather than recomputing them, BTree is a multiset whosesecond insert sets
pk_has_overridespermanently. A full scan looked healthy while everyindex-accelerated query silently returned duplicates, and it compounded: the WAL then held
those rows twice, so the next crash replayed both copies. (Reproduced: one 2-row put after a
reopen grew the WAL from 8 rows to 18.)
The durable watermark was meaningless across a memtable rotation.
WalFlusheris builtonce per writer and its watch channel is never reset, but the targets put against it were
memtable-local batch positions — and positions restart at 0 in every memtable. So after
memtable A flushed N batches, the first put into memtable B targeted position 1, saw N ≥ 1,
and acked immediately with no WAL append at all. The first N puts into every
post-rotation memtable were falsely acked as durable;
durable_write: truesilentlydegraded to non-durable. When B's append landed it sent a smaller value, walking the
watermark backwards — so a watcher from A could miss its target entirely and block forever.
The visibility cursor's zero was ambiguous. An inclusive position starting at 0 meant
both "nothing is visible" and "batch 0 is visible".
BatchStore::appendpublishescommitted_lenon the put path before the flush that indexes the batch is even triggered,and that flush is a ~100ms S3 PUT on another task — so batch 0 of every memtable sat
committed and readable for a full round-trip before it was indexed or durable, and only
through the arms backed by the batch store. The index-backed arms saw nothing. The tiers
actively disagreed.
A failed WAL append still published its rows. The append and the index apply ran
concurrently under one
tokio::join!, which runs both arms to completion and does notcancel the index arm when the append fails. So a failed append still advanced the cursor
every reader keyed off.
Index failures were never terminal. An index error carries no fence reason, so it was
always non-terminal — the writer limped on with a corrupt index. But
insert_batchesjoinsevery index thread unconditionally, so a failure leaves the others fully applied, and none
of HNSW/FTS/BTree has a delete. Both ways out are corrupt: retry re-covers the range and
re-inserts into the indexes that did succeed; skip it and the failed index is permanently
short rows the others have.
The flush interval was inert. It routed to a timer only ever evaluated on the write
path, so it could add a redundant trigger but never delay or batch one — and since every
durable put triggered its own append, tuning the knob did nothing.
The design
Two cursors, one derived view.
durableis a writer-global exclusive count on the writer;indexedis a per-memtable count on itsIndexStore; andvisibleis derived, neverstored —
min(indexed, durable)underdurable_write, or justindexedwithout it.Nothing caches
visible, deliberately. A cachedminrecomputed by two independent tasks isthe classic store-buffer race: under Release/Acquire both tasks can read the other's
pre-store value, both compute a minimum below the true one, and a max-clamped publish leaves
it permanently short — hanging any put blocked on it. With nothing cached there is nothing to
leave stale. The notify channel is a bare wake-up; every waiter recomputes.
The append and the index apply become two tasks on two channels. They have nothing in
common: one is an in-memory microsecond write, the other a ~100ms S3 PUT billed per call.
Batching exists to bound API cost, which an index apply does not incur. They need separate
channels because
TaskDispatcher::runawaitshandle()inline — sharing would queue everyindex apply behind an S3 PUT. Each is a single sequential consumer, which is the safety
property:
HnswGraph::insert_batchhard-rejects any range whose start is not itsindexed_len, and a single consumer guarantees contiguous in-order ranges however manyputters race behind it. Ordering comes from the task, not the flush interval — so
triggering per-put is exactly as safe as triggering on a timer.
The WAL append moves onto a real background ticker, and a tick names no store: it is resolved
when handled, by walking the live stores oldest-first and taking the first that still
owes an append. Not "the active memtable" — WAL entry positions are assigned in append-call
order, replay walks them ascending, row positions follow, and PK recency is "newest visible
row position wins". So append order is dedup order, and a tick enqueued before a freeze but
handled after it would append the incoming memtable ahead of the outgoing one's tail,
silently handing the key to the stale row on the next replay. It survives the crash that
caused it, and a full scan cannot see it.
What this buys
durable_write: falsenow costs durability only, not visibility. A non-durable put isread-your-writes through every arm, index-backed ones included. Previously the row sat
unindexed in the batch store until some later flush wandered by — a full scan could find it
while every index-accelerated query could not.
visible ⟹ durableis unconditional in durable mode, via themin.Accepted cost: single-client sequential durable throughput drops from ~10 writes/sec
(one PUT round-trip) to roughly one per tick. That is the policy choice — the interval should
mean what it says, and API cost should be bounded. Latency-sensitive callers want
durable_write: false.Breaking change
MemTableStats::max_flushed_batch_position: Option<usize>(per-memtable, inclusive) →durable_batch_count: usize+global_offset: usize(writer-global, exclusive). The field'smeaning changed, so it is renamed to force callers to notice rather than silently misread
it.
durable_write: truewith nomax_wal_flush_intervalis now rejected atopen(): the putpath no longer triggers its own append, so such a writer would block forever. Better a clear
error than a deadlock.
🤖 Generated with Claude Code