-
Notifications
You must be signed in to change notification settings - Fork 305
Add drop_view to the rest catalog #820
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
Conversation
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.
Hi @ndrluis - thank you for working on this. I like how you separated the NoSuchTable, NoSuchIdentifier and NoSuchViewErrors here. I think there's conflict resolution to handle, but otherwise looks good to merge
11a5fd4
to
6db3355
Compare
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.
Hi @ndrluis - sorry for the delay in reviewing this PR. It fell through my radar :(
I left a few more nits, hope you find those comments helpful!
pyiceberg/catalog/rest.py
Outdated
return {"namespace": NAMESPACE_SEPARATOR.join(identifier.namespace.root[1:]), "table": identifier.name} | ||
else: | ||
return {"namespace": NAMESPACE_SEPARATOR.join(identifier.namespace.root), "table": identifier.name} |
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.
The view identifier is also of TableIdentifier type, so I think this would be safer, and future proof (for when we support loading views):
return {"namespace": NAMESPACE_SEPARATOR.join(identifier.namespace.root[1:]), "table": identifier.name} | |
else: | |
return {"namespace": NAMESPACE_SEPARATOR.join(identifier.namespace.root), "table": identifier.name} | |
return {"namespace": NAMESPACE_SEPARATOR.join(identifier.namespace.root[1:]), kind: identifier.name} | |
else: | |
return {"namespace": NAMESPACE_SEPARATOR.join(identifier.namespace.root), kind: identifier.name} |
pyiceberg/catalog/rest.py
Outdated
return identifier_tuple | ||
|
||
def _split_identifier_for_path(self, identifier: Union[str, Identifier, TableIdentifier]) -> Properties: | ||
def _split_identifier_for_path(self, identifier: Union[str, Identifier, TableIdentifier], kind: str = "table") -> Properties: |
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.
nit: Should we introduce an enum instead of free form strings for the table kind? (table, view, there may be more kinds in the future like materialized views)
@sungwy, thank you for your review. I will work on this tomorrow! |
This change allows for the validation of both Tables and Views.
6db3355
to
6a07478
Compare
@sungwy Done! |
LGTM! Thank you for adding this new API @ndrluis |
* Use NoSuchIdentifier instead of NoTableIdentifier This change allows for the validation of both Tables and Views. * Add drop_view to the rest catalog * fixup! Add drop_view to the rest catalog
* Use NoSuchIdentifier instead of NoTableIdentifier This change allows for the validation of both Tables and Views. * Add drop_view to the rest catalog * fixup! Add drop_view to the rest catalog
No description provided.