[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user asfgit closed the pull request at: https://github.com/apache/spark/pull/20756 --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r179361159
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -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
+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,
+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])
--- End diff --
Ok. Done.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r179132793
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -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
+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,
+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])
--- End diff --
@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`.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r179108285
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -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
+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,
+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])
--- End diff --
ping @hvanhovell
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r177620534
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -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
+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,
+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])
--- End diff --
@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?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r177411427
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -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
+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,
+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])
--- End diff --
@viirya can you add a null check to both the interpreted and code generated
version? Thanks!
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r177302845
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ObjectExpressionsSuite.scala
---
@@ -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]),
+ 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(
--- End diff --
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.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r177302386
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ObjectExpressionsSuite.scala
---
@@ -68,6 +68,23 @@ 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]),
--- End diff --
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"))`
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user kiszk commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r177029939
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1261,8 +1261,39 @@ 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 = {
+val ObjectType(beanClass) = beanInstance.dataType
+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,
--- End diff --
Done in #20753 and #20797
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r176973244
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -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
+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,
+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])
--- End diff --
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.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r176956857
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ObjectExpressionsSuite.scala
---
@@ -68,6 +68,23 @@ 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]),
--- End diff --
Can you also add a test for when the parameter types do not match up?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r176956802
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -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
+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,
+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])
--- End diff --
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?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r176956651
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1261,8 +1261,39 @@ 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 = {
+val ObjectType(beanClass) = beanInstance.dataType
+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,
--- End diff --
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.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user kiszk commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r173405112
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1261,8 +1261,39 @@ 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 = {
+val ObjectType(beanClass) = beanInstance.dataType
+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,
--- End diff --
I see. Is it better to have separate map variables? cc @hvanhovell
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r173363964
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1261,8 +1261,39 @@ 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 = {
+val ObjectType(beanClass) = beanInstance.dataType
+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,
--- End diff --
If we want to expand the support, maybe we can have another PR to expand it
in `CallMethodViaReflection` and merge those variables.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r173346317
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1261,8 +1261,39 @@ 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 = {
+val ObjectType(beanClass) = beanInstance.dataType
+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,
--- End diff --
Is there any special reason it only supports basically primitives and
string?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r173291374
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -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
+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,
+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, nor through a
static import")
+}
+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[Object])
--- End diff --
`AnyRef` is a bit more scala like, that is all.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user kiszk commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r173224239
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1261,8 +1261,39 @@ 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 = {
+val ObjectType(beanClass) = beanInstance.dataType
+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,
--- End diff --
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
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r173159730
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -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
+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,
+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, nor through a
static import")
+}
+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[Object])
--- End diff --
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`?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r173158104
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1261,8 +1261,39 @@ 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 = {
+val ObjectType(beanClass) = beanInstance.dataType
+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,
--- End diff --
I may have no time to do tonight, if @kiszk do not make those changes, I
will do it tomorrow.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r173156685
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1261,8 +1261,39 @@ 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 = {
+val ObjectType(beanClass) = beanInstance.dataType
+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,
+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, nor through a
static import")
--- End diff --
Removed.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r173152685
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -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
+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,
+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, nor through a
static import")
+}
+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[Object])
--- End diff --
Do we need the cast to `.asInstanceOf[Object]`? And why not use `AnyRef`?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r173152492
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1261,8 +1261,39 @@ 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 = {
+val ObjectType(beanClass) = beanInstance.dataType
+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,
+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, nor through a
static import")
--- End diff --
Can you remove the last bit `, nor through a static import`?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r173152324
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1261,8 +1261,39 @@ 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 = {
+val ObjectType(beanClass) = beanInstance.dataType
+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,
--- End diff --
Ok, who is making those changes? You or @kiszk?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r173086617
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1261,8 +1261,39 @@ 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 = {
+val ObjectType(beanClass) = beanInstance.dataType
--- End diff --
Ok.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r173086568
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1261,8 +1261,39 @@ 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 = {
+val ObjectType(beanClass) = beanInstance.dataType
+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,
+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, nor through a
static import")
+}
+methods.head -> expr
+}
+ }
+
+ override def eval(input: InternalRow): Any = {
+val instance = beanInstance.eval(input).asInstanceOf[Object]
--- End diff --
Ok.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r173070339
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1261,8 +1261,39 @@ 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 = {
+val ObjectType(beanClass) = beanInstance.dataType
--- End diff --
better to put `assert(beanInstance.dataType.isInstanceOf[ObjectType])` in
the constructor?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r173067172
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1261,8 +1261,39 @@ 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 = {
+val ObjectType(beanClass) = beanInstance.dataType
+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,
+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, nor through a
static import")
+}
+methods.head -> expr
+}
+ }
+
+ override def eval(input: InternalRow): Any = {
+val instance = beanInstance.eval(input).asInstanceOf[Object]
--- End diff --
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
```
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r173063007
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1261,8 +1261,39 @@ 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 = {
+val ObjectType(beanClass) = beanInstance.dataType
+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,
--- End diff --
As #20753, we need to add other types into the mapping.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r173062788
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1261,8 +1261,39 @@ 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 = {
+val ObjectType(beanClass) = beanInstance.dataType
+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,
+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, nor through a
static import")
--- End diff --
This error message is copied from compiling error thrown in codegen
execution. So we can test the same error message for both execution.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r173062087
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala
---
@@ -111,12 +112,14 @@ trait ExpressionEvalHelper extends
GeneratorDrivenPropertyChecks {
val errMsg = intercept[T] {
eval
}.getMessage
-if (errMsg != expectedErrMsg) {
+if (!errMsg.contains(expectedErrMsg)) {
--- End diff --
For codegen error, it has longer 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)
```
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r173031229
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1261,8 +1261,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]
+if (instance != null) {
+ setters.foreach { case (setterMethod, fieldExpr) =>
--- End diff --
I see. Thanks! :)
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r172901366
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1261,8 +1261,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]
+if (instance != null) {
+ setters.foreach { case (setterMethod, fieldExpr) =>
--- End diff --
See https://github.com/apache/spark/pull/20753#issuecomment-371195614 :)
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r172881690
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1261,8 +1261,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]
+if (instance != null) {
+ setters.foreach { case (setterMethod, fieldExpr) =>
--- End diff --
Or should we go back to reflection?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r172864835
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1261,8 +1261,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]
+if (instance != null) {
+ setters.foreach { case (setterMethod, fieldExpr) =>
--- End diff --
`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
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?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r172852988
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1261,8 +1261,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]
+if (instance != null) {
+ setters.foreach { case (setterMethod, fieldExpr) =>
--- End diff --
Hmmm it should be lenient. Can you try
`MethodType.methodType(classOf[Object], fieldClass))`?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r172849324
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1261,8 +1261,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]
+if (instance != null) {
+ setters.foreach { case (setterMethod, fieldExpr) =>
--- End diff --
This condition `MethodType.methodType(classOf[Unit], fieldClass))` is more
strict than codegen execution. Codegen execution does not care about return
type. So the method such as `def add(x$1: Int): Boolean` of
`java.util.LinkedList.add` can work for codegen version. But it fails the
method looking here.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r172842972
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala
---
@@ -49,7 +49,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.
--- End diff --
Are we using a literal? Ok, makes sense.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r172837282
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala
---
@@ -49,7 +49,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.
--- End diff --
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]`.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r172836946
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1261,8 +1261,31 @@ 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 = {
+val ObjectType(beanClass) = beanInstance.dataType
+
+setters.map { case (setterMethod, fieldExpr) =>
+ val foundMethods = beanClass.getMethods.filter { method =>
--- End diff --
Will updated later.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r172836786
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1261,8 +1261,31 @@ 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 = {
+val ObjectType(beanClass) = beanInstance.dataType
+
+setters.map { case (setterMethod, fieldExpr) =>
+ val foundMethods = beanClass.getMethods.filter { method =>
--- End diff --
(Picking up our earlier conversation) You are not checking the argument?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r172836291
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala
---
@@ -49,7 +49,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.
--- End diff --
Why this change?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r172834120
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1261,8 +1261,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]
+if (instance != null) {
+ setters.foreach { case (setterMethod, fieldExpr) =>
--- End diff --
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.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r172833264
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1261,8 +1261,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]
+if (instance != null) {
+ setters.foreach { case (setterMethod, fieldExpr) =>
--- End diff --
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.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r172833058
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1261,8 +1261,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]
+if (instance != null) {
+ setters.foreach { case (setterMethod, fieldExpr) =>
--- End diff --
`lookup.findSetter` means direct field access in the `MethodHandle` world.
So you need to use `lookup.findVirtual`.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r172830414
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1261,8 +1261,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]
+if (instance != null) {
+ setters.foreach { case (setterMethod, fieldExpr) =>
--- End diff --
`lookup.findVirtual` or `lookup.findSetter`? Strictly I think
`InitializeJavaBean` should be used only for setters not for general methods.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r172819668
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -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]
+if (instance != null) {
+ setters.foreach { case (setterMethod, fieldExpr) =>
+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 " +
--- End diff --
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).
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r172801767
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1261,8 +1261,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]
+if (instance != null) {
+ setters.foreach { case (setterMethod, fieldExpr) =>
--- End diff --
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):
```scala
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)
resolvedSetters.foreach {
case (setter, expr) =>
setter.invoke(bean, expr.eval(input))
}
bean
}
```
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20756#discussion_r172731452
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -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]
+if (instance != null) {
+ setters.foreach { case (setterMethod, fieldExpr) =>
+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 " +
--- End diff --
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). cc @hvanhovell
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20756: [SPARK-23593][SQL] Add interpreted execution for ...
GitHub user viirya opened a pull request: https://github.com/apache/spark/pull/20756 [SPARK-23593][SQL] Add interpreted execution for InitializeJavaBean expression ## What changes were proposed in this pull request? Add interpreted execution for `InitializeJavaBean` expression. ## How was this patch tested? Added unit test. You can merge this pull request into a Git repository by running: $ git pull https://github.com/viirya/spark-1 SPARK-23593 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/spark/pull/20756.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #20756 commit 978080bd8f5a1b095fd0d58ff529e16dd9cbadba Author: Liang-Chi Hsieh Date: 2018-03-07T02:56:53Z Add interpreted execution for InitializeJavaBean expression. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
