[GitHub] spark pull request #20753: [SPARK-23582][SQL] StaticInvoke should support in...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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 IshizakiDate: 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