ebyhr commented on code in PR #4319:
URL: https://github.com/apache/iceberg-python/pull/4319#discussion_r4520888839
##########
pyiceberg/table/update/snapshot.py:
##########
@@ -1131,7 +1136,15 @@ def remove_tag(self, tag_name: str) -> ManageSnapshots:
tag_name (str): name of tag to remove
Returns:
This for method chaining
+
+ Raises:
+ ValueError: If the tag does not exist, or the name belongs to a
branch.
"""
+ ref = self._effective_refs().get(tag_name)
+ if ref is None:
+ raise ValueError(f"Tag does not exist: {tag_name}")
Review Comment:
The rest of this class raises domain exceptions for ref problems.
`fast_forward_branch` uses `NoSuchSnapshotRefError` and `SnapshotRefTypeError`
for the same conditions. Could we do the same here (and in `remove_branch`)?
- `Tag does not exist: x` / `Branch does not exist: x` ->
`NoSuchSnapshotRefError`
- `Ref x is a branch not a tag` / `Ref x is a tag not a branch` ->
`SnapshotRefTypeError`
This follows the project convention to use domain-specific exceptions for
Iceberg-specific conditions. If you'd rather keep `ValueError`, please say so
in the PR description, since the raised type is part of the user-facing change.
##########
tests/table/test_manage_snapshots.py:
##########
@@ -82,6 +82,62 @@ def table_v2_main_behind(example_table_metadata_v2:
dict[str, Any]) -> Table:
)
+def test_create_ref_rejects_existing_name(table_v2: Table) -> None:
+ table_v2.catalog = MagicMock()
+
+ # "test" is a tag in the fixture; neither kind may take an existing name.
+ with pytest.raises(ValueError, match="Ref test already exists"):
+
table_v2.manage_snapshots().create_branch(snapshot_id=CHILD_SNAPSHOT_ID,
branch_name="test")
+ with pytest.raises(ValueError, match="Ref test already exists"):
+ table_v2.manage_snapshots().create_tag(snapshot_id=CHILD_SNAPSHOT_ID,
tag_name="test")
+
+ table_v2.catalog.commit_table.assert_not_called()
+ assert table_v2.metadata.refs["test"].snapshot_id == PARENT_SNAPSHOT_ID
+
+
+def test_create_ref_rejects_name_staged_earlier_in_chain(table_v2: Table) ->
None:
+ table_v2.catalog = MagicMock()
+ ms =
table_v2.manage_snapshots().create_branch(snapshot_id=PARENT_SNAPSHOT_ID,
branch_name="dev")
+
+ with pytest.raises(ValueError, match="Ref dev already exists"):
+ ms.create_branch(snapshot_id=CHILD_SNAPSHOT_ID, branch_name="dev")
+ with pytest.raises(ValueError, match="Ref dev already exists"):
+ ms.create_tag(snapshot_id=CHILD_SNAPSHOT_ID, tag_name="dev")
+
+ table_v2.catalog.commit_table.assert_not_called()
+
+
+def test_remove_ref_rejects_main_missing_and_wrong_kind(table_v2_main_behind:
Table) -> None:
+ table_v2_main_behind.catalog = MagicMock()
+ ms = table_v2_main_behind.manage_snapshots()
+
+ for method, name, message in [
+ (ms.remove_branch, "main", "Cannot remove main branch"),
+ (ms.remove_branch, "missing", "Branch does not exist: missing"),
+ (ms.remove_branch, "test", "Ref test is a tag not a branch"),
+ (ms.remove_tag, "missing", "Tag does not exist: missing"),
+ (ms.remove_tag, "audit", "Ref audit is a branch not a tag"),
+ ]:
Review Comment:
nit: The loop over (`bound_method`, `name`, `message`) tuples is hard to
read, and a failure won't say which case broke. Plain consecutive
`pytest.raises` blocks, one per case, would be clearer. Five short blocks cost
little.
##########
tests/table/test_manage_snapshots.py:
##########
@@ -82,6 +82,62 @@ def table_v2_main_behind(example_table_metadata_v2:
dict[str, Any]) -> Table:
)
+def test_create_ref_rejects_existing_name(table_v2: Table) -> None:
+ table_v2.catalog = MagicMock()
+
+ # "test" is a tag in the fixture; neither kind may take an existing name.
+ with pytest.raises(ValueError, match="Ref test already exists"):
+
table_v2.manage_snapshots().create_branch(snapshot_id=CHILD_SNAPSHOT_ID,
branch_name="test")
+ with pytest.raises(ValueError, match="Ref test already exists"):
+ table_v2.manage_snapshots().create_tag(snapshot_id=CHILD_SNAPSHOT_ID,
tag_name="test")
+
+ table_v2.catalog.commit_table.assert_not_called()
+ assert table_v2.metadata.refs["test"].snapshot_id == PARENT_SNAPSHOT_ID
+
+
+def test_create_ref_rejects_name_staged_earlier_in_chain(table_v2: Table) ->
None:
+ table_v2.catalog = MagicMock()
+ ms =
table_v2.manage_snapshots().create_branch(snapshot_id=PARENT_SNAPSHOT_ID,
branch_name="dev")
+
+ with pytest.raises(ValueError, match="Ref dev already exists"):
+ ms.create_branch(snapshot_id=CHILD_SNAPSHOT_ID, branch_name="dev")
+ with pytest.raises(ValueError, match="Ref dev already exists"):
+ ms.create_tag(snapshot_id=CHILD_SNAPSHOT_ID, tag_name="dev")
+
+ table_v2.catalog.commit_table.assert_not_called()
+
+
+def test_remove_ref_rejects_main_missing_and_wrong_kind(table_v2_main_behind:
Table) -> None:
+ table_v2_main_behind.catalog = MagicMock()
+ ms = table_v2_main_behind.manage_snapshots()
+
+ for method, name, message in [
+ (ms.remove_branch, "main", "Cannot remove main branch"),
+ (ms.remove_branch, "missing", "Branch does not exist: missing"),
+ (ms.remove_branch, "test", "Ref test is a tag not a branch"),
+ (ms.remove_tag, "missing", "Tag does not exist: missing"),
+ (ms.remove_tag, "audit", "Ref audit is a branch not a tag"),
+ ]:
+ with pytest.raises(ValueError, match=message):
+ method(name)
+
+ table_v2_main_behind.catalog.commit_table.assert_not_called()
+
+
+def test_remove_then_create_same_name_in_one_chain(table_v2_main_behind:
Table) -> None:
Review Comment:
Could we add a case where the ref kind changes within a chain, e.g.
`remove_tag("test").create_branch(..., "test")`? It would confirm
`_effective_refs` handles the type switch, not just same-kind replacement.
--
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]
