[SPARK-29628][SQL] Forcibly create a temporary view in CREATE VIEW if referencing a temporary view #26317

amanomer · 2019-10-30T08:57:38Z

What changes were proposed in this pull request?

When creating permanent view based on temporary view, it will be created as a temporary view. Previously, Spark this was not allowed.

Why are the changes needed?

To match the behavior of postgreSQL.

Does this PR introduce any user-facing change?

No

How was this patch tested?

UT added.

AmplabJenkins · 2019-10-30T09:03:03Z

Can one of the admins verify this patch?

amanomer · 2019-10-30T10:30:55Z

@MaxGekk @srowen @cloud-fan

sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala

MaxGekk · 2019-10-30T10:50:12Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala

-    throw new AnalysisException(
-      s"It is not allowed to add database prefix `$database` for the TEMPORARY view name.")
+    // Temporary view names should NOT contain database prefix like "database.table"
+    if (name.database.isDefined) {


nit:

name.database.foreach { database => throw new AnalysisException( s"It is not allowed to add database prefix `$database` for the TEMPORARY view name.") }

name.database returns String.

It's not, it's an Option[String], I imagine, in which case this is indeed a little more idiomatic.

My bad. name.database is an Option[String].

Now, I see how foreach is working here.
Thanks @MaxGekk

amanomer · 2019-10-30T11:38:53Z

@MaxGekk

dongjoon-hyun · 2019-10-30T16:22:52Z

cc @maropu

dongjoon-hyun · 2019-10-30T16:24:54Z

Hi, @amanomer . Thank you for making a PR. FYI, for SQL PR, you need to add [SQL] after JIRA ID in the PR title.

sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala

sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala

dongjoon-hyun · 2019-10-30T16:32:12Z

cc @gatorsmile since this is a behavior change.

sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala

dongjoon-hyun

@amanomer . I finished my first round review. After your updating, I'll review again.

maropu · 2019-10-31T00:18:54Z

btw, is this behaviour is common in the other DMBS-like? If no, better to add this feature in the PgSQL dialect.

amanomer · 2019-10-31T05:11:14Z

cc @dongjoon-hyun @gengliangwang

cloud-fan · 2019-11-04T06:28:24Z

This is a weird feature. Does pgsql give warning when this happens?

maropu · 2019-11-04T06:55:08Z

Yea, pgSQL does so:

-- should be created in temp object schema
CREATE VIEW v1_temp AS SELECT * FROM temp_table;
NOTICE:  view "v1_temp" will be a temporary view

https://github.com/postgres/postgres/blob/REL_12_STABLE/src/test/regress/expected/create_view.out#L86

cloud-fan · 2019-11-04T08:03:48Z

If it's a feature that pgsql would warn you, I'm not sure we need to have it in Spark. This at lease should be protected by the pgsql dialect.

srowen · 2019-11-04T14:38:05Z

I generally agree, unless it's important to match semantics, I don't think we should just let this work?

maropu · 2019-11-04T22:50:30Z

I'm not sure though, if this feature is not common in the other DMBS implementations (e.g., Oracle?), I personally think it might be worth doing in the pgSQL dialect.

amanomer · 2019-11-05T03:58:04Z

I will move this code to pgSQL dialect.

amanomer · 2019-11-11T11:28:55Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala

-          throw new AnalysisException(s"Not allowed to create a permanent view $name by " +
-            s"referencing a temporary view $ident")
+          // Temporary views are only stored in the session catalog
+          if (sparkSession.sqlContext.conf.usePostgreSQLDialect) {


I have handled pgSQLDialect case here. Since class CreateViewCommand of sql/core is not accessible from PostgreSQLDialect sql/catalyst because sql/core is dependent on sql/catalyst.

amanomer · 2019-11-12T14:27:09Z

I have handled the review comments.Kindly check this.

sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala

srowen · 2019-11-13T12:58:23Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala

+    }
  }

+  private var isTempReferred = false


I'm not sure about the semantics of this. It's used in a different place from where it's checked. Is this always set to true by a code path before it needs to be used?

isTempReferred A flag which will be true when permanent view is based on temporary view while using pgSQL dialect.

Yes that's not quite my question. How do we know the code path that sets this to true below will execute first?

In run(), before using isTempReferred, a call to verifyTemporaryObjectsNotExists() is made which will update it's value.

sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala

amanomer · 2019-11-13T17:26:24Z

cc @brkyvz

amanomer · 2019-11-19T06:45:14Z

cc @cloud-fan

amanomer · 2019-11-25T04:47:18Z

@maropu @gengliangwang kindly review this

gatorsmile · 2019-12-01T22:16:18Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala

-            throw new AnalysisException(s"Not allowed to create a permanent view $name by " +
-              s"referencing a temporary view $ident")
+            // Temporary views are only stored in the session catalog
+            if (sparkSession.sqlContext.conf.usePostgreSQLDialect) {


We are removing this dialect as we discussed in the mailing list.

gatorsmile · 2019-12-01T22:17:49Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala

            if sparkSession.sessionState.catalog.isTemporaryTable(ident) =>
-            // temporary views are only stored in the session catalog
-            throw new AnalysisException(s"Not allowed to create a permanent view $name by " +
-              s"referencing a temporary view $ident")


We can improve the error message and suggest end users to add the keyword TEMPORARY to create a temporary view instead.

PR for above suggestion #26731

@gatorsmile also review #26712

### What changes were proposed in this pull request? Improved error message while creating views. ### Why are the changes needed? Error message should suggest user to use TEMPORARY keyword while creating permanent view referred by temporary view. #26317 (comment) ### Does this PR introduce any user-facing change? No ### How was this patch tested? Updated test case. Closes #26731 from amanomer/imp_err_msg. Authored-by: Aman Omer <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

### What changes were proposed in this pull request? Improved error message while creating views. ### Why are the changes needed? Error message should suggest user to use TEMPORARY keyword while creating permanent view referred by temporary view. apache#26317 (comment) ### Does this PR introduce any user-facing change? No ### How was this patch tested? Updated test case. Closes apache#26731 from amanomer/imp_err_msg. Authored-by: Aman Omer <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

amanomer · 2020-02-10T06:16:11Z

I think, this PR is not required.

[SPARK-29628] Initial commit

6e804e2

MaxGekk reviewed Oct 30, 2019

View reviewed changes

amanomer added 3 commits October 30, 2019 16:30

[SPARK-29628] Initial commit

ff1174c

Handled review comment

cd2833c

Handled review comments

4e5228c

amanomer force-pushed the 29628 branch from 4e5228c to 6e804e2 Compare October 30, 2019 11:43

dongjoon-hyun changed the title ~~[SPARK-29628] Forcibly create a temporary view in CREATE VIEW if referencing a temporary view~~ [SPARK-29628][SQL] Forcibly create a temporary view in CREATE VIEW if referencing a temporary view Oct 30, 2019

dongjoon-hyun added the SQL label Oct 30, 2019

dongjoon-hyun reviewed Oct 30, 2019

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Oct 30, 2019

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Oct 30, 2019

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Oct 30, 2019

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala Outdated Show resolved Hide resolved

dongjoon-hyun requested changes Oct 30, 2019

View reviewed changes

Handled review comments

d04292b

amanomer force-pushed the 29628 branch from d4a6bc8 to d04292b Compare October 30, 2019 16:53

Removed extra line from test suite.

937da50

amanomer requested a review from dongjoon-hyun October 30, 2019 17:02

imback82 mentioned this pull request Nov 1, 2019

[SPARK-29630][SQL] Disallow creating a permanent view that references a temporary view in an expression #26361

Closed

amanomer requested a review from maropu November 2, 2019 04:21

Handled review comments

18dad1c

Use pgSQLDialect

3b3aa1d

amanomer commented Nov 11, 2019

View reviewed changes

amanomer and others added 3 commits November 11, 2019 17:19

Resolved conflicts

5d11ddf

Merge branch 'master' into 29628

cf1b5ee

Scala style fix

3f12e09

amanomer mentioned this pull request Nov 13, 2019

[SPARK-29860][SQL] Fix dataType mismatch issue for InSubquery. #26485

Closed

amanomer requested a review from srowen November 13, 2019 05:26

amanomer mentioned this pull request Nov 13, 2019

[SPARK-29462] The data type of "array()" should be array<null> #26324

Closed

srowen reviewed Nov 13, 2019

View reviewed changes

nit

87557e0

amanomer requested a review from srowen November 13, 2019 16:01

amanomer mentioned this pull request Nov 13, 2019

[SPARK-29818][MLLIB] Missing persist on RDD #26454

Closed

gatorsmile reviewed Dec 1, 2019

View reviewed changes

amanomer mentioned this pull request Dec 2, 2019

[SPARK-30093][SQL] Improve error message for creating view #26731

Closed

amanomer closed this Feb 10, 2020

[SPARK-29628][SQL] Forcibly create a temporary view in CREATE VIEW if referencing a temporary view #26317

[SPARK-29628][SQL] Forcibly create a temporary view in CREATE VIEW if referencing a temporary view #26317

Uh oh!

Conversation

amanomer commented Oct 30, 2019

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

AmplabJenkins commented Oct 30, 2019

Uh oh!

amanomer commented Oct 30, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amanomer Nov 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amanomer commented Oct 30, 2019

Uh oh!

dongjoon-hyun commented Oct 30, 2019

Uh oh!

dongjoon-hyun commented Oct 30, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dongjoon-hyun commented Oct 30, 2019

Uh oh!

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

maropu commented Oct 31, 2019

Uh oh!

amanomer commented Oct 31, 2019

Uh oh!

cloud-fan commented Nov 4, 2019

Uh oh!

maropu commented Nov 4, 2019

Uh oh!

cloud-fan commented Nov 4, 2019

Uh oh!

srowen commented Nov 4, 2019

Uh oh!

maropu commented Nov 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amanomer commented Nov 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amanomer commented Nov 12, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amanomer commented Nov 13, 2019

Uh oh!

amanomer commented Nov 19, 2019

Uh oh!

amanomer commented Nov 25, 2019

Uh oh!

amanomer Nov 4, 2019 •

edited

Loading

maropu commented Nov 4, 2019 •

edited

Loading