[SPARK-31312][SQL] Cache Class instance for the UDF instance in HiveFunctionWrapper by HeartSaVioR · Pull Request #28079 · apache/spark

HeartSaVioR · 2020-03-31T08:12:04Z

What changes were proposed in this pull request?

This patch proposes to cache Class instance for the UDF instance in HiveFunctionWrapper to fix the case where Hive simple UDF is somehow transformed (expression is copied) and evaluated later with another classloader (for the case current thread context classloader is somehow changed). In this case, Spark throws CNFE as of now.

It's only occurred for Hive simple UDF, as HiveFunctionWrapper caches the UDF instance whereas it doesn't do for UDF type. The comment says Spark has to create instance every time for UDF, so we cannot simply do the same. This patch caches Class instance instead, and switch current thread context classloader to which loads the Class instance.

This patch extends the test boundary as well. We only tested with GenericUDTF for SPARK-26560, and this patch actually requires only UDF. But to avoid regression for other types as well, this patch adds all available types (UDF, GenericUDF, AbstractGenericUDAFResolver, UDAF, GenericUDTF) into the boundary of tests.

Credit to @cloud-fan as he discovered the problem and proposed the solution.

Why are the changes needed?

Above section describes why it's a bug and how it's fixed.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

New UTs added.

…ntext classloader after transformed

HeartSaVioR · 2020-03-31T08:13:07Z

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

    }
  }
-
-  test("SPARK-26560 Spark should be able to run Hive UDF using jar regardless of " +


This test is moved to HiveUDFDynamicLoadSuite - now it's being tested with 5 available Hive UDF types.

HeartSaVioR · 2020-03-31T08:15:49Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveShim.scala

+          clazz = Utils.getContextOrSparkClassLoader.loadClass(functionClassName)
+            .asInstanceOf[Class[_ <: AnyRef]]
+        }
+        val func = clazz.getConstructor().newInstance().asInstanceOf[UDFType]


If we add clazz = null below this line, the new UT (SPARK-31312) fails with UDF type (only one of 5 fails, because other cases this cases instance instead).

HeartSaVioR · 2020-03-31T08:17:40Z

cc. @cloud-fan @maropu

cloud-fan · 2020-03-31T09:04:37Z

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

+    val jarUrl = getHiveUDFTestJarUrl
+    test("SPARK-26560 Spark should be able to run Hive UDF using jar regardless of " +
+      s"current thread context classloader (${udfInfo.identifier}") {
+      testHiveUDFUsingJarWithChangingClassloader(


nit: if the method is only called once, can we inline it?

cloud-fan · 2020-03-31T09:04:42Z

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

+
+    test("SPARK-31312 Transformed Hive UDF using jar expression should not be failed to run " +
+      s"regardless of current thread context classloader (${udfInfo.identifier})") {
+      testHiveUDFUsingJarWithChangingClassloaderWithCopyUDFExpression(


cloud-fan · 2020-03-31T09:07:26Z

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

+        assert(Thread.currentThread().getContextClassLoader eq
+          spark.sqlContext.sharedState.jarClassLoader)
+
+        val udfExpr = fnCreateHiveUDFExpression()


I think this test should start here. The above test is the same as testHiveUDFUsingJarWithChangingClassloader.

HeartSaVioR · 2020-03-31T10:41:06Z

Thanks for the review comments. I've just consolidated two tests into one, and inlined. Please take a look again.

SparkQA · 2020-03-31T11:00:04Z

Test build #120634 has finished for PR 28079 at commit 5d8df3b.

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

cloud-fan · 2020-03-31T11:06:04Z

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

+  )
+
+  udfTestInfos.foreach { udfInfo =>
+    val jarUrl = getHiveUDFTestJarUrl


We can inline it as well

SparkQA · 2020-03-31T11:21:33Z

Test build #120641 has finished for PR 28079 at commit 7d34816.

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

SparkQA · 2020-03-31T14:10:52Z

Test build #120642 has finished for PR 28079 at commit 5159423.

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

…unctionWrapper ### What changes were proposed in this pull request? This patch proposes to cache Class instance for the UDF instance in HiveFunctionWrapper to fix the case where Hive simple UDF is somehow transformed (expression is copied) and evaluated later with another classloader (for the case current thread context classloader is somehow changed). In this case, Spark throws CNFE as of now. It's only occurred for Hive simple UDF, as HiveFunctionWrapper caches the UDF instance whereas it doesn't do for `UDF` type. The comment says Spark has to create instance every time for UDF, so we cannot simply do the same. This patch caches Class instance instead, and switch current thread context classloader to which loads the Class instance. This patch extends the test boundary as well. We only tested with GenericUDTF for SPARK-26560, and this patch actually requires only UDF. But to avoid regression for other types as well, this patch adds all available types (UDF, GenericUDF, AbstractGenericUDAFResolver, UDAF, GenericUDTF) into the boundary of tests. Credit to cloud-fan as he discovered the problem and proposed the solution. ### Why are the changes needed? Above section describes why it's a bug and how it's fixed. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? New UTs added. Closes #28079 from HeartSaVioR/SPARK-31312. Authored-by: Jungtaek Lim (HeartSaVioR) <kabhwan.opensource@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 2a6aa8e) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

cloud-fan · 2020-03-31T16:24:04Z

thanks, merging to master/3.0!

cloud-fan · 2020-03-31T16:25:14Z

@HeartSaVioR can you send a backport PR for 2.4? thanks!

…unctionWrapper This patch proposes to cache Class instance for the UDF instance in HiveFunctionWrapper to fix the case where Hive simple UDF is somehow transformed (expression is copied) and evaluated later with another classloader (for the case current thread context classloader is somehow changed). In this case, Spark throws CNFE as of now. It's only occurred for Hive simple UDF, as HiveFunctionWrapper caches the UDF instance whereas it doesn't do for `UDF` type. The comment says Spark has to create instance every time for UDF, so we cannot simply do the same. This patch caches Class instance instead, and switch current thread context classloader to which loads the Class instance. This patch extends the test boundary as well. We only tested with GenericUDTF for SPARK-26560, and this patch actually requires only UDF. But to avoid regression for other types as well, this patch adds all available types (UDF, GenericUDF, AbstractGenericUDAFResolver, UDAF, GenericUDTF) into the boundary of tests. Credit to cloud-fan as he discovered the problem and proposed the solution. Above section describes why it's a bug and how it's fixed. No. New UTs added. Closes apache#28079 from HeartSaVioR/SPARK-31312. Authored-by: Jungtaek Lim (HeartSaVioR) <kabhwan.opensource@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

HeartSaVioR · 2020-03-31T23:41:01Z

Thanks for the quick review and merge! #28086 is for branch-2.4.

maropu · 2020-04-01T00:06:38Z

late LGTM, thanks for the work, @HeartSaVioR !

HyukjinKwon · 2020-04-07T13:26:15Z

(very late) LGTM!

…unctionWrapper ### What changes were proposed in this pull request? This patch proposes to cache Class instance for the UDF instance in HiveFunctionWrapper to fix the case where Hive simple UDF is somehow transformed (expression is copied) and evaluated later with another classloader (for the case current thread context classloader is somehow changed). In this case, Spark throws CNFE as of now. It's only occurred for Hive simple UDF, as HiveFunctionWrapper caches the UDF instance whereas it doesn't do for `UDF` type. The comment says Spark has to create instance every time for UDF, so we cannot simply do the same. This patch caches Class instance instead, and switch current thread context classloader to which loads the Class instance. This patch extends the test boundary as well. We only tested with GenericUDTF for SPARK-26560, and this patch actually requires only UDF. But to avoid regression for other types as well, this patch adds all available types (UDF, GenericUDF, AbstractGenericUDAFResolver, UDAF, GenericUDTF) into the boundary of tests. Credit to cloud-fan as he discovered the problem and proposed the solution. ### Why are the changes needed? Above section describes why it's a bug and how it's fixed. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? New UTs added. Closes apache#28079 from HeartSaVioR/SPARK-31312. Authored-by: Jungtaek Lim (HeartSaVioR) <kabhwan.opensource@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

[SPARK-31312][SQL] Ensure Hive simple UDF can work with any thread co…

5d8df3b

…ntext classloader after transformed

HeartSaVioR commented Mar 31, 2020

View reviewed changes

HeartSaVioR mentioned this pull request Mar 31, 2020

[SPARK-26560][SQL] Spark should be able to run Hive UDF using jar regardless of current thread context classloader #27025

Closed

cloud-fan reviewed Mar 31, 2020

View reviewed changes

Reflect review comments

7d34816

cloud-fan reviewed Mar 31, 2020

View reviewed changes

cloud-fan approved these changes Mar 31, 2020

View reviewed changes

More inlining

5159423

cloud-fan closed this in 2a6aa8e Mar 31, 2020

HeartSaVioR deleted the SPARK-31312 branch March 31, 2020 23:41

Conversation

HeartSaVioR commented Mar 31, 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!

HeartSaVioR Mar 31, 2020

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR Mar 31, 2020

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR commented Mar 31, 2020

Uh oh!

cloud-fan Mar 31, 2020

Choose a reason for hiding this comment

Uh oh!

cloud-fan Mar 31, 2020

Choose a reason for hiding this comment

Uh oh!

cloud-fan Mar 31, 2020

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR commented Mar 31, 2020

Uh oh!

SparkQA commented Mar 31, 2020

Uh oh!

cloud-fan Mar 31, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 31, 2020

Uh oh!

SparkQA commented Mar 31, 2020

Uh oh!

cloud-fan commented Mar 31, 2020

Uh oh!

cloud-fan commented Mar 31, 2020

Uh oh!

HeartSaVioR commented Mar 31, 2020

Uh oh!

maropu commented Apr 1, 2020

Uh oh!

HyukjinKwon commented Apr 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments