Skip to content
Closed
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -1261,8 +1261,42 @@ 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.")
private lazy val resolvedSetters = {
assert(beanInstance.dataType.isInstanceOf[ObjectType])

val ObjectType(beanClass) = beanInstance.dataType

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ok.

setters.map {
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,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

@kiszk kiszk Mar 9, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Done in #20753 and #20797

Seq(expr.dataType.asInstanceOf[ObjectType].cls)) ++ Seq(classOf[Object])
val methods = paramTypes.flatMap { fieldClass =>
try {
Some(beanClass.getDeclaredMethod(name, fieldClass))
} catch {
case e: NoSuchMethodException => None
}
}
if (methods.isEmpty) {
throw new NoSuchMethodException(s"""A method named "$name" is not declared """ +
"in any enclosing class nor any supertype")
}
methods.head -> expr
}
}

override def eval(input: InternalRow): Any = {
val instance = beanInstance.eval(input)
if (instance != null) {
val bean = instance.asInstanceOf[Object]
resolvedSetters.foreach {
case (setter, expr) =>
setter.invoke(bean, expr.eval(input).asInstanceOf[AnyRef])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ok. Done.

}
}
instance
}

override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val instanceGen = beanInstance.genCode(ctx)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,8 @@ trait ExpressionEvalHelper extends GeneratorDrivenPropertyChecks {

protected def checkEvaluation(
expression: => Expression, expected: Any, inputRow: InternalRow = EmptyRow): Unit = {
val expr = prepareEvaluation(expression)
// Make it as method to obtain fresh expression everytime.

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Member Author

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
Copy Markdown
Contributor

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 = prepareEvaluation(expression)
val catalystValue = CatalystTypeConverters.convertToCatalyst(expected)
checkEvaluationWithoutCodegen(expr, catalystValue, inputRow)
checkEvaluationWithGeneratedMutableProjection(expr, catalystValue, inputRow)
Expand Down Expand Up @@ -111,12 +112,14 @@ trait ExpressionEvalHelper extends GeneratorDrivenPropertyChecks {
val errMsg = intercept[T] {
eval
}.getMessage
if (errMsg != expectedErrMsg) {
if (!errMsg.contains(expectedErrMsg)) {

@viirya viirya Mar 8, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

fail(s"Expected error message is `$expectedErrMsg`, but `$errMsg` found")
}
}
}
val expr = prepareEvaluation(expression)

// Make it as method to obtain fresh expression everytime.
def expr = prepareEvaluation(expression)
checkException(evaluateWithoutCodegen(expr, inputRow), "non-codegen mode")
checkException(evaluateWithGeneratedMutableProjection(expr, inputRow), "codegen mode")
if (GenerateUnsafeProjection.canSupport(expr.dataType)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,32 @@ class ObjectExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
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()))

val initializeWithNonexistingMethod = InitializeJavaBean(
Literal.fromObject(new java.util.LinkedList[Int]),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@viirya viirya Mar 27, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Map("nonexisting" -> Literal(1)))
checkExceptionInExpression[Exception](initializeWithNonexistingMethod,
InternalRow.fromSeq(Seq()),
"""A method named "nonexisting" is not declared in any enclosing class """ +
"nor any supertype")

val initializeWithWrongParamType = InitializeJavaBean(
Literal.fromObject(new TestBean),
Map("setX" -> Literal("1")))
intercept[Exception] {
evaluateWithoutCodegen(initializeWithWrongParamType, InternalRow.fromSeq(Seq()))
}.getMessage.contains(

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

"""A method named "setX" is not declared in any enclosing class """ +
"nor any supertype")
}

test("SPARK-23585: UnwrapOption should support interpreted execution") {
val cls = classOf[Option[Int]]
val inputObject = BoundReference(0, ObjectType(cls), nullable = true)
Expand Down Expand Up @@ -110,3 +136,9 @@ class ObjectExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
"The 0th field 'c0' of input row cannot be null.")
}
}

class TestBean extends Serializable {
private var x: Int = 0

def setX(i: Int): Unit = x = i
}