Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
viirya wants to merge 14 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
978080b
Add interpreted execution for InitializeJavaBean expression.
viirya Mar 7, 2018
0c48a9b
Merge remote-tracking branch 'upstream/master' into SPARK-23593
viirya Mar 7, 2018
b8f171e
Test error message of interpreted execution.
viirya Mar 7, 2018
b51303a
Resolving setters outside eval.
viirya Mar 7, 2018
6372f04
Merge remote-tracking branch 'upstream/master' into SPARK-23593
viirya Mar 8, 2018
970ed6c
Simplifying it.
viirya Mar 8, 2018
e20c207
Address comment.
viirya Mar 8, 2018
f3fdf57
Change error message a bit.
viirya Mar 8, 2018
1a78334
Use AnyRef.
viirya Mar 9, 2018
e7640e1
Add a test.
viirya Mar 27, 2018
0d6b3d3
Prevent null input to setters.
viirya Apr 5, 2018
2954e8d
Merge remote-tracking branch 'upstream/master' into SPARK-23593
viirya Apr 5, 2018
96a8fe6
Prevent null input to setters.
viirya Apr 5, 2018
573db59
Fix out-of-date comment.
viirya Apr 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Add interpreted execution for InitializeJavaBean expression.
  • Loading branch information
@viirya
viirya committed Mar 7, 2018
commit 978080bd8f5a1b095fd0d58ff529e16dd9cbadba
20 changes: 18 additions & 2 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1254,8 +1254,24 @@ case class InitializeJavaBean(beanInstance: Expression, setters: Map[String, Exp
override def children: Seq[Expression] = beanInstance +: setters.values.toSeq
override def dataType: DataType = beanInstance.dataType

override def eval(input: InternalRow): Any =
throw new UnsupportedOperationException("Only code-generated evaluation is supported.")
override def eval(input: InternalRow): Any = {
val instance = beanInstance.eval(input).asInstanceOf[Object]
Copy link
Member

@maropu maropu Mar 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@viirya viirya Mar 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

if (instance != null) {
setters.foreach { case (setterMethod, fieldExpr) =>
Copy link
Contributor

@hvanhovell hvanhovell Mar 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
  }

Copy link
Member Author

@viirya viirya Mar 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@hvanhovell hvanhovell Mar 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@viirya viirya Mar 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@hvanhovell hvanhovell Mar 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@hvanhovell hvanhovell Mar 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@viirya viirya Mar 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@viirya viirya Mar 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or should we go back to reflection?

Copy link
Contributor

@hvanhovell hvanhovell Mar 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #20753 (comment) :)

Copy link
Member Author

@viirya viirya Mar 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Thanks! :)

val fieldValue = fieldExpr.eval(input).asInstanceOf[Object]

val foundMethods = instance.getClass.getMethods.filter { method =>
method.getName == setterMethod && Modifier.isPublic(method.getModifiers) &&
method.getParameterTypes.length == 1
}
assert(foundMethods.length == 1,
throw new RuntimeException("The Java Bean instance should have only one " +
Copy link
Member Author

@viirya viirya Mar 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@hvanhovell hvanhovell Mar 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

s"setter $setterMethod method, but ${foundMethods.length} methods found."))
foundMethods.head.invoke(instance, fieldValue)
}
}
instance
}

override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val instanceGen = beanInstance.genCode(ctx)
Expand Down
3 changes: 2 additions & 1 deletion ...alyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ trait ExpressionEvalHelper extends GeneratorDrivenPropertyChecks {
expression: => Expression, expected: Any, inputRow: InternalRow = EmptyRow): Unit = {
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.
Copy link
Contributor

@hvanhovell hvanhovell Mar 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Member Author

@viirya viirya Mar 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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].

Copy link
Contributor

@hvanhovell hvanhovell Mar 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we using a literal? Ok, makes sense.

def expr = resolver.resolveTimeZones(serializer.deserialize(serializer.serialize(expression)))
val catalystValue = CatalystTypeConverters.convertToCatalyst(expected)
checkEvaluationWithoutCodegen(expr, catalystValue, inputRow)
checkEvaluationWithGeneratedMutableProjection(expr, catalystValue, inputRow)
Expand Down
11 changes: 10 additions & 1 deletion ...yst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ObjectExpressionsSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ package org.apache.spark.sql.catalyst.expressions
import org.apache.spark.SparkFunSuite
import org.apache.spark.sql.catalyst.InternalRow
import org.apache.spark.sql.catalyst.encoders.ExpressionEncoder
import org.apache.spark.sql.catalyst.expressions.objects.Invoke
import org.apache.spark.sql.catalyst.expressions.objects._
import org.apache.spark.sql.catalyst.util.{ArrayBasedMapData, GenericArrayData}
import org.apache.spark.sql.types.{IntegerType, ObjectType}

Expand Down Expand Up @@ -66,4 +66,13 @@ class ObjectExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
checkEvalutionWithUnsafeProjection(
mapEncoder.serializer.head, mapExpected, mapInputRow)
}

test("SPARK-23593: InitializeJavaBean should support interpreted execution") {
val list = new java.util.LinkedList[Int]()
list.add(1)

val initializeBean = InitializeJavaBean(Literal.fromObject(new java.util.LinkedList[Int]),
Map("add" -> Literal(1)))
checkEvaluation(initializeBean, list, InternalRow.fromSeq(Seq()))
}
}