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

imback82 · 2019-11-01T16:33:01Z

What changes were proposed in this pull request?

Disallow creating a permanent view that references a temporary view in expressions.

Why are the changes needed?

Creating a permanent view that references a temporary view is currently disallowed. For example,

# The following throws org.apache.spark.sql.AnalysisException
# Not allowed to create a permanent view `per_view` by referencing a temporary view `tmp`;
CREATE VIEW per_view AS SELECT t1.a, t2.b FROM base_table t1, (SELECT * FROM tmp) t2"

However, the following is allowed.

CREATE VIEW per_view AS SELECT * FROM base_table WHERE EXISTS (SELECT * FROM tmp);

This PR fixes the bug where temporary views used inside expressions are not checked.

Does this PR introduce any user-facing change?

Yes. Now the following SQL query throws an exception as expected:

# The following throws org.apache.spark.sql.AnalysisException
# Not allowed to create a permanent view `per_view` by referencing a temporary view `tmp`;
CREATE VIEW per_view AS SELECT * FROM base_table WHERE EXISTS (SELECT * FROM tmp);

How was this patch tested?

Added new unit tests.

imback82 · 2019-11-01T16:33:18Z

cc: @cloud-fan

SparkQA · 2019-11-01T16:59:32Z

Test build #113102 has finished for PR 26361 at commit 44d3d08.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

imback82 · 2019-11-01T18:27:39Z

Looking at #26317, a "temporary" view will be automatically created. I can wait for that to be merged first. (This PR makes searching for temporary views more exhaustive).

cc: @amanomer

SparkQA · 2019-11-01T21:00:23Z

Test build #113110 has finished for PR 26361 at commit 80ab9ad.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

sql/core/src/test/resources/sql-tests/inputs/postgreSQL/create_view.sql

viirya · 2019-11-01T22:46:22Z

sql/core/src/test/resources/sql-tests/inputs/postgreSQL/create_view.sql

-- [SPARK-29628] Forcibly create a temporary view in CREATE VIEW if referencing a temporary view
 CREATE VIEW v6_temp AS SELECT * FROM base_table WHERE id IN (SELECT id FROM temp_table);
 CREATE VIEW v7_temp AS SELECT t1.id, t2.a FROM base_table t1, (SELECT * FROM temp_table) t2;
 -- [SPARK-29630] Not allowed to create a permanent view by referencing a temporary view in EXISTS


Isn't SPARK-29630 the JIRA ticket for this PR? Why it was there before this?

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

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

SparkQA · 2019-11-02T07:05:02Z

Test build #113121 has finished for PR 26361 at commit c9ddf81.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

imback82 · 2019-11-02T15:13:28Z

retest this please

SparkQA · 2019-11-02T18:43:38Z

Test build #113131 has finished for PR 26361 at commit c9ddf81.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

imback82 · 2019-11-02T18:51:27Z

retest this please

SparkQA · 2019-11-02T22:27:40Z

Test build #113136 has finished for PR 26361 at commit c9ddf81.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2019-11-02T22:40:27Z

retest this please

sql/core/src/test/resources/sql-tests/results/postgreSQL/create_view.sql.out

SparkQA · 2019-11-03T02:18:19Z

Test build #113143 has finished for PR 26361 at commit c9ddf81.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2019-11-04T06:30:42Z

sql/core/src/test/resources/sql-tests/inputs/postgreSQL/create_view.sql

 CREATE TABLE tbl2 (c int, d int) using parquet;
 CREATE TABLE tbl3 (e int, f int) using parquet;
 CREATE TABLE tbl4 (g int, h int) using parquet;
 -- Since Spark doesn't support CREATE TEMPORARY TABLE, we used CREATE TEMPORARY VIEW instead


can we update this comment? Now we use table as temp table in this test.

thanks, fixed.

cloud-fan · 2019-11-04T06:32:01Z

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

+          "CREATE VIEW v AS SELECT * FROM base_table WHERE EXISTS (SELECT * FROM temp_view)",
+          "CREATE VIEW v AS SELECT id, (SELECT count(*) FROM temp_view) FROM base_table",
+          "CREATE VIEW v AS SELECT * FROM base_table " +
+            "WHERE id IN (SELECT id FROM base_table WHERE EXISTS (SELECT 1 FROM temp_view))"


can we move them to create_view.sql? That's a better place for end-to-end tests.

I moved these to create_view.sql. The first two scenarios were already there, so I just moved the last one. thanks for the feedback.

SparkQA · 2019-11-04T08:05:01Z

Test build #113190 has finished for PR 26361 at commit 8ca1f4a.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-11-04T18:57:08Z

Test build #113213 has finished for PR 26361 at commit 24f15f4.

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

cloud-fan · 2019-11-05T05:19:56Z

thanks, merging to master!

gatorsmile · 2019-12-28T02:28:02Z

sql/core/src/test/resources/sql-tests/results/postgreSQL/create_view.sql.out

 -- !query 71 output
-
+org.apache.spark.sql.AnalysisException
+Not allowed to create a permanent view `temporal5` by referencing a temporary view `tt`;


I think we should also provide the workaround in the error message. Instead just telling the users temp views are not allowed, we can say users can use permanent views in these cases?

cc @imback82 @cloud-fan

We can add something like Create a temporary view to reference temporary views; a permanent view can only reference permanent views, but sounds a bit redundant with the error message?

Permanent views can also reference to tables.

There exists a general issue in our existing error messages. We do not have a message reference book. When users hit the unclear error messages, what they can do is to read the source code or try different options until it works. This is not natural to end users who have zero background about Spark SQL. We need to correct them one by one.

Got it. I will do a follow-up PR.

Note: this is the existing error message. This PR just extends the temp view check to subqueries. Agree that we can still improve the error message.

Looks like this is already addressed by #26731.

imback82 added 2 commits November 1, 2019 08:46

initial checkin

8d57968

Add tests

44d3d08

fix tests

80ab9ad

maropu reviewed Nov 1, 2019

View reviewed changes

sql/core/src/test/resources/sql-tests/inputs/postgreSQL/create_view.sql Outdated Show resolved Hide resolved

maropu changed the title ~~[Spark-29630][SQL] Disallow creating a permanent view that references a temporary view in an expression~~ [SPARK-29630][SQL] Disallow creating a permanent view that references a temporary view in an expression Nov 1, 2019

viirya reviewed Nov 1, 2019

View reviewed changes

maropu reviewed Nov 1, 2019

View reviewed changes

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

maropu reviewed Nov 1, 2019

View reviewed changes

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

fix tests. address PR comments

c9ddf81

maropu reviewed Nov 2, 2019

View reviewed changes

sql/core/src/test/resources/sql-tests/results/postgreSQL/create_view.sql.out Outdated Show resolved Hide resolved

dongjoon-hyun added the SQL label Nov 3, 2019

Address PR comments

8ca1f4a

cloud-fan reviewed Nov 4, 2019

View reviewed changes

address PR comments

24f15f4

maropu approved these changes Nov 4, 2019

View reviewed changes

cloud-fan closed this in 66619b8 Nov 5, 2019

gatorsmile reviewed Dec 28, 2019

View reviewed changes

imback82 deleted the spark-29630 branch January 6, 2020 01:01

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

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

Uh oh!

Conversation

imback82 commented Nov 1, 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!

imback82 commented Nov 1, 2019

Uh oh!

SparkQA commented Nov 1, 2019

Uh oh!

imback82 commented Nov 1, 2019

Uh oh!

SparkQA commented Nov 1, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SparkQA commented Nov 2, 2019

Uh oh!

imback82 commented Nov 2, 2019

Uh oh!

SparkQA commented Nov 2, 2019

Uh oh!

imback82 commented Nov 2, 2019

Uh oh!

SparkQA commented Nov 2, 2019

Uh oh!

maropu commented Nov 2, 2019

Uh oh!

Uh oh!

SparkQA commented Nov 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan 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!

SparkQA commented Nov 4, 2019

Uh oh!

SparkQA commented Nov 4, 2019

Uh oh!

cloud-fan commented Nov 5, 2019

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

cloud-fan Nov 4, 2019 •

edited

Loading