-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Python: Alter table plumbing and REST support #6323
Conversation
d4b6a5b
to
d7c795c
Compare
python/pyiceberg/catalog/rest.py
Outdated
@@ -489,6 +493,39 @@ def rename_table(self, from_identifier: Union[str, Identifier], to_identifier: U | |||
|
|||
return self.load_table(to_identifier) | |||
|
|||
def _commit(self, *table_requests: CommitTableRequest) -> CommitTableResponse: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to combine these? Why not have separate commit_table
and commit_transaction
methods? Then we don't need the check that only one request is supported. I also like that you could previously pass requirements and updates to a public method. Are you trying to restrict access to those in the "public" API for some reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, given this is called afterwards in Transaction
, it feels more nature to call it just commit_table
so we an use self._table.catalog.commit_table
instead of self._table.catalog._commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thinking of it, that makes sense. Also, the return type is different for a table and a transaction. Thanks!
Looks good to me. We can always update the method signature later since this one is internal. I'd merge, but the lock file has conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly looks good to me, +1 for merging so we can start adding implementations for the other catalogs.
python/pyiceberg/catalog/rest.py
Outdated
@@ -489,6 +493,39 @@ def rename_table(self, from_identifier: Union[str, Identifier], to_identifier: U | |||
|
|||
return self.load_table(to_identifier) | |||
|
|||
def _commit(self, *table_requests: CommitTableRequest) -> CommitTableResponse: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, given this is called afterwards in Transaction
, it feels more nature to call it just commit_table
so we an use self._table.catalog.commit_table
instead of self._table.catalog._commit
|
||
ALWAYS_TRUE = AlwaysTrue() | ||
|
||
|
||
class Transaction: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we plan to have another MiltiTableTransaction
for transactions across tables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather expect a List[Transaction]
Thank @jackye1995 for chiming in here! 🙏🏻 |
The plumbing to do an
table.alter().commit()
with actions in between. We can reuse this later on to do multi table commits: