[SPARK-33692][SQL] View should use captured catalog and namespace to lookup function #30662

linhongliu-db · 2020-12-08T03:49:33Z

What changes were proposed in this pull request?

Using the view captured catalog and namespace to lookup function, so the view
referred functions won't be overridden by newly created function with the same name,
but different database or function type (i.e. temporary function)

Why are the changes needed?

bug fix, without this PR, changing database or create a temporary function with
the same name may cause failure when querying a view.

Does this PR introduce any user-facing change?

Yes, bug fix.

How was this patch tested?

newly added and existing test cases.

…lookup function

SparkQA · 2020-12-08T04:34:22Z

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

SparkQA · 2020-12-08T05:01:35Z

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

SparkQA · 2020-12-08T05:51:11Z

Test build #132405 has finished for PR 30662 at commit f3c2fd6.

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

SparkQA · 2020-12-08T07:27:12Z

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

SparkQA · 2020-12-08T07:51:33Z

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

SparkQA · 2020-12-08T08:58:26Z

Test build #132414 has finished for PR 30662 at commit c104cfe.

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

SparkQA · 2020-12-08T10:22:25Z

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

SparkQA · 2020-12-08T10:32:48Z

Test build #132423 has finished for PR 30662 at commit 00e32d6.

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

SparkQA · 2020-12-08T10:48:26Z

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

linhongliu-db · 2020-12-08T10:51:17Z

retest this please

SparkQA · 2020-12-08T11:46:10Z

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

SparkQA · 2020-12-08T12:14:05Z

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

SparkQA · 2020-12-08T14:19:33Z

Test build #132430 has finished for PR 30662 at commit 00e32d6.

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

linhongliu-db · 2020-12-08T15:22:39Z

retest this please

SparkQA · 2020-12-08T16:33:06Z

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

SparkQA · 2020-12-08T17:03:45Z

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

SparkQA · 2020-12-08T19:46:57Z

Test build #132436 has finished for PR 30662 at commit 00e32d6.

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

sql/hive/src/test/scala/org/apache/spark/sql/hive/UDFSuite.scala

SparkQA · 2020-12-09T03:20:34Z

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

SparkQA · 2020-12-09T03:47:11Z

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

SparkQA · 2020-12-09T06:29:48Z

Test build #132455 has finished for PR 30662 at commit ef7c6b2.

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

cloud-fan · 2020-12-09T06:39:00Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala

+      case Seq(_, db) => db
+      case Seq(catalog, namespace @ _*) =>
+        throw new AnalysisException(
+          s"Unsupported catalog ${catalog} and " +


we can just say: V2 catalog does not support functions yet

cloud-fan · 2020-12-09T06:39:27Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala

-      return functionRegistry.lookupFunction(name, children)
+      val isResolvingView = AnalysisContext.get.catalogAndNamespace.nonEmpty
+      // We lookup function without database in two cases:
+      // 1. the function is not a temporary function


the function is built-in

there are also external functions, in isTemporaryFunction:

name.database.isEmpty && functionRegistry.functionExists(name) && !FunctionRegistry.builtin.functionExists(name) && !hiveFunctions.contains(name.funcName.toLowerCase(Locale.ROOT))

cloud-fan · 2020-12-09T06:41:48Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveSQLViewSuite.scala

-            // temporary view
-            sql(s"CREATE TEMPORARY VIEW tempView1 AS SELECT $tempFunctionName(id) from tab1")
-            checkAnswer(sql("select count(*) FROM tempView1"), Row(10))
+            // TODO: temporary function support for temporary view with sql text stored will


nit: TODO (SPARK-XXX): support temporary functions in temp views

yeah, but there are also test failure in udf-inner-join.sql related to temp function in temp views. I changed to another way to find these test cases. could you check again?
https://github.com/apache/spark/pull/30662/files#diff-ed19f376a63eba52eea59ca71f3355d4495fad4fad4db9a3324aade0d4986a47R118

cloud-fan

LGTM except some minor comments

SparkQA · 2020-12-09T09:29:46Z

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

SparkQA · 2020-12-09T09:57:06Z

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

SparkQA · 2020-12-09T11:41:42Z

Test build #132476 has finished for PR 30662 at commit 11523f4.

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

cloud-fan · 2020-12-09T14:36:43Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

    relationCache: mutable.Map[Seq[String], LogicalPlan] = mutable.Map.empty,
-    referredTempViewNames: Seq[Seq[String]] = Seq.empty)
+    referredTempViewNames: Seq[Seq[String]] = Seq.empty,
+    isTempView: Boolean = false)


then shall we just use referredTempFuncNames?

if we change the type to referredTempFuncNames: Option[Seq[String]], then it can replace isTempView. Because Seq.empty can be permanent view or temp view refers non temp functions.

yea let's do it then. We can use isTempView: Boolean = false in the backport PR of 3.0/2.4.

you are correct in the first comment. we can just use referredTempFunctionNames. But this approach will fix both permanent and temp view problems, which I was planning to fix them separately. Anyway, I switch to use referredTempFunctionNames in the latest commit.

BTW, in 3.0/2.4, temp view stores analyzed plan, and won't be wrapped in View, so we don't need the isTempView flag.

SparkQA · 2020-12-10T03:35:17Z

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

SparkQA · 2020-12-10T03:47:03Z

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

SparkQA · 2020-12-10T03:47:04Z

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

SparkQA · 2020-12-10T03:48:56Z

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

SparkQA · 2020-12-10T03:59:53Z

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

SparkQA · 2020-12-10T04:17:31Z

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

cloud-fan · 2020-12-10T05:24:07Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala

+      case Seq(catalog, namespace @ _*) =>
+        throw new AnalysisException(
+          s"V2 catalog does not support functions yet. " +
+            s"catalog: ${catalog}, namespace '${namespace.mkString("[", ".", "]")}'")


nit: namespace.quoted. We need to import CatalogV2Implicits._ first.

SparkQA · 2020-12-10T05:31:50Z

Test build #132531 has finished for PR 30662 at commit a8c7722.

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

SparkQA · 2020-12-10T07:15:53Z

Test build #132532 has finished for PR 30662 at commit b30272c.

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

SparkQA · 2020-12-10T07:29:48Z

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

SparkQA · 2020-12-10T07:56:40Z

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

SparkQA · 2020-12-10T08:57:54Z

Test build #132546 has finished for PR 30662 at commit 632f3d6.

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

linhongliu-db · 2020-12-10T09:13:48Z

retest this please

cloud-fan · 2020-12-10T09:14:05Z

The failed 3 tests are the well known flaky tests from the thrift-server module. I'm merging it to master/3.1, thanks!

…lookup function ### What changes were proposed in this pull request? Using the view captured catalog and namespace to lookup function, so the view referred functions won't be overridden by newly created function with the same name, but different database or function type (i.e. temporary function) ### Why are the changes needed? bug fix, without this PR, changing database or create a temporary function with the same name may cause failure when querying a view. ### Does this PR introduce _any_ user-facing change? Yes, bug fix. ### How was this patch tested? newly added and existing test cases. Closes #30662 from linhongliu-db/SPARK-33692. Lead-authored-by: Linhong Liu <[email protected]> Co-authored-by: Linhong Liu <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 1554977) Signed-off-by: Wenchen Fan <[email protected]>

[SPARK-33692][SQL] View should use captured catalog and namespace to …

f3c2fd6

…lookup function

github-actions bot added the SQL label Dec 8, 2020

test fix

c104cfe

code clean

00e32d6

linhongliu-db mentioned this pull request Dec 8, 2020

[SPARK-33142][SPARK-33647][SQL][FOLLOW-UP] Add docs and test cases #30666

Closed

HyukjinKwon reviewed Dec 9, 2020

View reviewed changes

sql/hive/src/test/scala/org/apache/spark/sql/hive/UDFSuite.scala Outdated Show resolved Hide resolved

Update UDFSuite.scala

ef7c6b2

cloud-fan reviewed Dec 9, 2020

View reviewed changes

cloud-fan approved these changes Dec 9, 2020

View reviewed changes

linhongliu-db added 2 commits December 9, 2020 16:07

fix test cases in temp view

d87f249

remove code

11523f4

cloud-fan reviewed Dec 9, 2020

View reviewed changes

linhongliu-db added 3 commits December 10, 2020 10:45

another way

a8c7722

add test cases

ab440e3

code clean

b30272c

cloud-fan reviewed Dec 10, 2020

View reviewed changes

address comment

632f3d6

cloud-fan approved these changes Dec 10, 2020

View reviewed changes

cloud-fan closed this in 1554977 Dec 10, 2020

[SPARK-33692][SQL] View should use captured catalog and namespace to lookup function #30662

[SPARK-33692][SQL] View should use captured catalog and namespace to lookup function #30662

Uh oh!

Conversation

linhongliu-db commented Dec 8, 2020

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!

SparkQA commented Dec 8, 2020

Uh oh!

SparkQA commented Dec 8, 2020

Uh oh!

SparkQA commented Dec 8, 2020

Uh oh!

SparkQA commented Dec 8, 2020

Uh oh!

SparkQA commented Dec 8, 2020

Uh oh!

SparkQA commented Dec 8, 2020

Uh oh!

SparkQA commented Dec 8, 2020

Uh oh!

SparkQA commented Dec 8, 2020

Uh oh!

SparkQA commented Dec 8, 2020

Uh oh!

linhongliu-db commented Dec 8, 2020

Uh oh!

SparkQA commented Dec 8, 2020

Uh oh!

SparkQA commented Dec 8, 2020

Uh oh!

SparkQA commented Dec 8, 2020

Uh oh!

linhongliu-db commented Dec 8, 2020

Uh oh!

SparkQA commented Dec 8, 2020

Uh oh!

SparkQA commented Dec 8, 2020

Uh oh!

SparkQA commented Dec 8, 2020

Uh oh!

Uh oh!

SparkQA commented Dec 9, 2020

Uh oh!

SparkQA commented Dec 9, 2020

Uh oh!

SparkQA commented Dec 9, 2020

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!

cloud-fan left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 9, 2020

Uh oh!

SparkQA commented Dec 9, 2020

Uh oh!

SparkQA commented Dec 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

linhongliu-db Dec 9, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

linhongliu-db Dec 9, 2020 •

edited

Loading