Add ManageSnapshots.fast_forward_branch - #3900
Conversation
Fast-forward a branch to the snapshot of another ref, following Java's ManageSnapshots.fastForwardBranch: the ref to fast-forward to can be a branch or a tag, a missing branch is created with default retention, and a branch that is not an ancestor raises ValueError. Operations chained in one manage_snapshots() see each other, and a ref set twice in a chain results in a single set-snapshot-ref update. rollback_to_snapshot and rollback_to_timestamp stage pending ref updates before validating, like commitIfRefUpdatesExist in Java. Supersedes apache#3649.
SnapshotRef requires min-snapshots-to-keep, max-snapshot-age-ms and max-ref-age-ms to be greater than 0, but SetSnapshotRefUpdate did not, so an invalid value only failed when the ref was built at apply time. Java validates these in SnapshotRef.Builder when they are set.
rambleraptor
left a comment
There was a problem hiding this comment.
We already went through reviews, so this looks good to me. Thanks for doing this!
| self._updates = () | ||
| self._requirements = () | ||
|
|
||
| def _pending_table_metadata(self) -> TableMetadata: |
There was a problem hiding this comment.
I'm tempted to also call this table_metadata, similar to Transaction:
iceberg-python/pyiceberg/table/__init__.py
Lines 259 to 261 in 9299bdb
| Raises: | ||
| ValueError: If the snapshot does not exist or is not an ancestor of the current table state. | ||
| """ | ||
| self._commit_if_ref_updates_exist() |
There was a problem hiding this comment.
This is outside of the scope of this PR, but I was a bit surprised by this. Based on the method name, I expected it to perform an actual commit, but after reading the code, it turns out it stages the commit within the transaction.
|
Thanks a lot for the review @Fokko ! Before I push anything: @JeroenSchmidt pushed the pending changes to #3649 this morning, so the original PR is alive again. I had opened this one because his had been untouched since August, but he is back on it, so I am happy to let him finish and close this once #3649 lands. That said, it is your call which implementation fits best. Since the two diverged, here is what actually differs. Your comments here are on code that only exists in this PR: #3649 reads the pending refs through a hand-rolled Only in this PR:
Only in #3649: three new public exception types ( |
|
Thanks for your work on this @Ferdinanddb I'm slightly inclined to use #3649 since it's approved. If we can't get the CI passing on that PR soon, let's move forward with yours. |
Adds
ManageSnapshots.fast_forward_branch, following Java'sManageSnapshots.fastForwardBranchand Spark'ssystem.fast_forwardprocedure.ValueErrormanage_snapshots()see each other, and a ref set twice in a chain results in a singleset-snapshot-refupdateSupersedes #3649 by @JeroenSchmidt, which has been conflicting since August. It keeps the same API and intra-chain behaviour and addresses the open review comments from @rambleraptor (refs injected directly in the tests, default retention of a created branch documented, tag target and no-op covered, shorter docs). Differences: plain
ValueErrorinstead of new exception classes, the pending refs come fromupdate_table_metadatainstead of a separate overlay, and chained updates on the same ref are collapsed.The second commit is separable: it adds the
gt=0bounds thatSnapshotRefalready has toSetSnapshotRefUpdate, so an invalid retention value fails when it is passed instead of at apply time, likeSnapshotRef.Builderin Java. Happy to split it out if preferred.