[SPARK-32677][SQL] Load function resource before create #29502

ulysses-you · 2020-08-21T06:38:28Z

What changes were proposed in this pull request?

Change CreateFunctionCommand code that add class check before create function.

Why are the changes needed?

We have different behavior between create permanent function and temporary function when function class is invaild. e.g.,

create function f as 'test.non.exists.udf';
-- Time taken: 0.104 seconds

create temporary function f as 'test.non.exists.udf'
-- Error in query: Can not load class 'test.non.exists.udf' when registering the function 'f', please make sure it is on the classpath;

And Hive also fails both of them.

Does this PR introduce any user-facing change?

Yes, user will get exception when create a invalid udf.

How was this patch tested?

New test.

SparkQA · 2020-08-21T07:05:02Z

Test build #127726 has finished for PR 29502 at commit ce83487.

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

SparkQA · 2020-08-21T14:26:06Z

Test build #127740 has finished for PR 29502 at commit def8c4e.

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

ulysses-you · 2020-08-22T11:53:42Z

This action seems break many things, update title to WIP.

ulysses-you · 2020-08-30T11:13:32Z

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


          sql(s"CREATE FUNCTION ${udfInfo.funcName} AS '${udfInfo.className}' USING JAR '$jarUrl'")

-          assert(Thread.currentThread().getContextClassLoader eq sparkClassLoader)


We need load resource during create function.

This test was created in #27025.

SparkQA · 2020-08-30T12:24:01Z

Test build #128043 has finished for PR 29502 at commit 3e6320d.

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

SparkQA · 2020-08-30T13:16:03Z

Test build #128044 has finished for PR 29502 at commit ba248c9.

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

SparkQA · 2020-08-30T14:50:33Z

Test build #128047 has finished for PR 29502 at commit 9fb3c13.

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

SparkQA · 2020-08-31T03:38:03Z

Test build #128058 has finished for PR 29502 at commit fe98b71.

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

SparkQA · 2020-08-31T04:28:19Z

Test build #128060 has finished for PR 29502 at commit 8b3a509.

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

SparkQA · 2020-08-31T07:05:01Z

Test build #128071 has finished for PR 29502 at commit fe77698.

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

SparkQA · 2020-08-31T15:00:29Z

Test build #128090 has finished for PR 29502 at commit 4750e2e.

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

SparkQA · 2020-08-31T18:20:05Z

Test build #128100 has finished for PR 29502 at commit 882942f.

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

SparkQA · 2020-09-01T00:23:31Z

Test build #128122 has finished for PR 29502 at commit d55b8bc.

This patch fails Python style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-09-01T05:27:38Z

Test build #128123 has finished for PR 29502 at commit 75e61e9.

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

ulysses-you · 2020-09-01T06:12:21Z

It's ok to review. cc @cloud-fan

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

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

SparkQA · 2020-09-03T05:17:45Z

Test build #128217 has finished for PR 29502 at commit bb6d742.

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

cloud-fan · 2020-09-03T05:26:36Z

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

+    val className = funcDefinition.className
+    if (!Utils.classIsLoadable(className)) {
+      throw new AnalysisException(s"Can not load class '$className' when registering " +
+        s"the function '${funcDefinition.identifier}', please make sure it is on the classpath")


how about funcDefinition.identifier.unquotedString?

sounds good.

FunctionIdentifier already override toString = unquotedString, but it's fine to invoke unquotedString explicit.

cloud-fan · 2020-09-03T05:27:26Z

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

    val func = CatalogFunction(FunctionIdentifier(functionName, databaseName), className, resources)
+    catalog.loadFunctionResources(resources)
+    // We fail fast if function class is not exists.
+    catalog.requireFunctionClassExists(func)


I think we only need to do this for non-temp functions.

Yeah, I also think about it.

cloud-fan · 2020-09-03T06:09:58Z

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

+    val className = funcDefinition.className
+    if (!Utils.classIsLoadable(className)) {
+      throw new AnalysisException(s"Can not load class '$className' when registering " +
+        s"the function '${funcDefinition.identifier.unquotedString}', please make sure " +


one last thing: shall we fill the default database before putting the function identifier in the error message?

Not needed.

This method is used by both temporary and permanent function. The temporary has no database name and we can't fill the database name. The permanent follows user created.

SparkQA · 2020-09-03T07:05:02Z

Test build #128230 has finished for PR 29502 at commit 6584f39.

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

cloud-fan · 2020-09-03T08:46:04Z

sql/core/src/test/resources/sql-tests/results/udf/udf-udaf.sql.out

 struct<>
 -- !query output
 org.apache.spark.sql.AnalysisException
-Can not load class 'test.non.existent.udaf' when registering the function 'default.udaf1', please make sure it is on the classpath; line 1 pos 94


Here it was 'default.udaf1' previously. But the new message is 'udaf1': https://github.com/apache/spark/pull/29502/files#diff-2d3d6b47e6044b4d44590c5a73b7cd8bR46

Do you know why?

For permanent, the old code path is:

create function // no class check query on function -> lookup function -> fill database name if permanent -> register function -> check class

So the previously msg always contains database name.

Now the code path is:

create function -> check class

then shall we fill in the default database when creating permanent functions, to make the error message better?

Permanent function always need a database, it's ok to fill database name when creating.

SparkQA · 2020-09-05T16:55:05Z

Test build #128320 has finished for PR 29502 at commit 3cf971b.

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

cloud-fan · 2020-09-07T06:00:22Z

thanks, merging to master!

ulysses-you · 2020-09-07T10:20:59Z

thanks for merging !

cloud-fan · 2020-09-09T14:53:36Z

Sorry I have to revert it. This breaks a use case that: the udf class is not accessible in the cluster that creates the function, but people use this function in another cluster that can access the udf class.

I'll open a PR to write the use case down into a comment, so that we won't forget.

ulysses-you · 2020-09-10T00:07:17Z

Never mind.

That said, there are two Spark cluster(A, B) and one Hive metastore then create function with Spark A but using Spark B's local path. Seems strange but it actual exists.

…nCommand ### What changes were proposed in this pull request? We made a mistake in #29502, as there is no code comment to explain why we can't load the UDF class when creating functions. This PR improves the code comment. ### Why are the changes needed? To avoid making the same mistake. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? N/A Closes #29713 from cloud-fan/comment. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Takeshi Yamamuro <[email protected]>

…nCommand ### What changes were proposed in this pull request? We made a mistake in #29502, as there is no code comment to explain why we can't load the UDF class when creating functions. This PR improves the code comment. ### Why are the changes needed? To avoid making the same mistake. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? N/A Closes #29713 from cloud-fan/comment. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Takeshi Yamamuro <[email protected]> (cherry picked from commit 328d81a) Signed-off-by: Takeshi Yamamuro <[email protected]>

…nCommand ### What changes were proposed in this pull request? We made a mistake in apache#29502, as there is no code comment to explain why we can't load the UDF class when creating functions. This PR improves the code comment. ### Why are the changes needed? To avoid making the same mistake. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? N/A Closes apache#29713 from cloud-fan/comment. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Takeshi Yamamuro <[email protected]> (cherry picked from commit 328d81a) Signed-off-by: Takeshi Yamamuro <[email protected]>

ulysses-you added 2 commits August 21, 2020 14:32

init

9a81c65

test

ce83487

probot-autolabeler bot added the SQL label Aug 21, 2020

ulysses-you added 3 commits August 21, 2020 21:06

udaf

c75cff4

do not register

def8c4e

revert comment

97dcadf

udaf

3e6320d

ulysses-you changed the title ~~[SPARK-32677][SQL] Cache function directly after create~~ [WIP][SPARK-32677][SQL] Cache function directly after create Aug 30, 2020

HiveUDFDynamicLoadSuite

ba248c9

ulysses-you commented Aug 30, 2020

View reviewed changes

ulysses-you changed the title ~~[WIP][SPARK-32677][SQL] Cache function directly after create~~ [WIP][SPARK-32677][SQL] Load function resource before create Aug 30, 2020

fix

9fb3c13

ulysses-you added 2 commits August 31, 2020 08:17

udf-udaf

fe98b71

Merge branch 'master' of https://github.com/apache/spark into function

8b3a509

qualified name

fe77698

ulysses-you added 2 commits August 31, 2020 18:30

fix

4750e2e

fix

882942f

fix py

d55b8bc

probot-autolabeler bot added the PYTHON label Sep 1, 2020

style

75e61e9

ulysses-you changed the title ~~[WIP][SPARK-32677][SQL] Load function resource before create~~ [SPARK-32677][SQL] Load function resource before create Sep 1, 2020

cloud-fan reviewed Sep 2, 2020

View reviewed changes

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

cloud-fan reviewed Sep 2, 2020

View reviewed changes

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

fix

bb6d742

cloud-fan reviewed Sep 3, 2020

View reviewed changes

ulysses-you added 2 commits September 3, 2020 13:51

only check permanent

438291b

error msg

6584f39

cloud-fan reviewed Sep 3, 2020

View reviewed changes

ulysses-you added 2 commits September 5, 2020 20:20

fill database name

701c25d

fix ut

3cf971b

cloud-fan approved these changes Sep 7, 2020

View reviewed changes

cloud-fan closed this in 05fcf26 Sep 7, 2020

cloud-fan mentioned this pull request Sep 10, 2020

[SPARK-32677][SQL][DOCS][MINOR] Improve code comment in CreateFunctionCommand #29713

Closed


		sql(s"CREATE FUNCTION ${udfInfo.funcName} AS '${udfInfo.className}' USING JAR '$jarUrl'")

		assert(Thread.currentThread().getContextClassLoader eq sparkClassLoader)

[SPARK-32677][SQL] Load function resource before create #29502

[SPARK-32677][SQL] Load function resource before create #29502

Uh oh!

Conversation

ulysses-you commented Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 Aug 21, 2020

Uh oh!

SparkQA commented Aug 21, 2020

Uh oh!

ulysses-you commented Aug 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 30, 2020

Uh oh!

SparkQA commented Aug 30, 2020

Uh oh!

SparkQA commented Aug 30, 2020

Uh oh!

SparkQA commented Aug 31, 2020

Uh oh!

SparkQA commented Aug 31, 2020

Uh oh!

SparkQA commented Aug 31, 2020

Uh oh!

SparkQA commented Aug 31, 2020

Uh oh!

SparkQA commented Aug 31, 2020

Uh oh!

SparkQA commented Sep 1, 2020

Uh oh!

SparkQA commented Sep 1, 2020

Uh oh!

ulysses-you commented Sep 1, 2020

Uh oh!

Uh oh!

Uh oh!

SparkQA commented Sep 3, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 3, 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!

SparkQA commented Sep 5, 2020

Uh oh!

cloud-fan commented Sep 7, 2020

Uh oh!

ulysses-you commented Sep 7, 2020

Uh oh!

cloud-fan commented Sep 9, 2020

Uh oh!

ulysses-you commented Sep 10, 2020

ulysses-you commented Aug 21, 2020 •

edited

Loading