[SPARK-12558][SQL][WIP] AnalysisException when multiple functions applied in GROUP BY clause #10520

dilipbiswal · 2015-12-30T03:57:30Z

@cloud-fan Can you please take a look ?

In this case, we are failing during check analysis while validating the aggregation expression. I have added a semanticEquals for HiveGenericUDF to fix this. Please let me know if this is the right way to address this issue.

nongli · 2015-12-30T05:35:10Z

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

@@ -233,6 +233,18 @@ private[hive] case class HiveGenericUDF(funcWrapper: HiveFunctionWrapper, childr
    udfType != null && udfType.deterministic()
  }

+  override def semanticEquals(other: Expression): Boolean = {


Can you comment why the version in Expression doesn't work? What about it is too strict?

@nongli Thanks a lot for your feedback. In this case , we have two instances of HiveGenericUDF one from the grouping expression and other from aggregation expression. While doing a semantic equality between them , we unwrap the case class and fall through to the last case statement which does equality between two HiveFunctionWrapper(s) and is failing.

@nongli @cloud-fan We could also override equals in HiveFunctionWrapper to just compare the functionClassName. Would this be a better option ? Please let me know.

I prefer to override HiveFunctionWrapper.equals, if the HiveFunctionWrapper.instance has no contribution to equality.

dilipbiswal · 2016-01-05T06:35:39Z

@cloud-fan Hi Wenchen, thanks for your comments. I went through the processing. I think its ok to just override equals and compare the udf class names here. The number of arguments, their semantic equality and deterministic property is handled in the semanticEquals method in expression.scala. Please let me know what you think.

cloud-fan · 2016-01-05T07:13:29Z

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

@@ -125,6 +125,12 @@ private[hive] object HiveShim {
    // for Serialization
    def this() = this(null)

+    override def equals(other: Any): Boolean = other match {
+      case a: HiveFunctionWrapper =>
+        functionClassName == a.functionClassName


cc @yhuai , Does same function name means exactly same function in hive?

@dilipbiswal I feel it is general fine to compare the class name (it is what we do in 1.5. In 1.5, instance is not a part of the constructor). We added instance when we added the support of hive's GenericUDFMacro. I guess we can add a check to see if GenericUDFMacro is the class name. If so, we also check instance. Otherwise, we just use the class name. Also, I guess you need to override hashCode as well.

@yhuai Thanks Yin. Will make the changes per your suggestion.

@yhuai Have addressed your comments. Can you please take a look to see if it looks ok ?

yhuai · 2016-01-05T23:44:37Z

Can you explain more why the semanticEquals at https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala#L152 fails?

dilipbiswal · 2016-01-06T00:15:29Z

@yhuai Hi Yin, Here is what i had while answering the question from Nong Li. Please let me know if you have more questions that i need to find the answers for.

In this case , we have two instances of HiveGenericUDF one from the grouping expression and other from aggregation expression. While doing a semantic equality between them , we unwrap the case class and fall through to the last case statement which does equality between two HiveFunctionWrapper(s) and is failing.

Let me know what you think.

yhuai · 2016-01-06T00:29:39Z

so, two HiveFunctionWrappers equality check failed because they have different instances?

dilipbiswal · 2016-01-06T00:31:59Z

@yhuai Yes Yin.

yhuai · 2016-01-06T05:29:23Z

@dilipbiswal Sorry. I do not think I understand the cause. We only set the instance at https://github.com/apache/spark/blob/master/sql/hive/src/main/scala/org/apache/spark/sql/hive/hiveUDFs.scala#L78 (for GenericUDFMacro)? In most of the case, it will be null. Looks like the equality check should not be the problem?

dilipbiswal · 2016-01-06T20:22:30Z

@yhuai Hi Yin, let me double check. As far as i remember, we come to the last case in expression.scala:semanticEqual with two HiveFunctionWrapper and was failing in equal. With a overriden equal it worked. Let me debug this further and get back...

dilipbiswal · 2016-01-06T20:25:39Z

@yhuai Hi Yin, i remember that in this reproduction, the instances were actually having values i.e they were not null. I will double check on this and get back.

dilipbiswal · 2016-01-06T22:05:07Z

@yhuai So the instance is authored through createFunction() while computing returnInspector.

https://github.com/apache/spark/blob/master/sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveShim.scala#L198-#L201

So we think we need to cache the instance in this case as its not a simpleUDF. Let me know your thoughts.

dilipbiswal · 2016-01-08T17:38:45Z

@yhuai Hello Yin, did you have any further insights on this ? Is it safe to override equal to ignore the instance while comparing to see if two functions are same ?

…OUP BY clause

yhuai · 2016-01-11T04:21:29Z

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

+          Objects.hashCode(functionClassName, instance)
+        } else {
+          functionClassName.hashCode()
+        }


Seems we need to correct the format?

SparkQA · 2016-01-12T11:15:35Z

Test build #2371 has finished for PR 10520 at commit b1e2d13.

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

dilipbiswal · 2016-01-13T00:43:17Z

@yhuai Hi Yin, Could you please look at the changes when you get a chance. It turns out that we can't compare the classname and instance for equality for GenericUDFMacro as the instances are different. I am instead comparing the underlying function that is held by GenericUDFMacro. Please let me know what you think.

yhuai · 2016-01-13T00:46:04Z

test this please

SparkQA · 2016-01-13T02:32:06Z

Test build #49275 has finished for PR 10520 at commit 41f6f02.

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

yhuai · 2016-01-13T05:39:13Z

Can you remove the WIP from the title?

yhuai · 2016-01-13T05:40:07Z

Actually, let me change the title while merging it.

yhuai · 2016-01-13T05:40:20Z

LGTM. Merging to master and branch 1.6.

…in GROUP BY clause cloud-fan Can you please take a look ? In this case, we are failing during check analysis while validating the aggregation expression. I have added a semanticEquals for HiveGenericUDF to fix this. Please let me know if this is the right way to address this issue. Author: Dilip Biswal <[email protected]> Closes #10520 from dilipbiswal/spark-12558. (cherry picked from commit dc7b387) Signed-off-by: Yin Huai <[email protected]> Conflicts: sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveShim.scala

yhuai · 2016-01-13T05:46:42Z

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

+  test("Hive UDF in group by") {
+    Seq(Tuple1(1451400761)).toDF("test_date").registerTempTable("tab1")
+    val count = sql("select date(cast(test_date as timestamp))" +
+      " from tab1 group by date(cast(test_date as timestamp))").count()


Actually it will be good to use withTempTable, which will automatically drop the temp table. Also, it will be more robust if we create a temp function based on hive's org.apache.hadoop.hive.ql.udf.generic.GenericUDFToDate. So, even if we later have a native function called date, we can still test the hive udf. We can do the test change in a follow-up pr (the pr can re-use the jira number).

I just realize it after I merge the PR. Let's have a pr to improve the test. Thanks!

dilipbiswal · 2016-01-13T05:46:58Z

@yhuai Thank you very much !!

dilipbiswal · 2016-01-13T05:48:13Z

@yhuai I will make the change in a follow up PR re-using the same JIRA.

yhuai · 2016-01-13T05:49:31Z

Great. Thank you!

…plied in GROUP BY clause Addresses the comments from Yin. #10520 Author: Dilip Biswal <[email protected]> Closes #10758 from dilipbiswal/spark-12558-followup.

…plied in GROUP BY clause Addresses the comments from Yin. #10520 Author: Dilip Biswal <[email protected]> Closes #10758 from dilipbiswal/spark-12558-followup. (cherry picked from commit db9a860) Signed-off-by: Yin Huai <[email protected]> Conflicts: sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveUDFSuite.scala

nongli reviewed Dec 30, 2015
View reviewed changes

cloud-fan reviewed Jan 5, 2016
View reviewed changes

dilipbiswal added 4 commits January 8, 2016 13:36

[SPARK-12558] AnalysisException when multiple functions applied in GR…

c9f2e92

…OUP BY clause

review comments

de81c21

override equals

7dae8e1

Yin's comment

24b32d5

dilipbiswal force-pushed the spark-12558 branch from 54eeec0 to 24b32d5 Compare January 10, 2016 23:37

yhuai reviewed Jan 11, 2016
View reviewed changes

Yin's comments

b1e2d13

style fix

41f6f02

asfgit closed this in dc7b387 Jan 13, 2016

yhuai reviewed Jan 13, 2016
View reviewed changes

dilipbiswal mentioned this pull request Jan 14, 2016

[SPARK-12558][FOLLOW-UP] AnalysisException when multiple functions applied in GROUP BY clause #10758

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-12558][SQL][WIP] AnalysisException when multiple functions applied in GROUP BY clause #10520

[SPARK-12558][SQL][WIP] AnalysisException when multiple functions applied in GROUP BY clause #10520

dilipbiswal commented Dec 30, 2015

nongli Dec 30, 2015

dilipbiswal Dec 30, 2015

dilipbiswal Dec 30, 2015

cloud-fan Dec 31, 2015

dilipbiswal commented Jan 5, 2016

cloud-fan Jan 5, 2016

yhuai Jan 8, 2016

dilipbiswal Jan 8, 2016

dilipbiswal Jan 11, 2016

yhuai commented Jan 5, 2016

dilipbiswal commented Jan 6, 2016

yhuai commented Jan 6, 2016

dilipbiswal commented Jan 6, 2016

yhuai commented Jan 6, 2016

dilipbiswal commented Jan 6, 2016

dilipbiswal commented Jan 6, 2016

dilipbiswal commented Jan 6, 2016

dilipbiswal commented Jan 8, 2016

yhuai Jan 11, 2016

SparkQA commented Jan 12, 2016

dilipbiswal commented Jan 13, 2016

yhuai commented Jan 13, 2016

SparkQA commented Jan 13, 2016

yhuai commented Jan 13, 2016

yhuai commented Jan 13, 2016

yhuai commented Jan 13, 2016

yhuai Jan 13, 2016

yhuai Jan 13, 2016

dilipbiswal commented Jan 13, 2016

dilipbiswal commented Jan 13, 2016

yhuai commented Jan 13, 2016

[SPARK-12558][SQL][WIP] AnalysisException when multiple functions applied in GROUP BY clause #10520

[SPARK-12558][SQL][WIP] AnalysisException when multiple functions applied in GROUP BY clause #10520

Conversation

dilipbiswal commented Dec 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dilipbiswal commented Jan 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yhuai commented Jan 5, 2016

dilipbiswal commented Jan 6, 2016

yhuai commented Jan 6, 2016

dilipbiswal commented Jan 6, 2016

yhuai commented Jan 6, 2016

dilipbiswal commented Jan 6, 2016

dilipbiswal commented Jan 6, 2016

dilipbiswal commented Jan 6, 2016

dilipbiswal commented Jan 8, 2016

Choose a reason for hiding this comment

SparkQA commented Jan 12, 2016

dilipbiswal commented Jan 13, 2016

yhuai commented Jan 13, 2016

SparkQA commented Jan 13, 2016

yhuai commented Jan 13, 2016

yhuai commented Jan 13, 2016

yhuai commented Jan 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dilipbiswal commented Jan 13, 2016

dilipbiswal commented Jan 13, 2016

yhuai commented Jan 13, 2016