[SPARK-25768][SQL] fix constant argument expecting UDAFs #22766

peter-toth · 2018-10-18T13:55:17Z

What changes were proposed in this pull request?

Without this PR some UDAFs like GenericUDAFPercentileApprox can throw an exception because expecting a constant parameter (object inspector) as a particular argument.

The exception is thrown because toPrettySQL call in ResolveAliases analyzer rule transforms a Literal parameter to a PrettyAttribute which is then transformed to an ObjectInspector instead of a ConstantObjectInspector.
The exception comes from getEvaluator method of GenericUDAFPercentileApprox that actually shouldn't be called during toPrettySQL transformation. The reason why it is called are the non lazy fields in HiveUDAFFunction.

This PR makes all fields of HiveUDAFFunction lazy.

How was this patch tested?

added new UT

mgaido91

@peter-toth may you please elaborate a bit more the change in the PR description? What happened before the PR? Why? Thanks.

mgaido91 · 2018-10-18T14:07:25Z

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

    }
  }
+
+  test("constant argument expecting Hive UDF") {


may you please reference the JIRA?

mgaido91 · 2018-10-18T14:07:57Z

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

+  test("constant argument expecting Hive UDF") {
+    val testData = spark.range(10).toDF()
+    withTempView("inputTable") {
+      testData.createOrReplaceTempView("inputTable")


nit: we can move here spark.range(10)

Change-Id: If5a53df72c616d8b54662619426f5c8f34a9c0c0

peter-toth · 2018-10-18T15:10:29Z

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

    resolver.getEvaluator(parameterInfo)
  }
+
+  private case class Mode(evaluator: GenericUDAFEvaluator, objectInspector: ObjectInspector)


evaluator and object inspector need to be initialized together

mgaido91

the change makes sense to me. I have just one minor comment about naming.

cc @cloud-fan @dongjoon-hyun do you think we can trigger a build?

mgaido91 · 2018-10-18T15:12:09Z

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

    resolver.getEvaluator(parameterInfo)
  }
+
+  private case class Mode(evaluator: GenericUDAFEvaluator, objectInspector: ObjectInspector)


nit: maybe we can find a better name for this class

maybe just PartialEvaluator?

peter-toth · 2018-10-18T15:14:28Z

cc @cloud-fan I believe this is a regression because https://issues.apache.org/jira/browse/SPARK-18186

cloud-fan · 2018-10-18T16:22:51Z

OK to test

dongjoon-hyun · 2018-10-18T17:35:42Z

ok to test

SparkQA · 2018-10-18T17:40:34Z

Test build #97547 has finished for PR 22766 at commit 00115ff.

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

Change-Id: I7379b21c00336e389af9b5186fac71c566699c14

SparkQA · 2018-10-18T19:58:08Z

Test build #97549 has finished for PR 22766 at commit 844d0ba.

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

dongjoon-hyun · 2018-10-18T20:38:07Z

Retest this please.

SparkQA · 2018-10-18T23:36:36Z

Test build #97558 has finished for PR 22766 at commit 844d0ba.

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

cloud-fan · 2018-10-19T01:25:55Z

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

-    GenericUDAFEvaluator.Mode.PARTIAL1,
-    inputInspectors
-  )
+  private lazy val partial1Mode = {


partial1ModeEvaluator

cloud-fan · 2018-10-19T01:26:10Z

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

-    GenericUDAFEvaluator.Mode.FINAL,
-    Array(partialResultInspector)
-  )
+  private lazy val finalMode = {


ah it's also used in final mode, then maybe HiveEvaluator is a better name than PartialEvaluator

SparkQA · 2018-10-19T07:05:02Z

Test build #97592 has finished for PR 22766 at commit 6e6eca4.

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

mgaido91 · 2018-10-19T07:36:43Z

retest this please

mgaido91 · 2018-10-19T07:37:09Z

LGTM

SparkQA · 2018-10-19T10:00:21Z

Test build #97603 has finished for PR 22766 at commit 6e6eca4.

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

## What changes were proposed in this pull request? Without this PR some UDAFs like `GenericUDAFPercentileApprox` can throw an exception because expecting a constant parameter (object inspector) as a particular argument. The exception is thrown because `toPrettySQL` call in `ResolveAliases` analyzer rule transforms a `Literal` parameter to a `PrettyAttribute` which is then transformed to an `ObjectInspector` instead of a `ConstantObjectInspector`. The exception comes from `getEvaluator` method of `GenericUDAFPercentileApprox` that actually shouldn't be called during `toPrettySQL` transformation. The reason why it is called are the non lazy fields in `HiveUDAFFunction`. This PR makes all fields of `HiveUDAFFunction` lazy. ## How was this patch tested? added new UT Closes #22766 from peter-toth/SPARK-25768. Authored-by: Peter Toth <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit f38594f) Signed-off-by: Wenchen Fan <[email protected]>

cloud-fan · 2018-10-19T13:19:57Z

thanks, merging to master/2.4/2.3!

## What changes were proposed in this pull request? Without this PR some UDAFs like `GenericUDAFPercentileApprox` can throw an exception because expecting a constant parameter (object inspector) as a particular argument. The exception is thrown because `toPrettySQL` call in `ResolveAliases` analyzer rule transforms a `Literal` parameter to a `PrettyAttribute` which is then transformed to an `ObjectInspector` instead of a `ConstantObjectInspector`. The exception comes from `getEvaluator` method of `GenericUDAFPercentileApprox` that actually shouldn't be called during `toPrettySQL` transformation. The reason why it is called are the non lazy fields in `HiveUDAFFunction`. This PR makes all fields of `HiveUDAFFunction` lazy. ## How was this patch tested? added new UT Closes apache#22766 from peter-toth/SPARK-25768. Authored-by: Peter Toth <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

[SPARK-25768][SQL] fix constant argument expecting UDFs

0305497

mgaido91 reviewed Oct 18, 2018

View reviewed changes

fix review findings

00115ff

Change-Id: If5a53df72c616d8b54662619426f5c8f34a9c0c0

peter-toth commented Oct 18, 2018

View reviewed changes

mgaido91 reviewed Oct 18, 2018

View reviewed changes

fix review findings 2

844d0ba

Change-Id: I7379b21c00336e389af9b5186fac71c566699c14

cloud-fan reviewed Oct 19, 2018

View reviewed changes

fix review findings 3

6e6eca4

asfgit closed this in f38594f Oct 19, 2018

[SPARK-25768][SQL] fix constant argument expecting UDAFs #22766

[SPARK-25768][SQL] fix constant argument expecting UDAFs #22766

Uh oh!

Conversation

peter-toth commented Oct 18, 2018 • 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!

mgaido91 left a comment

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!

mgaido91 left a comment

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!

peter-toth commented Oct 18, 2018

Uh oh!

cloud-fan commented Oct 18, 2018

Uh oh!

dongjoon-hyun commented Oct 18, 2018

Uh oh!

SparkQA commented Oct 18, 2018

Uh oh!

SparkQA commented Oct 18, 2018

Uh oh!

dongjoon-hyun commented Oct 18, 2018

Uh oh!

SparkQA commented Oct 18, 2018

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 Oct 19, 2018

Uh oh!

mgaido91 commented Oct 19, 2018

Uh oh!

mgaido91 commented Oct 19, 2018

Uh oh!

SparkQA commented Oct 19, 2018

Uh oh!

cloud-fan commented Oct 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

peter-toth commented Oct 18, 2018 •

edited

Loading