[SPARK-32243][SQL]HiveSessionCatalog call super.makeFunctionExpression should throw earlier when got Spark UDAF Invalid arguments number error #29054

AngersZhuuuu · 2020-07-09T13:08:15Z

What changes were proposed in this pull request?

When we create a UDAF function use class extended UserDefinedAggregeteFunction, when we call the function, in support hive mode, in HiveSessionCatalog, it will call super.makeFunctionExpression,

but it will catch error such as the function need 2 parameter and we only give 1, throw exception only show

No handler for UDF/UDAF/UDTF xxxxxxxx

This is confused for develop , we should show error thrown by super method too,

For this pr's UT :
Before change, throw Exception like

No handler for UDF/UDAF/UDTF 'org.apache.spark.sql.hive.execution.LongProductSum'; line 1 pos 7

After this pr, throw exception

Spark UDAF Error: Invalid number of arguments for function longProductSum. Expected: 2; Found: 1;
Hive UDF/UDAF/UDTF Error: No handler for UDF/UDAF/UDTF 'org.apache.spark.sql.hive.execution.LongProductSum'; line 1 pos 7

Why are the changes needed?

Show more detail error message when define UDAF

Does this PR introduce any user-facing change?

People will see more detail error message when use spark sql's UDAF in hive support Mode

How was this patch tested?

Added UT

AngersZhuuuu · 2020-07-09T13:10:36Z

cc @dongjoon-hyun @maropu

SparkQA · 2020-07-09T13:14:16Z

Test build #125474 has started for PR 29054 at commit 88bf5d7.

AngersZhuuuu · 2020-07-10T01:31:57Z

retest this please

HyukjinKwon · 2020-07-10T03:32:45Z

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

 import org.apache.spark.unsafe.UnsafeAlignedOffset

-
-class ScalaAggregateFunction(schema: StructType) extends UserDefinedAggregateFunction {


@AngersZhuuuu why do we need to move classes? Let's separate refactoring and the fixes when possible.

@AngersZhuuuu why do we need to move classes? Let's separate refactoring and the fixes when possible.

Make a mistake, change back, thanks

HyukjinKwon · 2020-07-10T03:34:45Z

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

-          } else if (classOf[GenericUDTF].isAssignableFrom(clazz)) {
-            udfExpr = Some(HiveGenericUDTF(name, new HiveFunctionWrapper(clazz.getName), input))
-            udfExpr.get.asInstanceOf[HiveGenericUDTF].elementSchema // Force it to check data types.
+      Try(super.makeFunctionExpression(name, clazz, input)) match {


@AngersZhuuuu Seems we don't need to change getOrElse to match.

@AngersZhuuuu Seems we don't need to change getOrElse to match.

Confused, with getOrElse how can I get the exception

If so, how about using try instead of Try?

try is the way as in the guideline. But in general I would like to avoid changing the indentation for unrelated codes and make it difficult to see what's the real diff. Such pattern makes it more difficult to backport and revert. Let's avoid this @AngersZhuuuu next time. The actual diff seems just adding more message in the exception.

try is the way as in the guideline. But in general I would like to avoid changing the indentation for unrelated codes and make it difficult to see what's the real diff. Such pattern makes it more difficult to backport and revert. Let's avoid this @AngersZhuuuu next time. The actual diff seems just adding more message in the exception.

Since I always backport pr, I know the point you mentioned, I will notice this more carefully.
Yea, the actual diff is to show more message but since we need to know the exception in return Failure(exception), so we can't use getOrElse()

maropu · 2020-07-10T05:22:35Z

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

+    val functionClass = "org.apache.spark.sql.hive.execution.LongProductSum"
+    withUserDefinedFunction(functionName -> true) {
+      sql(s"CREATE TEMPORARY FUNCTION $functionName AS '$functionClass'")
+      val e1 = intercept[AnalysisException] {


nit: e1 -> e

nit: e1 -> e

Done

maropu · 2020-07-10T05:28:44Z

Please describe exception messages before/after this PR in the description.

SparkQA · 2020-07-10T06:00:27Z

Test build #125532 has finished for PR 29054 at commit 88bf5d7.

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

SparkQA · 2020-07-10T07:05:02Z

Test build #125550 has finished for PR 29054 at commit 5dd3169.

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

AngersZhuuuu · 2020-07-10T09:24:07Z

retest this please

AngersZhuuuu · 2020-07-10T09:24:15Z

Please describe exception messages before/after this PR in the description.

Done

maropu · 2020-07-10T10:25:11Z

Make Expression failed in SessionCatalog for function name = longProductSum: Invalid number of arguments for function longProductSum. Expected: 2; Found: 1;

No handler for UDF/UDAF/UDTF 'org.apache.spark.sql.hive.execution.LongProductSum'; line 1 pos 7

We need the number 1 and 2? How about reformatting it like this?

No handler for UDF/UDAF/UDTF 'org.apache.spark.sql.hive.execution.LongProductSum' because; Invalid number of arguments for function longProductSum

AngersZhuuuu · 2020-07-10T10:28:06Z

We need the number 1 and 2? How about reformatting it like this?

No handler for UDF/UDAF/UDTF 'org.apache.spark.sql.hive.execution.LongProductSum' because; Invalid number of arguments for function longProductSum

For me , I prefer more clear information let user know it's wrong form super.makeExpressionFunction. and

No handler for UDF/UDAF/UDTF 'org.apache.spark.sql.hive.execution.LongProductSum

don't have relation about

Invalid number of arguments for function longProductSum

SparkQA · 2020-07-10T10:33:35Z

Test build #125595 has finished for PR 29054 at commit 4e6b506.

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

HyukjinKwon · 2020-07-10T12:29:46Z

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

    }
  }
+
+  test("Hive mode use spark udaf should show error") {


Let's add a JIRA prefix.

Let's add a JIRA prefix.

Done

SparkQA · 2020-07-10T14:32:50Z

Test build #125592 has finished for PR 29054 at commit 5dd3169.

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

SparkQA · 2020-07-10T15:54:53Z

Test build #125601 has finished for PR 29054 at commit 91ceea0.

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

SparkQA · 2020-07-10T16:36:32Z

Test build #125620 has finished for PR 29054 at commit 8c3faed.

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

maropu · 2020-07-10T22:45:24Z

For me , I prefer more clear information let user know it's wrong form super.makeExpressionFunction. and

But, the relationship of 1. and 2. looks ambiguous to me. Does the message mean that there are two reasons for the failure? Could you brush up the message more?

AngersZhuuuu · 2020-07-10T23:35:07Z

But, the relationship of 1. and 2. looks ambiguous to me. Does the message mean that there are two reasons for the failure? Could you brush up the message more?

Yea, when we use Spark SQL in support hive mode, the real SessionCatalog is HiveSessionCatalog, when we create and use UDAF, it call makeExpressionFunction. In the code for UDAf, you can see that it will call super.makeExpressionFunction first, so it will check if the UDAF is follow spark's UDAF (class implement UserDefinedAggregateFunction), if failed, it will try hive's UDAF solution.

So in my test case, it's follow spark's UDAF definition, but makeExpressionFunction failed since when I use it, the argument number is wrong. In origin code, it will only show

No handler for UDF/UDAF/UDTF 'org.apache.spark.sql.hive.execution.LongProductSum'

But what we real need know is

Invalid number of arguments for function longProductSum

These two error is from different level 1. Spark UDAF 2. Hive UDAF.
1. 2. seems really ambiguous, how about For Spark UDAF: & For Hive UDAF to make it more clear

SparkQA · 2020-07-11T14:30:42Z

Test build #125681 has finished for PR 29054 at commit b813656.

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

AngersZhuuuu · 2020-07-15T02:24:01Z

cc @HyukjinKwon @maropu Any update?

HyukjinKwon · 2020-08-19T11:52:19Z

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

    // Current thread context classloader may not be the one loaded the class. Need to switch
    // context classloader to initialize instance properly.
    Utils.withContextClassLoader(clazz.getClassLoader) {
-      Try(super.makeFunctionExpression(name, clazz, input)).getOrElse {


@AngersZhuuuu, can you get rid of this unrelated diffs?

@AngersZhuuuu, can you get rid of this unrelated diffs?

How about current change, it won't change indentation. cc @maropu

SparkQA · 2020-09-29T11:23:22Z

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

SparkQA · 2020-09-29T12:39:07Z

Test build #129217 has finished for PR 29054 at commit 2db41fc.

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

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

cloud-fan · 2020-09-29T13:09:24Z

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

-        throw new AnalysisException(s"Invalid number of arguments for function $name. " +
-          s"Expected: ${e.inputTypes.size}; Found: ${input.size}")
+        throw new AnalysisException(s"Invalid number of arguments for " +
+          s"function $name. Expected: ${e.inputTypes.size}; Found: ${input.size}")


unnecessary change

unnecessary change

Done

cloud-fan · 2020-09-29T13:10:47Z

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

+          // If `super.makeFunctionExpression` throw `InvalidUDFClassException`, we construct
+          // Hive UDF/UDAF/UDTF with function definition.
+          makeHiveFunctionExpression(name, clazz, input)
+        case e: AnalysisException =>


nit: case e => throw e

The only exception is InvalidUDFClassException, where we need to try with Hive UDF class. For other exceptions, just re-throw.

nit: case e => throw e

The only exception is InvalidUDFClassException, where we need to try with Hive UDF class. For other exceptions, just re-throw.

Done

cloud-fan · 2020-09-29T13:11:04Z

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

      parser,
      functionResourceLoader) {

+  def makeHiveFunctionExpression(


private

Done

SparkQA · 2020-09-29T13:38:36Z

Test build #129240 has finished for PR 29054 at commit 36229e3.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-09-29T14:04:11Z

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

SparkQA · 2020-09-29T14:20:25Z

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

SparkQA · 2020-09-29T14:21:21Z

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

AngersZhuuuu · 2020-09-29T14:35:43Z

retest this please

SparkQA · 2020-09-29T14:39:46Z

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

AngersZhuuuu · 2020-09-29T15:25:19Z

retest this please

SparkQA · 2020-09-29T15:27:30Z

Test build #129231 has finished for PR 29054 at commit 3d9b6e3.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class InvalidUDFClassException protected[sql](message: String)

SparkQA · 2020-09-29T15:40:15Z

Test build #129244 has finished for PR 29054 at commit 36229e3.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-09-29T16:11:18Z

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

SparkQA · 2020-09-29T16:27:56Z

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

SparkQA · 2020-09-29T18:38:20Z

Test build #129239 has finished for PR 29054 at commit 42064d7.

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

cloud-fan · 2020-10-06T09:05:30Z

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

+ * Thrown when a query failed for invalid function class, usually because a SQL
+ * function's class does not follow the rules of the UDF/UDAF/UDTF class definition.
+ */
+class InvalidUDFClassException private[sql](message: String)


We don't need private[sql] as it's already in a private package.

cloud-fan · 2020-10-06T09:09:13Z

thanks, merging to master!

cloud-fan · 2020-10-06T14:17:22Z

Sorry I misread the test report. The last commit actually failed to compile.

Since the last commit is very minor and the second last commit passed all tests, I'm sending a followup PR to fix compilation instead of reverting it. #29955

Fix a mistake when merging #29054 Closes #29955 from cloud-fan/hot-fix. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

AngersZhuuuu · 2020-10-07T02:27:53Z

Sorry I misread the test report. The last commit actually failed to compile.

Since the last commit is very minor and the second last commit passed all tests, I'm sending a followup PR to fix compilation instead of reverting it. #29955

Yea, I miss this too...

probot-autolabeler bot added the SQL label Jul 9, 2020

HyukjinKwon reviewed Jul 10, 2020

View reviewed changes

save

5dd3169

AngersZhuuuu force-pushed the SPARK-32243 branch from 88bf5d7 to 5dd3169 Compare July 10, 2020 04:05

maropu reviewed Jul 10, 2020

View reviewed changes

Update HiveUDAFSuite.scala

4e6b506

Update HiveSessionCatalog.scala

91ceea0

HyukjinKwon reviewed Jul 10, 2020

View reviewed changes

Update HiveUDAFSuite.scala

8c3faed

Update HiveSessionCatalog.scala

b813656

HyukjinKwon reviewed Aug 19, 2020

View reviewed changes

follow comment

42064d7

cloud-fan reviewed Sep 29, 2020

View reviewed changes

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

cloud-fan reviewed Sep 29, 2020

View reviewed changes

follow comment

36229e3

cloud-fan approved these changes Sep 29, 2020

View reviewed changes

cloud-fan reviewed Oct 6, 2020

View reviewed changes

cloud-fan closed this in ddc7012 Oct 6, 2020

cloud-fan mentioned this pull request Oct 6, 2020

[SPARK-32243][SQL][followup] Fix compilation in HiveSessionCatalog #29955

Closed

		import org.apache.spark.unsafe.UnsafeAlignedOffset


		class ScalaAggregateFunction(schema: StructType) extends UserDefinedAggregateFunction {

[SPARK-32243][SQL]HiveSessionCatalog call super.makeFunctionExpression should throw earlier when got Spark UDAF Invalid arguments number error #29054

[SPARK-32243][SQL]HiveSessionCatalog call super.makeFunctionExpression should throw earlier when got Spark UDAF Invalid arguments number error #29054

Uh oh!

Conversation

AngersZhuuuu commented Jul 9, 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!

AngersZhuuuu commented Jul 9, 2020

Uh oh!

SparkQA commented Jul 9, 2020

Uh oh!

AngersZhuuuu commented Jul 10, 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!

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!

maropu commented Jul 10, 2020

Uh oh!

SparkQA commented Jul 10, 2020

Uh oh!

SparkQA commented Jul 10, 2020

Uh oh!

AngersZhuuuu commented Jul 10, 2020

Uh oh!

AngersZhuuuu commented Jul 10, 2020

Uh oh!

maropu commented Jul 10, 2020

Uh oh!

AngersZhuuuu commented Jul 10, 2020

Uh oh!

SparkQA commented Jul 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 10, 2020

Uh oh!

SparkQA commented Jul 10, 2020

Uh oh!

SparkQA commented Jul 10, 2020

Uh oh!

maropu commented Jul 10, 2020

Uh oh!

AngersZhuuuu commented Jul 10, 2020

Uh oh!

SparkQA commented Jul 11, 2020

Uh oh!

AngersZhuuuu commented Jul 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AngersZhuuuu Aug 21, 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!

SparkQA commented Sep 29, 2020

Uh oh!

SparkQA commented Sep 29, 2020

AngersZhuuuu commented Jul 9, 2020 •

edited

Loading

AngersZhuuuu Aug 21, 2020 •

edited

Loading