[SPARK-45807][SQL] Add createOrReplaceView(..) / replaceView(..) to ViewCatalog #43677
Conversation
nastra
changed the title
|
@aokolnychyi could you also take a look at this please? |
|
Will review today. |
| String[] columnAliases, | ||
| String[] columnComments, | ||
| Map<String, String> properties) throws NoSuchNamespaceException { | ||
| if (!viewExists(ident)) { |
There was a problem hiding this comment.
Minor: I think it is easier to read conditions that don't have negation. I'd flip the branches.
|
thanks for reviewing @aokolnychyi, I've applied your suggestion |
HyukjinKwon
changed the title
|
@holdenk Could you merge this PR if you think it is ready? |
|
Sure let me merge on Monday assuming no objections from folks (taking a weekend vacation with mostly books instead of computers). |
| String[] queryColumnNames, | ||
| String[] columnAliases, | ||
| String[] columnComments, | ||
| Map<String, String> properties) throws NoSuchViewException, NoSuchNamespaceException { |
There was a problem hiding this comment.
shall we have a class ViewInfo to wrap these fields? It looks verbose to make so many parameters in 3 methods.
| * @param properties the view properties | ||
| * @throws NoSuchNamespaceException If the identifier namespace does not exist (optional) | ||
| */ | ||
| default void createOrReplaceView( |
There was a problem hiding this comment.
3 APIs might be too much. How about we follow the create/replace table logical plan? We have a CreateTable with a bool field ignoreIfExists, and a ReplaceTable with a bool field orCreate.
So we can have just two methods here
def createView(... , ignoreIfExists: Boolean)
def replaceView(..., orCreate: Boolean)
There was a problem hiding this comment.
thanks for the feedback @cloud-fan. I'll follow up with your suggestions and will ping you for a review
There was a problem hiding this comment.
@cloud-fan I have made the suggested improvements in #44330 but I'm not sure about having ignoreIfExists on the API when creating a view.
I checked and the TableCatalog also doesn't have such functionality and I wonder if it wouldn't be better to have ignoreIfExists be part of the physical plan that creates the view (similar to how it's done in https://github.com/apache/spark/pull/44197/files#diff-d491b3b4f1ac890067f57f4c30cbdecf3585ea91dabf0dced59b31a4e1643462R83)
There was a problem hiding this comment.
Makes sense. A simpler API is better. The caller side (Spark) can just ignore the error if ignoreIfExists is true.
…iewCatalog ### What changes were proposed in this pull request? ViewCatalog API improvements described in [SPIP](https://docs.google.com/document/d/1XOxFtloiMuW24iqJ-zJnDzHl2KMxipTjJoxleJFz66A/edit?usp=sharing) that didn't make it into the codebase as part of apache#37556 ### Why are the changes needed? Required for DataSourceV2 view support. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? N/A ### Was this patch authored or co-authored using generative AI tooling? N/A Closes apache#43677 from nastra/SPARK-45807. Authored-by: Eduard Tudenhoefner <etudenhoefner@gmail.com> Signed-off-by: Holden Karau <holden@pigscanfly.ca>
### What changes were proposed in this pull request? Follow-up API improvements based on comments from #43677 ### Why are the changes needed? Required for DataSourceV2 view support. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? N/A ### Was this patch authored or co-authored using generative AI tooling? N/A Closes #44330 from nastra/SPARK-45807-api-improvements. Authored-by: Eduard Tudenhoefner <etudenhoefner@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>
### What changes were proposed in this pull request? Follow-up API improvements based on from #43677 ### Why are the changes needed? Required for DataSourceV2 view support. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? N/A ### Was this patch authored or co-authored using generative AI tooling? N/A Closes #44970 from nastra/SPARK-45807-return-type. Authored-by: Eduard Tudenhoefner <etudenhoefner@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>
What changes were proposed in this pull request?
ViewCatalog API improvements described in SPIP that didn't make it into the codebase as part of #37556
Why are the changes needed?
Required for DataSourceV2 view support.
Does this PR introduce any user-facing change?
No
How was this patch tested?
N/A
Was this patch authored or co-authored using generative AI tooling?
N/A