Clear updates/requirements after commit #1961
Conversation
| if len(self._updates) > 0: | ||
| self._table._do_commit( # pylint: disable=W0212 | ||
| updates=self._updates, | ||
| requirements=(AssertCreate(),), | ||
| ) | ||
|
|
||
| self._updates = () | ||
| self._requirements = () |
There was a problem hiding this comment.
nit: can we use super().commit_transaction here?
I want to make sure there's a way for future subclass of Transaction to also clear the updates and requirements
There was a problem hiding this comment.
https://github.com/apache/iceberg-python/pull/1961/files#r2252535367 is a reply to the comment above :p
| self._updates = () | ||
| self._requirements = () |
There was a problem hiding this comment.
We could refactor it like:
| self._updates = () | |
| self._requirements = () | |
| super().commit_transaction() |
But it doesn't actually commit, but just clear the self_requirements, which is also a bit confusing to me. For me, it makes less sense to commit a CreateTableTransaction twice in the first place. With the original version, it would just error since the table is already created.
| if len(self._updates) > 0: | ||
| self._table._do_commit( # pylint: disable=W0212 | ||
| updates=self._updates, | ||
| requirements=(AssertCreate(),), | ||
| ) | ||
|
|
||
| self._updates = () | ||
| self._requirements = () |
There was a problem hiding this comment.
ah we cant use super().commit_transaction() here because the base Transaction class' commit_transaction() method adds a AssertTableUUID(uuid=self.table_metadata.table_uuid), as a requirement.
Since requirements are checked before updates, this wont work.
👍 im fine with leaving it as is.
We just need to remember to clear _updates and _requirements for future subclasses of Transaction. And hopefully we'll follow the same pattern
There was a problem hiding this comment.
Yes, exactly. The AssertTableUUID is orthogonal to the CreateTable operation, thanks for checking 👍
# Rationale for this change Resolves apache#1946 # Are these changes tested? Yes, using a test that used to fail before :) # Are there any user-facing changes? <!-- In the case of user-facing changes, please add the changelog label. -->
Rationale for this change
Resolves #1946
Are these changes tested?
Yes, using a test that used to fail before :)
Are there any user-facing changes?