kevinjqliu commented on code in PR #2871:
URL: https://github.com/apache/iceberg-python/pull/2871#discussion_r2663963235
##########
pyiceberg/table/update/snapshot.py:
##########
@@ -843,6 +843,13 @@ def _commit(self) -> UpdatesAndRequirements:
"""Apply the pending changes and commit."""
return self._updates, self._requirements
+ def _commit_if_ref_updates_exist(self) -> None:
+ """Commit any pending ref updates to the transaction."""
+ if self._updates:
+ self._transaction._apply(*self._commit(),
commit_transaction_if_autocommit=False)
+ self._updates = ()
+ self._requirements = ()
Review Comment:
I think adding `commit_transaction_if_autocommit` here can be confusing
since we're toggling commit behavior through `_autocommit` but then disabling
it using `commit_transaction_if_autocommit`
I was looking up how the java side implemented this functionality,
https://github.com/apache/iceberg/blob/bc7bfa5de4743853d9647ad095322ba71e304221/core/src/main/java/org/apache/iceberg/SnapshotManager.java#L159-L172
It seems that the updates are applied to the transaction state (but not yet
to the catalog's table state, i.e. commit to the catalog). So that other
operations can reference the new transaction state.
Taking inspiration from the java abstraction, maybe a better approach can be
to create a new function in the Transaction class called `_stage`. And we can
call that to update the transaction state without updating the catalog.
--
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]
