1fanwang opened a new pull request, #3770:
URL: https://github.com/apache/iceberg-python/pull/3770
# Rationale for this change
`create_branch()` accepts `max_ref_age_ms`, `max_snapshot_age_ms` and
`min_snapshots_to_keep`, and the next write to that branch drops all three.
```python
tbl.manage_snapshots().create_branch(
snapshot_id=tbl.metadata.current_snapshot_id,
branch_name="audit",
max_ref_age_ms=86400000,
).commit()
tbl.metadata.refs["audit"].max_ref_age_ms # 86400000
tbl.append(rows, branch="audit")
tbl.metadata.refs["audit"].max_ref_age_ms # None
```
The branch loses its retention policy at the moment the policy starts to
matter — a branch is created to hold a staged write, and the staged write is
what erases it.
The cause is in `_SnapshotProducer._commit()`: it emits a
`SetSnapshotRefUpdate` without the retention fields, so `_apply_table_update`
rebuilds the ref from that update alone and the previous values are gone.
`SetSnapshotRefUpdate` already carries all three fields; nothing was populating
them.
Java preserves them. `TableMetadata.Builder.setBranchSnapshotInternal`
rebuilds the moved ref with `SnapshotRef.builderFrom(ref,
replacementSnapshotId)`, which copies the existing retention config onto the
new ref. This does the same, reading the existing ref when there is one and
leaving the fields unset otherwise, so a branch with no policy does not acquire
one.
[#3649](https://github.com/apache/iceberg-python/pull/3649) fixes the same
class of bug on the fast-forward path, carrying retention across a
`fast_forward_branch`. It does not touch the write path, so the two are
complementary.
Scoped to the write path deliberately. `Transaction._set_ref_snapshot` has
the same defaulted parameters and is reached by `set_current_snapshot` and
`rollback_to_snapshot`; those move `main` rather than a configured branch, so I
left them alone rather than widening the diff.
# Are these changes tested?
**Integration**, against the REST catalog from
`dev/docker-compose-integration.yml`: create a branch with all three retention
fields, write to it twice, assert the policy survives and that `main` gains
nothing.
<details><summary>Red, with <code>pyiceberg/table/update/snapshot.py</code>
at upstream/main</summary>
```
$ git checkout upstream/main -- pyiceberg/table/update/snapshot.py
$ pytest tests/integration/test_writes/test_writes.py -m integration -k
retention -q
> assert ref.max_ref_age_ms == 86400000
E assert None == 86400000
tests/integration/test_writes/test_writes.py:2258: AssertionError
FAILED
tests/integration/test_writes/test_writes.py::test_write_to_branch_preserves_retention
```
</details>
<details><summary>Green, with the change restored</summary>
```
$ pytest tests/integration/test_writes/test_writes.py -m integration -k
retention -q
1 passed, 123 deselected
$ pytest tests/integration/test_writes/test_writes.py -m integration -k
branch -q
8 passed, 116 deselected
```
</details>
**Unit**, across the `memory`, `sql`, and `sql_without_rowcount` catalogs: a
configured branch keeps all three fields across three successive writes, and a
branch created without a policy still reports `None` afterwards. The second
case passes either way and is there to pin that this does not invent defaults.
Whole unit and integration suites pass; `prek run -a` clean.
# Are there any user-facing changes?
A branch keeps the retention policy it was created with. No API change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
