-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Spark: Add view support to SparkSessionCatalog #11388
Spark: Add view support to SparkSessionCatalog #11388
Conversation
13f39eb
to
47ac7f9
Compare
47ac7f9
to
c89d454
Compare
e0eb166
to
1401a5f
Compare
open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java
Outdated
Show resolved
Hide resolved
open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java
Outdated
Show resolved
Hide resolved
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/BaseCatalog.java
Outdated
Show resolved
Hide resolved
1401a5f
to
2d8e5de
Compare
2d8e5de
to
37f6c7b
Compare
properties); | ||
} else if (isViewCatalog()) { | ||
try { | ||
getSessionCatalog().dropView(ident); |
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'm not sure if we want to allow this. If a catalog implementation doesn't support replace view, this could technically be a lossy operation. For example, if someone has DROP
but not CREATE
permissions, then we would drop the view and not be able to create leaving this in a bad state. I feel like it's safer to just fail and require that the user perform the drop.
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.
this was mostly following Spark's example in https://github.com/apache/spark/blob/6cd13344bffd411289ec20170ae2da2035bb2859/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/ViewCatalog.java#L124-L152 but I agree that we maybe don't want to do this non-atomically. If needed, we can always add that back later
730cf39
to
252de8b
Compare
* Spark: Add view support to SparkSessionCatalog * Don't replace view non-atomically
No description provided.