perf(dataset): reuse session-cached manifest on checkout by zhangyue19921010 · Pull Request #7661 · lance-format/lance

zhangyue19921010 · 2026-07-07T09:43:47Z

Checking out a version or branch (Dataset::checkout_version / checkout_branch) always read the manifest from storage, even when that same version was already loaded on the Session.

This path now goes through the session metadata cache (keyed by version + e_tag), reusing a cached manifest on a hit and only reading + caching on a miss — the same helper (get_manifest) already used by URI-open and load_new_transactions.

Summary by CodeRabbit

Bug Fixes
- Improved version checkout reliability when manifests are no longer available in storage.
- Prevented removed dataset versions from being served from cache.
- Reduced unnecessary read I/O during dataset checkout and open operations, improving performance.

codecov · 2026-07-07T12:41:27Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

zhangyue19921010 · 2026-07-08T03:47:51Z

Hi @wjones127 Would u mind to take a look ? Thanks!

This PR also fixes a stale manifest cache "zombie read" bug: after a version is removed (e.g. by auto-cleanup), checking out or opening that version would return the stale cached manifest from the session cache instead of correctly surfacing a NotFound — fixed by having get_manifest trust the cache only when version resolution confirms the manifest still exists (size.is_some()).

wjones127 · 2026-07-08T23:28:09Z

        uri: &str,
        session: &Session,
    ) -> Result<Arc<Manifest>> {
+        if manifest_location.size.is_none() {


question(blocking): why gate on this? Usually manifest_location.size is present.

Thanks for your review!

When size.is_none() takes this fallback branch, it usually means the manifest file is already gone from storage (removed by auto-cleanup, or deleted by mistake), while its entry still lingers in the cache.

wjones127 · 2026-07-08T23:30:00Z

+    // A cached manifest for a version removed from storage (keyed without an
+    // e_tag) must not be served: the checkout has to fail, not return the zombie.
+    let mut zombie = dataset.manifest().clone();
+    zombie.version = 999;
+    session
+        .metadata_cache
+        .for_dataset(&dataset.uri)
+        .insert_with_key(
+            &ManifestKey {
+                version: 999,
+                e_tag: None,
+            },
+            Arc::new(zombie),
+        )
+        .await;
+
+    assert!(
+        dataset.checkout_version(999u64).await.is_err(),
+        "checkout of a version absent from storage must not be served from cache"
+    );


nitpick: this test would be a lot more convincing if you wrote a real manifest, loaded it successfully once, asserted it was in the cache, then deleted it from storage, and then checked it out again and errored.

wjones127 · 2026-07-08T23:30:56Z

+        // version-resolution head, not a manifest read.
        let io_stats = dataset.object_store.as_ref().io_stats_incremental();
-        assert_io_eq!(io_stats, read_iops, 4, "load dataset + check version");
+        assert_io_eq!(io_stats, read_iops, 3, "load dataset + check version");


praise: always nice to see these counters go down :)

🙏 Thanks

coderabbitai · 2026-07-09T07:19:51Z

📝 Walkthrough

Walkthrough

Dataset checkout logic switches from directly loading manifests to using a cache-aware accessor, with a fallback that bypasses the cache when the manifest location lacks a size. Tests are added and updated to verify cache behavior and adjust expected I/O counts accordingly.

Changes

Manifest cache-aware checkout

Layer / File(s)	Summary
Cache-aware manifest resolution `rust/lance/src/dataset.rs`	`checkout_by_ref` now resolves manifests via `get_manifest` instead of `load_manifest`, passing the resulting `Arc<Manifest>` directly; `get_manifest` adds an early-return branch loading the manifest directly when `manifest_location.size` is `None`, bypassing the metadata cache.
Cache validation and IOPS test updates `rust/lance/src/dataset/tests/dataset_io.rs`, `rust/lance/src/dataset/write/commit.rs`, `rust/lance/src/io/commit/s3_test.rs`	A new test confirms removed versions with size-less locations and stale cache entries are not served from cache; existing tests update expected read I/O counts and comments to reflect manifest data served from the session cache during checkout.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant Dataset
  participant MetadataCache
  participant Storage

  Caller->>Dataset: checkout_by_ref(ref)
  Dataset->>Dataset: get_manifest(manifest_location)
  alt manifest_location.size is None
    Dataset->>Storage: load_manifest()
    Storage-->>Dataset: Manifest
  else size known
    Dataset->>MetadataCache: lookup ManifestKey
    MetadataCache-->>Dataset: Arc<Manifest>
  end
  Dataset-->>Caller: checkout_manifest(manifest)

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: checkout now reuses the session-cached manifest instead of always reading it from storage.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

♻️ Duplicate comments (1)

rust/lance/src/dataset.rs (1)
768-772: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Add a comment explaining the guard.

This early-return bypasses the metadata cache whenever size is None. Given this is the crux of the "zombie read" fix (serving a stale cached manifest for a removed version) and was already the subject of reviewer confusion, a short comment stating why the bypass exists (avoid serving stale cache when version resolution can't confirm the manifest still exists) would prevent future re-litigation of this same question.

As per coding guidelines, "Comment fallback or guard code paths with when they trigger and why they exist."
📝 Proposed doc comment
     pub(crate) async fn get_manifest(
         object_store: &ObjectStore,
         manifest_location: &ManifestLocation,
         uri: &str,
         session: &Session,
     ) -> Result<Arc<Manifest>> {
+        // If version resolution could not confirm the manifest still exists
+        // (e.g. the version was removed by cleanup), `size` is `None`. Bypass
+        // the metadata cache in this case so a stale cached manifest for a
+        // removed version is never served; load directly and let a missing
+        // file surface as `NotFound`.
         if manifest_location.size.is_none() {
             return Ok(Arc::new(
                 Self::load_manifest(object_store, manifest_location, uri, session).await?,
             ));
         }
🤖 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.rs` around lines 768 - 772, Add a brief comment in the
manifest-loading guard inside `Dataset::load`/`load_manifest` explaining that
when `manifest_location.size` is `None` we intentionally bypass the metadata
cache to avoid serving a stale cached manifest for a removed version. Make clear
that this path exists because version resolution could not confirm the manifest
still exists, so we must reload it directly from storage. Keep the comment
adjacent to the early return so future readers understand the zombie-read
safeguard.
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.

Duplicate comments:
In `@rust/lance/src/dataset.rs`:
- Around line 768-772: Add a brief comment in the manifest-loading guard inside
`Dataset::load`/`load_manifest` explaining that when `manifest_location.size` is
`None` we intentionally bypass the metadata cache to avoid serving a stale
cached manifest for a removed version. Make clear that this path exists because
version resolution could not confirm the manifest still exists, so we must
reload it directly from storage. Keep the comment adjacent to the early return
so future readers understand the zombie-read safeguard.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: b7d81509-87d0-4f9d-aa09-3907be92ad5a

📥 Commits

Reviewing files that changed from the base of the PR and between 104ef4f and fb5248a.

📒 Files selected for processing (4)

rust/lance/src/dataset.rs
rust/lance/src/dataset/tests/dataset_io.rs
rust/lance/src/dataset/write/commit.rs
rust/lance/src/io/commit/s3_test.rs

…n caches `read_transaction_by_version` delegated to `checkout_version`, which constructs a historical Dataset and has session-cache side effects: the historical manifest is inserted into the metadata cache, manifest loading opportunistically decodes and caches the IndexSection into the index cache, and the transaction is inserted into the metadata cache. A long-lived process scanning many historical transactions therefore fills the shared Dataset and index caches with historical state it never reuses. This became more visible after lance-format#7661 made version checkout populate the session manifest cache. Read the transaction directly instead: resolve the version through the dataset's current branch and CommitHandler, decode the manifest transiently (no cache read or write, no IndexSection decode), and read the inline or external transaction. `checkout_version` caching is unchanged. Also add `read_version_transaction`, returning a compact `{ version, timestamp, transaction }` so callers that need the commit timestamp do not have to check out the dataset; `read_transaction_by_version` delegates to it. Fixes lance-format#7801.

…n caches (#7817) ## Problem `read_transaction_by_version` delegated to `checkout_version` + `read_transaction`. A caller requesting one transaction does not need a historical `Dataset`, and the checkout path has session-cache side effects: the historical manifest is inserted into the metadata cache, manifest loading opportunistically decodes and caches the `IndexSection` into the index cache, and the transaction is inserted into the metadata cache. A long-lived process scanning many historical versions fills the shared caches with entries it never reuses (more visible after #7661). Fixes #7801. ## Change - `read_version_transaction(version) -> VersionTransaction { version, timestamp, transaction }` (new): resolves the version through the dataset's current branch and `CommitHandler`, decodes the manifest transiently (no cache read or write, no `IndexSection` decode), and reads the inline or external transaction. No historical `Dataset` is constructed. The compact record carries the manifest timestamp so callers don't have to check out for it. - The resolved manifest is validated to belong to the dataset's branch (matching `checkout_by_ref`), so a branch-insensitive commit handler errors instead of returning another branch's transaction. - `read_transaction_by_version` now delegates to it; signature and error semantics unchanged (missing/cleaned-up version is still an error). - `read_transaction` (current version) is unchanged apart from factoring the storage read into a shared helper; it still checks/populates the transaction cache. - `checkout_version` caching behavior is untouched. Known trade-off: an inline transaction costs one extra ranged read vs the old path (the manifest tail is read for the timestamp/offsets, then the transaction message separately). Callers scanning history are expected to memoize per-version results; a combined read is a possible follow-up. ## Tests - `test_read_version_transaction_does_not_populate_caches` — 20-version dataset with a BTree index; reads every version via the new API on a fresh session, asserts results and timestamps equal `checkout_version(v)`, that the session's index and metadata cache entries/bytes do not grow, and that a missing version errors with `Error::NotFound` (the version resolves to a manifest path that does not exist). - `test_read_version_transaction_v1_manifest_naming` — V1-named manifests (asserts the naming premise) resolve and match a checkout. - `test_read_version_transaction_on_branch` — versions resolve against the branch chain and match a full branch checkout. - `test_inline_transaction` (existing) extended: the external-transaction-file fallback is asserted for the direct read using the manifest that test already constructs. `read_version_transaction` carries a compiling doc example. Missing-version behavior of `read_transaction_by_version` itself is already covered by the existing `test_read_transaction_properties`, which now runs through the new implementation.  ## Summary by CodeRabbit * **New Features** * Added an API to retrieve transaction details for a specific dataset version, including the UTC commit timestamp and an optional transaction payload. * **Bug Fixes** * Historical transaction reads avoid unnecessary cache/index updates when no transaction is present. * **Tests** * Added coverage for inline-versus-external transaction fallback, correct behavior without cache pollution, and accurate handling of historical manifest formats across versions and branches.

perf(dataset): reuse session-cached manifest on checkout

fa7638d

github-actions Bot added the performance label Jul 7, 2026

perf(dataset): reuse session-cached manifest on checkout

57c53d9

zhangyue19921010 added 3 commits July 7, 2026 09:31

perf(dataset): reuse session-cached manifest on checkout

925487f

perf(dataset): reuse session-cached manifest on checkout

0262022

perf(dataset): reuse session-cached manifest on checkout

9e41dda

zhangyue19921010 force-pushed the cache-manifest-on-checkout branch from c17c0da to 9e41dda Compare July 7, 2026 13:41

wjones127 self-requested a review July 8, 2026 05:42

wjones127 requested changes Jul 8, 2026

View reviewed changes

code review

fb5248a

coderabbitai Bot reviewed Jul 9, 2026

View reviewed changes

wjones127 approved these changes Jul 9, 2026

View reviewed changes

wjones127 merged commit 3a3854c into lance-format:main Jul 9, 2026
35 checks passed

Xuanwo mentioned this pull request Jul 15, 2026

read_transaction_by_version should not populate Dataset Session caches #7801

Closed

xuanyu-z mentioned this pull request Jul 15, 2026

perf(dataset): read transactions by version without populating session caches #7817

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

perf(dataset): reuse session-cached manifest on checkout#7661

perf(dataset): reuse session-cached manifest on checkout#7661
wjones127 merged 6 commits into
lance-format:mainfrom
zhangyue19921010:cache-manifest-on-checkout

zhangyue19921010 commented Jul 7, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

codecov Bot commented Jul 7, 2026

Uh oh!

zhangyue19921010 commented Jul 8, 2026

Uh oh!

wjones127 Jul 8, 2026

Uh oh!

zhangyue19921010 Jul 9, 2026

Uh oh!

wjones127 Jul 8, 2026

Uh oh!

zhangyue19921010 Jul 9, 2026

Uh oh!

wjones127 Jul 8, 2026

Uh oh!

zhangyue19921010 Jul 9, 2026

Uh oh!

coderabbitai Bot commented Jul 9, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

zhangyue19921010 commented Jul 7, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

codecov Bot commented Jul 7, 2026

Codecov Report

Uh oh!

zhangyue19921010 commented Jul 8, 2026

Uh oh!

wjones127 Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

zhangyue19921010 Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

wjones127 Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

zhangyue19921010 Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

wjones127 Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

zhangyue19921010 Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zhangyue19921010 commented Jul 7, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 9, 2026 •

edited

Loading