[GitHub] spark pull request #20778: [SPARK-23584][SQL] NewInstance should support int...

[asfgit]

Github user asfgit closed the pull request at:

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


---

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

[GitHub] spark pull request #20778: [SPARK-23584][SQL] NewInstance should support int...

[maropu]

Github user maropu commented on a diff in the pull request:

https://github.com/apache/spark/pull/20778#discussion_r180975252
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
 ---
@@ -392,8 +392,44 @@ case class NewInstance(
 childrenResolved && !needOuterPointer
   }
 
-  override def eval(input: InternalRow): Any =
-throw new UnsupportedOperationException("Only code-generated 
evaluation is supported.")
+  private lazy val constructor: (Seq[AnyRef]) => Any = {
+val paramTypes = arguments.map { expr =>
+  CallMethodViaReflection.typeMapping.getOrElse(expr.dataType,
+Seq(expr.dataType.asInstanceOf[ObjectType].cls))
+}
+val findConstructor = (types: Seq[Seq[Class[_]]]) => {
+  val constructorOption = cls.getConstructors.find { c =>
--- End diff --

I checked the resolution rule in `Class.getConstructor` and I found it only 
supported the exact matching for parameter types (so, we couldn't handle class 
inheritances correctly). Then, in the latest fix, I tried to use 
[getMatchingAccessibleConstructor](https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/reflect/ConstructorUtils.html#getMatchingAccessibleConstructor-java.lang.Class-java.lang.Class...-)
 in `commons.lang3`. It seems useful to find an acceptable constructor in a 
class. How about this? @hvanhovell @viirya 


---

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

[GitHub] spark pull request #20778: [SPARK-23584][SQL] NewInstance should support int...

[hvanhovell]

Github user hvanhovell commented on a diff in the pull request:

https://github.com/apache/spark/pull/20778#discussion_r179464207
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
 ---
@@ -392,8 +392,44 @@ case class NewInstance(
 childrenResolved && !needOuterPointer
   }
 
-  override def eval(input: InternalRow): Any =
-throw new UnsupportedOperationException("Only code-generated 
evaluation is supported.")
+  private lazy val constructor: (Seq[AnyRef]) => Any = {
+val paramTypes = arguments.map { expr =>
+  CallMethodViaReflection.typeMapping.getOrElse(expr.dataType,
+Seq(expr.dataType.asInstanceOf[ObjectType].cls))
+}
+val findConstructor = (types: Seq[Seq[Class[_]]]) => {
+  val constructorOption = cls.getConstructors.find { c =>
--- End diff --

How does this compare to JVM resolution rules? The code generated version 
will follow those.


---

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

[GitHub] spark pull request #20778: [SPARK-23584][SQL] NewInstance should support int...

[maropu]

Github user maropu commented on a diff in the pull request:

https://github.com/apache/spark/pull/20778#discussion_r173382375
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
 ---
@@ -392,8 +392,44 @@ case class NewInstance(
 childrenResolved && !needOuterPointer
   }
 
-  override def eval(input: InternalRow): Any =
-throw new UnsupportedOperationException("Only code-generated 
evaluation is supported.")
+  private lazy val constructor: (Seq[AnyRef]) => Any = {
+val paramTypes = arguments.map { expr =>
+  CallMethodViaReflection.typeMapping.getOrElse(expr.dataType,
+Seq(expr.dataType.asInstanceOf[ObjectType].cls))
+}
+val findConstructor = (types: Seq[Seq[Class[_]]]) => {
+  val constructorOption = cls.getConstructors.find { c =>
--- End diff --

ok, I'll re-check.


---

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

[GitHub] spark pull request #20778: [SPARK-23584][SQL] NewInstance should support int...

[viirya]

Github user viirya commented on a diff in the pull request:

https://github.com/apache/spark/pull/20778#discussion_r173370038
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
 ---
@@ -392,8 +392,44 @@ case class NewInstance(
 childrenResolved && !needOuterPointer
   }
 
-  override def eval(input: InternalRow): Any =
-throw new UnsupportedOperationException("Only code-generated 
evaluation is supported.")
+  private lazy val constructor: (Seq[AnyRef]) => Any = {
+val paramTypes = arguments.map { expr =>
+  CallMethodViaReflection.typeMapping.getOrElse(expr.dataType,
+Seq(expr.dataType.asInstanceOf[ObjectType].cls))
+}
+val findConstructor = (types: Seq[Seq[Class[_]]]) => {
+  val constructorOption = cls.getConstructors.find { c =>
--- End diff --

Can we directly call `cls.getConstructor` with required `Class` to get it?


---

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

[GitHub] spark pull request #20778: [SPARK-23584][SQL] NewInstance should support int...

[maropu]

Github user maropu commented on a diff in the pull request:

https://github.com/apache/spark/pull/20778#discussion_r173361185
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ObjectExpressionsSuite.scala
 ---
@@ -138,4 +154,40 @@ class ObjectExpressionsSuite extends SparkFunSuite 
with ExpressionEvalHelper {
   checkEvaluation(decodeUsingSerializer, null, 
InternalRow.fromSeq(Seq(null)))
 }
   }
+
+  // This is an alternative version of `checkEvaluation` to compare results
--- End diff --

This function was copy from #20757, so I'll remove later.


---

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

[GitHub] spark pull request #20778: [SPARK-23584][SQL] NewInstance should support int...

[maropu]

GitHub user maropu opened a pull request:

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

[SPARK-23584][SQL] NewInstance should support interpreted execution

## What changes were proposed in this pull request?
This pr supported interpreted mode for `NewInstance`.

## 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/maropu/spark SPARK-23584

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

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


commit 077a0bf1f66b1224f75880622237add12e9f23db
Author: Takeshi Yamamuro 
Date:   2018-03-08T12:31:39Z

NewInstance should support interpreted execution




---

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