[GitHub] spark pull request #20753: [SPARK-23582][SQL] StaticInvoke should support in...

2018-04-05 Thread asfgit
Github user asfgit closed the pull request at:

https://github.com/apache/spark/pull/20753


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20753: [SPARK-23582][SQL] StaticInvoke should support in...

2018-03-27 Thread kiszk
Github user kiszk commented on a diff in the pull request:

https://github.com/apache/spark/pull/20753#discussion_r177631719
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
 ---
@@ -184,12 +217,21 @@ case class StaticInvoke(
 returnNullable: Boolean = true) extends InvokeLike {
 
   val objectName = staticObject.getName.stripSuffix("$")
+  val cls = if (staticObject.getName == objectName) {
+staticObject
+  } else {
+Utils.classForName(objectName)
+  }
 
   override def nullable: Boolean = needNullCheck || returnNullable
   override def children: Seq[Expression] = arguments
 
-  override def eval(input: InternalRow): Any =
-throw new UnsupportedOperationException("Only code-generated 
evaluation is supported.")
+  lazy val argClasses = ScalaReflection.expressionJavaClasses(arguments)
--- End diff --

We need to make this `lazy`. This is because `StaticInvoke` is also called 
before `dataType` is resolved.
For example, `test.org.apache.spark.sql.JavaDatasetSuite.test` was failed 
without `lazy`.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20753: [SPARK-23582][SQL] StaticInvoke should support in...

2018-03-27 Thread kiszk
Github user kiszk commented on a diff in the pull request:

https://github.com/apache/spark/pull/20753#discussion_r177628991
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
 ---
@@ -188,8 +189,30 @@ case class StaticInvoke(
   override def nullable: Boolean = needNullCheck || returnNullable
   override def children: Seq[Expression] = arguments
 
-  override def eval(input: InternalRow): Any =
-throw new UnsupportedOperationException("Only code-generated 
evaluation is supported.")
+  override def eval(input: InternalRow): Any = {
+val args = arguments.map(e => e.eval(input).asInstanceOf[Object])
+val argClasses = ScalaReflection.expressionJavaClasses(arguments)
+val cls = if (staticObject.getName == objectName) {
+  staticObject
+} else {
+  Utils.classForName(objectName)
+}
+val method = cls.getDeclaredMethod(functionName, argClasses : _*)
--- End diff --

Yeah, finally I added `@transient lazy`.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20753: [SPARK-23582][SQL] StaticInvoke should support in...

2018-03-27 Thread hvanhovell
Github user hvanhovell commented on a diff in the pull request:

https://github.com/apache/spark/pull/20753#discussion_r177480722
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
 ---
@@ -188,8 +189,30 @@ case class StaticInvoke(
   override def nullable: Boolean = needNullCheck || returnNullable
   override def children: Seq[Expression] = arguments
 
-  override def eval(input: InternalRow): Any =
-throw new UnsupportedOperationException("Only code-generated 
evaluation is supported.")
+  override def eval(input: InternalRow): Any = {
+val args = arguments.map(e => e.eval(input).asInstanceOf[Object])
+val argClasses = ScalaReflection.expressionJavaClasses(arguments)
+val cls = if (staticObject.getName == objectName) {
+  staticObject
+} else {
+  Utils.classForName(objectName)
+}
+val method = cls.getDeclaredMethod(functionName, argClasses : _*)
--- End diff --

@kiszk just to be absolutely sure: you were seeing this before making the 
val lazy? Anyway please add `@transient` here.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20753: [SPARK-23582][SQL] StaticInvoke should support in...

2018-03-27 Thread kiszk
Github user kiszk commented on a diff in the pull request:

https://github.com/apache/spark/pull/20753#discussion_r177468352
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
 ---
@@ -188,8 +189,30 @@ case class StaticInvoke(
   override def nullable: Boolean = needNullCheck || returnNullable
   override def children: Seq[Expression] = arguments
 
-  override def eval(input: InternalRow): Any =
-throw new UnsupportedOperationException("Only code-generated 
evaluation is supported.")
+  override def eval(input: InternalRow): Any = {
+val args = arguments.map(e => e.eval(input).asInstanceOf[Object])
+val argClasses = ScalaReflection.expressionJavaClasses(arguments)
+val cls = if (staticObject.getName == objectName) {
+  staticObject
+} else {
+  Utils.classForName(objectName)
+}
+val method = cls.getDeclaredMethod(functionName, argClasses : _*)
--- End diff --

Naive static resolution caused the following Serialization exception. Now, 
I succeeded to resolve once using `lazy` as follows:
```
  lazy val method = {
val argClasses = ScalaReflection.expressionJavaClasses(arguments)
val cls = if (staticObject.getName == objectName) {
  staticObject
} else {
  Utils.classForName(objectName)
}
cls.getDeclaredMethod(functionName, argClasses : _*)
  }
```

```
java.lang.reflect.Method
Serialization stack:
- object not serializable (class: java.lang.reflect.Method, value: 
public static java.lang.Boolean java.lang.Boolean.valueOf(java.lang.String))
- field (class: 
org.apache.spark.sql.catalyst.expressions.objects.StaticInvoke, name: method, 
type: class java.lang.reflect.Method)
- object (class 
org.apache.spark.sql.catalyst.expressions.objects.StaticInvoke, 
staticinvoke(class java.lang.Boolean, ObjectType(class java.lang.Boolean), 
valueOf, input[0, java.lang.String, true], true, true))
java.io.NotSerializableException: java.lang.reflect.Method
Serialization stack:
- object not serializable (class: java.lang.reflect.Method, value: 
public static java.lang.Boolean java.lang.Boolean.valueOf(java.lang.String))
- field (class: 
org.apache.spark.sql.catalyst.expressions.objects.StaticInvoke, name: method, 
type: class java.lang.reflect.Method)
- object (class 
org.apache.spark.sql.catalyst.expressions.objects.StaticInvoke, 
staticinvoke(class java.lang.Boolean, ObjectType(class java.lang.Boolean), 
valueOf, input[0, java.lang.String, true], true, true))
at 
org.apache.spark.serializer.SerializationDebugger$.improveException(SerializationDebugger.scala:40)
at 
org.apache.spark.serializer.JavaSerializationStream.writeObject(JavaSerializer.scala:46)
at 
org.apache.spark.serializer.JavaSerializerInstance.serialize(JavaSerializer.scala:100)
at 
org.apache.spark.sql.catalyst.expressions.ObjectExpressionsSuite.org$apache$spark$sql$catalyst$expressions$ObjectExpressionsSuite$$checkObjectExprEvaluation(ObjectExpressionsSuite.scala:180)
at 
org.apache.spark.sql.catalyst.expressions.ObjectExpressionsSuite$$anonfun$3$$anonfun$apply$mcV$sp$1.apply(ObjectExpressionsSuite.scala:99)
at 
org.apache.spark.sql.catalyst.expressions.ObjectExpressionsSuite$$anonfun$3$$anonfun$apply$mcV$sp$1.apply(ObjectExpressionsSuite.scala:98)
at scala.collection.immutable.List.foreach(List.scala:381)
at 
org.apache.spark.sql.catalyst.expressions.ObjectExpressionsSuite$$anonfun$3.apply$mcV$sp(ObjectExpressionsSuite.scala:98)
at 
org.apache.spark.sql.catalyst.expressions.ObjectExpressionsSuite$$anonfun$3.apply(ObjectExpressionsSuite.scala:90)
at 
org.apache.spark.sql.catalyst.expressions.ObjectExpressionsSuite$$anonfun$3.apply(ObjectExpressionsSuite.scala:90)
...
```


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20753: [SPARK-23582][SQL] StaticInvoke should support in...

2018-03-27 Thread hvanhovell
Github user hvanhovell commented on a diff in the pull request:

https://github.com/apache/spark/pull/20753#discussion_r177413074
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
 ---
@@ -188,8 +189,30 @@ case class StaticInvoke(
   override def nullable: Boolean = needNullCheck || returnNullable
   override def children: Seq[Expression] = arguments
 
-  override def eval(input: InternalRow): Any =
-throw new UnsupportedOperationException("Only code-generated 
evaluation is supported.")
+  override def eval(input: InternalRow): Any = {
+val args = arguments.map(e => e.eval(input).asInstanceOf[Object])
+val argClasses = ScalaReflection.expressionJavaClasses(arguments)
+val cls = if (staticObject.getName == objectName) {
+  staticObject
+} else {
+  Utils.classForName(objectName)
+}
+val method = cls.getDeclaredMethod(functionName, argClasses : _*)
+if (needNullCheck && args.exists(_ == null)) {
+  // return null if one of arguments is null
+  null
+} else {
+  val ret = method.invoke(null, args: _*)
+
+  if (CodeGenerator.defaultValue(dataType) == "null") {
--- End diff --

This is a bit scary. Can you just use 
`ScalaReflection.typeBoxedJavaMapping`?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20753: [SPARK-23582][SQL] StaticInvoke should support in...

2018-03-27 Thread hvanhovell
Github user hvanhovell commented on a diff in the pull request:

https://github.com/apache/spark/pull/20753#discussion_r177411749
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
 ---
@@ -188,8 +189,30 @@ case class StaticInvoke(
   override def nullable: Boolean = needNullCheck || returnNullable
   override def children: Seq[Expression] = arguments
 
-  override def eval(input: InternalRow): Any =
-throw new UnsupportedOperationException("Only code-generated 
evaluation is supported.")
+  override def eval(input: InternalRow): Any = {
+val args = arguments.map(e => e.eval(input).asInstanceOf[Object])
+val argClasses = ScalaReflection.expressionJavaClasses(arguments)
+val cls = if (staticObject.getName == objectName) {
+  staticObject
+} else {
+  Utils.classForName(objectName)
+}
+val method = cls.getDeclaredMethod(functionName, argClasses : _*)
--- End diff --

Why are we resolving this during evaluation, shouldn't we be able to 
determine this statically?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20753: [SPARK-23582][SQL] StaticInvoke should support in...

2018-03-08 Thread maropu
Github user maropu commented on a diff in the pull request:

https://github.com/apache/spark/pull/20753#discussion_r173361869
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ObjectExpressionsSuite.scala
 ---
@@ -95,6 +162,21 @@ class ObjectExpressionsSuite extends SparkFunSuite with 
ExpressionEvalHelper {
 checkEvaluation(createExternalRow, Row.fromSeq(Seq(1, "x")), 
InternalRow.fromSeq(Seq()))
   }
 
+  // This is an alternative version of `checkEvaluation` to compare results
--- End diff --

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


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20753: [SPARK-23582][SQL] StaticInvoke should support in...

2018-03-08 Thread kiszk
Github user kiszk commented on a diff in the pull request:

https://github.com/apache/spark/pull/20753#discussion_r173361648
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
 ---
@@ -188,8 +189,30 @@ case class StaticInvoke(
   override def nullable: Boolean = needNullCheck || returnNullable
   override def children: Seq[Expression] = arguments
 
-  override def eval(input: InternalRow): Any =
-throw new UnsupportedOperationException("Only code-generated 
evaluation is supported.")
+  override def eval(input: InternalRow): Any = {
+val args = arguments.map(e => e.eval(input).asInstanceOf[Object])
+val argClasses = 
CallMethodViaReflection.expressionJavaClasses(arguments)
+val cls = if (staticObject.getName == objectName) {
+  staticObject
+} else {
+  Utils.classForName(objectName)
+}
+val method = cls.getDeclaredMethod(functionName, argClasses : _*)
+if (needNullCheck && args.exists(_ == null)) {
+  // return null if one of arguments is null
+  null
+} else {
+  val ret = method.invoke(null, args: _*)
+
+  if (CodeGenerator.defaultValue(dataType) == "null") {
+ret
+  } else {
+// cast a primitive value using Boxed class
+val boxedClass = 
CallMethodViaReflection.typeBoxedJavaMapping(dataType)
--- End diff --

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


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20753: [SPARK-23582][SQL] StaticInvoke should support in...

2018-03-08 Thread kiszk
Github user kiszk commented on a diff in the pull request:

https://github.com/apache/spark/pull/20753#discussion_r173339081
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ObjectExpressionsSuite.scala
 ---
@@ -95,6 +162,21 @@ class ObjectExpressionsSuite extends SparkFunSuite with 
ExpressionEvalHelper {
 checkEvaluation(createExternalRow, Row.fromSeq(Seq(1, "x")), 
InternalRow.fromSeq(Seq()))
   }
 
+  // This is an alternative version of `checkEvaluation` to compare results
--- End diff --

This code is intentionally imported for testing from 
https://github.com/apache/spark/pull/20757


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20753: [SPARK-23582][SQL] StaticInvoke should support in...

2018-03-08 Thread viirya
Github user viirya commented on a diff in the pull request:

https://github.com/apache/spark/pull/20753#discussion_r173327074
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/CallMethodViaReflection.scala
 ---
@@ -127,6 +128,52 @@ object CallMethodViaReflection {
 StringType -> Seq(classOf[String])
   )
 
+  val typeJavaMapping = Map[DataType, Class[_]](
+BooleanType -> classOf[Boolean],
+ByteType -> classOf[Byte],
+ShortType -> classOf[Short],
+IntegerType -> classOf[Int],
+LongType -> classOf[Long],
+FloatType -> classOf[Float],
+DoubleType -> classOf[Double],
+StringType -> classOf[UTF8String],
+DateType -> classOf[DateType.InternalType],
+TimestampType -> classOf[TimestampType.InternalType],
+BinaryType -> classOf[BinaryType.InternalType],
+CalendarIntervalType -> classOf[CalendarInterval]
+  )
+
+  val typeBoxedJavaMapping = Map[DataType, Class[_]](
+BooleanType -> classOf[java.lang.Boolean],
+ByteType -> classOf[java.lang.Byte],
+ShortType -> classOf[java.lang.Short],
+IntegerType -> classOf[java.lang.Integer],
+LongType -> classOf[java.lang.Long],
+FloatType -> classOf[java.lang.Float],
+DoubleType -> classOf[java.lang.Double],
+DateType -> classOf[java.lang.Integer],
+TimestampType -> classOf[java.lang.Long]
+  )
+
+  def dataTypeJavaClass(dt: DataType): Class[_] = {
+dt match {
+  case _: DecimalType => classOf[Decimal]
+  case _: StructType => classOf[InternalRow]
+  case _: ArrayType => classOf[ArrayData]
+  case _: MapType => classOf[MapData]
+  case ObjectType(cls) => cls
+  case _ => typeJavaMapping.getOrElse(dt, classOf[java.lang.Object])
+}
+  }
--- End diff --

The above should be in `CallMethodViaReflection` or `CodeGenerator`?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20753: [SPARK-23582][SQL] StaticInvoke should support in...

2018-03-08 Thread viirya
Github user viirya commented on a diff in the pull request:

https://github.com/apache/spark/pull/20753#discussion_r173326718
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
 ---
@@ -188,8 +189,30 @@ case class StaticInvoke(
   override def nullable: Boolean = needNullCheck || returnNullable
   override def children: Seq[Expression] = arguments
 
-  override def eval(input: InternalRow): Any =
-throw new UnsupportedOperationException("Only code-generated 
evaluation is supported.")
+  override def eval(input: InternalRow): Any = {
+val args = arguments.map(e => e.eval(input).asInstanceOf[Object])
+val argClasses = 
CallMethodViaReflection.expressionJavaClasses(arguments)
+val cls = if (staticObject.getName == objectName) {
+  staticObject
+} else {
+  Utils.classForName(objectName)
+}
+val method = cls.getDeclaredMethod(functionName, argClasses : _*)
+if (needNullCheck && args.exists(_ == null)) {
+  // return null if one of arguments is null
+  null
+} else {
+  val ret = method.invoke(null, args: _*)
+
+  if (CodeGenerator.defaultValue(dataType) == "null") {
+ret
+  } else {
+// cast a primitive value using Boxed class
+val boxedClass = 
CallMethodViaReflection.typeBoxedJavaMapping(dataType)
--- End diff --

nit: I think we can directly do like this without the `if`.
```scala
val boxedClass = CallMethodViaReflection.typeBoxedJavaMapping.get(dataType)
boxedClass.map(_.cast(ret)).getOrElse(ret)
```


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20753: [SPARK-23582][SQL] StaticInvoke should support in...

2018-03-08 Thread kiszk
Github user kiszk commented on a diff in the pull request:

https://github.com/apache/spark/pull/20753#discussion_r173159210
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
 ---
@@ -188,8 +189,32 @@ case class StaticInvoke(
   override def nullable: Boolean = needNullCheck || returnNullable
   override def children: Seq[Expression] = arguments
 
-  override def eval(input: InternalRow): Any =
-throw new UnsupportedOperationException("Only code-generated 
evaluation is supported.")
+
--- End diff --

Will remove these lines in the next commit


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20753: [SPARK-23582][SQL] StaticInvoke should support in...

2018-03-08 Thread kiszk
Github user kiszk commented on a diff in the pull request:

https://github.com/apache/spark/pull/20753#discussion_r173158406
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ObjectExpressionsSuite.scala
 ---
@@ -71,6 +79,66 @@ class ObjectExpressionsSuite extends SparkFunSuite with 
ExpressionEvalHelper {
   mapEncoder.serializer.head, mapExpected, mapInputRow)
   }
 
+  test("SPARK-23582: StaticInvoke should support interpreted execution") {
--- End diff --

Will use `checkObjectExprEvaluation` in 
https://github.com/apache/spark/pull/20757/files


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20753: [SPARK-23582][SQL] StaticInvoke should support in...

2018-03-08 Thread kiszk
Github user kiszk commented on a diff in the pull request:

https://github.com/apache/spark/pull/20753#discussion_r173136700
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
 ---
@@ -133,8 +134,21 @@ case class StaticInvoke(
   override def nullable: Boolean = needNullCheck || returnNullable
   override def children: Seq[Expression] = arguments
 
-  override def eval(input: InternalRow): Any =
-throw new UnsupportedOperationException("Only code-generated 
evaluation is supported.")
+  override def eval(input: InternalRow): Any = {
+if (staticObject == null) {
+  throw new RuntimeException("The static class cannot be null.")
+}
+
+val parmTypes = arguments.map(e =>
+  CallMethodViaReflection.typeMapping.getOrElse(e.dataType,
+Seq(e.dataType.asInstanceOf[ObjectType].cls))(0))
+val parms = arguments.map(e => e.eval(input).asInstanceOf[Object])
+val method = staticObject.getDeclaredMethod(functionName, parmTypes : 
_*)
--- End diff --

Unfortunately, this movement caused failure of reflection.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20753: [SPARK-23582][SQL] StaticInvoke should support in...

2018-03-07 Thread rednaxelafx
Github user rednaxelafx commented on a diff in the pull request:

https://github.com/apache/spark/pull/20753#discussion_r173075682
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
 ---
@@ -133,8 +134,21 @@ case class StaticInvoke(
   override def nullable: Boolean = needNullCheck || returnNullable
   override def children: Seq[Expression] = arguments
 
-  override def eval(input: InternalRow): Any =
-throw new UnsupportedOperationException("Only code-generated 
evaluation is supported.")
+  override def eval(input: InternalRow): Any = {
+if (staticObject == null) {
--- End diff --

We don't need this check, for sure. See line 132,
```
val objectName = staticObject.getName.stripSuffix("$")
```
^^ this will run as a part of the constructor, which will throw an NPE if 
staticObject is `null`, so it's redundant to null check it here.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20753: [SPARK-23582][SQL] StaticInvoke should support in...

2018-03-07 Thread viirya
Github user viirya commented on a diff in the pull request:

https://github.com/apache/spark/pull/20753#discussion_r173064355
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
 ---
@@ -133,8 +134,21 @@ case class StaticInvoke(
   override def nullable: Boolean = needNullCheck || returnNullable
   override def children: Seq[Expression] = arguments
 
-  override def eval(input: InternalRow): Any =
-throw new UnsupportedOperationException("Only code-generated 
evaluation is supported.")
+  override def eval(input: InternalRow): Any = {
+if (staticObject == null) {
+  throw new RuntimeException("The static class cannot be null.")
+}
+
+val parmTypes = arguments.map(e =>
+  CallMethodViaReflection.typeMapping.getOrElse(e.dataType,
+Seq(e.dataType.asInstanceOf[ObjectType].cls))(0))
+val parms = arguments.map(e => e.eval(input).asInstanceOf[Object])
+val method = staticObject.getDeclaredMethod(functionName, parmTypes : 
_*)
--- End diff --

Looks like we can also move `getDeclaredMethod` outside `eval`?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20753: [SPARK-23582][SQL] StaticInvoke should support in...

2018-03-07 Thread kiszk
Github user kiszk commented on a diff in the pull request:

https://github.com/apache/spark/pull/20753#discussion_r172787347
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
 ---
@@ -133,8 +134,21 @@ case class StaticInvoke(
   override def nullable: Boolean = needNullCheck || returnNullable
   override def children: Seq[Expression] = arguments
 
-  override def eval(input: InternalRow): Any =
-throw new UnsupportedOperationException("Only code-generated 
evaluation is supported.")
+  override def eval(input: InternalRow): Any = {
+if (staticObject == null) {
+  throw new RuntimeException("The static class cannot be null.")
+}
+
+val parmTypes = arguments.map(e =>
+  CallMethodViaReflection.typeMapping.getOrElse(e.dataType,
+Seq(e.dataType.asInstanceOf[ObjectType].cls))(0))
+val parms = arguments.map(e => e.eval(input).asInstanceOf[Object])
--- End diff --

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

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



---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20753: [SPARK-23582][SQL] StaticInvoke should support in...

2018-03-06 Thread maropu
Github user maropu commented on a diff in the pull request:

https://github.com/apache/spark/pull/20753#discussion_r172755611
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
 ---
@@ -133,8 +134,21 @@ case class StaticInvoke(
   override def nullable: Boolean = needNullCheck || returnNullable
   override def children: Seq[Expression] = arguments
 
-  override def eval(input: InternalRow): Any =
-throw new UnsupportedOperationException("Only code-generated 
evaluation is supported.")
+  override def eval(input: InternalRow): Any = {
+if (staticObject == null) {
+  throw new RuntimeException("The static class cannot be null.")
+}
+
+val parmTypes = arguments.map(e =>
+  CallMethodViaReflection.typeMapping.getOrElse(e.dataType,
+Seq(e.dataType.asInstanceOf[ObjectType].cls))(0))
+val parms = arguments.map(e => e.eval(input).asInstanceOf[Object])
--- End diff --

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


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20753: [SPARK-23582][SQL] StaticInvoke should support in...

2018-03-06 Thread maropu
Github user maropu commented on a diff in the pull request:

https://github.com/apache/spark/pull/20753#discussion_r172754548
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
 ---
@@ -133,8 +134,21 @@ case class StaticInvoke(
   override def nullable: Boolean = needNullCheck || returnNullable
   override def children: Seq[Expression] = arguments
 
-  override def eval(input: InternalRow): Any =
-throw new UnsupportedOperationException("Only code-generated 
evaluation is supported.")
+  override def eval(input: InternalRow): Any = {
+if (staticObject == null) {
--- End diff --

We need this check?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20753: [SPARK-23582][SQL] StaticInvoke should support in...

2018-03-06 Thread kiszk
Github user kiszk commented on a diff in the pull request:

https://github.com/apache/spark/pull/20753#discussion_r172722081
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
 ---
@@ -133,8 +134,21 @@ case class StaticInvoke(
   override def nullable: Boolean = needNullCheck || returnNullable
   override def children: Seq[Expression] = arguments
 
-  override def eval(input: InternalRow): Any =
-throw new UnsupportedOperationException("Only code-generated 
evaluation is supported.")
+  override def eval(input: InternalRow): Any = {
+if (staticObject == null) {
+  throw new RuntimeException("The static class cannot be null.")
+}
+
+val parmTypes = arguments.map(e =>
+  CallMethodViaReflection.typeMapping.getOrElse(e.dataType,
+Seq(e.dataType.asInstanceOf[ObjectType].cls))(0))
--- End diff --

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



---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20753: [SPARK-23582][SQL] StaticInvoke should support in...

2018-03-06 Thread viirya
Github user viirya commented on a diff in the pull request:

https://github.com/apache/spark/pull/20753#discussion_r172718446
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
 ---
@@ -133,8 +134,21 @@ case class StaticInvoke(
   override def nullable: Boolean = needNullCheck || returnNullable
   override def children: Seq[Expression] = arguments
 
-  override def eval(input: InternalRow): Any =
-throw new UnsupportedOperationException("Only code-generated 
evaluation is supported.")
+  override def eval(input: InternalRow): Any = {
+if (staticObject == null) {
+  throw new RuntimeException("The static class cannot be null.")
+}
+
+val parmTypes = arguments.map(e =>
+  CallMethodViaReflection.typeMapping.getOrElse(e.dataType,
+Seq(e.dataType.asInstanceOf[ObjectType].cls))(0))
--- End diff --

The external types of native types `CalendarIntervalType` and `BinaryType` 
are not `ObjectType`.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20753: [SPARK-23582][SQL] StaticInvoke should support in...

2018-03-06 Thread viirya
Github user viirya commented on a diff in the pull request:

https://github.com/apache/spark/pull/20753#discussion_r172717961
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
 ---
@@ -133,8 +134,21 @@ case class StaticInvoke(
   override def nullable: Boolean = needNullCheck || returnNullable
   override def children: Seq[Expression] = arguments
 
-  override def eval(input: InternalRow): Any =
-throw new UnsupportedOperationException("Only code-generated 
evaluation is supported.")
+  override def eval(input: InternalRow): Any = {
+if (staticObject == null) {
+  throw new RuntimeException("The static class cannot be null.")
+}
+
+val parmTypes = arguments.map(e =>
+  CallMethodViaReflection.typeMapping.getOrElse(e.dataType,
+Seq(e.dataType.asInstanceOf[ObjectType].cls))(0))
+val parms = arguments.map(e => e.eval(input).asInstanceOf[Object])
+val method = staticObject.getDeclaredMethod(functionName, parmTypes : 
_*)
+val ret = method.invoke(null, parms : _*)
+val retClass = CallMethodViaReflection.typeMapping.getOrElse(dataType,
+  Seq(dataType.asInstanceOf[ObjectType].cls))(0)
--- End diff --

Will `dataType` always be an `ObjectType` here?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20753: [SPARK-23582][SQL] StaticInvoke should support in...

2018-03-06 Thread kiszk
Github user kiszk commented on a diff in the pull request:

https://github.com/apache/spark/pull/20753#discussion_r172713356
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
 ---
@@ -133,8 +134,21 @@ case class StaticInvoke(
   override def nullable: Boolean = needNullCheck || returnNullable
   override def children: Seq[Expression] = arguments
 
-  override def eval(input: InternalRow): Any =
-throw new UnsupportedOperationException("Only code-generated 
evaluation is supported.")
+  override def eval(input: InternalRow): Any = {
+if (staticObject == null) {
+  throw new RuntimeException("The static class cannot be null.")
+}
+
+val parmTypes = arguments.map(e =>
+  CallMethodViaReflection.typeMapping.getOrElse(e.dataType,
+Seq(e.dataType.asInstanceOf[ObjectType].cls))(0))
+val parms = arguments.map(e => e.eval(input).asInstanceOf[Object])
+val method = staticObject.getDeclaredMethod(functionName, parmTypes : 
_*)
+val ret = method.invoke(null, parms : _*)
+val retClass = CallMethodViaReflection.typeMapping.getOrElse(dataType,
--- End diff --

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


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20753: [SPARK-23582][SQL] StaticInvoke should support in...

2018-03-06 Thread kiszk
GitHub user kiszk opened a pull request:

https://github.com/apache/spark/pull/20753

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

## What changes were proposed in this pull request?

This pr added interpreted execution for `StaticInvoke`.

## How was this patch tested?

Added tests in `ObjectExpressionsSuite`.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/kiszk/spark SPARK-23582

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/20753.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 #20753


commit 0790deabf238201451438d2b95f108ced9bcd027
Author: Kazuaki Ishizaki 
Date:   2018-03-07T00:30:40Z

initial prototype




---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org