[SPARK-18217] [SQL] Disallow creating permanent views based on temporary views or UDFs #15764

gatorsmile · 2016-11-04T04:31:23Z

What changes were proposed in this pull request?

Based on the discussion in SPARK-18209. It doesn't really make sense to create permanent views based on temporary views or temporary UDFs.

To disallow the supports and issue the exceptions, this PR needs to detect whether a temporary view/UDF is being used when defining a permanent view. Basically, this PR can be split to two sub-tasks:

Task 1: detecting a temporary view from the query plan of view definition.
When finding an unresolved temporary view, Analyzer replaces it by a SubqueryAlias with the corresponding logical plan, which is stored in an in-memory HashMap. After replacement, it is impossible to detect whether the SubqueryAlias is added/generated from a temporary view. Thus, to detect the usage of a temporary view in view definition, this PR traverses the unresolved logical plan and uses the name of an UnresolvedRelation to detect whether it is a (global) temporary view.

Task 2: detecting a temporary UDF from the query plan of view definition.
Detecting usage of a temporary UDF in view definition is not straightfoward.

First, in the analyzed plan, we are having different forms to represent the functions. More importantly, some classes (e.g., HiveGenericUDF) are not accessible from CreateViewCommand, which is part of sql/core. Thus, we used the unanalyzed plan child of CreateViewCommand to detect the usage of a temporary UDF. Because the plan has already been successfully analyzed, we can assume the functions have been defined/registered.

Second, in Spark, the functions have four forms: Spark built-in functions, built-in hash functions, permanent UDFs and temporary UDFs. We do not have any direct way to determine whether a function is temporary or not. Thus, we introduced a function isTemporaryFunction in SessionCatalog. This function contains the detailed logics to determine whether a function is temporary or not.

How was this patch tested?

Added test cases.

rxin · 2016-11-04T05:06:38Z

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

+   * Returns whether it is a temporary function.
+   */
+  def isTempFunction(name: FunctionIdentifier): Boolean = {
+    // copied from HiveSessionCatalog


i'd update HiveSessionCatalog to say don't forget to update this place. Otherwise it will be inconsistent.

Will do. Thanks!

rxin · 2016-11-04T05:29:43Z

...alyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala

    child: LogicalPlan,
-    view: Option[TableIdentifier])
+    view: Option[TableIdentifier])(
+    val isGeneratedByTempTable: java.lang.Boolean = false)


is there a way to do it without introducing this?

Just updated the PR description. This might be the cleanest way. The reason is explained below.

When finding an unresolved temporary view, Analyzer replaces it by a SubqueryAlias with the corresponding logical plan, which is stored in an in-memory HashMap. After replacement, it is impossible to detect whether the SubqueryAlias is added/generated from a temporary view. Thus, to detect the usage of a temporary view in view definition, we added an extra flag isGeneratedByTempTable into SubqueryAlias. The flag is added into the curried arguments. Via this extra flag, we can easily detect the usage of temporary view from a logical plan traversal.

Also cc @cloud-fan and @liancheng Do you have any better solution?

Maybe we can do it from the unanalyzed plan, like what this PR did for temp functions.

rxin · 2016-11-04T06:18:37Z

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

+        case s: UnresolvedRelation
+            if sparkSession.sessionState.catalog.isTemporaryTable(s.tableIdentifier) =>
+          throw new AnalysisException(s"Not allowed to create a permanent view $name by " +
+            s"referencing a temp view ${s.tableIdentifier}. " +


temp -> temporary

rxin · 2016-11-04T06:18:58Z

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

+          case e: UnresolvedFunction
+              if sparkSession.sessionState.catalog.isTemporaryFunction(e.name) =>
+            throw new AnalysisException(s"Not allowed to create a permanent view $name by " +
+              s"referencing a temp function `${e.name}`. " +


temp -> temporary

rxin · 2016-11-04T06:19:14Z

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

+            if sparkSession.sessionState.catalog.isTemporaryTable(s.tableIdentifier) =>
+          throw new AnalysisException(s"Not allowed to create a permanent view $name by " +
+            s"referencing a temp view ${s.tableIdentifier}. " +
+            originalText.map(sql => s"""SQL: "$sql".""").getOrElse(""))


do we really need to show the entire sql? it's basically what the user just typed in

rxin · 2016-11-04T06:19:20Z

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

+              if sparkSession.sessionState.catalog.isTemporaryFunction(e.name) =>
+            throw new AnalysisException(s"Not allowed to create a permanent view $name by " +
+              s"referencing a temp function `${e.name}`. " +
+              originalText.map(sql => s"""SQL: "$sql".""").getOrElse(""))


same here do we really need to show the entire sql? it's basically what the user just typed in

rxin · 2016-11-04T06:21:16Z

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

    }

+    // When creating a permanent view, not allowed to reference temporary objects.
+    if (!isTemporary) {


also i'd move the check into a function, so it is more obvious what's going on with the main workflow.

rxin · 2016-11-04T06:21:46Z

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

+  /**
+   * Returns whether it is a temporary function.
+   */
+  def isTemporaryFunction(name: FunctionIdentifier): Boolean = {


add a unit test for this function?

also what's the behavior if the function doesn't exist? make sure you test it in the unit test.

Will resolve this tomorrow.

Like isTemporaryTable, we return false when the function/table does not exist

Yea please docuemnt it.

rxin · 2016-11-04T06:29:33Z

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

+    // When creating a permanent view, not allowed to reference temporary objects.
+    if (!isTemporary) {
+      child.collect {
+        // Disallow creating permanent views based on temporary views.


i'd also copy paste what you put in the pr description in here, on why you are traversing the unresolved plan.

SparkQA · 2016-11-04T06:58:25Z

Test build #68107 has finished for PR 15764 at commit 695110f.

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

SparkQA · 2016-11-04T07:17:24Z

Test build #68112 has finished for PR 15764 at commit 7100a8f.

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

rxin · 2016-11-04T07:55:28Z

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

    }
  }
+
+  test("create a permanent/temp view using a hive function") {


while it's good to have separate test cases, i find these three separate cases a bit too verbose. why not just put hive, built-in, and permanent into a single query?

Sure, will do it.

SparkQA · 2016-11-04T10:01:46Z

Test build #68116 has finished for PR 15764 at commit 86e7f9d.

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

SparkQA · 2016-11-04T19:14:21Z

Test build #68143 has finished for PR 15764 at commit a4df82b.

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

rxin · 2016-11-04T19:59:53Z

Looks good but I didn't read super carefully.

cc @hvanhovell and @cloud-fan

SparkQA · 2016-11-04T23:48:41Z

Test build #68161 has finished for PR 15764 at commit 1c3899f.

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

cloud-fan · 2016-11-05T06:52:43Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala

+    // Returns false when the function is permanent
+    assert(externalCatalog.listFunctions("db2", "*").toSet == Set("func1"))
+    assert(!sessionCatalog.isTemporaryFunction(FunctionIdentifier("func1", Some("db2"))))
+    assert(!sessionCatalog.isTemporaryFunction(FunctionIdentifier("db2.func1")))


it's not a permanent function right? it's a function called db2.func1 which doesn't exist

The related codes in lookupFunction looks confusing.

Here, you are right, functionRegistry does not have such a function. Let me remove it. Thanks!

cloud-fan · 2016-11-05T06:53:19Z

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

+    // without a database name, and is neither a built-in function nor a Hive function
+    name.database.isEmpty &&
+      functionRegistry.functionExists(name.funcName) &&
+      !FunctionRegistry.builtin.functionExists(name.funcName) &&


toLowerCase?

Our built-in function registry is using SimpleFunctionRegistry, which is based on a case insensitive string key hash map.

Thus, no need to add toLowerCase. However, we need to add a test case to ensure we can capture the potential change in this part.

Added a test case: https://github.com/gatorsmile/spark/blob/fec0066331aaf085f8cd0318be23844560e72b4b/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/SQLViewSuite.scala#L496

cloud-fan · 2016-11-05T06:54:31Z

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

+   * Permanent views are not allowed to reference temp objects, including temp function and views
+   */
+  private def verifyTemporaryObjectsNotExists(sparkSession: SparkSession): Unit = {
+    if (!isTemporary) {


is it possible the child is resolved? e.g. by DataFrame API

nvm, we can only create permanent view by SQL API

tejasapatil · 2016-11-06T17:41:51Z

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

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


I thought HiveSessionCatalog is used in context of Hive and SessionCatalog has nothing to do with Hive. So if we remove this from here and override isTemporaryFunction in HiveSessionCatalog, this would look clean. Feels like I am missing something obvious here.

It is true - but we are working towards getting rid of HiveSessionCatalog though (including getting rid of the 3 fallback functions), so in practice this will make no difference soon.

SparkQA · 2016-11-06T19:55:18Z

Test build #68247 has finished for PR 15764 at commit fec0066.

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

rxin · 2016-11-08T02:33:34Z

Thanks - merging in master/branch-2.1.

…ry views or UDFs ### What changes were proposed in this pull request? Based on the discussion in [SPARK-18209](https://issues.apache.org/jira/browse/SPARK-18209). It doesn't really make sense to create permanent views based on temporary views or temporary UDFs. To disallow the supports and issue the exceptions, this PR needs to detect whether a temporary view/UDF is being used when defining a permanent view. Basically, this PR can be split to two sub-tasks: **Task 1:** detecting a temporary view from the query plan of view definition. When finding an unresolved temporary view, Analyzer replaces it by a `SubqueryAlias` with the corresponding logical plan, which is stored in an in-memory HashMap. After replacement, it is impossible to detect whether the `SubqueryAlias` is added/generated from a temporary view. Thus, to detect the usage of a temporary view in view definition, this PR traverses the unresolved logical plan and uses the name of an `UnresolvedRelation` to detect whether it is a (global) temporary view. **Task 2:** detecting a temporary UDF from the query plan of view definition. Detecting usage of a temporary UDF in view definition is not straightfoward. First, in the analyzed plan, we are having different forms to represent the functions. More importantly, some classes (e.g., `HiveGenericUDF`) are not accessible from `CreateViewCommand`, which is part of `sql/core`. Thus, we used the unanalyzed plan `child` of `CreateViewCommand` to detect the usage of a temporary UDF. Because the plan has already been successfully analyzed, we can assume the functions have been defined/registered. Second, in Spark, the functions have four forms: Spark built-in functions, built-in hash functions, permanent UDFs and temporary UDFs. We do not have any direct way to determine whether a function is temporary or not. Thus, we introduced a function `isTemporaryFunction` in `SessionCatalog`. This function contains the detailed logics to determine whether a function is temporary or not. ### How was this patch tested? Added test cases. Author: gatorsmile <[email protected]> Closes #15764 from gatorsmile/blockTempFromPermViewCreation. (cherry picked from commit 1da64e1) Signed-off-by: Reynold Xin <[email protected]>

…detection after implementing it natively ## What changes were proposed in this pull request? In apache#15764 we added a mechanism to detect if a function is temporary or not. Hive functions are treated as non-temporary. Of the three hive functions, now "percentile" has been implemented natively, and "hash" has been removed. So we should update the list. ## How was this patch tested? Unit tests. Author: Shuai Lin <[email protected]> Closes apache#16049 from lins05/update-temp-function-detect-hive-list.

…detection after implementing it natively ## What changes were proposed in this pull request? In #15764 we added a mechanism to detect if a function is temporary or not. Hive functions are treated as non-temporary. Of the three hive functions, now "percentile" has been implemented natively, and "hash" has been removed. So we should update the list. ## How was this patch tested? Unit tests. Author: Shuai Lin <[email protected]> Closes #16049 from lins05/update-temp-function-detect-hive-list. (cherry picked from commit e64a204) Signed-off-by: gatorsmile <[email protected]>

…detection after implementing it natively ## What changes were proposed in this pull request? In apache#15764 we added a mechanism to detect if a function is temporary or not. Hive functions are treated as non-temporary. Of the three hive functions, now "percentile" has been implemented natively, and "hash" has been removed. So we should update the list. ## How was this patch tested? Unit tests. Author: Shuai Lin <[email protected]> Closes apache#16049 from lins05/update-temp-function-detect-hive-list.

…ry views or UDFs ### What changes were proposed in this pull request? Based on the discussion in [SPARK-18209](https://issues.apache.org/jira/browse/SPARK-18209). It doesn't really make sense to create permanent views based on temporary views or temporary UDFs. To disallow the supports and issue the exceptions, this PR needs to detect whether a temporary view/UDF is being used when defining a permanent view. Basically, this PR can be split to two sub-tasks: **Task 1:** detecting a temporary view from the query plan of view definition. When finding an unresolved temporary view, Analyzer replaces it by a `SubqueryAlias` with the corresponding logical plan, which is stored in an in-memory HashMap. After replacement, it is impossible to detect whether the `SubqueryAlias` is added/generated from a temporary view. Thus, to detect the usage of a temporary view in view definition, this PR traverses the unresolved logical plan and uses the name of an `UnresolvedRelation` to detect whether it is a (global) temporary view. **Task 2:** detecting a temporary UDF from the query plan of view definition. Detecting usage of a temporary UDF in view definition is not straightfoward. First, in the analyzed plan, we are having different forms to represent the functions. More importantly, some classes (e.g., `HiveGenericUDF`) are not accessible from `CreateViewCommand`, which is part of `sql/core`. Thus, we used the unanalyzed plan `child` of `CreateViewCommand` to detect the usage of a temporary UDF. Because the plan has already been successfully analyzed, we can assume the functions have been defined/registered. Second, in Spark, the functions have four forms: Spark built-in functions, built-in hash functions, permanent UDFs and temporary UDFs. We do not have any direct way to determine whether a function is temporary or not. Thus, we introduced a function `isTemporaryFunction` in `SessionCatalog`. This function contains the detailed logics to determine whether a function is temporary or not. ### How was this patch tested? Added test cases. Author: gatorsmile <[email protected]> Closes apache#15764 from gatorsmile/blockTempFromPermViewCreation.

…detection after implementing it natively ## What changes were proposed in this pull request? In apache#15764 we added a mechanism to detect if a function is temporary or not. Hive functions are treated as non-temporary. Of the three hive functions, now "percentile" has been implemented natively, and "hash" has been removed. So we should update the list. ## How was this patch tested? Unit tests. Author: Shuai Lin <[email protected]> Closes apache#16049 from lins05/update-temp-function-detect-hive-list.

…ry views or UDFs ### What changes were proposed in this pull request? PR #15764 disabled creating permanent views based on temporary views or UDFs. But AlterViewCommand didn't block temporary objects. ### Why are the changes needed? More robust view canonicalization. ### Does this PR introduce _any_ user-facing change? Yes, now if you alter a permanent view based on temporary views or UDFs, the operation will fail. ### How was this patch tested? Add new unit tests. Closes #33204 from jerqi/alter_view. Authored-by: RoryQi <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

…ry views or UDFs ### What changes were proposed in this pull request? PR #15764 disabled creating permanent views based on temporary views or UDFs. But AlterViewCommand didn't block temporary objects. ### Why are the changes needed? More robust view canonicalization. ### Does this PR introduce _any_ user-facing change? Yes, now if you alter a permanent view based on temporary views or UDFs, the operation will fail. ### How was this patch tested? Add new unit tests. Closes #33204 from jerqi/alter_view. Authored-by: RoryQi <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit e0c6b2e) Signed-off-by: Wenchen Fan <[email protected]>

gatorsmile added 3 commits November 3, 2016 19:42

fix.

509327e

fix2

1b430bb

update code comments

695110f

rxin reviewed Nov 4, 2016

View reviewed changes

gatorsmile added 2 commits November 3, 2016 23:12

address comments

4dbd3b6

fix style

7100a8f

rxin reviewed Nov 4, 2016

View reviewed changes

resolve comments.

86e7f9d

rxin reviewed Nov 4, 2016

View reviewed changes

resolve comments.

a4df82b

add comments.

1c3899f

cloud-fan reviewed Nov 5, 2016

View reviewed changes

address comments

fec0066

tejasapatil reviewed Nov 6, 2016

View reviewed changes

asfgit closed this in 1da64e1 Nov 8, 2016

lins05 mentioned this pull request Nov 29, 2016

[SPARK-16282][SQL] Follow-up: remove "percentile" from temp function detection after implementing it natively #16049

Closed

jerqi mentioned this pull request Jul 4, 2021

[SPARK-36011][SQL] Disallow altering permanent views based on temporary views or UDFs #33204

Closed

This was referenced Jul 6, 2021

[SPARK-36011][SQL][3.1] Disallow altering permanent views based on temporary views or UDFs #33230

Closed

[SPARK-36011][SQL][3.0] Disallow altering permanent views based on temporary views or UDFs #33234

Closed

[SPARK-18217] [SQL] Disallow creating permanent views based on temporary views or UDFs #15764

[SPARK-18217] [SQL] Disallow creating permanent views based on temporary views or UDFs #15764

Uh oh!

Conversation

gatorsmile commented Nov 4, 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!

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!

gatorsmile Nov 4, 2016 • 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!

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!

rxin Nov 4, 2016 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 4, 2016

Uh oh!

SparkQA commented Nov 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 4, 2016

Uh oh!

SparkQA commented Nov 4, 2016

Uh oh!

rxin commented Nov 4, 2016

Uh oh!

SparkQA commented Nov 4, 2016

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!

gatorsmile Nov 6, 2016 • 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!

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

gatorsmile commented Nov 4, 2016 •

edited

Loading

gatorsmile Nov 4, 2016 •

edited

Loading

rxin Nov 4, 2016 •

edited

Loading

gatorsmile Nov 6, 2016 •

edited

Loading