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

viirya · 2018-03-07T03:13:04Z

What changes were proposed in this pull request?

Add interpreted execution for InitializeJavaBean expression.

How was this patch tested?

Added unit test.

viirya · 2018-03-07T03:17:08Z

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

+            method.getParameterTypes.length == 1
+        }
+        assert(foundMethods.length == 1,
+          throw new RuntimeException("The Java Bean instance should have only one " +


codegen evaluation does not check method existence. But for non-codegen evaluation here, it is a bit weird to directly invoke first found method (we may not find it and cause non such element exception). cc @hvanhovell

Well the compiler will check if the method exists. We could throw a NoSuchMethodError if plan to keep doing resolution in the eval method (see my other comment).

SparkQA · 2018-03-07T06:36:49Z

Test build #88032 has finished for PR 20756 at commit 0c48a9b.

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

SparkQA · 2018-03-07T06:37:11Z

Test build #88034 has finished for PR 20756 at commit b8f171e.

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

hvanhovell · 2018-03-07T10:40:38Z

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

+  override def eval(input: InternalRow): Any = {
+    val instance = beanInstance.eval(input).asInstanceOf[Object]
+    if (instance != null) {
+      setters.foreach { case (setterMethod, fieldExpr) =>


Why are we resolving setters during eval? That seems a bit expensive. How about we create the setters before we execute eval? For example (I got a bit carried away):

private lazy val resolvedSetters = { val ObjectType(beanClass) = beanInstance.dataType val lookup = MethodHandles.lookup() setters.map { case (name, expr) => // Resolve expression type (should be better!) val fieldClass = CallMethodViaReflection.typeMapping(expr.dataType).head val handle = lookup.findVirtual( beanClass, name, MethodType.methodType(classOf[Unit], fieldClass)) handle -> expr } } override def eval(input: InternalRow): Any = { val bean = beanInstance.eval(input) if (bean != null) { resolvedSetters.foreach { case (setter, expr) => // nullability? setter.invoke(bean, expr.eval(input)) } } bean }

lookup.findVirtual or lookup.findSetter? Strictly I think InitializeJavaBean should be used only for setters not for general methods.

lookup.findSetter means direct field access in the MethodHandle world. So you need to use lookup.findVirtual.

Another question: Do we need to find method with argument types? Codegen execution also does not do this check. I would think the caller of InitializeJavaBean should give correct arguments to the setters.

Yes, we do. The compiler will check the generated code right? It is just a matter of where you decide to fail. In a perfect world this would happen on the driver.

Hmmm it should be lenient. Can you try MethodType.methodType(classOf[Object], fieldClass))?

MethodType.methodType(classOf[Object], fieldClass)) does not work too.

Actually I am not sure if MethodHandle can find the def add(x$1: Int): Boolean of java.util.LinkedList.

scala> lookup.findVirtual(classOf[java.util.LinkedList[Int]], "add", MethodType.methodType(classOf[Boolean], classOf[Int])) java.lang.NoSuchMethodException: no such method: java.util.LinkedList.add(int)boolean/invokeVirtual

Am I missing something?

Or should we go back to reflection?

See #20753 (comment) :)

I see. Thanks! :)

hvanhovell · 2018-03-07T13:10:00Z

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala

    val serializer = new JavaSerializer(new SparkConf()).newInstance
    val resolver = ResolveTimeZone(new SQLConf)
-    val expr = resolver.resolveTimeZones(serializer.deserialize(serializer.serialize(expression)))
+    // Make it as method to obtain fresh expression everytime.


Why this change?

The content of bean instance will be changed after first evaluation of interpreted execution. For example, in the added unit test, the input bean of the later evaluation will become [1] not []. So the later evaluation result will be [1, 1].

Are we using a literal? Ok, makes sense.

hvanhovell · 2018-03-07T13:12:17Z

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

+    val ObjectType(beanClass) = beanInstance.dataType
+
+    setters.map { case (setterMethod, fieldExpr) =>
+      val foundMethods = beanClass.getMethods.filter { method =>


(Picking up our earlier conversation) You are not checking the argument?

Will updated later.

SparkQA · 2018-03-07T16:47:24Z

Test build #88044 has finished for PR 20756 at commit b51303a.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds the following public classes (experimental):
throw new RuntimeException(\"The Java Bean class should have only one \" +

viirya · 2018-03-08T04:35:24Z

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala

          eval
        }.getMessage
-        if (errMsg != expectedErrMsg) {
+        if (!errMsg.contains(expectedErrMsg)) {


For codegen error, it has very verbose message like:

Code generation of initializejavabean([], (nonexisting,1)) failed: [info] java.util.concurrent.ExecutionException: org.codehaus.commons.compiler.CompileException: File 'generated.java', Line 41, Column 22 : failed to compile: org.codehaus.commons.compiler.CompileException: File 'generated.java', Line 41, Column 22: A method named "nonexisting " is not declared in any enclosing class nor any supertype, nor through a static import [info] java.util.concurrent.ExecutionException: org.codehaus.commons.compiler.CompileException: File 'generated.java', Line 41, Column 22 : failed to compile: org.codehaus.commons.compiler.CompileException: File 'generated.java', Line 41, Column 22: A method named "nonexisting " is not declared in any enclosing class nor any supertype, nor through a static import [info] at com.google.common.util.concurrent.AbstractFuture$Sync.getValue(AbstractFuture.java:306) [info] at com.google.common.util.concurrent.AbstractFuture$Sync.get(AbstractFuture.java:293)

So changes it to test if it contains the given error message.

viirya · 2018-03-08T04:43:03Z

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

+        }
+        if (methods.isEmpty) {
+          throw new NoSuchMethodException(s"""A method named "$name" is not declared """ +
+            "in any enclosing class nor any supertype, nor through a static import")


This error message is copied from compiling error thrown in codegen execution. So we can test the same error message for both execution.

Can you remove the last bit , nor through a static import?

viirya · 2018-03-08T04:45:39Z

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

+      case (name, expr) =>
+        // Looking for known type mapping first, then using Class attached in `ObjectType`.
+        // Finally also looking for general `Object`-type parameter for generic methods.
+        val paramTypes = CallMethodViaReflection.typeMapping.getOrElse(expr.dataType,


As #20753, we need to add other types into the mapping.

Ok, who is making those changes? You or @kiszk?

I may have no time to do tonight, if @kiszk do not make those changes, I will do it tomorrow.

I added the similar variable typeJavaMapping and typeJavaMapping. This is because typeMapping is used for CallMethodViaReflection whose comment says For now, only types defined in Reflect.typeMapping are supported (basically primitives and string) as input types.

If we can expand this support to additional classes (e.g. DateType, TimestampType, BinaryType, and CalendarIntervalType), I can merge three variables into one typeMapping.

WDYT? @hvanhovell and @viirya

Is there any special reason it only supports basically primitives and string?

If we want to expand the support, maybe we can have another PR to expand it in CallMethodViaReflection and merge those variables.

I see. Is it better to have separate map variables, for now? cc @hvanhovell

Sorry for not coming back to this sooner. AFAIK CallMethodViaReflection expression was only designed to work with a couple of primitives. I think we are looking for something a little bit more complete here, i.e. support all types in Spark SQL's type system. I also don't think that we should put the mappings in CallMethodViaReflection because the mapping is now using in more expressions, ScalaReflection is IMO a better place for this logic.

And finally which PR will implement this. cc @maropu for visibility.

Done in #20753 and #20797

viirya · 2018-03-08T04:46:13Z

also cc @kiszk @maropu

maropu · 2018-03-08T05:30:22Z

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

+  }
+
+  override def eval(input: InternalRow): Any = {
+    val instance = beanInstance.eval(input).asInstanceOf[Object]


super nit: better to put the cast inside if for avoiding unnecessary casts in case of many null cases?;

val instance = beanInstance.eval(input) if (instance != null) { val obj = instance.asInstanceOf[Object] resolvedSetters.foreach { case (setter: Method, expr) => setter.invoke(obj, expr.eval(input).asInstanceOf[Object]) } } instance

maropu · 2018-03-08T05:30:43Z

LGTM except for one minor comment

maropu · 2018-03-08T06:00:16Z

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.")
+  private lazy val resolvedSetters = {
+    val ObjectType(beanClass) = beanInstance.dataType


better to put assert(beanInstance.dataType.isInstanceOf[ObjectType]) in the constructor?

SparkQA · 2018-03-08T07:54:16Z

Test build #88074 has finished for PR 20756 at commit 970ed6c.

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

SparkQA · 2018-03-08T07:58:44Z

Test build #88073 has finished for PR 20756 at commit 6372f04.

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

SparkQA · 2018-03-08T12:18:34Z

Test build #88080 has finished for PR 20756 at commit e20c207.

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

hvanhovell · 2018-03-08T13:11:54Z

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

+      val bean = instance.asInstanceOf[Object]
+      resolvedSetters.foreach {
+        case (setter, expr) =>
+          setter.invoke(bean, expr.eval(input).asInstanceOf[Object])


Do we need the cast to .asInstanceOf[Object]? And why not use AnyRef?

Yes. Without cast:

type mismatch; [error] found : Any [error] required: Object [error] setter.invoke(bean, expr.eval(input))

As you seen it is required Object, but I think AnyRef is equivalent and works too. Do you prefer AnyRef?

AnyRef is a bit more scala like, that is all.

SparkQA · 2018-03-08T17:11:15Z

Test build #88092 has finished for PR 20756 at commit f3fdf57.

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

SparkQA · 2018-03-09T08:05:01Z

Test build #88113 has finished for PR 20756 at commit 1a78334.

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

maropu · 2018-03-09T08:07:16Z

retest this please

SparkQA · 2018-03-09T11:33:33Z

Test build #88118 has finished for PR 20756 at commit 1a78334.

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

hvanhovell · 2018-03-25T22:26:26Z

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

+      val bean = instance.asInstanceOf[Object]
+      resolvedSetters.foreach {
+        case (setter, expr) =>
+          setter.invoke(bean, expr.eval(input).asInstanceOf[AnyRef])


There is a subtle difference between code generation and interpreted mode here. A null value for an expression that maps to a java primitive will be some default value (e.g. -1) for code generation and null for interpreted mode, this can lead to different results.

I am not sure we should address this, because I am not 100% if this can ever happen. @cloud-fan could you shed some light on this?

Correct me if I'm wrong:
If the setter takes primitive types, like setAge(int i), and we pass a null via reflection, the actual passed value would be 0. This is different from the codegen version, seems like a potential bug.

IMO, I think the codegen version is wrong. In general we should not read the codegen value if it's marked as null.

This doesn't cause any problem, because we only use these object expressions to generate encoders, which means the parameter for a primitive setter won't be null. But if we treat these expressions as a general DSL, we should be careful about this.

@viirya can you add a null check to both the interpreted and code generated version? Thanks!

@hvanhovell For non-primitive setter, seems it is valid to pass a null into a setter method. A null check means we don't allow such case at all?

ping @hvanhovell

@viirya it might be valid to invoke method with a null reference. However let's - for now - make the behavior consistent, and avoid calling a method when its argument is null.

hvanhovell · 2018-03-25T22:28:36Z

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

+    checkEvaluation(initializeBean, list, InternalRow.fromSeq(Seq()))
+
+    val initializeWithNonexistingMethod = InitializeJavaBean(
+      Literal.fromObject(new java.util.LinkedList[Int]),


Can you also add a test for when the parameter types do not match up?

Added below.

Note that because for generic method, its parameter type is Object, so for LinkedList[Int], it doesn't make sense to test it with something like Map("add" -> Literal("a string")). So I add TestBean to test this case.

viirya · 2018-03-27T04:04:36Z

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

+      Map("setX" -> Literal("1")))
+    intercept[Exception] {
+      evaluateWithoutCodegen(initializeWithWrongParamType, InternalRow.fromSeq(Seq()))
+    }.getMessage.contains(


For codegen the compile exception is like:

No applicable constructor/method found for actual parameters "org.apache.spark.unsafe.types.UTF8String"; candidates are: "public void org.apache.spark.sql.catalyst.expressions.TestBean.setX(int)"

I'm not sure if we want to exactly match this kind of exception message from interpreted execution. Might be a little overkill to do that by looking methods with same name. So currently I only test interpreted execution.

SparkQA · 2018-03-27T07:05:02Z

Test build #88618 has finished for PR 20756 at commit e7640e1.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class TestBean extends Serializable

viirya · 2018-03-27T08:22:29Z

retest this please.

SparkQA · 2018-03-27T12:13:00Z

Test build #88623 has finished for PR 20756 at commit e7640e1.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class TestBean extends Serializable

SparkQA · 2018-04-05T07:05:02Z

Test build #88929 has finished for PR 20756 at commit 573db59.

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

SparkQA · 2018-04-05T07:05:02Z

Test build #88927 has finished for PR 20756 at commit 96a8fe6.

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

viirya · 2018-04-05T07:12:53Z

retest this please.

SparkQA · 2018-04-05T08:00:44Z

Test build #88931 has finished for PR 20756 at commit 573db59.

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

viirya · 2018-04-05T09:01:41Z

retest this please.

hvanhovell

LGTM - merging to master. Thanks!

SparkQA · 2018-04-05T11:42:49Z

Test build #88936 has finished for PR 20756 at commit 573db59.

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

hvanhovell · 2018-04-05T11:50:05Z

@viirya I thought it passed tests, but your last change is causing errors. I am going to revert this for now. Can you reopen? Sorry about this.

viirya · 2018-04-05T13:06:33Z

@hvanhovell Looks like we have tests using null as value for setters. So the added null check fails.

hvanhovell · 2018-04-05T13:34:51Z

I think the semantic should be that we do not invoke the setter if the input is null.

viirya · 2018-04-05T13:44:54Z

We don't invoke the setter if the input is null, sounds like we simply skip calling the setter if the input is null?

hvanhovell · 2018-04-05T13:47:39Z

Yes that would effectively be the same.

Do you think we should always call setters for reference types?

viirya · 2018-04-05T14:07:15Z

I'm not sure if this setter is always for Initialization of bean's property. Will it be possibly used to update the property with a null value? If no, I think skipping is safe.

…xpression ## What changes were proposed in this pull request? Add interpreted execution for `InitializeJavaBean` expression. ## How was this patch tested? Added unit test. Author: Liang-Chi Hsieh <[email protected]> Closes apache#20756 from viirya/SPARK-23593.

viirya added 2 commits March 7, 2018 02:56

Add interpreted execution for InitializeJavaBean expression.

978080b

Merge remote-tracking branch 'upstream/master' into SPARK-23593

0c48a9b

viirya commented Mar 7, 2018

View reviewed changes

Test error message of interpreted execution.

b8f171e

hvanhovell reviewed Mar 7, 2018

View reviewed changes

Resolving setters outside eval.

b51303a

hvanhovell reviewed Mar 7, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into SPARK-23593

6372f04

viirya commented Mar 8, 2018

View reviewed changes

Simplifying it.

970ed6c

viirya commented Mar 8, 2018

View reviewed changes

maropu reviewed Mar 8, 2018

View reviewed changes

Address comment.

e20c207

hvanhovell reviewed Mar 8, 2018

View reviewed changes

Change error message a bit.

f3fdf57

Use AnyRef.

1a78334

hvanhovell reviewed Mar 25, 2018

View reviewed changes

Add a test.

e7640e1

viirya commented Mar 27, 2018

View reviewed changes

viirya added 4 commits April 5, 2018 06:03

Prevent null input to setters.

0d6b3d3

Merge remote-tracking branch 'upstream/master' into SPARK-23593

2954e8d

Prevent null input to setters.

96a8fe6

Fix out-of-date comment.

573db59

hvanhovell approved these changes Apr 5, 2018

View reviewed changes

asfgit closed this in c5c8b54 Apr 5, 2018

viirya deleted the SPARK-23593 branch December 27, 2023 18:35

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

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

Uh oh!

Conversation

viirya commented Mar 7, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

viirya Mar 7, 2018 • 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!

SparkQA commented Mar 7, 2018

Uh oh!

SparkQA commented Mar 7, 2018

Uh oh!

hvanhovell Mar 7, 2018 • 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!

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!

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!

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 Mar 7, 2018

Uh oh!

viirya Mar 8, 2018 • 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!

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!

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!

kiszk Mar 9, 2018 • 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 Mar 7, 2018 •

edited

Loading

hvanhovell Mar 7, 2018 •

edited

Loading

viirya Mar 8, 2018 •

edited

Loading

kiszk Mar 9, 2018 •

edited

Loading

maropu commented Mar 8, 2018 •

edited

Loading