[BubbleCal]

Cache stable-row-ID lookups per immutable fragment so append and snapshot changes reuse unchanged work. Lookup mappings and snapshot deletion visibility are charged separately. Sparse mappings rebuild when deletions remove at least half of the retained rows; compact range mappings stay reusable, and pruned mappings are reused only when they cover the requested snapshot. Batched sparse lookups use interleaved search while preserving physical addresses, input order, duplicates and deletion visibility.

Resident candidates are checked before bounded source reads. The demand path keeps fragment-level lookups; existing complete-index callers and no-e-tag/ignore-deletions fallbacks retain their behavior. Persisted formats are unchanged. The fragment registry now amortizes dead-entry cleanup and releases sparse high-water allocations; regression coverage checks live builders and unadmitted lookups survive cleanup.

Scenario / metric (lower is better)	Baseline b5ee153	Measured revision 2663559	Benefit
Small updates: warm row-ID lookup p50	145.868 ms	58.398 ms	2.50x speedup
Small updates: seven snapshots, 56 full takes	67.288 s	64.392 s	1.04x speedup
Small updates: warm full-take p50	1060.976 ms	1171.175 ms	1.10x slowdown
Heavy deletes: warm row-ID lookup p50	79.492 ms	52.717 ms	1.51x speedup
Heavy deletes: seven snapshots, 56 full takes	66.455 s	67.439 s	1.01x slowdown
Heavy deletes: full-take peak RSS	2.809 GiB	1.630 GiB	1.72x lower peak
No deletes: longer warm full-take p50	1126.683 ms	1255.890 ms	1.11x slowdown
Heavy deletes, 128 MiB cache: longer warm full-take p50	121.888 ms	145.620 ms	1.19x slowdown

Current-head performance is not yet measured. The table preserves the accepted tradeoffs of measured ancestor 2663559, before the registry cleanup, test and documentation fixes in 61cdfcd9; it is not a performance claim for the latest head. Matched real-S3 measurements compare the previous PR head b5ee153064583d39019ca15939d4de9dfeeef815 with 266355944c437cad88c4a2bbedc0ab48f6057908: 100M shuffled/spilled IDs, 1000 initial fragments, five UInt64 columns, c7i.8xlarge, Rust 1.98.1, release-with-debug. Both arms use the same helper, inputs and limits: index 4 GiB, metadata 1 GiB, total 4 GiB (8 GiB for snapshot full takes), swap 0 B. The pressure case uses a 128 MiB index cache and 91% deletions. Values average two fresh processes with reversed order; the final two rows are separate 32-warm-call follow-ups, not pooled with the initial 8-call runs. There is no matched main comparison for either revision.

Full-take gains vary by run and workload; warm latency can regress despite lower lookup CPU and snapshot memory. High overlap still probes candidate fragments, directory work remains O(F), and cold preparation or held readers need memory beyond cache accounting. The deletion-rebuild policy supplies the memory savings; batch search adds no resident index. Preserve the latency regressions above when evaluating latency-sensitive workloads.

Validation at measured ancestor 2663559: 395 targeted core/rowids/public-take test executions, full x86 workspace Clippy, and 48 S3 processes with 1920 independent physical-address/payload checks. For prior head 61cdfcd9, project CI passed, including Linux x86/ARM Rust tests, workspace Clippy, Python compatibility tests and Java tests. Prior integration head 2581cf909 also passed applicable CI. Current head c537979af merges main e0aacd774; the only manual resolution preserves the combined imports for fragment lookup and quantized refinement cache keys. Main's take-address overflow fix is retained. Formatting, diff checks and commit hooks passed; exact-head Rust, Python and Java CI passed. Gatekeeper skipped duplicate review because Will's approving review is still active; this is not a new review of the merge commit. Benchmark results above remain specific to measured ancestor 2663559.

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

_{Tip: disable this comment in your organization's Code Review settings.}

[wjones127]

Thanks for this. Content-addressing the per-fragment units is a sound way to reuse work across snapshots. The tests are strong: the fixtures check results against oracles computed from the fixture's own arithmetic, and they cover duplicates, deletions, eviction, admission rejection and old snapshots. The append result (8s to 39ms for old-key takes) is a real win.

Blocking: the single current entry means handles on different versions evict each other's view, and that likely forces full rebuilds when a delete lands between them. I've suggested a way around it inline: cache only the parts, and make the per-version view lightweight.

Also, could you trim the PR description? It will become the commit message. It references local paths (/Users/yang/Documents/Codex/...) and internal terms ("frozen v7", "mbx", "wake9", rejected v4/v5) that won't mean anything to future readers. A short paragraph on the design (three tiers: complete view, prepared fragment units, span metadata) plus the benchmark table would be enough.

[wjones127]

question: why do we cache without knowing the deleted rows? Should we just switch to always knowing deletions when we cache?

[wjones127]

question(blocking): With one current entry per dataset, two handles on different versions keep evicting each other's view. The PR's own numbers show old-snapshot revisit going 0.002 ms -> 2.1 ms in the append case. If a delete lands between the versions, reuse_merged can't keep the changed group, so build() reloads every fragment's sequence on each alternation. Why one entry rather than per-version entries that share the prepared allocations? To clear this: either keep per-version keys with shared allocations, or add a test where two handles alternate across a delete and assert bounded read_bytes on the second round trip. One way to unblock this is the suggestion on resolve.rs:535: if the per-version view didn't own the shared allocations, per-version entries would be cheap.

[wjones127]

issue(non-blocking): When there is no complete view, every call takes this dataset-wide lock, including warm calls answered entirely from resident units. The lock is then held across the cold prepare_fragment reads below. A cold lookup on fragment A blocks an unrelated warm lookup on fragment B, which probably explains the C=4 regression in the description. Could the resident-hit pass run before taking the lock, with cold preparation deduplicated per fragment (e.g. get_or_insert_with_key on PreparedRowIdFragmentKey) instead of per dataset?

[wjones127]

suggestion: The retirement pass (and, I think, the single current entry) exists because the complete view and the per-fragment units are both cache entries charged for the same shared allocations. Could we avoid the overlap instead? Cache only the parts — per-fragment units, plus merged chunks keyed by their source group — each charged once, and make the per-version view a lightweight value (probes/end_tree plus Arcs into the parts) charged only for its own overhead. That would remove the need to retire units (including the fallback case where they're retired but no view is admitted), and per-version entries would be cheap enough to avoid the thrashing above.

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

_{Tip: disable this comment in your organization's Code Review settings.}

[wjones127]

Thanks for reworking this. Going fragment-only removes the single current entry, the retirement pass, and the dataset-wide lock, so my earlier blocking concern about handles on different versions evicting each other no longer applies. The tests are strong: oracles come from fixture arithmetic or physical scans, and they cover inline and spilled lineage, duplicates, deletions, seven snapshots, compaction, restore, eviction, and concurrent preparation.

Nothing here is blocking. The changes are crate-internal and easy to revisit, so I'm approving. Follow-ups worth considering:

• Deletions out of the lookup: building each fragment's lookup without deletions and filtering after the lookup would mean a delete never invalidates or rereads a fragment, and snapshots that differ only in deletions would share one entry.
• Warm path: every call recomputes two identities and probes the cache for every fragment, which likely explains the 1.38x slower warm p50 and lower C=4 throughput.
• Machinery: RowIdIndexBuildLocks duplicates the single-flight behavior LanceCache already provides.
• Identity drift with #8880: both PRs define a fragment's row-id/deletion content identity, and they disagree on which fields matter.
• The e-tag / ignore_deletions fallback still routes to the complete index.

Smaller nits are inline.

[wjones127]

question: Could the per-fragment lookup leave deletions out? Right now the deletion vector goes into RowIdIndex::new and into the key, so a delete on a fragment invalidates its entry and rereads its row-id source (read_row_id_sequence doesn't go through the sequence cache). Every deletion state of a fragment is a separate entry competing for capacity, which the description calls out.

If the lookup were built without deletions and keyed by the row-id source only, deletions could be applied after the lookup: the result is already an address, so it's a cheap deletion_vector.contains(offset) check, and record_matches still catches an id that's live in two fragments. That would also keep the merged representation on its O(1) Range path, since any deletion currently forces per-row decomposition. In the seven-snapshot workload, all seven snapshots would share one entry per fragment.

Is there a reason the deletions need to be built in?

[wjones127]

question: On a warm call this loop still does work for every fragment in the manifest: it builds two KeyBuilder identities (the deletion one rehashes inline deletion bytes, which aren't memoized like the row-id digest), then an index_cache probe, then a metadata_cache probe on a miss. With 1000 fragments that's thousands of hashes and cache lookups before any ID is resolved. That seems like the likely source of the 1.38x slower seven-snapshot warm p50 and the C=4 throughput drop. Earlier I suggested keeping a per-version view as a lightweight value: sorted spans plus Arcs into the cached parts, charged only for its own overhead. That would make the warm path a binary search over spans. Did you try that and drop it, or would it fit on top of this?

[wjones127]

question: What does this registry buy over LanceCache::get_or_insert_with_key? The cache already coalesces concurrent loads for the same key. As far as I can tell, the registry exists for two cases: (1) a load that only learns the span and finds no candidates, and (2) sharing an oversized lookup the cache refuses to admit. For (1), the span could go through metadata_cache.get_or_insert_with_key(RowIdFragmentSpanKey, ..), followed by index_cache.get_or_insert_with_key(PreparedRowIdFragmentKey, ..) only when the span overlaps. (2) seems rare enough not to need its own mechanism. That would drop RowIdIndexBuildLocks, its Session field, and its DeepSizeOf impl, and leave one place that manages these entries.

[wjones127]

issue: This retain scans the whole map on every for_fragment call. Entries survive while active is upgradable, i.e. while the index cache holds the PreparedFragment, so the map grows to about the number of resident fragments, not just in-flight builds. A cold query with M misses then does M full scans under a std::sync::Mutex, so O(M·N), and each call also allocates uri.to_owned() (here and in prepared). Moot if the registry goes away, otherwise prune lazily (e.g. only when an upgrade fails, or every Nth insert).

[wjones127]

question: This identity is RowIdSequenceKey plus physical_rows, the resolved base path, and the row-id column index. #8880 (in review now) factors the same identity into a shared helper without those extras, and uses it for its per-fragment allow-list pieces:

lance/rust/lance/src/session/caches.rs

Lines 349 to 373 in 0c0698e

	fn write_row_id_identity(
	builder: &mut KeyBuilder,
	row_id_meta: &RowIdMeta,
	lineage_file: Option<&DataFile>,
	) {
	match row_id_meta {
	RowIdMeta::Inline(data) => {
	builder.write_variant(0);
	builder.write_fixed_bytes(data.digest());
	}
	// The sequence lives in one of the fragment's data files, which is
	// named freshly per rewrite; the file identifies the contents the way
	// the inline digest does.
	RowIdMeta::Column => {
	builder.write_variant(1);
	match lineage_file {
	Some(file) => {
	builder.write_str(&file.path);
	builder.write_u64(file.base_id.map_or(u64::MAX, u64::from));
	}
	None => builder.write_str(""),
	}
	}
	}
	}

Which is right? If a base_id can be remapped to a different path, or the same lineage file can be projected through a different column, then RowIdSequenceKey can already serve a stale sequence, and the fix belongs there so every per-fragment cache benefits. If not, could this reuse the shared helper so the two caches can't drift apart?

[wjones127]

suggestion: #8880 fixes the same gap at the source: it adds the inline payload to DeletionFileKey itself (schema v2) via write_deletion_file_identity:

lance/rust/lance/src/session/caches.rs

Lines 130 to 149 in 0c0698e

	fn write_deletion_file_identity(builder: &mut KeyBuilder, deletion_file: &DeletionFile) {
	builder.write_u64(deletion_file.read_version);
	builder.write_u64(deletion_file.id);
	builder.write_variant(match &deletion_file.file_type {
	DeletionFileType::Array => 0,
	DeletionFileType::Bitmap => 1,
	});
	if let Some(base_id) = deletion_file.base_id {
	builder.write_some();
	builder.write_u32(base_id);
	} else {
	builder.write_none();
	}
	if let Some(inline) = &deletion_file.inline {
	builder.write_some();
	builder.write_bytes(inline);
	} else {
	builder.write_none();
	}
	}

Whichever lands second should drop its own copy, or the inline bytes get hashed twice per fragment per call. It'd be nice if deletion_identity here were just DeletionFileKey plus the base path, once that's in.

[wjones127]

question: Why is the fragment path gated on an e-tag? That gate comes from the version-keyed complete index:

lance/rust/lance/src/dataset/rowids.rs

Lines 231 to 239 in 4266ce5

	// The cache is shared by every dataset opened at this URI, and one dropped
	// and recreated there restarts at version 1. Without a token for this
	// manifest generation a cached index could be the old generation's, so
	// build a private one instead of sharing.
	let Some(e_tag) = dataset.manifest_location.e_tag.as_deref() else {
	return Ok(Some(Arc::new(
	load_row_id_index(dataset, with_deletions).await?,
	)));
	};

The fragment keys here are content-addressed (the same reasoning as #7645), so a dataset recreated at the same URI can't collide with them. As written, a dataset without an e-tag gets a private, uncached complete build on every take, which is the slowest path. Separately, ignore_deletions takes still use the complete index. So both representations of the same lookup can be resident at once (the ANN test now asserts a RowIdIndex entry). Is the plan to move those callers to fragments too?

[wjones127]

nitpick: This assertion is always true. If this call is expected to be warm (fragment 3's source and deletion state are unchanged), could it be an assert_eq! like the ones below? Otherwise I'd drop it.

Suggested change

	assert!(after_distinct.read_bytes >= before.read_bytes);
	assert_eq!(after_distinct.read_bytes, before.read_bytes);

[wjones127]

nitpick: This hardcodes the cache's per-entry overhead (two AtomicUsize plus the key). If the entry layout changes, the eviction test's budget silently stops meaning "admits one but not both". Could the test measure the weight from the cache instead, e.g. insert one entry and read size_bytes()?

[wjones127]

nitpick: This doc doesn't say what the function returns. Also on naming: comments and test names call PreparedFragment a "graph" (bounds_survive_graph_eviction...), and RowIdIndexBuildLocks also holds weak refs to finished lookups, so it's more than locks. Could we pick one name for each and stick to it? For the doc (lines 52-53), something like:

/// Map stable row ids to row addresses, preserving input order and duplicates.
/// With `with_deletions`, deleted or unknown ids map to `None`. Lookups are
/// prepared and cached per fragment, keyed by its row-id and deletion content,
/// so snapshots that leave a fragment unchanged share them.

[lance-gatekeeper]

⚠️ Gate recommendation: approve with a non-blocking risk.

Amortized registry cleanup preserves live preparation and lookup sharing without scanning every resident entry on each miss. Shared fragment mappings and snapshot coverage checks retain safe reuse across deletion and restored snapshots.

Warm end-to-end latency remains workload dependent. Measured ancestor 2663559 reported 1.10–1.11× slower warm full takes and a 1.19× slowdown under cache pressure versus b5ee153, despite faster row-ID lookups and lower heavy-delete RSS. The latest cleanup has not been benchmarked, and neither revision has a matched main comparison. Use warm full-take latency and throughput for workload sizing, and allow memory headroom for cold preparation and separately loaded complete indices.