[SPARK-30039][SQL] CREATE FUNCTION should do multi-catalog resolution #26890

planga82 · 2019-12-14T08:01:44Z

What changes were proposed in this pull request?

Add CreateFunctionStatement and make CREATE FUNCTION go through the same catalog/table resolution framework of v2 commands.

Why are the changes needed?

It's important to make all the commands have the same table resolution behavior, to avoid confusing
CREATE FUNCTION namespace.function

Does this PR introduce any user-facing change?

Yes. When running CREATE FUNCTION namespace.function Spark fails the command if the current catalog is set to a v2 catalog.

How was this patch tested?

Unit tests.

planga82 · 2019-12-14T08:02:22Z

cc: @cloud-fan @viirya @imback82 @dongjoon-hyun

dongjoon-hyun · 2019-12-14T21:20:09Z

ok to test.

dongjoon-hyun · 2019-12-15T00:04:02Z

sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala

+  }
+
+  private def parseSessionCatalogFunctionIdentifier(sql: String, catalog: CatalogPlugin,
+      functionIdent: Identifier): (Option[String], String) = {


- private def parseSessionCatalogFunctionIdentifier(sql: String, catalog: CatalogPlugin, + private def parseSessionCatalogFunctionIdentifier( + sql: String, + catalog: CatalogPlugin,

dongjoon-hyun · 2019-12-15T00:05:49Z

sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala

-          } else {
-            throw new AnalysisException ("SHOW FUNCTIONS is only supported in v1 catalog")
-          }
+          val (db, fn) =


~~Maybe, val (database, function) = for consistency.~~ Oh. Got it. It's used in the above.

dongjoon-hyun · 2019-12-15T00:27:20Z

sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala

-      }
-      DescribeFunctionCommand(functionIdentifier, extended)
+      val (database, function) =
+        parseSessionCatalogFunctionIdentifier("DESCRIBE FUNCTION", catalog, functionIdent)


Just a question. Is it hard to make SessionCatalogAndFunction?

It would be possible in the case that we would have v2 catalog functions to resolve it. But for now we need to handle an specific error for this case

throw new AnalysisException(s"$sql is only supported in v1 catalog")

If we don't handle it here, we find and unresolved exception that is not clear.
It is the same situation that we have with the commands that use parseV1Table function.

I think we can do it if we make the error message generic (Function command is only supported in v1 catalog.)? Either approach sounds reasonable to me, but +1 for using extractor.

I'm not sure what are you proposing exactly. We could have something like that:

case DescribeFunctionStatement(SessionCatalogAndFunction(catalog, functionIdent), extended) => ... case DropFunctionStatement(SessionCatalogAndFunction(catalog, functionIdent), ifExists, isTemp) => ... case CreateFunctionStatement(SessionCatalogAndFunction(catalog, functionIdent), className, resources, isTemp, ignoreIfExists, replace) => ...

where SessionCatalogAndFunction is an extractor that tests if isSessionCatalog, something like SessionCatalogAndTable do.
In the case that !isSessionCatalog it implies that any of this cases match and the result is
unresolved operator 'CreateFunctionStatement [testcat, ns1, ns2, fun] for Create function

How could we catch this case generically and throw a generic exception like you propose?

Ah OK. This requires throwing an exception inside the extractor, which may not be ideal.

SparkQA · 2019-12-15T01:28:54Z

Test build #115339 has finished for PR 26890 at commit 23920b2.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class CreateFunctionStatement(

SparkQA · 2019-12-15T13:14:16Z

Test build #115355 has finished for PR 26890 at commit c74974c.

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

imback82 · 2019-12-15T18:57:30Z

sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala

-      }
-      DescribeFunctionCommand(functionIdentifier, extended)
+      val (database, function) =
+        parseSessionCatalogFunctionIdentifier("DESCRIBE FUNCTION", catalog, functionIdent)


I think we can do it if we make the error message generic (Function command is only supported in v1 catalog.)? Either approach sounds reasonable to me, but +1 for using extractor.

imback82 · 2019-12-15T18:59:45Z

sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala

+        replace)
+  }
+
+  private def parseSessionCatalogFunctionIdentifier(


I would return FunctionIdentifier to match the function name, and also using FunctionIdentifier makes the intention / usage clear on the caller site.

oh, yes, it sounds good!

imback82 · 2019-12-16T00:06:33Z

sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala

+        parseSessionCatalogFunctionIdentifier("DROP FUNCTION", catalog, functionIdent)
+      DropFunctionCommand(database, function, ifExists, isTemp)
+
+    case CreateFunctionStatement(CatalogAndIdentifier(catalog, functionIdent),


I would use ident when you use CatalogAndIdentifier instead of functionIdent. (And use functionIdent when it references a FunctionIdentifier object).

SparkQA · 2019-12-16T01:23:52Z

Test build #115368 has finished for PR 26890 at commit 74275ff.

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

viirya · 2019-12-16T22:30:52Z

sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala

+        case Seq(db, fn) => FunctionIdentifier(fn, Some(db))
+        case Seq(fn) => FunctionIdentifier(fn, None)
+        case _ =>
+          throw new AnalysisException(s"Unsupported function name '${functionIdent.quoted}'")


This is not caused by this PR. This error message looks too general, and I do not see much info from it.

Maybe we can make it clearer? Like v1 function name should not have multiple namespaces?

In the show columns statement we decide this message

Namespace name should have only one part if specified

How about the same message or similar? Both are clearer

SparkQA · 2019-12-17T00:51:29Z

Test build #115409 has finished for PR 26890 at commit 707b037.

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

[SPARK-30039][SQL] Pull requests comments [SPARK-30039][SQL] Pull request comments [SPARK-30039][SQL] Pull request comments

SparkQA · 2020-01-06T19:22:14Z

Test build #116184 has finished for PR 26890 at commit 3796a3a.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class CreateFunctionStatement(

SparkQA · 2020-01-06T23:33:49Z

Test build #116185 has finished for PR 26890 at commit d9eb441.

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

planga82 · 2020-01-07T06:58:26Z

Hi @cloud-fan @viirya @imback82 @dongjoon-hyun! Any more comments on this?

cloud-fan · 2020-01-07T16:38:25Z

thanks, merging to master!

dongjoon-hyun added the SQL label Dec 14, 2019

dongjoon-hyun reviewed Dec 15, 2019

View reviewed changes

imback82 reviewed Dec 15, 2019

View reviewed changes

imback82 reviewed Dec 16, 2019

View reviewed changes

viirya reviewed Dec 16, 2019

View reviewed changes

[SPARK-30039][SQL] CREATE FUNCTION multicatalog resolution

3796a3a

[SPARK-30039][SQL] Pull requests comments [SPARK-30039][SQL] Pull request comments [SPARK-30039][SQL] Pull request comments

planga82 force-pushed the feature/SPARK-30039_CreateFunctionV2Command branch from 707b037 to 3796a3a Compare January 6, 2020 19:13

Fix scala style

d9eb441

cloud-fan closed this in 9479887 Jan 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-30039][SQL] CREATE FUNCTION should do multi-catalog resolution #26890

[SPARK-30039][SQL] CREATE FUNCTION should do multi-catalog resolution #26890

planga82 commented Dec 14, 2019

planga82 commented Dec 14, 2019

dongjoon-hyun commented Dec 14, 2019

dongjoon-hyun Dec 15, 2019

dongjoon-hyun Dec 15, 2019 •

edited

dongjoon-hyun Dec 15, 2019

planga82 Dec 15, 2019

imback82 Dec 15, 2019

planga82 Dec 15, 2019

imback82 Dec 16, 2019

SparkQA commented Dec 15, 2019

SparkQA commented Dec 15, 2019

imback82 Dec 15, 2019

imback82 Dec 15, 2019

planga82 Dec 15, 2019

imback82 Dec 16, 2019 •

edited

SparkQA commented Dec 16, 2019

viirya Dec 16, 2019

planga82 Dec 17, 2019 •

edited

SparkQA commented Dec 17, 2019

SparkQA commented Jan 6, 2020

SparkQA commented Jan 6, 2020

planga82 commented Jan 7, 2020

cloud-fan commented Jan 7, 2020

[SPARK-30039][SQL] CREATE FUNCTION should do multi-catalog resolution #26890

[SPARK-30039][SQL] CREATE FUNCTION should do multi-catalog resolution #26890

Conversation

planga82 commented Dec 14, 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?

planga82 commented Dec 14, 2019

dongjoon-hyun commented Dec 14, 2019

Choose a reason for hiding this comment

dongjoon-hyun Dec 15, 2019 • edited

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

SparkQA commented Dec 15, 2019

SparkQA commented Dec 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imback82 Dec 16, 2019 • edited

Choose a reason for hiding this comment

SparkQA commented Dec 16, 2019

Choose a reason for hiding this comment

planga82 Dec 17, 2019 • edited

Choose a reason for hiding this comment

SparkQA commented Dec 17, 2019

SparkQA commented Jan 6, 2020

SparkQA commented Jan 6, 2020

planga82 commented Jan 7, 2020

cloud-fan commented Jan 7, 2020

dongjoon-hyun Dec 15, 2019 •

edited

imback82 Dec 16, 2019 •

edited

planga82 Dec 17, 2019 •

edited