-
Notifications
You must be signed in to change notification settings - Fork 413
[View Support] Add ViewMetadata read support and 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! |
sungwy
left a comment
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?
|
@sungwy @jayceslesar please take a look when you can! |
|
addressed the comments, thanks for the review! |
|
Hi @rambleraptor the CI is failing due to a lint error. Could you run |
|
@sungwy whoops, thanks! all fixed. |
|
@sungwy @jayceslesar @Fokko can I get a review on this when you can? Thanks! |
Co-authored-by: Sung Yun <107272191+sungwy@users.noreply.github.com>
Co-authored-by: Sung Yun <107272191+sungwy@users.noreply.github.com>
Co-authored-by: Sung Yun <107272191+sungwy@users.noreply.github.com>
Co-authored-by: Sung Yun <107272191+sungwy@users.noreply.github.com>
Co-authored-by: Sung Yun <107272191+sungwy@users.noreply.github.com>
e6323fd to
1c7ff03
Compare
create_view to REST Catalogcreate_view to REST Catalog
create_view to REST Catalogcreate_view to REST Catalog
Part of #818
This adds
create_viewto 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