feat: add ManageSnapshots.fast_forward_branch - #3649
Conversation
test: fast_forward_branch auto-creates missing from_branch test: fast_forward_branch is a no-op when snapshots already equal test: fast_forward_branch rejects a tag as the source ref test: fast_forward_branch rejects missing to_ref test: fast_forward_branch rejects non-ancestor target test: fast_forward_branch preserves retention fields test: fast_forward_branch composes in a manage_snapshots chain test(integration): fast_forward_branch on real catalogs docs: add fast_forward_branch section with WAP example chore: apply ruff-format and add None-checks for mypy
|
I'm personally in favor of making those changes. I understand how a follow-up PR would be cleaner, but I worry about a situation where we merge this in + don't get around to the follow-up. I'm happy to do a review as-is. Which works better for you? |
Hi @rambleraptor I'm still getting familiar with the inner workings of this library so some guidance would help. Do you know if it's safe for me to construct the refs to point to in fast_forward by inspecting |
|
There's almost certainly a lot of edge cases, but we'll probably have to discover them as we go. I'll do a review and we can go from there. |
|
Sounds good. I'll try come up with something and then see where it leads us. 😄 |
|
Hi @rambleraptor, I also added an explicit integration test to validate everything works as expected. Let me know if this solution looks ok and Ill proceed to implement some explicit tests for the |
|
Hi @rambleraptor, |
| # Create a lagging branch at the parent snapshot, then reset the mock so | ||
| # the next commit's updates are the fast-forward alone. | ||
| table_v2.catalog = MagicMock() | ||
| table_v2.catalog.commit_table.return_value = _mock_commit_response(table_v2) |
There was a problem hiding this comment.
You should inject in the ref directly here like you do in other tests (preserve_retention_fields)
table_v2.metadata.refs["lagging"] = SnapshotRef(
snapshot_id=parent_snapshot_id,
snapshot_ref_type="branch",
)
|
I did a quick pass on this and it broadly looks correct! I'll take another look over the docs in particular on Monday. I want to make sure that our documentation is succinct and tells the appropriate amount of detail. |
|
|
||
| Fast-forward `from_branch` to point at the snapshot referenced by | ||
| `to_ref`. `to_ref` may be a branch or tag; `from_branch` must be a | ||
| branch. If `from_branch` does not yet exist it is created pointing at |
There was a problem hiding this comment.
Worth adding that an auto-created from_branch gets default retention properties.
| assert main_update.snapshot_id == snapshot_one | ||
|
|
||
|
|
||
| def test_fast_forward_branch_advances_to_descendant(table_v2: Table) -> None: |
There was a problem hiding this comment.
Some test coverage gaps:
- A successful fast-forward where to_ref is a tag
- A no-op
2 participants
Rationale for this change
Adds
ManageSnapshots.fast_forward_branch(from_branch, to_ref)to advance abranch to the snapshot referenced by another branch or tag, provided the
current snapshot of
from_branchis an ancestor ofto_ref's snapshot.The method almost matches the behavior of Apache Iceberg's Java reference
implementation (
SnapshotManager.fastForwardBranch/UpdateSnapshotReferencesOperation.replaceBranch(..., fastForward=true))and, by extension, the Spark
system.fast_forwardstored procedure thatdepends on it.
However, while the implementation is very close there is one exception;
For the sake of scope I left out the changes needed to ensure chain sematic parity between the java and python.
Would the maintainers like a followup PR or have it included in this PR? My only worry is that it will increase the complexity of an otherwise straightforward PR.
Are these changes tested?
Yes. Both unit tests and integration tests were writen
Are there any user-facing changes?
Yes.
fast_forward_branch.See
mkdocs/docs/api.mdfor more details.Related Issues / Pull Requests
LLM Disclosure
PR was written with the assistance of Claude Opus 4.7 (1M context) (effort: high)
It implemented the majority of the tests I defined. Specifically to assist with what fixtures to use and how best to write the unit tests in an atomic fashion appropriate for pyIceberg.