[SPARK-21533][SQL] Print warning messages when override function `configure` found in Hive UDFs by maropu · Pull Request #18768 · apache/spark

maropu · 2017-07-29T03:14:28Z

What changes were proposed in this pull request?

This pr added code to print warning messages when an unsupported function public void configure(MapredContext mapredContext) used in GenericUDF and GenericUDF. This is because Spark does not call configure internally and this is error-prone.

How was this patch tested?

Manually checked. If this pr applied, you hit a warning message below;

scala> sql("CREATE TEMPORARY FUNCTION test AS 'test.TestUDTF'")
17/07/29 11:57:08 WARN HiveShim: Found an overridden method `configure` in TestUDTF, but Spark does not call the method during initialization because Spark does not use MapredContext inside (See SPARK-21533). So, you might reconsider the implementation of TestUDTF.

maropu · 2017-07-29T03:15:12Z

@gatorsmile @hvanhovell What do you think this? Thanks!

SparkQA · 2017-07-29T03:18:51Z

Test build #80035 has finished for PR 18768 at commit ff1b88b.

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

SparkQA · 2017-07-29T06:06:39Z

Test build #80037 has finished for PR 18768 at commit 1752f70.

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

SparkQA · 2017-07-29T08:52:09Z

Test build #80038 has finished for PR 18768 at commit 2d586a8.

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

maropu · 2017-07-29T09:03:31Z

Jenkins, retest this please.

viirya · 2017-07-29T09:58:17Z

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

+  }
+
+  def validateHiveUserDefinedFunction(udfClass: Class[_]): Unit = {
+    if (hasInheritanceOf[GenericUDF]("configure", udfClass) ||


When GenericUDF API has configure method? Seems GenericUDF at 0.10.0 has no such method?

https://hive.apache.org/javadocs/r0.10.0/api/org/apache/hadoop/hive/ql/udf/generic/GenericUDF.html

I checked it seemed this API had been implemented in v0.11.0 (https://github.com/apache/hive/blob/release-0.11.0/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDF.java). This is the commit for this API.

If ran with a Hive version that configure is not implemented yet, is hasInheritanceOf safe from NoSuchMethodException ?

IIUC spark always refers to hive-exec-1.2.1.spark2.jar , so it seems we have no chance to get the exception. But, I think it is not a bad idea to catch NoSuchMethodException there for understandability.

Oh, that's right. As you said, it is still good to catch the exception.

SparkQA · 2017-07-29T10:57:08Z

Test build #80040 has finished for PR 18768 at commit 2d586a8.

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

viirya · 2017-07-30T13:31:44Z

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

  }

+  private def isSubClassOf(t: Type, parent: Class[_]): Boolean = t match {
+    case cls: Class[_] => parent.isAssignableFrom(cls)


Will you pass in something not a Class[_]? If not, we can simply inline isAssignableFrom check into hasInheritanceOf. Then we don't need to have Type.

viirya · 2017-07-30T14:01:41Z

Btw, is it possibly to add an unit test for it?

SparkQA · 2017-07-30T14:11:45Z

Test build #80052 has finished for PR 18768 at commit c9b6080.

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

SparkQA · 2017-07-30T15:53:57Z

Test build #80053 has finished for PR 18768 at commit 34238ff.

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

gatorsmile · 2017-07-30T17:39:39Z

Is this the only function we miss for different types of Hive UD(A/T)Fs?

gatorsmile · 2017-07-30T17:40:56Z

Could we document this limit in the https://spark.apache.org/docs/latest/sql-programming-guide.html#unsupported-hive-functionality?

gatorsmile · 2017-07-30T17:50:22Z

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

   * Construct a [[FunctionBuilder]] based on the provided class that represents a function.
   */
  private def makeFunctionBuilder(name: String, clazz: Class[_]): FunctionBuilder = {
+    validateHiveUserDefinedFunction(clazz)


I am neural to introduce a warning message for this case. Not sure how helpful the warning message will be.

ok. So, I think it's ok to revisit this again if we have more reports from users.

maropu · 2017-07-31T04:52:59Z

@viirya I've no idea about a way to add tests for this kind of cases and I think the previous prs similar to this case (just add code for warning) also had no test. But, if we could, we'd better to do...

@gatorsmile I checked these Hive classes and I found some other functions that Spark currently ignores... (e.g., GenericUDF.getRequiredJars, GenericUDF.close...). Also, Spark currently invokes the deprecated initialize interface only in GenericUDTF, so users get stuck if they use the non-deprecated initialize interface. (See: #18527). As you suggested above, it seems we better leave some documents about these unsupported behaviours.

gatorsmile · 2017-07-31T04:59:09Z

Yes! Thanks for your efforts! Please update the documentation at first.

maropu · 2017-07-31T05:01:34Z

ok, I'll make a pr later. Thanks!

maropu · 2017-08-01T06:02:08Z

I think the document about these unsupported functions is enough for users, so I'll close this for now. If we have more response from users, we could revisit this again. Thanks.

## What changes were proposed in this pull request? This pr added documents about unsupported functions in Hive UDF/UDTF/UDAF. This pr relates to apache#18768 and apache#18527. ## How was this patch tested? N/A Author: Takeshi Yamamuro <yamamuro@apache.org> Closes apache#18792 from maropu/HOTFIX-20170731.

maropu force-pushed the SPARK-21533 branch from ff1b88b to 1752f70 Compare July 29, 2017 04:49

Print warning messages when override function configure found

2d586a8

maropu force-pushed the SPARK-21533 branch from 1752f70 to 2d586a8 Compare July 29, 2017 07:26

viirya reviewed Jul 29, 2017

View reviewed changes

Apply viirya's comment

c9b6080

viirya reviewed Jul 30, 2017

View reviewed changes

Apply more comment

34238ff

gatorsmile reviewed Jul 30, 2017

View reviewed changes

maropu mentioned this pull request Aug 1, 2017

[SPARK-21589][SQL][DOC] Add documents about Hive UDF/UDTF/UDAF #18792

Closed

maropu closed this Aug 1, 2017

Conversation

maropu commented Jul 29, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

maropu commented Jul 29, 2017

Uh oh!

SparkQA commented Jul 29, 2017

Uh oh!

SparkQA commented Jul 29, 2017

Uh oh!

SparkQA commented Jul 29, 2017

Uh oh!

maropu commented Jul 29, 2017

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 Jul 29, 2017

Uh oh!

viirya Jul 30, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

viirya commented Jul 30, 2017

Uh oh!

SparkQA commented Jul 30, 2017

Uh oh!

SparkQA commented Jul 30, 2017

Uh oh!

gatorsmile commented Jul 30, 2017

Uh oh!

gatorsmile commented Jul 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maropu commented Jul 31, 2017

Uh oh!

gatorsmile commented Jul 31, 2017

Uh oh!

maropu commented Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maropu commented Aug 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

viirya Jul 30, 2017 •

edited

Loading

maropu commented Jul 31, 2017 •

edited

Loading