[SPARK-3891][SQL] Add array support to percentile, percentile_approx and constant inspectors support #2802

gvramana · 2014-10-14T14:49:35Z

Supported passing array to percentile and percentile_approx UDAFs
To support percentile_approx, constant inspectors are supported for GenericUDAF
Constant folding support added to CreateArray expression
Avoided constant udf expression re-evaluation

Author: Venkata Ramana G ramana.gollamudi@huawei.com

AmplabJenkins · 2014-10-14T14:52:10Z

Can one of the admins verify this patch?

AmplabJenkins · 2014-10-21T23:14:40Z

Can one of the admins verify this patch?

liancheng · 2014-10-22T08:30:07Z

test this please

SparkQA · 2014-10-25T08:08:34Z

Test build #447 has started for PR 2802 at commit f425daf.

This patch merges cleanly.

SparkQA · 2014-10-25T09:35:50Z

Test build #447 has finished for PR 2802 at commit f425daf.

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

gvramana · 2014-11-03T10:18:45Z

Reworked on this over the changes of #2762. Updated the pull request comment.
Please test the same

liancheng · 2014-11-03T11:15:33Z

test this please

SparkQA · 2014-11-03T11:19:48Z

Test build #22810 has started for PR 2802 at commit 2a59843.

This patch merges cleanly.

SparkQA · 2014-11-03T11:54:57Z

Test build #22810 has finished for PR 2802 at commit 2a59843.

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

AmplabJenkins · 2014-11-03T11:55:00Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22810/
Test FAILed.

gvramana · 2014-11-03T18:55:39Z

fixed test, please re-trigger test

liancheng · 2014-11-04T04:31:44Z

retest this please

SparkQA · 2014-11-04T04:35:11Z

Test build #22860 has started for PR 2802 at commit b5f0ce6.

This patch merges cleanly.

SparkQA · 2014-11-04T05:30:46Z

Test build #22860 has finished for PR 2802 at commit b5f0ce6.

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

AmplabJenkins · 2014-11-04T05:30:49Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22860/
Test PASSed.

chenghao-intel · 2014-11-04T07:04:59Z

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

-
-  private val function = resolver.getEvaluator(exprs.map(_.dataType.toTypeInfo).toArray)
+
+  private val inspectors = 


Should we have to distinguish the isUDAFBridgeRequired? Can we just use the exprs.map(toInspector).toArray?

previously flag added to not disturb UDAFBridge path. Fixed and tested the same.

chenghao-intel · 2014-11-04T07:17:01Z

Thank you @gvramana, this is a good follow up for #2762 , which I didn't cover the constant value for Hive UDAF. I have some comments on it, let's keep the minimum change for the fixing.

gvramana · 2014-11-04T19:14:49Z

@chenghao-intel , Thanks for review, fixed and replied on comments. Please check the same.

marmbrus · 2014-11-04T21:13:44Z

ok to test

SparkQA · 2014-11-04T21:19:53Z

Test build #22897 has started for PR 2802 at commit 2b2859a.

This patch merges cleanly.

SparkQA · 2014-11-04T22:38:28Z

Test build #22897 has finished for PR 2802 at commit 2b2859a.

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

AmplabJenkins · 2014-11-04T22:38:31Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22897/
Test PASSed.

chenghao-intel · 2014-11-05T01:10:36Z

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

+  private val inspectors = exprs.map(toInspector).toArray
+
+  private val function = { 
+    val parameterInfo = new SimpleGenericUDAFParameterInfo(inspectors,false,false)


Nit: spaces after ,.

Fixed HiveUdaf wrap object issue.

- fixes failure due to return iterator and value type mismatch

SparkQA · 2014-11-23T04:17:45Z

Test build #23759 has started for PR 2802 at commit a18f917.

This patch merges cleanly.

SparkQA · 2014-11-23T05:30:27Z

Test build #23759 has finished for PR 2802 at commit a18f917.

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

AmplabJenkins · 2014-11-23T05:30:30Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23759/
Test PASSed.

gvramana · 2014-12-01T03:34:07Z

@marmbrus Please review and merge the same, thanks

chenghao-intel · 2014-12-01T04:56:02Z

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

@@ -172,6 +177,8 @@ private[hive] case class HiveGenericUdf(functionClassName: String, children: Seq

  override def eval(input: Row): Any = {
    returnInspector // Make sure initialized.
+    if(foldable) return constantReturnValue


This is probably unnecessary, as constant folding rule in Optimizer will replace the foldable expression with Literal. Please correct me if there is exceptional case. (and above property constantreturnValue)

Constant check and returning value is required for two reasons:

some UDF functions returns constant iterator when initializeAndFoldConstants called with constant iterators, by executing them once.
But if the same are called with "function.evaluate" they will not return the same constant value type. There will be mismatch in the datatype expected by constantReturnInspector datatype vs datatype returned by function.evaluate.(Ex: org.apache.hadoop.io.Text vs String). This fails unwrap.
So if return Inspector is constant we don't need to call "function.evaluate" as the expression is already evaluated and return value is already present in constant iterator.
This I have uncovered, when I made CreateArrayExression as foldable, then test fails. So modified as part of current defect fix only.

Even if Literal is identified during optimization, expression is evaluated twice, once during return Inspector creation and next during eval function.

Thanks for the explanation, I guess there is probably a bug in master as you described, can you paste a query to reproduce the failure? (Text V.S. String).

In HiveQuerySuite, "constant array" testcase was failing
SELECT sort_array(
sort_array(
array("hadoop distributed file system",
"enterprise databases", "hadoop map-reduce")))
FROM src LIMIT 1

[info] - constant array *** FAILED *** (596 milliseconds)
[info] Failed to execute query using catalyst:
[info] Error: java.lang.String cannot be cast to org.apache.hadoop.io.Text

@chenghao-intel, I think you understand this code better than I do. Are you satisfied with the explanation? Does this approach seem reasonable?

I think this failure is due to the bug of nested constant Expression <-> ObjectInspector in HiveInspectors, and I will fix that in #3429.

@gvramana , how about revert the changes in CreateArray and HiveGenericUDF? I think we can merge the others first. And you can create a new PR for CreateArray.foldable which depends on #3429, since it currently doesn't break anything.

@chenghao-intel This PR cannot be separately merged without CreateArray, as percentile_approx accepts only constant array iterator and fails otherwise. I think we can go ahead and merge all these changes as they don't break build or tests, and are not directly dependent on #3429 in order of merge.

I've updated the #3429, I think this PR can be more simpler after #3429 merged. Besides, invoking the foldable win eval probably too heavy, which is supposed to be eliminated in Optimizer.

chenghao-intel · 2014-12-01T05:06:37Z

The code looks good to me in general, except the unnecessary change in the class HiveGenericUDF. Sorry, I didn't test that, please correct me if I am wrong.

gvramana · 2014-12-01T06:18:57Z

That change is required, please find the updated comment above. Thanks

gvramana · 2014-12-01T12:43:56Z

@marmbrus, @chenghao-intel any other comments? Can you merge the same, thanks

marmbrus · 2014-12-02T00:55:33Z

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

@@ -165,6 +165,11 @@ private[hive] case class HiveGenericUdf(functionClassName: String, children: Seq
    isUDFDeterministic && returnInspector.isInstanceOf[ConstantObjectInspector]

  @transient
+  protected lazy val constantReturnValue = unwrap(


Should this just be a def? It will only be called once correct?

Yes, fixed the same.

SparkQA · 2014-12-09T12:17:55Z

Test build #24246 has started for PR 2802 at commit a0182e5.

This patch merges cleanly.

SparkQA · 2014-12-09T13:22:00Z

Test build #24246 has finished for PR 2802 at commit a0182e5.

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

AmplabJenkins · 2014-12-09T13:22:03Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24246/
Test PASSed.

marmbrus · 2014-12-17T23:42:00Z

Thanks! Merged to master.

Since #3429 has been merged, the bug of wrapping to Writable for HiveGenericUDF is resolved, we can safely remove the foldable checking in `HiveGenericUdf.eval`, which discussed in #2802. Author: Cheng Hao <hao.cheng@intel.com> Closes #3745 from chenghao-intel/generic_udf and squashes the following commits: 622ad03 [Cheng Hao] Remove the unnecessary code change in Generic UDF

…and constant inspectors support Supported passing array to percentile and percentile_approx UDAFs To support percentile_approx, constant inspectors are supported for GenericUDAF Constant folding support added to CreateArray expression Avoided constant udf expression re-evaluation Author: Venkata Ramana G <ramana.gollamudihuawei.com> Author: Venkata Ramana Gollamudi <ramana.gollamudi@huawei.com> Closes apache#2802 from gvramana/percentile_array_support and squashes the following commits: a0182e5 [Venkata Ramana Gollamudi] fixed review comment a18f917 [Venkata Ramana Gollamudi] avoid constant udf expression re-evaluation - fixes failure due to return iterator and value type mismatch c46db0f [Venkata Ramana Gollamudi] Removed TestHive reset 4d39105 [Venkata Ramana Gollamudi] Unified inspector creation, style check fixes f37fd69 [Venkata Ramana Gollamudi] Fixed review comments 47f6365 [Venkata Ramana Gollamudi] fixed test cb7c61e [Venkata Ramana Gollamudi] Supported ConstantInspector for UDAF Fixed HiveUdaf wrap object issue. 7f94aff [Venkata Ramana Gollamudi] Added foldable support to CreateArray

gvramana mentioned this pull request Oct 15, 2014

[SPARK-3904] [SQL] add constant objectinspector support for udfs #2762

Closed

gvramana force-pushed the percentile_array_support branch from f425daf to 2a59843 Compare November 3, 2014 10:10

chenghao-intel reviewed Nov 4, 2014
View reviewed changes

chenghao-intel reviewed Nov 5, 2014
View reviewed changes

gvramana added 6 commits November 22, 2014 11:16

Supported ConstantInspector for UDAF

cb7c61e

Fixed HiveUdaf wrap object issue.

fixed test

47f6365

Fixed review comments

f37fd69

Unified inspector creation, style check fixes

4d39105

Removed TestHive reset

c46db0f

avoid constant udf expression re-evaluation

a18f917

- fixes failure due to return iterator and value type mismatch

gvramana force-pushed the percentile_array_support branch from e287693 to a18f917 Compare November 23, 2014 04:14

chenghao-intel reviewed Dec 1, 2014
View reviewed changes

marmbrus reviewed Dec 2, 2014
View reviewed changes

fixed review comment

a0182e5

chenghao-intel mentioned this pull request Dec 10, 2014

[SPARK-4573] [SQL] Add SettableStructObjectInspector support in "wrap" function #3429

Closed

asfgit closed this in f33d550 Dec 17, 2014

chenghao-intel mentioned this pull request Dec 19, 2014

[SPARK-4904] [SQL] Remove the unnecessary code change in Generic UDF #3745

Closed


		private val function = resolver.getEvaluator(exprs.map(_.dataType.toTypeInfo).toArray)

		private val inspectors =

[SPARK-3891][SQL] Add array support to percentile, percentile_approx and constant inspectors support #2802

[SPARK-3891][SQL] Add array support to percentile, percentile_approx and constant inspectors support #2802

Conversation

gvramana commented Oct 14, 2014

AmplabJenkins commented Oct 14, 2014

AmplabJenkins commented Oct 21, 2014

liancheng commented Oct 22, 2014

SparkQA commented Oct 25, 2014

SparkQA commented Oct 25, 2014

gvramana commented Nov 3, 2014

liancheng commented Nov 3, 2014

SparkQA commented Nov 3, 2014

SparkQA commented Nov 3, 2014

AmplabJenkins commented Nov 3, 2014

gvramana commented Nov 3, 2014

liancheng commented Nov 4, 2014

SparkQA commented Nov 4, 2014

SparkQA commented Nov 4, 2014

AmplabJenkins commented Nov 4, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenghao-intel commented Nov 4, 2014

gvramana commented Nov 4, 2014

marmbrus commented Nov 4, 2014

SparkQA commented Nov 4, 2014

SparkQA commented Nov 4, 2014

AmplabJenkins commented Nov 4, 2014

Choose a reason for hiding this comment

SparkQA commented Nov 23, 2014

SparkQA commented Nov 23, 2014

AmplabJenkins commented Nov 23, 2014

gvramana commented Dec 1, 2014

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

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

chenghao-intel commented Dec 1, 2014

gvramana commented Dec 1, 2014

gvramana commented Dec 1, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Dec 9, 2014

SparkQA commented Dec 9, 2014

AmplabJenkins commented Dec 9, 2014

marmbrus commented Dec 17, 2014