[SPARK-16660][SQL] CreateViewCommand should not take CatalogTable #14297
Conversation
|
cc @yhuai @liancheng |
|
Test build #62661 has finished for PR 14297 at commit
|
|
Test build #62663 has finished for PR 14297 at commit
|
|
Test build #62671 has finished for PR 14297 at commit
|
| originalText = None, | ||
| child = logicalPlan, | ||
| allowExisting = false, | ||
| replace = true, |
There was a problem hiding this comment.
createOrReplaceTempView and createTempView share all the input values, except this replace. Should we create a private help function to avoid duplicate codes?
|
Test build #62701 has finished for PR 14297 at commit
|
|
retest this please |
|
Test build #62775 has finished for PR 14297 at commit
|
|
Test build #62786 has finished for PR 14297 at commit
|
| originalText: Option[String], | ||
| child: LogicalPlan, | ||
| allowExisting: Boolean, | ||
| replace: Boolean, |
There was a problem hiding this comment.
Shall we replace allowExisting and replace with SaveMode? We have the following mapping here:
allowExisting |
replace |
SaveMode |
|---|---|---|
true |
false |
Ignore |
false |
false |
ErrorIfExists |
false |
true |
Overwrite |
true |
true |
Overwrite |
Append can't be used here though.
There was a problem hiding this comment.
I'd like to do it in another PR, to move SaveMode into catalyst module and use it for CREATE TABLE/VIEW
There was a problem hiding this comment.
btw, we can rename allowExisting to ignoreIfExists. I am not sure we need to SaveMode (a public API) to catalyst.
| if (!isTemporary) { | ||
| require(tableDesc.viewText.isDefined, | ||
| require(originalText.isDefined, | ||
| "The table to created with CREATE VIEW must have 'viewText'.") |
There was a problem hiding this comment.
Nit: "viewText" to "originalText".
|
LGTM except for one minor issue. |
|
Test build #62818 has finished for PR 14297 at commit
|
|
Merging to master, thanks! |
5 participants
What changes were proposed in this pull request?
CreateViewCommandonly needs some information of aCatalogTable, but not all of them. We have some tricks(e.g. we need to check the table type isVIEW, we need to makeCatalogColumn.dataTypenullable) to allow it to take aCatalogTable.This PR cleans it up and only pass in necessary information to
CreateViewCommand.How was this patch tested?
existing tests.