[SPARK-17180] [SQL] Fix View Resolution Order in ALTER VIEW AS SELECT #14746
Conversation
|
Test build #64182 has finished for PR 14746 at commit
|
|
cc @cloud-fan @yhuai |
| withView("testView", "default.testView") { | ||
| val catalog = spark.sessionState.catalog | ||
| val oldViewQuery = "SELECT id FROM jt" | ||
| val newViewQuery = "SELECT id, id1 FROM jt" |
There was a problem hiding this comment.
nit: remove extra spaces
|
Test build #64396 has finished for PR 14746 at commit
|
| // 1) CREATE VIEW: create a temp view when users explicitly specify the keyword TEMPORARY; | ||
| // otherwise, create a permanent view no matter whether the temporary view | ||
| // with the same name exists or not. | ||
| // 2) ALTER VIEW: alter the temporary view if the temp view exists; otherwise, try to alter |
There was a problem hiding this comment.
question: how can you tell whether it's CREATE VIEW or ALTER VIEW?
There was a problem hiding this comment.
Yeah! The only way is to pass a flag.
There was a problem hiding this comment.
it already has 3 flags: allowExisting, replace, isTemporary. We should rethink about it and decide what flags we really need for this command.
There was a problem hiding this comment.
True, let me think about it. Thanks!
There was a problem hiding this comment.
CREATE (OR REPLACE)? TEMPORARY? VIEW (IF NOT EXISTS)? tableIdentifier
identifierCommentList? (COMMENT STRING)?
(PARTITIONED ON identifierList)?
(TBLPROPERTIES tablePropertyList)? AS queryEach flag corresponds to a keyword in the CREATE VIEW DDL command.
OR REPLACE->replaceTEMPORARY->isTemporaryIF NOT EXISTS->allowExisting
If we want to use the same command CreateViewCommand to process both CREATE VIEW and ALTER VIEW, it sounds reasonable to add a new flag. So far, I have not found a way to combine them.
There was a problem hiding this comment.
like CreateMode, can we use SaveMode for replace and allowExisting?
There was a problem hiding this comment.
uh, I got it. : ) Copied what @liancheng wrote one month ago and did the update in the last case.
| allowExisting | replace | SaveMode |
|---|---|---|
| true | false | Ignore |
| false | false | ErrorIfExists |
| false | true | Overwrite |
| true | true | AnalysisException |
Let me try it. Thanks!
|
Test build #64458 has finished for PR 14746 at commit
|
| replace: Boolean, | ||
| isTemporary: Boolean): LogicalPlan = { | ||
| isTemporary: Boolean, | ||
| isAlterViewAsSelect: Boolean): LogicalPlan = { |
There was a problem hiding this comment.
how about a viewType parameter which is an enum?
public enum ViewType {
Temporary,
Permanent,
Any
}
We are going to add global temp view soon, so this enum will also be useful at that time.
There was a problem hiding this comment.
Sure, will do it. Thanks!
|
Test build #64526 has finished for PR 14746 at commit
|
|
Test build #64547 has finished for PR 14746 at commit
|
|
Test build #64549 has finished for PR 14746 at commit
|
| /** | ||
| * ViewType is used to specify the type of views. | ||
| */ | ||
| public enum ViewType { |
There was a problem hiding this comment.
This doesn't need to be public to end users, we can put it in view.scala and use sealed trait to implement it.
There was a problem hiding this comment.
I thought you want me to use public enum. Let me change it now. Thanks!
|
Test build #64588 has finished for PR 14746 at commit
|
|
retest this please |
|
Test build #64603 has finished for PR 14746 at commit
|
|
after thinking about it more, I think it's better to use a different code path for ALTER VIEW, instead of sharing the same implementation of CREATE VIEW. Do you mind review #14874? thanks! |
|
Yeah, I am also fine about splitting them into two commands. Let me close it now. |
3 participants
What changes were proposed in this pull request?
In the current master branch, when users do not specify the database name in the
ALTER VIEW AS SELECTcommand, we always try to alter the permanent view even if the temporary view exists. This PR is to resolve this issue.After the fix,
ALTER VIEW AS SELECTcommand alters the temporary view if the temp view exists; otherwise, try to alter the permanent view. This order is consistent with another commandDROP VIEW, since users are unable to specify the keyword TEMPORARY.How was this patch tested?
Added test cases.