refactor!: rearchitect CommitResult
#1343
Merged
Conversation
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
github-actions
bot
added
the
breaking-change
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1343 +/- ##
==========================================
- Coverage 84.90% 84.87% -0.03%
==========================================
Files 114 114
Lines 28935 28971 +36
Branches 28935 28971 +36
==========================================
+ Hits 24566 24588 +22
- Misses 3200 3213 +13
- Partials 1169 1170 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zachschuermann
force-pushed
the
txn-commit-stuff
branch
from
98d75fd to
96d8dcc
Compare
zachschuermann
force-pushed
the
txn-commit-stuff
branch
from
96d8dcc to
ee6ff2b
Compare
nicklan
approved these changes
Sep 25, 2025
kernel/src/transaction/mod.rs
Outdated
|
|
||
| /// Compute a new snapshot for the table at the commit version. Note this is generally more | ||
| /// efficient than creating a new snapshot from scratch. | ||
| pub fn post_commit_snapshot(&self, engine: &dyn Engine) -> DeltaResult<SnapshotRef> { |
There was a problem hiding this comment.
If we know what we just commited, can we construct the snapshot without having to do any work at all?
Not for this PR, but should we have a follow-up?
There was a problem hiding this comment.
actually realized this was more complex. removed and tracking in #916
zachschuermann
changed the title
CommitResult, add post-commit-snapshotCommitResult
zachschuermann
force-pushed
the
txn-commit-stuff
branch
from
5794208 to
84e877a
Compare
zachschuermann
commented
Oct 6, 2025
nicklan
reviewed
Oct 8, 2025
kernel/src/transaction/mod.rs
Outdated
| self.into_conflicted(commit_version), | ||
| )), | ||
| // TODO: we may want to be more selective about what is retryable | ||
| Err(e) => Ok(CommitResult::RetryableTransaction(self.into_retryable(e))), |
There was a problem hiding this comment.
should we not ensure at least that this is an IO error of some sort?
There was a problem hiding this comment.
yea i went back-and-forth on this. I expanded the comment and went ahead and made it only do "retryable" on IOError. I think the main item here is to more clearly define what write_json_files can return in error cases. We likely don't want the entirety of Error struct there. opened #1388
| )), | ||
| // TODO: we may want to be more or less selective about what is retryable (this is tied | ||
| // to the idea of "what kind of Errors should write_json_file return?") | ||
| Err(e @ Error::IOError(_)) => { |
There was a problem hiding this comment.
woah never seen this syntax!
OussamaSaoudi
approved these changes
Oct 9, 2025
samansmink
pushed a commit
to samansmink/delta-kernel-rs
that referenced
this pull request
Oct 19, 2025
## What changes are proposed in this pull request?
Our current `Transaction::commit(self, engine)` returns a `CommitResult`
but it doesn't retain enough information for a post-commit Snapshot (and
feels a little clunky)
This PR refactors to a new pattern in which `commit` returns a new
`CommitResult` with one of: committed/conflicted/retryable transaction -
each just holds a transaction and a little more metadata instead of the
old Committed/Conflicted pattern.
```rust
// New
pub enum CommitResult {
CommittedTransaction(CommittedTransaction),
ConflictedTransaction(ConflictedTransaction),
RetryableTransaction(RetryableTransaction),
}
// Old
pub enum CommitResult {
Committed {
version: Version,
post_commit_stats: PostCommitStats,
},
Conflict(Transaction, Version),
}
```
This opens the door for a post-commit-snapshot. it requires a bit more
thought/testing but something like:
```rust
pub fn post_commit_snapshot(&self, engine: &dyn Engine) -> DeltaResult<SnapshotRef> {
Snapshot::builder_from(self.transaction.read_snapshot.clone())
.with_log_tail(new_path)
.at_version(self.commit_version)
.build(engine)
}
```
### This PR affects the following public APIs
`CommitResult` API change, new enum + structs for
`CommittedTransaction`, `ConflictedTransaction`, and
`RetryableTransaction`
## How was this change tested?
refactor
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
What changes are proposed in this pull request?
Our current
Transaction::commit(self, engine)returns aCommitResultbut it doesn't retain enough information for a post-commit Snapshot (and feels a little clunky)This PR refactors to a new pattern in which
commitreturns a newCommitResultwith one of: committed/conflicted/retryable transaction - each just holds a transaction and a little more metadata instead of the old Committed/Conflicted pattern.This opens the door for a post-commit-snapshot. it requires a bit more thought/testing but something like:
This PR affects the following public APIs
CommitResultAPI change, new enum + structs forCommittedTransaction,ConflictedTransaction, andRetryableTransactionHow was this change tested?
refactor