feat(mem-wal): derive index catch-up from the version a commit read - #8481
Merged
jackye1995 merged 5 commits intoAug 11, 2026
Conversation
Contributor
|
Important This PR touches the Lance format specification. Substantive changes to the format specification — the If this is a meaningful format change:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
xuanyu-z
force-pushed
the
feat/derive-index-catchup
branch
from
August 11, 2026 17:46
8ff3eb7 to
505c205
Compare
xuanyu-z
force-pushed
the
feat/derive-index-catchup
branch
from
August 11, 2026 18:02
505c205 to
83b1f92
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Replaces the IndexCatchupAdvance mechanism. A commit no longer carries a claim about index coverage; the coverage is derived from the version the transaction read. Coverage has only one possible proof: nothing maps a compaction generation to the fragments its rows landed in, so the only way an index can show it holds those rows is to span the table as the transaction read it. Rather than accept that claim and re-validate it, derive it -- an index whose segments cover every fragment live at read_version is caught up to that version's compacted_sstables. A claim can no longer go stale between inspection and commit, the answer survives rebase (read_version is fixed for a transaction's life), and any operation can earn coverage: an ordinary index build that covers the table records catch-up as a side effect, where before only a dedicated repair could. Kept: the feature bit and its half-set refusal, the index_catchup field and the missing-entry rule, activation, and withdraw-on-change. An index is compared by whole segment metadata rather than UUID, because an Update prunes a segment's fragment bitmap in place while keeping its UUID. Removed: IndexCatchupAdvance and its proto message, the mem_wal_index_catchup_advances field on CreateIndex, OptimizeOptions::mem_wal_index_catchup, the advance-validation pass, and the rebase handling that carried an advance through.
ConcreteFileVersion no longer implements From<LanceFileVersion>, so these test call sites do not compile. Pre-existing on main; unrelated to this branch, and droppable once main is green.
…afeguards Review round 3. A no-work optimize on an activated table committed whenever the table had ever compacted, minting an empty version on every maintenance pass. It now dry-runs the derivation and commits only when the position would actually move, so the rule lives in one place rather than being restated. migrate_indices reports which indices it recalculated instead of the caller snapshotting every fragment bitmap to find out, which drops the second per-commit snapshot the post-migration safeguard needed. The reserved proto name goes: the field never reached a release, so only the field number is worth holding back. Comments and the format spec catch up with CoverageIdentity, the committed-progress clamp, and the empty-read-version decision.
xuanyu-z
force-pushed
the
feat/derive-index-catchup
branch
from
August 11, 2026 18:59
83b1f92 to
2fb2dc3
Compare
It never reached a release, so no stored transaction file can carry it.
Contributor
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The revision fixes both no-work repair boundaries: catch-up is committed when the authoritative derivation would advance, and the next maintenance pass remains a true no-op once current. Post-build bitmap recovery also withdraws coverage conservatively, and the unreleased protobuf cleanup follows the repository unstable-format policy.
Please mark this PR with the breaking-change label.
jackye1995
approved these changes
Aug 11, 2026
jackye1995
left a comment
Contributor
There was a problem hiding this comment.
looks good to me, pending CI
Contributor
This is still experimental feature so it's fine to just make changes |
xuanyu-z
added a commit
to lancedb/lancedb
that referenced
this pull request
Aug 14, 2026
…#3911) > Stacked on #3780. Blocked only on #3922 (`lance` → `v11.0.0-beta.6`), so CI > stays red until that lands. ## Missing coverage must mean "not known to be covered" #3780 caps the SSTable exclusion watermark at an index's recorded catch-up when there is one, and silently ignores the case where there is none. On a table that requires catch-up, an absent entry means the index is *not* known to hold the compacted rows — and the LSM base arm reads base through the index (`fast_search`, no brute-force tail), so dropping that SSTable loses those rows for that query. ```rust Some(caught_up) => watermark = watermark.min(caught_up), None if catchup_required => watermark = 0, // retain everything None => {} ``` `catchup_required` reads the manifest feature bit directly, and requires both words: a half-set manifest is treated as legacy, which is the conservative side. Without the bit the field is not maintained at all, so absence carries no information and behaviour is unchanged. ## Activation, as a table-level entry point `Table::require_mem_wal_index_catchup()` performs the one-way switch, separate from `set_lsm_write_spec`: a table carrying the bit retains every generation until something records catch-up, so it has to follow the deployment of whatever repairs coverage, not the creation of the table. This is a convenience, not the only path — a writer holding the dataset calls the equivalent on `DatasetMemWalExt`, which is what the WAL pod does. Lance enforces the preconditions either way: the MemWAL index must exist, and the table must not already carry `compacted_sstables` from before this protocol, since those numbers cannot be validated. ## Still correct after the Lance rework lance-format/lance#8481 replaced the transmitted `IndexCatchupAdvance` with a position derived at commit time from the version a transaction read. That changed how a writer earns coverage; it did not change what a reader may conclude from its absence. The rule here, and the field it reads, are unchanged. ## Tests Existing `exclusion_watermarks` unit tests carry the new argument. Coverage against a real dataset follows once #3922 lands and this can build.
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.
Replaces the
IndexCatchupAdvancemechanism from #8263. A commit no longercarries a claim about index coverage; the coverage is derived from the version
the transaction read.
Why
Under #8263, a worker that extended an index had to describe what it had done —
the index name, the exact segment UUIDs it expected to publish, the fragments
those segments covered when it looked, and every fragment live at that moment —
and the commit re-validated all of it. Four fields and a validation pass to
transmit a fact the commit can already see.
It can see it because coverage has only one possible proof. Nothing maps a
compaction generation to the fragments its rows landed in. The only way an index
can show it holds those rows is to span the table as the transaction read it. So
rather than accept a claim and check it, derive it: an index whose segments
together cover every fragment live at
read_versionis caught up to thatversion's
compacted_sstables.Three things follow that the advance model could not offer:
committing, because there is nothing to inspect.
read_versionis fixed for a transaction'slife, so every commit attempt derives the same result. feat(mem-wal): record index catch-up positions and withdraw them on index change #8263 needed the
advance carried through the rebase untouched and re-validated.
that happens to cover the table records catch-up as a side effect. Under
feat(mem-wal): record index catch-up positions and withdraw them on index change #8263 only a dedicated repair could, so a build that fully covered had to
throw the fact away and wait for a repair to re-establish it. Because the
position is only written by a commit,
optimize_indiceskeeps committing onan activated table even when it has no new segment to publish.
What it keeps from #8263
The parts that were not about transmission:
FLAG_MEM_WAL_INDEX_CATCHUP, both words, and the refusal of a half-set stateindex_catchuponMemWalIndexDetails, and the reader rule that a missingentry means "not caught up"
require_index_catchup), including its refusal of a table thatalready carries beta-protocol compaction progress
re-earn
Two rules bound what a commit may record. It never credits past its own
compacted_sstables, so a read version since rolled back cannot retire SSTablesno live commit copied in. And it never lowers a position an index already held,
provided the index is unchanged.
"Unchanged" compares whole segment metadata, not segment UUIDs.
Operation::Updateprunes a segment's fragment bitmap in place when it touches an indexed field,
keeping the UUID — so a UUID-only comparison carries a position forward for an
index that now covers less. That is not hypothetical; it is reachable from an
ordinary merge-insert.
a_bitmap_pruned_in_place_does_not_keep_its_positionpins it.
What it removes
IndexCatchupAdvanceand its proto message, themem_wal_index_catchup_advancesfield onCreateIndex,OptimizeOptions::mem_wal_index_catchup, the advance-validation pass, and therebase handling that carried an advance through.
Tests
34 unit tests over the derivation, in
dataset/transaction.rs, and 6 through areal commit, in
index/mem_wal.rs. The derivation aloneis not the feature —
commit_transactionhas to load the read version and handit down, and only for tables carrying the bit — so the commit-path tests cover
that an index earns coverage, a legacy table earns none, and a rebase past an
append does not move what a commit earns.
Twelve fences were regressed one at a time and the failing test confirmed. Four
of the first attempts caught nothing, because the test asserted an outcome that
both the correct and the broken path produce; each was replaced with one that
discriminates.
One guard is deliberately untested: skipping the read-version index load on
legacy tables is a cost guard, not a correctness one, and regressing it changes
no observable behaviour.
cargo test -p lance --lib: 2914 passed. fmt and clippy clean.Follow-ups
UpdateMemWalStatecommit — only thesystem index may, anything else is rejected outright rather than retried
(
conflict_resolver.rs, unchanged since January). Now that an ordinary buildcan earn coverage, that race is worth revisiting: it costs a completed build.
segments_beforestill clones every index segment on each commit for tableson the protocol. The snapshot has to be owned because the operation rewrites
the list, but a smaller snapshot would do.