[SPARK-34421][SQL] Resolve temporary functions and views in views with CTEs #31550

peter-toth · 2021-02-11T14:52:17Z

What changes were proposed in this pull request?

This PR:

Fixes a bug that prevents analysis of:

CREATE TEMPORARY VIEW temp_view AS WITH cte AS (SELECT temp_func(0)) SELECT * FROM cte;
SELECT * FROM temp_view

by throwing:

Undefined function: 'temp_func'. This function is neither a registered temporary function nor a permanent function registered in the database 'default'.

and doesn't report analysis error when it should:

CREATE TEMPORARY VIEW temp_view AS SELECT 0;
CREATE VIEW view_on_temp_view AS WITH cte AS (SELECT * FROM temp_view) SELECT * FROM cte

by properly collecting temporary objects from VIEW definitions with CTEs.

Minor refactor to make the affected code more readable.

Why are the changes needed?

To fix a bug introduced with #30567

Does this PR introduce any user-facing change?

Yes, the query works again.

How was this patch tested?

Added new UT + existing ones.

…h CTEs

peter-toth · 2021-02-11T15:01:30Z

cc @linhongliu-db, @cloud-fan

SparkQA · 2021-02-11T16:18:00Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39696/

SparkQA · 2021-02-11T16:48:34Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39696/

gatorsmile · 2021-02-11T19:15:21Z

Thanks for the fix! Not sure whether @cloud-fan and @LinhongLiu will review it, because today is the lunar new year!

CC @allisonwang-db @maryannxue

SparkQA · 2021-02-11T19:27:07Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39698/

SparkQA · 2021-02-11T19:54:07Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39698/

SparkQA · 2021-02-11T20:09:59Z

Test build #135114 has finished for PR 31550 at commit 1f13cfe.

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

allisonwang-db · 2021-02-11T22:21:03Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

+    withTempView("temp_view") {
+      sql("CREATE TEMPORARY VIEW temp_view AS SELECT 0")
+
+      intercept[AnalysisException] {


Can we also add an assertion for the error message here?

Sure, added in 1cd1cb3

SparkQA · 2021-02-11T23:47:57Z

Test build #135116 has finished for PR 31550 at commit f8564cb.

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

peter-toth · 2021-02-12T08:51:45Z

cc @HyukjinKwon as this became a blocker of 3.1.1

SparkQA · 2021-02-12T09:19:16Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39701/

SparkQA · 2021-02-12T09:49:54Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39701/

SparkQA · 2021-02-12T12:14:23Z

Test build #135120 has finished for PR 31550 at commit 1cd1cb3.

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

HyukjinKwon · 2021-02-13T01:42:01Z

Thanks for pinging me @peter-toth. I will share it in the vote thread.

HyukjinKwon · 2021-02-13T02:02:20Z

@cloud-fan @LinhongLiu @allisonwang-db @maryannxue, it would be great if reviewing this is prioritized to unblock the release.

HyukjinKwon · 2021-02-16T02:22:41Z

ping @cloud-fan @LinhongLiu @allisonwang-db @maryannxue

linhongliu-db · 2021-02-18T03:39:41Z

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

@@ -557,23 +557,30 @@ object ViewHelper {
  private def collectTemporaryObjects(
      catalog: SessionCatalog, child: LogicalPlan): (Seq[Seq[String]], Seq[String]) = {
    def collectTempViews(child: LogicalPlan): Seq[Seq[String]] = {
-      child.collect {
+      child.flatMap {


@peter-toth, thanks for fixing this.
any particular reason to replace collect to flatMap?

I have the same comment, too. IMO the previous one looks okay.

seems like it simplifies collect(...).flatten to flatMap(...)

as @cloud-fan explained

linhongliu-db

LGTM with minor comments

linhongliu-db · 2021-02-18T04:26:06Z

@peter-toth, just read the description, do we have analysis error when creating the permanent view with CTE and refers temp objects before #30567

maropu · 2021-02-18T04:54:55Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

+    val tempViewName = "temp_view"
+
+    withTempView(tempViewName) {
+      spark.udf.register("temp_func", identity[Int](_))


Could you use withUserDefinedFunction?

Sure, fixed in: ce34b5e

maropu · 2021-02-18T04:56:11Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

@@ -3943,6 +3943,43 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
      }
    }
  }
+
+  test("SPARK-34421: Resolve temporary functions and views in views with CTEs") {


Is this issue only related to a temp function case? How about a permanent one?

Same question

No, permanent functions are not affected: #31550 (comment)

maropu · 2021-02-18T05:00:04Z

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

@@ -557,23 +557,30 @@ object ViewHelper {
  private def collectTemporaryObjects(
      catalog: SessionCatalog, child: LogicalPlan): (Seq[Seq[String]], Seq[String]) = {
    def collectTempViews(child: LogicalPlan): Seq[Seq[String]] = {
-      child.collect {
+      child.flatMap {


I have the same comment, too. IMO the previous one looks okay.

maropu · 2021-02-18T05:24:24Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

+            |""".stripMargin)
+      }
+      assert(e.message.contains(s"Not allowed to create a permanent view `default`.`$viewName` " +
+        s"by referencing a temporary view $tempViewName."))


hm, this message looks correct, so v3.0.2 has the same issue?

Welcome to ____ __ / __/__ ___ _____/ /__ _\ \/ _ \/ _ `/ __/ '_/ /___/ .__/\_,_/_/ /_/\_\ version 3.0.2 /_/ Using Scala version 2.12.10 (OpenJDK 64-Bit Server VM, Java 11.0.2) Type in expressions to have them evaluated. Type :help for more information. scala> val tempViewName = "temp_view" scala> val viewName = "view_on_temp_view" scala> sql(s"CREATE TEMPORARY VIEW $tempViewName AS SELECT 0") scala> sql( | s""" | |CREATE VIEW $viewName AS | |WITH cte AS ( | | SELECT * FROM $tempViewName | |) | |SELECT * FROM cte | |""".stripMargin) scala> spark.table(viewName).show() +---+ | 0| +---+ | 0| +---+

~~While I agree with you, the check came with #30567 that is a 3.1 fix only.~~
This looks like a bug yes, details: #31550 (comment)

cloud-fan · 2021-02-18T05:45:44Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

+      val e = intercept[AnalysisException] {
+        sql(
+          s"""
+            |CREATE VIEW $viewName AS


So the bug happens in permanent view. Is it really a regression?

This part isn't, please see #31550 (comment) for details.

linhongliu-db · 2021-02-18T06:25:12Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

@@ -3943,6 +3943,43 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
      }
    }
  }
+
+  test("SPARK-34421: Resolve temporary functions and views in views with CTEs") {


for the temp view, we should verify the view capture temp objects correctly, so you should also add a permanent view/function, table with the same name and see if "select * from temp_view" can return correct result

ignore my comment, I found that is not necessary after checked your code one more time.

peter-toth · 2021-02-18T10:02:31Z

So it looks like the test case caused a bit of confusion here so I've split it into 2 cases.

SPARK-34421: Resolve temporary objects in temporary views with CTEs tests the regression part due to [SPARK-33142][SPARK-33647][SQL] Store SQL text for SQL temp view #30567. Please note that the temp view in temp view part does work even without this fix and the bug affects only the temp function in temp view case. Permanent functions are not affected and the code change modifies collectTemporaryObjects only.
SPARK-34421: Resolve temporary objects in permanent views with CTEs tests the non regression part. That "temp object in permanent one" check ~~came with [SPARK-33142][SPARK-33647][SQL] Store SQL text for SQL temp view #30567 but it~~ doesn't work on permanent views with CTEs without this fix.

linhongliu-db · 2021-02-18T10:58:46Z

SPARK-34421: Resolve temporary objects in permanent views with CTEs tests the non regression part. That "temp object in permanent one" check came with #30567 but it doesn't work on permanent views with CTEs without this fix.

Just a correction, temporary objects check for permanent views are existing for a long time, not introduced by #30567. But we never check the CTE scenario which is fixed by this PR. You can try the below queries on 3.0

CREATE TEMPORARY VIEW v0 AS SELECT 0
CREATE VIEW v1 AS SELECT FROM v0

peter-toth · 2021-02-18T11:08:48Z

SPARK-34421: Resolve temporary objects in permanent views with CTEs tests the non regression part. That "temp object in permanent one" check came with #30567 but it doesn't work on permanent views with CTEs without this fix.

Just a correction, temporary objects check for permanent views are existing for a long time, not introduced by #30567. But we never check the CTE scenario which is fixed by this PR. You can try the below queries on 3.0
CREATE TEMPORARY VIEW v0 AS SELECT 0
CREATE VIEW v1 AS SELECT FROM v0

Indeed, thanks for the correction.

cloud-fan · 2021-02-18T12:22:59Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

+    }
+  }
+
+  test("SPARK-34421: Resolve temporary objects in permanent views with CTEs") {


Can we move the test to SQLViewTestSuite, so that it's tested against temp view, global temp view, and permanent view automatically?

because temp view and permanent view have different behaviors, it's not easy to add this to SQLViewTestSuite

ah I see, makes sense.

cloud-fan · 2021-02-18T12:25:09Z

Please note that the temp view in temp view part does work even without this fix

hmm why is that? Looking at the code I think temp view and function should both trigger this bug.

cloud-fan · 2021-02-18T12:39:25Z

BTW let's also put the error message caused by this bug in the PR description, so that people can easily understand what happens, instead of just seeing a bug that prevents analysis.

SparkQA · 2021-02-18T14:37:33Z

Test build #135222 has finished for PR 31550 at commit ce34b5e.

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

peter-toth · 2021-02-18T14:43:15Z

BTW let's also put the error message caused by this bug in the PR description, so that people can easily understand what happens, instead of just seeing a bug that prevents analysis.

Sure, added to the PR description.

Please note that the temp view in temp view part does work even without this fix

hmm why is that? Looking at the code I think temp view and function should both trigger this bug.

No idea yet, but can take a look at it tomorrow.

cloud-fan · 2021-02-18T14:52:38Z

No idea yet, but can take a look at it tomorrow.

It will be great to figure this out, in case there are other bugs. It doesn't block this PR though.

linhongliu-db · 2021-02-18T15:47:35Z

take a look at "why temp view refers temp view in CTE is working without this PR".
It's because we do ResolveTempViews both in ResolveRelations and ResolveTables. In this case, ResolveRelations try to resolve temp_view as default.temp_view since it's not in the referred list (caused by this cte bug). But in ResolveTables, it just try to resolve all UnreolvedRelations as a temporary view, so in this case, it succeeds (by mistake).

Here is an example to explain:

use default;
create table same_name as select 100 as a;
create temp view test_view as select * from same_name;
select * from test_view; -- output 100
create temp view same_name as select 99 as a; -- a temp view has the same name with the table
select * from test_view; -- output 100, expected, temp view should not override table
drop table default.same_name; -- drop the table
select * from test_view; -- output 99, wrong result, we expect table not found because we dropped it

cloud-fan · 2021-02-19T10:13:45Z

I created test_view as a permanent view and tried this example in Spark 3.0, it also gives the wrong result. I think it's another bug of the permanent view that gets inherited by the new SQL temp view. @linhongliu-db can you help to fix it?

cloud-fan · 2021-02-19T10:14:51Z

thanks, merging to master/3.1!

…h CTEs ### What changes were proposed in this pull request? This PR: - Fixes a bug that prevents analysis of: ``` CREATE TEMPORARY VIEW temp_view AS WITH cte AS (SELECT temp_func(0)) SELECT * FROM cte; SELECT * FROM temp_view ``` by throwing: ``` Undefined function: 'temp_func'. This function is neither a registered temporary function nor a permanent function registered in the database 'default'. ``` - and doesn't report analysis error when it should: ``` CREATE TEMPORARY VIEW temp_view AS SELECT 0; CREATE VIEW view_on_temp_view AS WITH cte AS (SELECT * FROM temp_view) SELECT * FROM cte ``` by properly collecting temporary objects from VIEW definitions with CTEs. - Minor refactor to make the affected code more readable. ### Why are the changes needed? To fix a bug introduced with #30567 ### Does this PR introduce _any_ user-facing change? Yes, the query works again. ### How was this patch tested? Added new UT + existing ones. Closes #31550 from peter-toth/SPARK-34421-temp-functions-in-views-with-cte. Authored-by: Peter Toth <peter.toth@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 27abb6a) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

cloud-fan · 2021-02-19T10:16:36Z

@peter-toth can you open backport PRs for 3.0/2.4 to fix the permanent view bug? thanks!

peter-toth · 2021-02-19T10:30:11Z

Thanks for the review. I will open backport PRs soon.

…s with CTEs This PR: - Fixes a bug that prevents analysis of: ``` CREATE TEMPORARY VIEW temp_view AS WITH cte AS (SELECT temp_func(0)) SELECT * FROM cte; SELECT * FROM temp_view ``` by throwing: ``` Undefined function: 'temp_func'. This function is neither a registered temporary function nor a permanent function registered in the database 'default'. ``` - and doesn't report analysis error when it should: ``` CREATE TEMPORARY VIEW temp_view AS SELECT 0; CREATE VIEW view_on_temp_view AS WITH cte AS (SELECT * FROM temp_view) SELECT * FROM cte ``` by properly collecting temporary objects from VIEW definitions with CTEs. - Minor refactor to make the affected code more readable. To fix a bug introduced with apache#30567 Yes, the query works again. Added new UT + existing ones. Closes apache#31550 from peter-toth/SPARK-34421-temp-functions-in-views-with-cte. Authored-by: Peter Toth <peter.toth@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

peter-toth · 2021-02-19T14:54:22Z

@cloud-fan, I've opened #31592 to backport the change to 3.0.
But it looks like https://issues.apache.org/jira/browse/SPARK-29630 is missing from 2.4 and probably we should backport that change before this one. So shall I open a SPARK-29630 backport PR to 2.4?

linhongliu-db · 2021-02-19T14:57:05Z

So shall I open a SPARK-29630 backport PR to 2.4?

@peter-toth No need, the function is 2.4 is verifyTemporaryObjectsNotExists, just manually apply your changes to that function should be fine.

cloud-fan · 2021-02-19T14:58:17Z

If we decided to not backport SPARK-29630 to 2.4 at that time, we probably shouldn't backport this fix as well. 3.0 should be good enough.

peter-toth · 2021-02-19T15:10:20Z

So shall I open a SPARK-29630 backport PR to 2.4?

@peter-toth No need, the function is 2.4 is verifyTemporaryObjectsNotExists, just manually apply your changes to that function should be fine.

Yes, found that, but the recursive verify() inside verifyTemporaryObjectsNotExists() is missing due to missing SPARK-29630 and without that function traversing the inner children of With would be an ugly change.

If we decided to not backport SPARK-29630 to 2.4 at that time, we probably shouldn't backport this fix as well. 3.0 should be good enough.

Ok.

[SPARK-34421][SQL] Resolve temporary functions and views in views wit…

1f13cfe

…h CTEs

github-actions bot added the SQL label Feb 11, 2021

peter-toth changed the title ~~[SPARK-34421][SQL] Resolve temporary functions and views in views with CTEs~~ [WIP][SPARK-34421][SQL] Resolve temporary functions and views in views with CTEs Feb 11, 2021

traverse only inner children of With, add temp view test

f8564cb

peter-toth changed the title ~~[WIP][SPARK-34421][SQL] Resolve temporary functions and views in views with CTEs~~ [SPARK-34421][SQL] Resolve temporary functions and views in views with CTEs Feb 11, 2021

allisonwang-db reviewed Feb 11, 2021

View reviewed changes

add error message check

1cd1cb3

linhongliu-db reviewed Feb 18, 2021

View reviewed changes

maropu reviewed Feb 18, 2021

View reviewed changes

cloud-fan reviewed Feb 18, 2021

View reviewed changes

linhongliu-db reviewed Feb 18, 2021

View reviewed changes

split test cases

ce34b5e

cloud-fan reviewed Feb 18, 2021

View reviewed changes

cloud-fan approved these changes Feb 18, 2021

View reviewed changes

HyukjinKwon mentioned this pull request Feb 19, 2021

[SPARK-34384][CORE] Add missing docs for ResourceProfile APIs #31496

Closed

cloud-fan closed this in 27abb6a Feb 19, 2021

[SPARK-34421][SQL] Resolve temporary functions and views in views with CTEs #31550

[SPARK-34421][SQL] Resolve temporary functions and views in views with CTEs #31550

Conversation

peter-toth commented Feb 11, 2021 • edited Loading

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?

peter-toth commented Feb 11, 2021

SparkQA commented Feb 11, 2021

SparkQA commented Feb 11, 2021

gatorsmile commented Feb 11, 2021

SparkQA commented Feb 11, 2021

SparkQA commented Feb 11, 2021

SparkQA commented Feb 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Feb 11, 2021

peter-toth commented Feb 12, 2021

SparkQA commented Feb 12, 2021

SparkQA commented Feb 12, 2021

SparkQA commented Feb 12, 2021

HyukjinKwon commented Feb 13, 2021

HyukjinKwon commented Feb 13, 2021

HyukjinKwon commented Feb 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linhongliu-db left a comment

Choose a reason for hiding this comment

linhongliu-db commented Feb 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peter-toth Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peter-toth commented Feb 18, 2021 • edited Loading

linhongliu-db commented Feb 18, 2021

peter-toth commented Feb 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Feb 18, 2021

cloud-fan commented Feb 18, 2021

SparkQA commented Feb 18, 2021

peter-toth commented Feb 18, 2021

cloud-fan commented Feb 18, 2021

linhongliu-db commented Feb 18, 2021

cloud-fan commented Feb 19, 2021

cloud-fan commented Feb 19, 2021

cloud-fan commented Feb 19, 2021

peter-toth commented Feb 19, 2021

peter-toth commented Feb 19, 2021

linhongliu-db commented Feb 19, 2021 • edited Loading

cloud-fan commented Feb 19, 2021

peter-toth commented Feb 19, 2021 • edited Loading

peter-toth commented Feb 11, 2021 •

edited

Loading

peter-toth Feb 18, 2021 •

edited

Loading

peter-toth commented Feb 18, 2021 •

edited

Loading

linhongliu-db commented Feb 19, 2021 •

edited

Loading

peter-toth commented Feb 19, 2021 •

edited

Loading