perf(dataset): read transactions by version without populating session caches#7817
Merged
Xuanwo merged 4 commits intoJul 16, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesHistorical transaction reads
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Caller
participant Dataset
participant CommitHandler
participant Storage
Caller->>Dataset: read_version_transaction(version)
Dataset->>CommitHandler: resolve version on current branch
Dataset->>Storage: read manifest and transaction data
Storage-->>Dataset: return timestamp and optional Transaction
Dataset-->>Caller: return VersionTransaction
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
xuanyu-z
force-pushed
the
fix-read-transaction-by-version-direct
branch
from
July 15, 2026 22:43
9a1ec85 to
8803678
Compare
xuanyu-z
marked this pull request as ready for review
July 15, 2026 22:50
xuanyu-z
force-pushed
the
fix-read-transaction-by-version-direct
branch
from
July 15, 2026 22:50
8803678 to
2c17e72
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/lance/src/dataset.rs`:
- Around line 1249-1282: Document all three public APIs in
rust/lance/src/dataset.rs:1249-1282 by adding a compiling example for
read_version_transaction, linking VersionTransaction, and defining version as a
dataset version identifier; rust/lance/src/dataset.rs:233-249 by adding an
example that consumes the returned version, timestamp, and optional Transaction;
and rust/lance/src/dataset.rs:1284-1292 by adding a linked example for the
compatibility wrapper and clarifying its relationship to
read_version_transaction.
- Around line 1260-1271: In the checkout flow after resolve_version_location and
read_manifest, validate that the manifest’s branch matches the requested branch
before continuing. Reuse the existing branch-mismatch error behavior from
checkout_by_ref, rejecting differing branches while preserving successful
resolution for the current branch.
In `@rust/lance/src/dataset/tests/dataset_transactions.rs`:
- Around line 512-513: Update the read_version_transaction assertion in the
dataset transaction test to match the specific Error::VersionNotFound variant
for version 9999, and verify the resulting error message includes “9999” rather
than accepting any error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2c2e9063-59b6-445b-b071-9805df590407
📒 Files selected for processing (2)
rust/lance/src/dataset.rsrust/lance/src/dataset/tests/dataset_transactions.rs
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…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.
xuanyu-z
force-pushed
the
fix-read-transaction-by-version-direct
branch
from
July 15, 2026 23:31
2c17e72 to
c3aadbe
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/lance/src/dataset.rs`:
- Around line 1312-1321: Add a compiling Rust doc example to the public method
read_transaction_by_version, showing Dataset usage with an async
Result-returning example, awaiting version 5, and handling the optional
Transaction. Keep the existing link to read_version_transaction and ensure the
example matches the method’s actual signature.
- Around line 1221-1247: The inline transaction path in
read_transaction_from_storage reopens the manifest and performs a second read;
update the historical-read flow around read_version_transaction and
read_transaction_from_storage to reuse the manifest bytes or decoded transaction
from the initial fetch. Preserve external transaction_file handling and return
the inline transaction without reopening manifest_location.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 76a3547a-d785-422a-bbe3-c833d712c2fe
📒 Files selected for processing (2)
rust/lance/src/dataset.rsrust/lance/src/dataset/tests/dataset_transactions.rs
xuanyu-z
force-pushed
the
fix-read-transaction-by-version-direct
branch
from
July 15, 2026 23:56
c3aadbe to
5a89f71
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/lance/src/dataset.rs`:
- Around line 1265-1269: Remove the println! statements from the Rustdoc example
around read_version_transaction so it consumes the returned version metadata and
optional transaction without stdout output. Apply the same removal to the
Rustdoc example at rust/lance/src/dataset.rs lines 1325-1327, preserving the
examples’ existing API usage while avoiding debug output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2919058f-afda-4387-b77a-97f37b521384
📒 Files selected for processing (2)
rust/lance/src/dataset.rsrust/lance/src/dataset/tests/dataset_transactions.rs
- Validate the resolved manifest belongs to the dataset's branch, matching checkout_by_ref: a branch-insensitive commit handler must error rather than hand back another branch's transaction. - Assert the missing-version error variant (NotFound: the version resolves to a manifest path that does not exist) instead of a bare is_err. - Document the version parameter and add an example to read_version_transaction.
xuanyu-z
force-pushed
the
fix-read-transaction-by-version-direct
branch
from
July 16, 2026 00:05
5a89f71 to
af8018a
Compare
wjones127
self-requested a review
July 16, 2026 06:06
Xuanwo
reviewed
Jul 16, 2026
| let reader = if let Some(size) = manifest_location.size { | ||
| self.object_store | ||
| .open_with_size(&self.manifest_location.path, size as usize) | ||
| .open_with_size(&manifest_location.path, size as usize) |
Collaborator
There was a problem hiding this comment.
After read_manifest retries without a stale ManifestLocation.size, this inline transaction path still reuses the original size. A concurrent overwrite that leaves a too-small cached size can therefore decode the manifest successfully and then fail here with file size is too small, regressing the stale-size recovery contract established in #7542.
Contributor
Author
There was a problem hiding this comment.
New revision matches the contract
| pub async fn read_transaction_by_version(&self, version: u64) -> Result<Option<Transaction>> { | ||
| let dataset_version = self.checkout_version(version).await?; | ||
| dataset_version.read_transaction().await | ||
| Ok(self.read_version_transaction(version).await?.transaction) |
Collaborator
There was a problem hiding this comment.
Delegating the existing read_transaction_by_version API to this path changes a missing version from Error::DatasetNotFound to Error::NotFound, because the new path bypasses the mapping performed by load_manifest. This is an observable compatibility change for callers matching the existing public error variant, and it is unrelated to the cache-isolation goal.
Contributor
Author
There was a problem hiding this comment.
Thanks, updated to have the same error type as before
- read_version_transaction returned raw NotFound for a missing version, changing the error variant read_transaction_by_version callers see from the checkout-based path's DatasetNotFound. Map it back. - The inline transaction read trusted manifest_location.size, so a concurrent overwrite leaving that size too small decoded the manifest (which recovers) but then failed reading the transaction section. Retry the transaction read with the true size, matching the manifest reader.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/lance/src/dataset.rs`:
- Around line 1237-1247: Add regression coverage for the stale-size recovery in
the transaction-reading path containing read_message: construct a
ManifestLocation with an undersized explicit size so the first read fails with
“file size is too small,” then verify reopening the manifest without a size
succeeds and returns the inline transaction. Ensure the test exercises the retry
branch rather than only validating normal reads.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2ef2566d-1e77-4fbb-abc0-3485498af84d
📒 Files selected for processing (2)
rust/lance/src/dataset.rsrust/lance/src/dataset/tests/dataset_transactions.rs
…read Feed read_transaction_from_storage a ManifestLocation size smaller than the transaction offset so the first read fails "file size is too small", and assert the retry at the true size returns the inline transaction.
Xuanwo
approved these changes
Jul 16, 2026
wjones127
pushed a commit
that referenced
this pull request
Jul 21, 2026
…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. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## 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. <!-- end of auto-generated comment: release notes by coderabbit.ai --> (cherry picked from commit 74b2822)
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.
Problem
read_transaction_by_versiondelegated tocheckout_version+read_transaction. A caller requesting one transaction does not need a historicalDataset, and the checkout path has session-cache side effects: the historical manifest is inserted into the metadata cache, manifest loading opportunistically decodes and caches theIndexSectioninto 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 andCommitHandler, decodes the manifest transiently (no cache read or write, noIndexSectiondecode), and reads the inline or external transaction. No historicalDatasetis constructed. The compact record carries the manifest timestamp so callers don't have to check out for it.checkout_by_ref), so a branch-insensitive commit handler errors instead of returning another branch's transaction.read_transaction_by_versionnow 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_versioncaching 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 equalcheckout_version(v), that the session's index and metadata cache entries/bytes do not grow, and that a missing version errors withError::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_transactioncarries a compiling doc example. Missing-version behavior ofread_transaction_by_versionitself is already covered by the existingtest_read_transaction_properties, which now runs through the new implementation.Summary by CodeRabbit
New Features
Bug Fixes
Tests