kevinjqliu commented on code in PR #1880:
URL: https://github.com/apache/iceberg-python/pull/1880#discussion_r2051592966
##########
.gitignore:
##########
@@ -50,3 +50,5 @@ htmlcov
pyiceberg/avro/decoder_fast.c
pyiceberg/avro/*.html
pyiceberg/avro/*.so
+.vscode/settings.json
+pyiceberg/table/update/expire_snapshot.md
Review Comment:
nit remove unrelated code
##########
tests/expressions/test_literals.py:
##########
@@ -393,6 +393,22 @@ def test_string_to_boolean_literal() -> None:
assert literal("FALSE").to(BooleanType()) == literal(False)
+def test_string_to_float_literal() -> None:
+ assert literal("3.141").to(FloatType()) == literal(3.141).to(FloatType())
+
+
+def test_string_to_float_outside_bound() -> None:
+ big_lit_str = literal(str(FloatType.max + 1.0e37))
+ assert big_lit_str.to(FloatType()) == FloatAboveMax()
+
+ small_lit_str = literal(str(FloatType.min - 1.0e37))
+ assert small_lit_str.to(FloatType()) == FloatBelowMin()
+
+
+def test_string_to_double_literal() -> None:
+ assert literal("3.141").to(DoubleType()) == literal(3.141)
Review Comment:
nit: remove unrelated code
##########
tests/table/test_expire_snapshots.py:
##########
@@ -0,0 +1,43 @@
+from unittest.mock import MagicMock
+from uuid import uuid4
+
+from pyiceberg.table import CommitTableResponse, Table
+
+
+def test_expire_snapshot(table_v2: Table) -> None:
+ EXPIRE_SNAPSHOT = 3051729675574597004
+ KEEP_SNAPSHOT = 3055729675574597004
+ # Mock the catalog's commit_table method
+ mock_response = CommitTableResponse(
+ # Use the table's current metadata but keep only the snapshot not to
be expired
+ metadata=table_v2.metadata.model_copy(update={"snapshots":
[KEEP_SNAPSHOT]}),
+ metadata_location="mock://metadata/location",
+ uuid=uuid4(),
+ )
+
+ # Mock the commit_table method to return the mock response
+ table_v2.catalog.commit_table = MagicMock(return_value=mock_response)
+
+ # Print snapshot IDs for debugging
+ print(f"Snapshot IDs before expiration: {[snapshot.snapshot_id for
snapshot in table_v2.metadata.snapshots]}")
+
+ # Assert fixture data to validate test assumptions
+ assert len(table_v2.metadata.snapshots) == 2
+ assert len(table_v2.metadata.snapshot_log) == 2
+ assert len(table_v2.metadata.refs) == 2
+
+ # Expire the snapshot directly without using a transaction
+ try:
+
table_v2.manage_snapshots().expire_snapshot_by_id(EXPIRE_SNAPSHOT).commit()
Review Comment:
this is great! i like this method signature
##########
pyiceberg/table/update/snapshot.py:
##########
@@ -843,3 +849,52 @@ def remove_branch(self, branch_name: str) ->
ManageSnapshots:
This for method chaining
"""
return self._remove_ref_snapshot(ref_name=branch_name)
+
+ def _commit(self) -> UpdatesAndRequirements:
+ """
+ Commit the staged updates and requirements.
+ This will remove the snapshots with the given IDs.
+
+ Returns:
+ Tuple of updates and requirements to be committed,
+ as required by the calling parent apply functions.
+ """
+ update =
RemoveSnapshotsUpdate(snapshot_ids=self._snapshot_ids_to_expire)
+ self._updates += (update,)
+ return self._updates, self._requirements
Review Comment:
i dont think we'd want to include `RemoveSnapshotsUpdate` here, this
function is used by `ManageSnapshots` which has many different updates.
##########
tests/integration/test_rest_schema.py:
##########
@@ -154,7 +154,7 @@ def test_schema_evolution_via_transaction(catalog: Catalog)
-> None:
NestedField(field_id=4, name="col_integer", field_type=IntegerType(),
required=False),
)
- with pytest.raises(CommitFailedException) as exc_info:
+ with pytest.raises(CommitFailedException, match="Requirement failed:
current schema id has changed: expected 2, found 3"):
Review Comment:
nit remove unrelated code
##########
pyiceberg/table/update/snapshot.py:
##########
@@ -843,3 +849,52 @@ def remove_branch(self, branch_name: str) ->
ManageSnapshots:
This for method chaining
"""
return self._remove_ref_snapshot(ref_name=branch_name)
+
+ def _commit(self) -> UpdatesAndRequirements:
+ """
+ Commit the staged updates and requirements.
+ This will remove the snapshots with the given IDs.
+
+ Returns:
+ Tuple of updates and requirements to be committed,
+ as required by the calling parent apply functions.
+ """
+ update =
RemoveSnapshotsUpdate(snapshot_ids=self._snapshot_ids_to_expire)
+ self._updates += (update,)
+ return self._updates, self._requirements
Review Comment:
maybe move `RemoveSnapshotsUpdate` into the individual functions below
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
