kevinjqliu commented on code in PR #941:
URL: https://github.com/apache/iceberg-python/pull/941#discussion_r1852553109
##########
tests/table/test_init.py:
##########
@@ -982,28 +982,43 @@ def test_assert_table_uuid(table_v2: Table) -> None:
def test_assert_ref_snapshot_id(table_v2: Table) -> None:
base_metadata = table_v2.metadata
- AssertRefSnapshotId(ref="main",
snapshot_id=base_metadata.current_snapshot_id).validate(base_metadata)
+ AssertRefSnapshotId(ref="main",
snapshot_id=base_metadata.current_snapshot_id,
ref_type=SnapshotRefType.BRANCH).validate(
+ base_metadata
+ )
with pytest.raises(CommitFailedException, match="Requirement failed:
current table metadata is missing"):
- AssertRefSnapshotId(ref="main", snapshot_id=1).validate(None)
+ AssertRefSnapshotId(ref="main", snapshot_id=1,
ref_type=SnapshotRefType.BRANCH).validate(None)
with pytest.raises(
CommitFailedException,
match="Requirement failed: branch main was created concurrently",
):
- AssertRefSnapshotId(ref="main",
snapshot_id=None).validate(base_metadata)
+ AssertRefSnapshotId(ref="main", snapshot_id=None,
ref_type=SnapshotRefType.BRANCH).validate(base_metadata)
with pytest.raises(
CommitFailedException,
match="Requirement failed: branch main has changed: expected id 1,
found 3055729675574597004",
):
- AssertRefSnapshotId(ref="main", snapshot_id=1).validate(base_metadata)
+ AssertRefSnapshotId(ref="main", snapshot_id=1,
ref_type=SnapshotRefType.BRANCH).validate(base_metadata)
+
+ with pytest.raises(
+ CommitFailedException,
+ match="Requirement failed: branch or tag not_exist_branch is missing,
expected 1",
Review Comment:
nit, we know this is referencing a branch
##########
tests/table/test_init.py:
##########
@@ -982,28 +982,43 @@ def test_assert_table_uuid(table_v2: Table) -> None:
def test_assert_ref_snapshot_id(table_v2: Table) -> None:
base_metadata = table_v2.metadata
- AssertRefSnapshotId(ref="main",
snapshot_id=base_metadata.current_snapshot_id).validate(base_metadata)
+ AssertRefSnapshotId(ref="main",
snapshot_id=base_metadata.current_snapshot_id,
ref_type=SnapshotRefType.BRANCH).validate(
+ base_metadata
+ )
with pytest.raises(CommitFailedException, match="Requirement failed:
current table metadata is missing"):
- AssertRefSnapshotId(ref="main", snapshot_id=1).validate(None)
+ AssertRefSnapshotId(ref="main", snapshot_id=1,
ref_type=SnapshotRefType.BRANCH).validate(None)
with pytest.raises(
CommitFailedException,
match="Requirement failed: branch main was created concurrently",
):
- AssertRefSnapshotId(ref="main",
snapshot_id=None).validate(base_metadata)
+ AssertRefSnapshotId(ref="main", snapshot_id=None,
ref_type=SnapshotRefType.BRANCH).validate(base_metadata)
with pytest.raises(
CommitFailedException,
match="Requirement failed: branch main has changed: expected id 1,
found 3055729675574597004",
):
- AssertRefSnapshotId(ref="main", snapshot_id=1).validate(base_metadata)
+ AssertRefSnapshotId(ref="main", snapshot_id=1,
ref_type=SnapshotRefType.BRANCH).validate(base_metadata)
+
+ with pytest.raises(
+ CommitFailedException,
+ match="Requirement failed: branch or tag not_exist_branch is missing,
expected 1",
+ ):
+ AssertRefSnapshotId(ref="not_exist_branch", snapshot_id=1,
ref_type=SnapshotRefType.BRANCH).validate(base_metadata)
+
+ with pytest.raises(
+ CommitFailedException,
+ match="Requirement failed: branch or tag not_exist_tag is missing,
expected 1",
+ ):
+ AssertRefSnapshotId(ref="not_exist_tag", snapshot_id=1,
ref_type=SnapshotRefType.TAG).validate(base_metadata)
Review Comment:
i notice is the only test here for `SnapshotRefType.TAG` and everything else
is `SnapshotRefType.BRANCH`, do we need a more comprehensive tests for tag?
##########
tests/table/test_init.py:
##########
@@ -982,28 +982,43 @@ def test_assert_table_uuid(table_v2: Table) -> None:
def test_assert_ref_snapshot_id(table_v2: Table) -> None:
base_metadata = table_v2.metadata
- AssertRefSnapshotId(ref="main",
snapshot_id=base_metadata.current_snapshot_id).validate(base_metadata)
+ AssertRefSnapshotId(ref="main",
snapshot_id=base_metadata.current_snapshot_id,
ref_type=SnapshotRefType.BRANCH).validate(
+ base_metadata
+ )
with pytest.raises(CommitFailedException, match="Requirement failed:
current table metadata is missing"):
- AssertRefSnapshotId(ref="main", snapshot_id=1).validate(None)
+ AssertRefSnapshotId(ref="main", snapshot_id=1,
ref_type=SnapshotRefType.BRANCH).validate(None)
with pytest.raises(
CommitFailedException,
match="Requirement failed: branch main was created concurrently",
):
- AssertRefSnapshotId(ref="main",
snapshot_id=None).validate(base_metadata)
+ AssertRefSnapshotId(ref="main", snapshot_id=None,
ref_type=SnapshotRefType.BRANCH).validate(base_metadata)
with pytest.raises(
CommitFailedException,
match="Requirement failed: branch main has changed: expected id 1,
found 3055729675574597004",
):
- AssertRefSnapshotId(ref="main", snapshot_id=1).validate(base_metadata)
+ AssertRefSnapshotId(ref="main", snapshot_id=1,
ref_type=SnapshotRefType.BRANCH).validate(base_metadata)
+
+ with pytest.raises(
+ CommitFailedException,
+ match="Requirement failed: branch or tag not_exist_branch is missing,
expected 1",
+ ):
+ AssertRefSnapshotId(ref="not_exist_branch", snapshot_id=1,
ref_type=SnapshotRefType.BRANCH).validate(base_metadata)
+
+ with pytest.raises(
+ CommitFailedException,
+ match="Requirement failed: branch or tag not_exist_tag is missing,
expected 1",
Review Comment:
nit, same here with tag
##########
tests/table/test_init.py:
##########
@@ -982,28 +982,43 @@ def test_assert_table_uuid(table_v2: Table) -> None:
def test_assert_ref_snapshot_id(table_v2: Table) -> None:
base_metadata = table_v2.metadata
- AssertRefSnapshotId(ref="main",
snapshot_id=base_metadata.current_snapshot_id).validate(base_metadata)
+ AssertRefSnapshotId(ref="main",
snapshot_id=base_metadata.current_snapshot_id,
ref_type=SnapshotRefType.BRANCH).validate(
+ base_metadata
+ )
with pytest.raises(CommitFailedException, match="Requirement failed:
current table metadata is missing"):
- AssertRefSnapshotId(ref="main", snapshot_id=1).validate(None)
+ AssertRefSnapshotId(ref="main", snapshot_id=1,
ref_type=SnapshotRefType.BRANCH).validate(None)
with pytest.raises(
CommitFailedException,
match="Requirement failed: branch main was created concurrently",
):
- AssertRefSnapshotId(ref="main",
snapshot_id=None).validate(base_metadata)
+ AssertRefSnapshotId(ref="main", snapshot_id=None,
ref_type=SnapshotRefType.BRANCH).validate(base_metadata)
with pytest.raises(
CommitFailedException,
match="Requirement failed: branch main has changed: expected id 1,
found 3055729675574597004",
):
- AssertRefSnapshotId(ref="main", snapshot_id=1).validate(base_metadata)
+ AssertRefSnapshotId(ref="main", snapshot_id=1,
ref_type=SnapshotRefType.BRANCH).validate(base_metadata)
+
+ with pytest.raises(
+ CommitFailedException,
+ match="Requirement failed: branch or tag not_exist_branch is missing,
expected 1",
+ ):
+ AssertRefSnapshotId(ref="not_exist_branch", snapshot_id=1,
ref_type=SnapshotRefType.BRANCH).validate(base_metadata)
+
+ with pytest.raises(
+ CommitFailedException,
+ match="Requirement failed: branch or tag not_exist_tag is missing,
expected 1",
+ ):
+ AssertRefSnapshotId(ref="not_exist_tag", snapshot_id=1,
ref_type=SnapshotRefType.TAG).validate(base_metadata)
+ # existing Tag in metadata: test
Review Comment:
nit, add an assert that test is in existing metadata
##########
pyiceberg/table/__init__.py:
##########
@@ -1003,22 +1015,27 @@ def overwrite(
overwrite_filter: ALWAYS_TRUE when you overwrite all the data,
or a boolean expression in case of a partial
overwrite
snapshot_properties: Custom properties to be added to the snapshot
summary
+ branch: Branch Reference to run the delete operation
Review Comment:
nit: this one says `delete` for `overwrite`? how about something generic
for all the function docs above?
something like:
"The branch on which to perform the operation."
##########
pyiceberg/table/update/__init__.py:
##########
@@ -609,11 +609,14 @@ class AssertRefSnapshotId(ValidatableTableRequirement):
type: Literal["assert-ref-snapshot-id"] =
Field(default="assert-ref-snapshot-id")
ref: str = Field(...)
+ ref_type: SnapshotRefType = Field(...)
Review Comment:
nit instead of adding this extra field, can we do something like the java
side?
https://github.com/apache/iceberg/blob/90be5d7360bc7ff274e7d00cb7259afbf78f223b/core/src/main/java/org/apache/iceberg/UpdateRequirement.java#L91-L127
##########
tests/integration/test_writes/test_writes.py:
##########
@@ -1570,3 +1572,158 @@ def test_abort_table_transaction_on_exception(
# Validate the transaction is aborted and no partial update is applied
assert len(tbl.scan().to_pandas()) == table_size # type: ignore
+
+
[email protected]
+def test_append_to_non_existing_branch(session_catalog: Catalog,
arrow_table_with_null: pa.Table) -> None:
+ identifier = "default.test_non_existing_branch"
+ tbl = _create_table(session_catalog, identifier, {"format-version": "2"},
[])
+ with pytest.raises(CommitFailedException, match="No snapshot available in
table for ref:"):
Review Comment:
nit: branch instead of ref
--
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]
