[SPARK-17180][SPARK-17309][SPARK-17323][SQL] create AlterViewAsCommand to handle ALTER VIEW AS by cloud-fan · Pull Request #14874 · apache/spark

cloud-fan · 2016-08-30T09:38:05Z

What changes were proposed in this pull request?

Currently we use CreateViewCommand to implement ALTER VIEW AS, which has 3 bugs:

SPARK-17180: ALTER VIEW AS should alter temp view if view name has no database part and temp view exists
SPARK-17309: ALTER VIEW AS should issue exception if view does not exist.
SPARK-17323: ALTER VIEW AS should keep the previous table properties, comment, create_time, etc.

The root cause is, ALTER VIEW AS is quite different from CREATE VIEW, we need different code path to handle them. However, in CreateViewCommand, there is no way to distinguish ALTER VIEW AS and CREATE VIEW, we have to introduce extra flag. But instead of doing this, I think a more natural way is to separate the ALTER VIEW AS logic into a new command.

How was this patch tested?

new tests in SQLViewSuite

cloud-fan · 2016-08-30T09:38:16Z

cc @yhuai @gatorsmile

SparkQA · 2016-08-30T11:37:20Z

Test build #64645 has finished for PR 14874 at commit 69e6cd4.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class AlterViewCommand(

gatorsmile · 2016-08-30T16:53:49Z

+ * Alter the query of a view. This creates a [[AlterViewCommand]] command.
 */
 override def visitAlterViewQuery(ctx: AlterViewQueryContext): LogicalPlan = withOrigin(ctx) {
- createView(


Now, we can remove this function createView. It is only being used by visitCreateView. We just need to directly create a CreateViewCommand in visitCreateView

gatorsmile · 2016-08-30T17:38:12Z

This is another bug in the existing ALTER VIEW. Let me show what I got in Hive:

hive> create view t1_view comment 'test_comment' TBLPROPERTIES ('p1'='v1') asselect*from t1;

hive> describe formatted t1_view;
OK
# col_name data_type comment col int # Detailed Table Information Database: default Owner: root CreateTime: Tue Aug 30 13:32:41 EDT 2016 LastAccessTime: UNKNOWN Protect Mode: None Retention: 0 Table Type: VIRTUAL_VIEW Table Parameters: comment test_comment p1 v1 transient_lastDdlTime 1472578361 # Storage Information SerDe Library: null InputFormat: org.apache.hadoop.mapred.SequenceFileInputFormat OutputFormat: org.apache.hadoop.hive.ql.io.HiveSequenceFileOutputFormat Compressed: No Num Buckets: -1 Bucket Columns: [] Sort Columns: [] # View Information View Original Text: select * from t1 View Expanded Text: select `t1`.`col` from `default`.`t1` Time taken: 0.074 seconds, Fetched: 29 row(s)

hive> alter view t1_view asselect col from t1;

hive> describe formatted t1_view;
OK
# col_name data_type comment col int # Detailed Table Information Database: default Owner: root CreateTime: Tue Aug 30 13:32:41 EDT 2016 LastAccessTime: UNKNOWN Protect Mode: None Retention: 0 Table Type: VIRTUAL_VIEW Table Parameters: comment test_comment p1 v1 transient_lastDdlTime 1472578421 # Storage Information SerDe Library: null InputFormat: org.apache.hadoop.mapred.SequenceFileInputFormat OutputFormat: org.apache.hadoop.hive.ql.io.HiveSequenceFileOutputFormat Compressed: No Num Buckets: -1 Bucket Columns: [] Sort Columns: [] # View Information View Original Text: select col from t1 View Expanded Text: select `t1`.`col` from `default`.`t1` Time taken: 0.065 seconds, Fetched: 29 row(s)

After ALTER VIEW, we need to keep the existing table properties and comment.

cloud-fan · 2016-08-31T01:51:43Z

@gatorsmile good catch! I have fixed it, thanks!

SparkQA · 2016-08-31T03:51:55Z

Test build #64690 has finished for PR 14874 at commit 51726ff.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class AlterViewAsCommand(

gatorsmile · 2016-08-31T06:42:47Z

+ }
+
+ test("ALTER VIEW AS should keep the previous table properties, comment, create_time, etc.") {
+ withTempView("test_view") {


test_view is not a temporary view, right?

gatorsmile · 2016-08-31T06:43:00Z

LGTM except one minor comment.

gatorsmile · 2016-08-31T06:45:31Z

+
+ test("ALTER VIEW AS should alter permanent view if view name has database part") {
+ withTempView("test_view") {
+ withView("test_view") {


Based on my understanding, this will drop the temporary view because the resolution preference of drop view and then withTempView is unable to find any temporary view.

SparkQA · 2016-08-31T08:47:50Z

Test build #64703 has finished for PR 14874 at commit 8480945.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2016-08-31T09:09:50Z

thanks for the review, merging to master!
I'll send a new PR for 2.0

…ommand to handle ALTER VIEW AS ## What changes were proposed in this pull request? Currently we use `CreateViewCommand` to implement ALTER VIEW AS, which has 3 bugs: 1. SPARK-17180: ALTER VIEW AS should alter temp view if view name has no database part and temp view exists 2. SPARK-17309: ALTER VIEW AS should issue exception if view does not exist. 3. SPARK-17323: ALTER VIEW AS should keep the previous table properties, comment, create_time, etc. The root cause is, ALTER VIEW AS is quite different from CREATE VIEW, we need different code path to handle them. However, in `CreateViewCommand`, there is no way to distinguish ALTER VIEW AS and CREATE VIEW, we have to introduce extra flag. But instead of doing this, I think a more natural way is to separate the ALTER VIEW AS logic into a new command. backport #14874 to 2.0 ## How was this patch tested? new tests in SQLViewSuite Author: Wenchen Fan <wenchen@databricks.com> Closes#14893 from cloud-fan/minor4.

create AlterViewCommand to handle ALTER VIEW
69e6cd4

cloud-fan mentioned this pull request Aug 30, 2016
[SPARK-17180] [SQL] Fix View Resolution Order in ALTER VIEW AS SELECT #14746
Closed

gatorsmile reviewed Aug 30, 2016
View reviewed changes

cloud-fan changed the title ~~[SPARK-17180][SPARK-17309][SQL] create AlterViewCommand to handle ALTER VIEW~~[SPARK-17180][SPARK-17309][SPARK-17323][SQL] create AlterViewCommand to handle ALTER VIEWAug 31, 2016

cloud-fan changed the title ~~[SPARK-17180][SPARK-17309][SPARK-17323][SQL] create AlterViewCommand to handle ALTER VIEW~~[SPARK-17180][SPARK-17309][SPARK-17323][SQL] create AlterViewCommand to handle ALTER VIEW ASAug 31, 2016

cloud-fan changed the title ~~[SPARK-17180][SPARK-17309][SPARK-17323][SQL] create AlterViewCommand to handle ALTER VIEW AS~~[SPARK-17180][SPARK-17309][SPARK-17323][SQL] create AlterViewAsCommand to handle ALTER VIEW ASAug 31, 2016

cloud-fan force-pushed the minor4 branch 2 times, most recently from bbc08d9 to 5b139d9Compare August 31, 2016 01:49

address comments
51726ff

cloud-fan force-pushed the minor4 branch from 5b139d9 to 51726ffCompare August 31, 2016 01:51

gatorsmile reviewed Aug 31, 2016
View reviewed changes

fix some tests
8480945

asfgit closed this in 12fd0cdAug 31, 2016

cloud-fan mentioned this pull request Aug 31, 2016
[SPARK-17180][SPARK-17309][SPARK-17323][SQL][2.0] create AlterViewAsCommand to handle ALTER VIEW AS #14893
Closed

Navigation Menu

Uh oh!

[SPARK-17180][SPARK-17309][SPARK-17323][SQL] create AlterViewAsCommand to handle ALTER VIEW AS - #14874

[SPARK-17180][SPARK-17309][SPARK-17323][SQL] create AlterViewAsCommand to handle ALTER VIEW AS#14874
cloud-fan wants to merge 3 commits into
apache:masterfrom
cloud-fan:minor4

cloud-fan commented Aug 30, 2016•
edited
Loading

Uh oh!

cloud-fan commented Aug 30, 2016

Uh oh!

SparkQA commented Aug 30, 2016

Uh oh!

gatorsmileAug 30, 2016

Uh oh!

gatorsmile commented Aug 30, 2016

Uh oh!

cloud-fan commented Aug 31, 2016

Uh oh!

SparkQA commented Aug 31, 2016

Uh oh!

gatorsmileAug 31, 2016

Uh oh!

gatorsmile commented Aug 31, 2016

Uh oh!

gatorsmileAug 31, 2016

Uh oh!

SparkQA commented Aug 31, 2016

Uh oh!

cloud-fan commented Aug 31, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

cloud-fan commented Aug 30, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

cloud-fan commented Aug 30, 2016

Uh oh!

SparkQA commented Aug 30, 2016

Uh oh!

gatorsmileAug 30, 2016

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Aug 30, 2016

Uh oh!

cloud-fan commented Aug 31, 2016

Uh oh!

SparkQA commented Aug 31, 2016

Uh oh!

gatorsmileAug 31, 2016

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Aug 31, 2016

Uh oh!

gatorsmileAug 31, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 31, 2016

Uh oh!

cloud-fan commented Aug 31, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cloud-fan commented Aug 30, 2016•
edited
Loading