[SPARK-23582][SQL] StaticInvoke should support interpreted execution #20753

kiszk · 2018-03-07T00:51:19Z

What changes were proposed in this pull request?

This pr added interpreted execution for StaticInvoke.

How was this patch tested?

Added tests in ObjectExpressionsSuite.

SparkQA · 2018-03-07T00:54:28Z

Test build #88028 has finished for PR 20753 at commit 0790dea.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
throw new RuntimeException(\"The static class cannot be null.\")

kiszk · 2018-03-07T01:08:44Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala

+    val parms = arguments.map(e => e.eval(input).asInstanceOf[Object])
+    val method = staticObject.getDeclaredMethod(functionName, parmTypes : _*)
+    val ret = method.invoke(null, parms : _*)
+    val retClass = CallMethodViaReflection.typeMapping.getOrElse(dataType,


We have to support more types (e.g. ArrayType)

kiszk · 2018-03-07T01:14:36Z

ping @hvanhovell

viirya · 2018-03-07T01:37:17Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala

+    val method = staticObject.getDeclaredMethod(functionName, parmTypes : _*)
+    val ret = method.invoke(null, parms : _*)
+    val retClass = CallMethodViaReflection.typeMapping.getOrElse(dataType,
+      Seq(dataType.asInstanceOf[ObjectType].cls))(0)


Will dataType always be an ObjectType here? If returned data type is CalendarIntervalType?

viirya · 2018-03-07T01:40:52Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala

+
+    val parmTypes = arguments.map(e =>
+      CallMethodViaReflection.typeMapping.getOrElse(e.dataType,
+        Seq(e.dataType.asInstanceOf[ObjectType].cls))(0))


The external types of native types CalendarIntervalType and BinaryType are not ObjectType. If we have arguments in those types, we may not use ObjectType.

You are right. I have to support other types before merging this change.
This is a small prototype for discussing whether we use reflection or not.

SparkQA · 2018-03-07T04:32:19Z

Test build #88029 has finished for PR 20753 at commit f570692.

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

maropu · 2018-03-07T06:58:10Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala

-  override def eval(input: InternalRow): Any =
-    throw new UnsupportedOperationException("Only code-generated evaluation is supported.")
+  override def eval(input: InternalRow): Any = {
+    if (staticObject == null) {


We need this check?

We don't need this check, for sure. See line 132,

val objectName = staticObject.getName.stripSuffix("$")

^^ this will run as a part of the constructor, which will throw an NPE if staticObject is null, so it's redundant to null check it here.

maropu · 2018-03-07T07:06:17Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala

+    val parmTypes = arguments.map(e =>
+      CallMethodViaReflection.typeMapping.getOrElse(e.dataType,
+        Seq(e.dataType.asInstanceOf[ObjectType].cls))(0))
+    val parms = arguments.map(e => e.eval(input).asInstanceOf[Object])


We need null checks here for inputs? Also, can we add a common function in InvokeLike to handle input arguments for other InvokeLike eprs? (I mean the interpreted version of InvokeLike.prepareArguments).

Yeah, it is good since this would be used in 'Invoke', too. After supporting other types, I will create an utility function.

To discuss design choice (reflection or method handle), I put earlier version.

kiszk · 2018-03-07T14:50:19Z

@hvanhovell Is it better to implement method handler approach, too?

hvanhovell · 2018-03-07T15:06:15Z

@kiszk I think we should benchmark it. My rational for considering method handles is that they seem to be made for this purpose, and they should become more performant with newer versions of java.

kiszk · 2018-03-07T16:18:40Z

Which use case is typical? The same method is called many times. Are different method called?

If the same method is called, reflection can also improve performance by creating custom bytecode.

hvanhovell · 2018-03-07T16:27:07Z

Ok, let's go with reflection then.

cc @rednaxelafx

hvanhovell · 2018-03-07T16:28:19Z

and cc @viirya

kiszk · 2018-03-07T17:19:39Z

Sure

viirya · 2018-03-08T05:00:53Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala

+      CallMethodViaReflection.typeMapping.getOrElse(e.dataType,
+        Seq(e.dataType.asInstanceOf[ObjectType].cls))(0))
+    val parms = arguments.map(e => e.eval(input).asInstanceOf[Object])
+    val method = staticObject.getDeclaredMethod(functionName, parmTypes : _*)


Looks like we can also move getDeclaredMethod outside eval?

Unfortunately, this movement caused failure of reflection.

rednaxelafx

One comment following @maropu 's.

BTW, on the choice between reflection and method handles, the latter might be slightly faster if the code pattern is right (that the underlying JVM likes...). But in general invoking via old school reflection and invoking via MethodHandles are not that different. We always have the option to put the reflection-based version in first and then see if we can further improve it with MethodHandles.

rednaxelafx · 2018-03-08T06:44:48Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala

-  override def eval(input: InternalRow): Any =
-    throw new UnsupportedOperationException("Only code-generated evaluation is supported.")
+  override def eval(input: InternalRow): Any = {
+    if (staticObject == null) {


We don't need this check, for sure. See line 132,

val objectName = staticObject.getName.stripSuffix("$")

^^ this will run as a part of the constructor, which will throw an NPE if staticObject is null, so it's redundant to null check it here.

rednaxelafx · 2018-03-08T06:58:21Z

@hvanhovell @kiszk Yes, the old school JDK reflection does perform bytecode generation to speed it up, since the JDK1.4 era. But the advantage of fully optimized MethodHandle versus reflection Method.invoke is that the former performs privilege checks at MethodHandle creation time, whereas the latter is required to perform privilege checks at every invocation (and that's not covered by the generated bytecode). So theoretically the MethodHandle version can be faster.

But note that even in OpenJDK8, the HotSpot VM is still exploring ways to optimize MethodHandle performance, and it's somewhat picky about the shape of the MethodHandles, so it's not like we will always get better performance just by sticking it in place of reflection. It'll need some tuning / trial-and-error. We don't have to get it right in the first cut.

SparkQA · 2018-03-08T13:40:14Z

Test build #88090 has finished for PR 20753 at commit 9a36442.

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

SparkQA · 2018-03-08T13:45:07Z

Test build #88091 has finished for PR 20753 at commit 7d107a6.

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

SparkQA · 2018-03-08T15:20:56Z

Test build #88093 has finished for PR 20753 at commit 91a454c.

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

SparkQA · 2018-03-08T16:45:02Z

Test build #88097 has finished for PR 20753 at commit eb6171a.

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

SparkQA · 2018-03-08T17:05:04Z

Test build #88089 has finished for PR 20753 at commit f9f5701.

This patch fails Spark unit tests.
This patch does not merge cleanly.
This patch adds no public classes.

SparkQA · 2018-03-08T21:03:26Z

Test build #88100 has finished for PR 20753 at commit f74d48e.

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

viirya · 2018-03-08T23:38:01Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala

+        ret
+      } else {
+        // cast a primitive value using Boxed class
+        val boxedClass = CallMethodViaReflection.typeBoxedJavaMapping(dataType)


nit: I think we can directly do like this without the if.

val boxedClass = CallMethodViaReflection.typeBoxedJavaMapping.get(dataType) boxedClass.map(_.cast(ret)).getOrElse(ret)

Good catch. When discussions related to typeJavaMapping, typeBoxedJavaMapping`, and others are fixed, I will address this.

viirya · 2018-03-08T23:40:01Z

...alyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/CallMethodViaReflection.scala

+      case ObjectType(cls) => cls
+      case _ => typeJavaMapping.getOrElse(dt, classOf[java.lang.Object])
+    }
+  }


The above should be in CallMethodViaReflection or CodeGenerator?

kiszk · 2018-03-09T00:55:43Z

...talyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ObjectExpressionsSuite.scala

@@ -95,6 +162,21 @@ class ObjectExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
    checkEvaluation(createExternalRow, Row.fromSeq(Seq(1, "x")), InternalRow.fromSeq(Seq()))
  }

+  // This is an alternative version of `checkEvaluation` to compare results


This code is intentionally imported for testing from #20757

If this pr is merged first, I'll remove this in my pr.

kiszk · 2018-03-19T19:34:02Z

ping @hvanhovell

kiszk · 2018-03-26T00:59:30Z

ping @hvanhovell

SparkQA · 2018-03-28T10:03:51Z

Test build #88656 has finished for PR 20753 at commit 09cdf5e.

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

kiszk · 2018-03-28T12:54:59Z

retest this please

SparkQA · 2018-03-28T15:25:31Z

Test build #88663 has finished for PR 20753 at commit 09cdf5e.

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

kiszk · 2018-03-28T18:50:17Z

org.apache.spark.sql.execution.streaming.sources.RateSourceSuite.overflow is not a problem for this PR.
Other PRs also fails due to this test.

kiszk · 2018-03-29T01:17:05Z

retest this please

SparkQA · 2018-03-29T04:25:57Z

Test build #88687 has finished for PR 20753 at commit 09cdf5e.

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

kiszk · 2018-04-02T17:59:45Z

retest this please

SparkQA · 2018-04-02T22:08:28Z

Test build #88822 has finished for PR 20753 at commit 09cdf5e.

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

import checkObjectExprEvaluation for test

SparkQA · 2018-04-04T19:04:19Z

Test build #88901 has finished for PR 20753 at commit 0c61e60.

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

SparkQA · 2018-04-04T19:45:13Z

Test build #88905 has finished for PR 20753 at commit 984cee7.

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

SparkQA · 2018-04-05T06:32:16Z

Test build #88922 has finished for PR 20753 at commit 7b30c30.

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

hvanhovell

LGTM - merging to master. Thanks!

## What changes were proposed in this pull request? This pr added interpreted execution for `StaticInvoke`. ## How was this patch tested? Added tests in `ObjectExpressionsSuite`. Author: Kazuaki Ishizaki <ishizaki@jp.ibm.com> Closes apache#20753 from kiszk/SPARK-23582.

kiszk commented Mar 7, 2018

View reviewed changes

viirya reviewed Mar 7, 2018

View reviewed changes

maropu reviewed Mar 7, 2018

View reviewed changes

hvanhovell mentioned this pull request Mar 7, 2018

[SPARK-23593][SQL] Add interpreted execution for InitializeJavaBean expression #20756

Closed

viirya reviewed Mar 8, 2018

View reviewed changes

rednaxelafx reviewed Mar 8, 2018

View reviewed changes

kiszk force-pushed the SPARK-23582 branch 2 times, most recently from f9f5701 to 9a36442 Compare March 8, 2018 13:35

viirya reviewed Mar 8, 2018

View reviewed changes

kiszk commented Mar 9, 2018

View reviewed changes

kiszk added 11 commits April 5, 2018 01:51

initial prototype

9fea286

fix style error

76c61ad

address review comments

80579c5

fix scala style error

d02e926

fix test failures

b1f08c1

import checkObjectExprEvaluation for test

move utility methods to ScalaReflection

7d66bb9

fix style error

c66e452

fix style error

1b3ea29

address review comments

7279557

fix test failures

0530dcc

minimize changes

0c61e60

kiszk force-pushed the SPARK-23582 branch from 09cdf5e to 0c61e60 Compare April 4, 2018 18:54

fix build failure

984cee7

fix scala style error

7b30c30

hvanhovell approved these changes Apr 5, 2018

View reviewed changes

asfgit closed this in 1822ecd Apr 5, 2018

ulysses-you mentioned this pull request Sep 29, 2020

[SPARK-32585][SQL] Support scala enumeration in ScalaReflection #29403

Closed

[SPARK-23582][SQL] StaticInvoke should support interpreted execution #20753

[SPARK-23582][SQL] StaticInvoke should support interpreted execution #20753

Conversation

kiszk commented Mar 7, 2018

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Mar 7, 2018

Choose a reason for hiding this comment

kiszk commented Mar 7, 2018

viirya Mar 7, 2018 • edited Loading

Choose a reason for hiding this comment

viirya Mar 7, 2018 • edited Loading

Choose a reason for hiding this comment

kiszk Mar 7, 2018 • edited Loading

Choose a reason for hiding this comment

SparkQA commented Mar 7, 2018

Choose a reason for hiding this comment

rednaxelafx Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiszk commented Mar 7, 2018

hvanhovell commented Mar 7, 2018

kiszk commented Mar 7, 2018

hvanhovell commented Mar 7, 2018

hvanhovell commented Mar 7, 2018

kiszk commented Mar 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rednaxelafx left a comment

Choose a reason for hiding this comment

rednaxelafx Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

rednaxelafx commented Mar 8, 2018

SparkQA commented Mar 8, 2018

SparkQA commented Mar 8, 2018

SparkQA commented Mar 8, 2018

SparkQA commented Mar 8, 2018

SparkQA commented Mar 8, 2018

SparkQA commented Mar 8, 2018

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

kiszk commented Mar 19, 2018

kiszk commented Mar 26, 2018

SparkQA commented Mar 28, 2018

kiszk commented Mar 28, 2018

SparkQA commented Mar 28, 2018

kiszk commented Mar 28, 2018

kiszk commented Mar 29, 2018

SparkQA commented Mar 29, 2018

kiszk commented Apr 2, 2018

SparkQA commented Apr 2, 2018

SparkQA commented Apr 4, 2018

SparkQA commented Apr 4, 2018

SparkQA commented Apr 5, 2018

hvanhovell left a comment

Choose a reason for hiding this comment

viirya Mar 7, 2018 •

edited

Loading

viirya Mar 7, 2018 •

edited

Loading

kiszk Mar 7, 2018 •

edited

Loading

rednaxelafx Mar 8, 2018 •

edited

Loading

rednaxelafx Mar 8, 2018 •

edited

Loading