HonahX commented on code in PR #1246:
URL: https://github.com/apache/iceberg-python/pull/1246#discussion_r1817956072
##########
pyiceberg/table/__init__.py:
##########
@@ -231,9 +233,13 @@ def __enter__(self) -> Transaction:
"""Start a transaction to update the table."""
return self
- def __exit__(self, _: Any, value: Any, traceback: Any) -> None:
- """Close and commit the transaction."""
- self.commit_transaction()
+ def __exit__(
+ self, exctype: Optional[Type[BaseException]], excinst:
Optional[BaseException], exctb: Optional[TracebackType]
+ ) -> None:
+ """Close and commit the transaction, or handle exceptions."""
+ # Only commit the full transaction, if there is no exception in all
updates on the chain
+ if exctb is None:
Review Comment:
Since [python
doc](https://docs.python.org/3/reference/datamodel.html#object.__exit__)
mention that
> If the context was exited without an exception, all three arguments will
be [None](https://docs.python.org/3/library/constants.html#None).
Shall we check all three parameter is `None` here?
##########
tests/catalog/test_base.py:
##########
@@ -265,6 +268,42 @@ def drop_view(self, identifier: Union[str, Identifier]) ->
None:
raise NotImplementedError
+class TransactionThrowExceptionInOverwrite(Transaction):
+ def __init__(self, table: Table):
+ super().__init__(table)
+
+ # Override the default overwrite to simulate exception during append
+ def overwrite(
+ self,
+ df: pa.Table,
+ overwrite_filter: Union[BooleanExpression, str] = ALWAYS_TRUE,
+ snapshot_properties: Dict[str, str] = EMPTY_DICT,
+ ) -> None:
+ self.delete(delete_filter=overwrite_filter,
snapshot_properties=snapshot_properties)
+ raise Exception("Fail Append Commit Exception")
+
+
+class TableThrowExceptionInOverwrite(Table):
+ def __init__(self, identifier: Identifier, metadata: TableMetadata,
metadata_location: str, io: FileIO, catalog: Catalog):
+ # Call the constructor of the parent class
+ super().__init__(identifier, metadata, metadata_location, io, catalog)
+
+ def transaction(self) -> Transaction:
+ return TransactionThrowExceptionInOverwrite(self)
+
+
+def given_catalog_has_a_table_throw_exception_in_overwrite(
Review Comment:
How about testing this scenario by explicitly triggering errors during the
transaction?
like
```python
with pytest.raises(ValueError):
with tbl.transaction() as txn:
txn.overwrite(...)
raise ValueError
txn.append(...)
assert len(tbl.scan().to_pandas()) == 0
```
I think this is simpler and makes it more explicit that the problem is not
only with overwrites but also with any multi-update transactions.
--
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
