deepdave98 commented on code in PR #4319:
URL: https://github.com/apache/iceberg-python/pull/4319#discussion_r4530027139
##########
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:
Done. `remove_branch` and `remove_tag` now raise `NoSuchSnapshotRefError`
and `SnapshotRefTypeError`, with the same message shape as
`fast_forward_branch`. `create_branch`, `create_tag`, and the `main` check keep
`ValueError` as invalid arguments; I added that to the description.
##########
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:
Added `test_remove_then_create_same_name_as_other_kind_in_one_chain`:
`remove_tag("test").create_branch(..., "test")` commits a
`RemoveSnapshotRefUpdate` followed by a `SetSnapshotRefUpdate` of type branch.
##########
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:
Replaced the loop with five `pytest.raises` blocks, each asserting the
specific exception.
--
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]
