fix(mem_wal): read-your-writes via split index-apply and dual visibility cursors#7888
Merged
Merged
Conversation
…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) <noreply@anthropic.com>
`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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
`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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
…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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
…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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
…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) <noreply@anthropic.com>
📝 WalkthroughWalkthroughMemWAL durability and visibility tracking now use writer-global durable cursors, indexed counts, and exclusive visible-count boundaries. WAL appends and index application are decoupled, while replay, memtable rotation, scanners, query executors, statistics, and benchmarks consume the new model. ChangesMemWAL cursor and visibility refactor
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 (2)
rust/lance/src/dataset/mem_wal/memtable/scanner/exec/brute_force_vector.rs (1)
239-247: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winUse
>=consistently for the exclusive visibility boundary.Both loops treat
visible_countas though it were inclusive. Fix the boundary at each site:
rust/lance/src/dataset/mem_wal/memtable/scanner/exec/brute_force_vector.rs#L239-L247: skip the first invisible batch before performing vector distance computation.rust/lance/src/dataset/mem_wal/memtable/scanner/exec.rs#L44-L52: skip the first invisible batch before PK extraction.🤖 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, Both batch-scanning loops use visible_count as an exclusive boundary but currently compare with >; update the condition in brute_force_vector.rs lines 239-247 and exec.rs lines 44-52 to use >=, ensuring the first invisible batch is skipped before vector distance computation or primary-key extraction.rust/lance/src/dataset/mem_wal/memtable/scanner/exec.rs (1)
44-52: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winUse the exclusive boundary when skipping batches.
visible_countexposes[0, visible_count), sobatch_position == visible_countis already invisible. The current>condition lets one invisible batch enter PK extraction; the later row bound masks its output today, but this performs unnecessary work and leaves the helper inconsistent with the cursor contract.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 44 - 52, Update the batch-skipping condition in the memtable scanner loop to treat batch_position equal to visible_count as invisible, using the exclusive boundary before PK extraction. Preserve the current_row advancement for all skipped batches and continue processing only positions below visible_count.
🟡 Other comments (1)
rust/lance/src/dataset/mem_wal/memtable/scanner/builder.rs-1429-1429 (1)
1429-1429: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale visibility-test comments.
Both comments still describe
visible_count = 1, while their tests expose two batches withvisible_count = 2:
rust/lance/src/dataset/mem_wal/memtable/scanner/builder.rs#L1429-L1429: state that[0, 2)is visible.rust/lance/src/dataset/mem_wal/memtable/scanner/exec/scan.rs#L420-L421: state thatvisible_count = 2exposes positions 0 and 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/scanner/builder.rs` at line 1429, Update the stale visibility comments: in rust/lance/src/dataset/mem_wal/memtable/scanner/builder.rs lines 1429-1429, state that visibility range [0, 2) is exposed; in rust/lance/src/dataset/mem_wal/memtable/scanner/exec/scan.rs lines 420-421, state that visible_count = 2 exposes positions 0 and 1. Make comment-only changes and leave test behavior unchanged.
🧹 Nitpick comments (4)
rust/lance/src/dataset/mem_wal/index.rs (2)
202-215: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAvoid
.expect()in library code; return a descriptive error instead.Line 204 panics if the resolved Arrow column has no matching Lance-schema field. Even if this is an invariant today,
validate_index_configsis a public boundary and should reject with a typed error rather than panic.♻️ Proposed change
- 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 '{}' column '{}' resolved in the Arrow schema but is absent from the \ + Lance schema", + config.name(), + column, + )) + })? + .id;As per coding guidelines: "Never use
.unwrap(),.expect(),panic!(), orassert!()in library code for fallible operations; use?withResultand proper error 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 `@rust/lance/src/dataset/mem_wal/index.rs` around lines 202 - 215, Update validate_index_configs to replace the expect on lance_schema.field(column) with fallible error propagation, returning a descriptive typed error when the Arrow column has no matching Lance-schema field; preserve the existing field-ID mismatch validation for successfully resolved fields.Source: Coding guidelines
1054-1057: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace bare
.unwrap()with.expect("reason").
batchesis guaranteed non-empty by the early return at Line 932, so this cannot panic, but the guideline still prohibits bare.unwrap()in library code.♻️ Proposed change
- let max_bp = batches.iter().map(|b| b.batch_position).max().unwrap(); + let max_bp = batches + .iter() + .map(|b| b.batch_position) + .max() + .expect("batches is non-empty (checked at the top of insert_batches)");As per coding guidelines: "Avoid bare
.unwrap()... If unavoidable, use.expect("reason")."🤖 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 1054 - 1057, In the indexed-count update near the batch-position calculation, replace the bare unwrap on the max batch position iterator with expect and provide a concise reason stating that batches are guaranteed non-empty after the earlier return.Source: Coding guidelines
rust/lance/benches/mem_wal/vector/mem_wal_index_micro.rs (1)
258-258: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
target_indexed_counttruncates on a non-multiple checkpoint.
cp / batch_sizetruncates, so ifcpis not a multiple ofbatch_sizethe catch-up loop can break one batch before the final partial batch is indexed — the checkpoint's query/throughput numbers would then be measured with the last batch still un-indexed. The siblingmem_wal_recall_hnsw.rsusescp.div_ceil(batch_size). Consider matching it for consistency.♻️ Proposed change
- let target_indexed_count = cp / batch_size; + let target_indexed_count = cp.div_ceil(batch_size);🤖 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/vector/mem_wal_index_micro.rs` at line 258, Update the target_indexed_count calculation to use ceiling division via cp.div_ceil(batch_size), ensuring the catch-up loop indexes the final partial batch when the checkpoint is not a multiple of batch_size.rust/lance/src/dataset/mem_wal/memtable.rs (1)
496-509: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc comments still describe the old inclusive-position semantics. These methods now delegate to
BatchStore's exclusive visible-count model where a batch at positioniis visible iffi < visible_count(prefix[0, visible_count)), but the docs still say "inclusive" /i <= visible_count, which contradicts the new contract and thebatch_store.rsdocs.
rust/lance/src/dataset/mem_wal/memtable.rs#L496-L509: update theget_visible_batchesdoc — replace "(inclusive)", thei <= visible_countline, and the# Arguments"(inclusive)" wording with the exclusive-count semantics ([0, visible_count)).rust/lance/src/dataset/mem_wal/memtable.rs#L756-L757: fix thescandoc's "Maximum batch position visible (inclusive)" to describevisible_countas the exclusive count of visible batches.As per coding guidelines: "Ensure doc comments match actual semantics."
🤖 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.rs` around lines 496 - 509, Update the doc comments for MemTable::get_visible_batches at rust/lance/src/dataset/mem_wal/memtable.rs:496-509 to describe exclusive visibility as the prefix [0, visible_count), replacing all inclusive wording and i <= visible_count semantics. Also update the scan documentation at rust/lance/src/dataset/mem_wal/memtable.rs:756-757 to describe visible_count as the exclusive count of visible batches.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.
Outside diff comments:
In `@rust/lance/src/dataset/mem_wal/memtable/scanner/exec.rs`:
- Around line 44-52: Update the batch-skipping condition in the memtable scanner
loop to treat batch_position equal to visible_count as invisible, using the
exclusive boundary before PK extraction. Preserve the current_row advancement
for all skipped batches and continue processing only positions below
visible_count.
In `@rust/lance/src/dataset/mem_wal/memtable/scanner/exec/brute_force_vector.rs`:
- Around line 239-247: Both batch-scanning loops use visible_count as an
exclusive boundary but currently compare with >; update the condition in
brute_force_vector.rs lines 239-247 and exec.rs lines 44-52 to use >=, ensuring
the first invisible batch is skipped before vector distance computation or
primary-key extraction.
---
Other comments:
In `@rust/lance/src/dataset/mem_wal/memtable/scanner/builder.rs`:
- Line 1429: Update the stale visibility comments: in
rust/lance/src/dataset/mem_wal/memtable/scanner/builder.rs lines 1429-1429,
state that visibility range [0, 2) is exposed; in
rust/lance/src/dataset/mem_wal/memtable/scanner/exec/scan.rs lines 420-421,
state that visible_count = 2 exposes positions 0 and 1. Make comment-only
changes and leave test behavior unchanged.
---
Nitpick comments:
In `@rust/lance/benches/mem_wal/vector/mem_wal_index_micro.rs`:
- Line 258: Update the target_indexed_count calculation to use ceiling division
via cp.div_ceil(batch_size), ensuring the catch-up loop indexes the final
partial batch when the checkpoint is not a multiple of batch_size.
In `@rust/lance/src/dataset/mem_wal/index.rs`:
- Around line 202-215: Update validate_index_configs to replace the expect on
lance_schema.field(column) with fallible error propagation, returning a
descriptive typed error when the Arrow column has no matching Lance-schema
field; preserve the existing field-ID mismatch validation for successfully
resolved fields.
- Around line 1054-1057: In the indexed-count update near the batch-position
calculation, replace the bare unwrap on the max batch position iterator with
expect and provide a concise reason stating that batches are guaranteed
non-empty after the earlier return.
In `@rust/lance/src/dataset/mem_wal/memtable.rs`:
- Around line 496-509: Update the doc comments for MemTable::get_visible_batches
at rust/lance/src/dataset/mem_wal/memtable.rs:496-509 to describe exclusive
visibility as the prefix [0, visible_count), replacing all inclusive wording and
i <= visible_count semantics. Also update the scan documentation at
rust/lance/src/dataset/mem_wal/memtable.rs:756-757 to describe visible_count as
the exclusive count of visible batches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 301d9d8b-b0c1-470c-ad8e-f4a33644f2fc
📒 Files selected for processing (21)
rust/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/write/mem_wal_shard_writer_backpressure.rsrust/lance/src/dataset/mem_wal/index.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
…elds The writer-global durability cursor work replaced MemTableStats' max_flushed_batch_position with durable_batch_count and global_offset, but the Java and Python bindings still referenced the removed field, breaking the JNI and pylance builds (and every downstream CI job). Expose durable_batch_count and global_offset across both bindings and drop max_flushed_batch_position. In the Python closed-writer snapshot, synthesize both as the writer's global end, since a successful close leaves every buffered batch flushed and WAL-durable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…sors Address review feedback on the exclusive-count visibility switch: - Correct MemTable docs that still described `visible_count` as an inclusive position (`i <= visible_count`); it is an exclusive count (`i < visible_count`), matching BatchStore's helpers. - Switch the dedup and brute-force-vector scan skip conditions from `batch_position > visible_count` to `>= visible_count` so the batch at position == visible_count is excluded. Correct today only because the shared max_visible_row guard already drops those rows; this makes the skip match the exclusive-count semantics used everywhere else and removes the latent dirty-read a relaxed guard would expose. - Add unit tests for WriterCursors (advance/durable monotonicity, visible_count under both durable_write modes) and for the apply_index_range happy path (cursor advance, row count, coalesced no-op). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Basically what this PR does is that when we write we use two simultaneous paths (1) durably persist to objectstore and (2) write to our in-memory structures (memtable + indexes). In case of durable writes we only mark the data as "visible" once both succeed, for non-durable we mark it as visible when the in-memory appends succeeds. Previously, we had issues where read visibility was incorrect, this PR updates so that we always have read-your-writes -- meaning data is visible once the write mechanism (durable or non-durable) says it is. |
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 (2)
rust/lance/src/dataset/mem_wal/wal.rs (1)
1876-1889: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLeftover comment fragment before this test.
The plain
//comment ("Regression test for the visibility-cursor bug: with an empty IndexStore (the common case for WAL-managed tables that mirror an index-less base") is cut off mid-sentence and immediately followed by a new///doc block describing the actual test. This looks like an edit artifact where the old comment wasn't fully removed when the new doc comment was added.🧹 Proposed fix
- // Regression test for the visibility-cursor bug: with an empty IndexStore - // (the common case for WAL-managed tables that mirror an index-less base /// The index apply and the WAL append are separate tasks with separate /// cursors. Appending makes a range durable; it does not index it. Indexing🤖 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 1876 - 1889, Remove the leftover truncated `//` comment immediately before the `#[rstest::rstest]` test, keeping the complete `///` documentation block that follows it unchanged.rust/lance/src/dataset/mem_wal/memtable.rs (1)
752-781: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale doc comment on
scan(): wrong "Arguments" and copy-pasted "Panics" section.
scan()takes no arguments (only&self), but the doc still has an# Argumentsblock describing avisible_countparameter — and line 758 even explains it's now captured internally, contradicting the block above it. The# Panicstext ("Panics if called aftersignal_memtable_flush_complete()has consumed the cell") is verbatim fromcreate_memtable_flush_completion's doc (lines 310-312) and has nothing to do withscan(), which doesn't panic on that condition.📝 Proposed fix
/// Create a scanner for querying this MemTable. /// - /// # Arguments - /// - /// * `visible_count` - Maximum batch position visible (inclusive) - /// /// The scanner captures the current `visible_count` from the /// `IndexStore` at construction time to ensure consistent visibility. /// /// # Panics /// - /// Panics if called after `signal_memtable_flush_complete()` has consumed the cell. + /// Panics if the memtable has no indexes configured (see `set_indexes`/`set_indexes_arc`). /// /// # ExampleAs per coding guidelines, "Ensure doc comments match actual semantics; distinguish mutates-in-place (
&mut self) from returns-new-value behavior" and "Add doc comments to public API elements that convey semantic meaning... do not restate type signatures" — this doc violates both by describing a nonexistent parameter and an unrelated panic condition.🤖 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.rs` around lines 752 - 781, Update the documentation above MemTable::scan to remove the nonexistent visible_count Arguments section and the unrelated Panics section. Retain only documentation describing scan’s actual behavior, including that the scanner captures visibility from IndexStore at construction, and keep the existing usage example accurate.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.
Outside diff comments:
In `@rust/lance/src/dataset/mem_wal/memtable.rs`:
- Around line 752-781: Update the documentation above MemTable::scan to remove
the nonexistent visible_count Arguments section and the unrelated Panics
section. Retain only documentation describing scan’s actual behavior, including
that the scanner captures visibility from IndexStore at construction, and keep
the existing usage example accurate.
In `@rust/lance/src/dataset/mem_wal/wal.rs`:
- Around line 1876-1889: Remove the leftover truncated `//` comment immediately
before the `#[rstest::rstest]` test, keeping the complete `///` documentation
block that follows it unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 15ab2ead-6b4d-47ca-acb3-65a9162d501d
📒 Files selected for processing (7)
java/lance-jni/src/mem_wal.rsjava/src/main/java/org/lance/memwal/MemTableStats.javapython/src/mem_wal.rsrust/lance/src/dataset/mem_wal/memtable.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/wal.rs
`WalFlushResult` leaks into the public API through a re-exported type's signature, so rustdoc documents it publicly. Its doc linked to `apply_index_range` — a `pub` free fn in the private `wal` module that is not re-exported, hence private to rustdoc — tripping `rustdoc::private_intra_doc_links` under `-D warnings`. The function is an internal task entry point that shouldn't join the public surface, so keep the readable name but drop the link. Same fix applied to `IndexApplyStats` where the identical link was a latent trap. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The open-time check rejecting durable_write without a max_wal_flush_interval belongs with the ticker-driven append design in the stacked follow-up, not here. On this branch a durable put still self-triggers its own WAL append (put_memtable_no_wait), so the check was incorrect -- durable+no-ticker makes progress fine -- and it broke 17 tests that use that combination for deterministic, background-flush-free behavior. Re-point test_doomed_open_does_not_fence_incumbent at the index-config validation (also pre-claim_epoch) as its doomed-open trigger. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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/write.rs (1)
2217-2219: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the no-wait API docs for non-durable watchers.
This now returns
Some(BatchDurableWatcher)in both modes; for non-durable writes it waits for index application, not WAL durability. Update theput_no_waitanddelete_no_waitdocs, which still promiseNonewhendurable_writeis off and say resolution follows a flush.As per coding guidelines, “Ensure doc comments match actual semantics.”
🤖 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 2217 - 2219, Update the documentation for put_no_wait and delete_no_wait to state that both return Some(BatchDurableWatcher) regardless of durable_write; for non-durable writes, the watcher resolves after index application rather than WAL flush or durability. Remove the outdated None/flush guarantees while preserving the documented behavior for durable writes.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.
Outside diff comments:
In `@rust/lance/src/dataset/mem_wal/write.rs`:
- Around line 2217-2219: Update the documentation for put_no_wait and
delete_no_wait to state that both return Some(BatchDurableWatcher) regardless of
durable_write; for non-durable writes, the watcher resolves after index
application rather than WAL flush or durability. Remove the outdated None/flush
guarantees while preserving the documented behavior for durable writes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 8c14ce66-bd60-470b-9f41-43ab5e46bc76
📒 Files selected for processing (1)
rust/lance/src/dataset/mem_wal/write.rs
wjones127
approved these changes
Jul 21, 2026
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
This was referenced Jul 21, 2026
hamersaw
added a commit
that referenced
this pull request
Jul 22, 2026
## WAL flush-interval ticker The feature the original #7791 was titled for, now small because it sits on the read-visibility redesign (#7888) that landed ahead of it. `flush_interval_ms` 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. - **append the WAL on a background ticker, resolved by cursor** — give the WAL flusher a real ticker (`MessageHandler::tickers`) and take the append off the put path. The append is the only thing on that schedule — it's an S3 PUT, billed per call, and bounding that cost is the whole reason a flush interval exists. The index apply stays per-put on its own task. `WalFlushSource::NextPending` is resolved when the message is handled, by walking live stores oldest-first and taking the first that still owes an append. - **remove the inert `sync_indexed_write` config family** — `sync_indexed_write`, `async_index_buffer_rows`, and `async_index_interval` had no behavioral consumers after the index-apply-task split (#7888) made every write read-your-writes. Removed across Rust core, benches, and Python/Java bindings. **Stack** — this is the last of three; the first two have merged into `main`: 1. #7872 — foundational shard-open hardening ✅ merged 2. #7888 — read-your-writes / two-cursor visibility redesign ✅ merged 3. **this PR** — flush-interval ticker → `main` Closes the original monolithic #7791. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WAL read-visibility redesign
The core of the former #7791. Rows become visible to readers only once they are both indexed and WAL-durable, resolved through two independent cursors instead of a single conflated watermark. Index application runs on its own task rather than as an arm of the WAL flush, giving read-your-writes in every mode.
Commits (bottom → top):
indexedcursor and a writer-global durability cursor; readers snapshotvisible_countderived from both.claim_epoch(a doomed open must not fence the incumbent); reject a config whosefield_idnames a different column than itscolumn; retain the outgoing memtable and poison on a failed freeze dispatch.Stack (the former #7791, split for reviewability)
main🤖 Generated with Claude Code