-
Notifications
You must be signed in to change notification settings - Fork 319
Add create_view
to REST Catalog
#2154
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
base: main
Are you sure you want to change the base?
Conversation
Is it worth setting integration tests in rest integration tests? |
I'm a little unclear what Catalog the REST integration tests run against, so I can't say if there's been enough implemented to actually have a valid REST Catalog test. The integration tests are missing most of the methods that have been implemented, so my reaction is that we're still missing pieces for an integration test. |
@Fokko @kevinjqliu mind taking a look when you can? thanks! |
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.
Thanks for taking this up @rambleraptor
I've taken a stab at reviewing the PR. I think adding an integration test is a must-have for introducing a new API. I think a great way to verify that the created view works as expected is to add a view as a spark
dialect sql view and read the view table through a spark session, similar to how we validate that PyIceberg writes are reflected correctly even when the table is read through a Spark session accessing the same REST Catalog (example). WDYT?
Co-authored-by: Sung Yun <[email protected]>
Co-authored-by: Sung Yun <[email protected]>
Co-authored-by: Sung Yun <[email protected]>
Co-authored-by: Sung Yun <[email protected]>
Co-authored-by: Sung Yun <[email protected]>
@sungwy @jayceslesar please take a look when you can! |
addressed the comments, thanks for the review! |
Part of #818
This adds
create_view
to the REST Catalog. As part of this, it also defines the View and ViewMetadata class.Rationale for this change
PyIceberg's REST Catalog doesn't support
create_view
. Furthermore, PyIceberg doesn't support views at all. This is the first part in getting views supported.Are these changes tested?
Unit tests included
Are there any user-facing changes?
Added Iceberg REST Catalog support for
create_view