Skip to content

Preserve a ref's retention policy when its snapshot moves - #4020

Open
1fanwang wants to merge 2 commits into
apache:mainfrom
1fanwang:1fannnw/preserve-ref-retention-policy
Open

1fanwang wants to merge 2 commits into
apache:mainfrom
1fanwang:1fannnw/preserve-ref-retention-policy

Conversation

@1fanwang

@1fanwang 1fanwang commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Rationale for this change

A ref created with a retention policy (max ref age, max snapshot age and minimum snapshots to keep) loses all three settings once a branch is written to, or once main is moved with set_current_snapshot or rollback_to_snapshot. The ref is rebuilt without them, so the policy a table owner set is silently unset.

Fast-forward already carries the settings over. This change makes branch writes, set_current_snapshot and rollback_to_snapshot do the same, and a ref created without a policy still stays unset. It continues #3770 and is separate from #3760, which expires stale refs.

Are these changes tested?

Yes, with a live reproducer; the commands and observed output are under Testing Done.

Testing Done

The reproducer below uses the public API against a SQLite-backed catalog with a local warehouse. It creates a branch with a policy, writes to it three times, then rolls main back and forward again, printing the policy after each step. A branch created without a policy is the control. The same script runs twice on one checkout: first with the one changed source file taken from origin/main, then with the branch's own file.

Before, with the file from origin/main:

$ git checkout origin/main -- pyiceberg/table/update/snapshot.py
$ uv run --frozen --extra pyarrow python repro_ref_retention.py
branch created        : {'max_ref_age_ms': 86400000, 'max_snapshot_age_ms': 3600000, 'min_snapshots_to_keep': 5}
after 3 branch writes : {'max_ref_age_ms': None, 'max_snapshot_age_ms': None, 'min_snapshots_to_keep': None}
main with a policy    : {'max_ref_age_ms': 86400000, 'max_snapshot_age_ms': 3600000, 'min_snapshots_to_keep': 5}
after rollback        : {'max_ref_age_ms': None, 'max_snapshot_age_ms': None, 'min_snapshots_to_keep': None}
after set_current     : {'max_ref_age_ms': None, 'max_snapshot_age_ms': None, 'min_snapshots_to_keep': None}
branch with no policy : {'max_ref_age_ms': None, 'max_snapshot_age_ms': None, 'min_snapshots_to_keep': None}

After, with this branch's file:

$ git checkout HEAD -- pyiceberg/table/update/snapshot.py
$ uv run --frozen --extra pyarrow python repro_ref_retention.py
branch created        : {'max_ref_age_ms': 86400000, 'max_snapshot_age_ms': 3600000, 'min_snapshots_to_keep': 5}
after 3 branch writes : {'max_ref_age_ms': 86400000, 'max_snapshot_age_ms': 3600000, 'min_snapshots_to_keep': 5}
main with a policy    : {'max_ref_age_ms': 86400000, 'max_snapshot_age_ms': 3600000, 'min_snapshots_to_keep': 5}
after rollback        : {'max_ref_age_ms': 86400000, 'max_snapshot_age_ms': 3600000, 'min_snapshots_to_keep': 5}
after set_current     : {'max_ref_age_ms': 86400000, 'max_snapshot_age_ms': 3600000, 'min_snapshots_to_keep': 5}
branch with no policy : {'max_ref_age_ms': None, 'max_snapshot_age_ms': None, 'min_snapshots_to_keep': None}

Only the SQLite-backed SqlCatalog on a local filesystem was exercised.

Reproducer source: repro_ref_retention.py
from __future__ import annotations

import tempfile
from pathlib import Path

import pyarrow as pa

from pyiceberg.catalog.sql import SqlCatalog
from pyiceberg.table import Table

SCHEMA: pa.Schema = pa.schema([pa.field("id", pa.int64())])
POLICY: dict[str, int] = {"max_ref_age_ms": 86400000, "max_snapshot_age_ms": 3600000, "min_snapshots_to_keep": 5}


def retention(table: Table, ref: str) -> dict[str, int | None]:
    metadata_ref = table.metadata.refs[ref]
    return {
        "max_ref_age_ms": metadata_ref.max_ref_age_ms,
        "max_snapshot_age_ms": metadata_ref.max_snapshot_age_ms,
        "min_snapshots_to_keep": metadata_ref.min_snapshots_to_keep,
    }


def reload(catalog: SqlCatalog, name: str) -> Table:
    return catalog.load_table(identifier=name)


def main() -> None:
    with tempfile.TemporaryDirectory() as directory:
        root: Path = Path(directory)
        catalog = SqlCatalog("demo", uri=f"sqlite:///{root}/catalog.db", warehouse=f"file://{root}/warehouse")
        catalog.create_namespace(namespace="demo")

        table = catalog.create_table(identifier="demo.audit", schema=SCHEMA)
        table.append(pa.table({"id": [1]}, schema=SCHEMA))
        table = reload(catalog, "demo.audit")
        first = table.metadata.current_snapshot_id
        table.manage_snapshots().create_branch(snapshot_id=first, branch_name="audit", **POLICY).commit()
        print("branch created        :", retention(reload(catalog, "demo.audit"), "audit"))
        for value in (2, 3, 4):
            reload(catalog, "demo.audit").append(pa.table({"id": [value]}, schema=SCHEMA), branch="audit")
        print("after 3 branch writes :", retention(reload(catalog, "demo.audit"), "audit"))

        table = catalog.create_table(identifier="demo.moves", schema=SCHEMA)
        table.append(pa.table({"id": [1]}, schema=SCHEMA))
        table = reload(catalog, "demo.moves")
        first = table.metadata.current_snapshot_id
        table.append(pa.table({"id": [2]}, schema=SCHEMA))
        table = reload(catalog, "demo.moves")
        head = table.metadata.current_snapshot_id
        table.manage_snapshots().create_branch(snapshot_id=head, branch_name="main", **POLICY).commit()
        print("main with a policy    :", retention(reload(catalog, "demo.moves"), "main"))
        reload(catalog, "demo.moves").manage_snapshots().rollback_to_snapshot(snapshot_id=first).commit()
        print("after rollback        :", retention(reload(catalog, "demo.moves"), "main"))
        reload(catalog, "demo.moves").manage_snapshots().set_current_snapshot(snapshot_id=head).commit()
        print("after set_current     :", retention(reload(catalog, "demo.moves"), "main"))

        table = catalog.create_table(identifier="demo.plain", schema=SCHEMA)
        table.append(pa.table({"id": [1]}, schema=SCHEMA))
        table = reload(catalog, "demo.plain")
        table.manage_snapshots().create_branch(snapshot_id=table.metadata.current_snapshot_id, branch_name="plain").commit()
        reload(catalog, "demo.plain").append(pa.table({"id": [2]}, schema=SCHEMA), branch="plain")
        print("branch with no policy :", retention(reload(catalog, "demo.plain"), "plain"))


if __name__ == "__main__":
    main()

Are there any user-facing changes?

A ref keeps the retention policy it was created with when its snapshot moves. There is no API change.

AI Disclosure

AI disclosure: GitHub Copilot CLI (GPT-6 Astra and Claude Sonnet 5.5) helped rebase the stale PR, bring it up to date with main, rewrite this description around a live reproducer, and rerun the checks.

Branch writes and current-snapshot moves rebuilt refs without carrying their retention fields, clearing policies that create_branch had set. Preserve the existing ref's max ref age, max snapshot age, and min snapshots to keep on those move paths. Refs without retention stay unset.

Signed-off-by: 1fanwang <1fannnw@gmail.com>
Copilot AI lite review requested due to automatic review settings September 25, 2026 06:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

No unresolved issues were identified.

Review effort: Lite
Findings: None

What changed in this PR

Preserves ref retention policies when snapshots move through writes, current-snapshot changes, and rollbacks.

Changes:

  • Carries retention fields through ref updates.
  • Adds unit and integration regression coverage.
File Description
tests/​table/​test_manage_snapshots.py Adds unit coverage for retention behavior.
tests/​integration/​test_writes/​test_writes.py Adds catalog integration coverage.
pyiceberg/​table/​update/​snapshot.py Preserves retention metadata during snapshot moves.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@github-actions

Copy link
Copy Markdown

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that's incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

@github-actions github-actions Bot added the stale label Oct 26, 2026
…retention-policy

Signed-off-by: 1fanwang <1fannnw@gmail.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants