[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user asfgit closed the pull request at: https://github.com/apache/spark/pull/23062 --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r235033743
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala
---
@@ -788,12 +788,37 @@ object ScalaReflection extends ScalaReflection {
}
/**
- * Finds an accessible constructor with compatible parameters. This is a
more flexible search
- * than the exact matching algorithm in `Class.getConstructor`. The
first assignment-compatible
- * matching constructor is returned. Otherwise, it returns `None`.
+ * Finds an accessible constructor with compatible parameters. This is a
more flexible search than
+ * the exact matching algorithm in `Class.getConstructor`. The first
assignment-compatible
+ * matching constructor is returned if it exists. Otherwise, we check
for additional compatible
+ * constructors defined in the companion object as `apply` methods.
Otherwise, it returns `None`.
*/
- def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Constructor[_]] = {
-Option(ConstructorUtils.getMatchingAccessibleConstructor(cls,
paramTypes: _*))
+ def findConstructor[T](cls: Class[T], paramTypes: Seq[Class[_]]):
Option[Seq[AnyRef] => T] = {
+Option(ConstructorUtils.getMatchingAccessibleConstructor(cls,
paramTypes: _*)) match {
+ case Some(c) => Some(x => c.newInstance(x: _*).asInstanceOf[T])
--- End diff --
I think this cast isn't needed because Class[T].newInstance returns T
already, but it's fine to leave.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234861969
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/ScalaReflectionSuite.scala
---
@@ -109,6 +109,35 @@ object TestingUDT {
}
}
+/** An example derived from Twitter/Scrooge codegen for thrift */
+object ScroogeLikeExample {
+ def apply(x: Int): ScroogeLikeExample = new Immutable(x)
+
+ def unapply(_item: ScroogeLikeExample): Option[Int] = Some(_item.x)
+
+ class Immutable(val x: Int) extends ScroogeLikeExample
+}
+
+trait ScroogeLikeExample extends Product1[Int] with Serializable {
+ import ScroogeLikeExample._
+
+ def x: Int
+
+ override def _1: Int = x
+
+ def copy(x: Int = this.x): ScroogeLikeExample = new Immutable(x)
+
+ override def canEqual(other: Any): Boolean =
other.isInstanceOf[ScroogeLikeExample]
+
+ private def _equals(x: ScroogeLikeExample, y: ScroogeLikeExample):
Boolean =
+ x.productArity == y.productArity &&
--- End diff --
That's OK, leave in Product, if it's actually testing the case you have in
mind. Yes I know equals() is needed. The new implementation looks good.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user drewrobb commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234861629
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/ScalaReflectionSuite.scala
---
@@ -109,6 +109,35 @@ object TestingUDT {
}
}
+/** An example derived from Twitter/Scrooge codegen for thrift */
+object ScroogeLikeExample {
+ def apply(x: Int): ScroogeLikeExample = new Immutable(x)
+
+ def unapply(_item: ScroogeLikeExample): Option[Int] = Some(_item.x)
+
+ class Immutable(val x: Int) extends ScroogeLikeExample
+}
+
+trait ScroogeLikeExample extends Product1[Int] with Serializable {
+ import ScroogeLikeExample._
+
+ def x: Int
+
+ override def _1: Int = x
+
+ def copy(x: Int = this.x): ScroogeLikeExample = new Immutable(x)
+
+ override def canEqual(other: Any): Boolean =
other.isInstanceOf[ScroogeLikeExample]
+
+ private def _equals(x: ScroogeLikeExample, y: ScroogeLikeExample):
Boolean =
+ x.productArity == y.productArity &&
--- End diff --
I'm worried about changing the tests to use a concrete subtype, because the
reflection calls might behave differently in that case either now or later on.
I simplified a little more. `canEqual` is necessary to implement product.
`equals` is necessary or tests will not pass (it will check object pointer
equality), and `hashCode` is needed for scalastyle to pass since `equals` is
necessary.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234859506
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/ScalaReflectionSuite.scala
---
@@ -109,6 +109,35 @@ object TestingUDT {
}
}
+/** An example derived from Twitter/Scrooge codegen for thrift */
+object ScroogeLikeExample {
+ def apply(x: Int): ScroogeLikeExample = new Immutable(x)
+
+ def unapply(_item: ScroogeLikeExample): Option[Int] = Some(_item.x)
+
+ class Immutable(val x: Int) extends ScroogeLikeExample
+}
+
+trait ScroogeLikeExample extends Product1[Int] with Serializable {
+ import ScroogeLikeExample._
+
+ def x: Int
+
+ override def _1: Int = x
+
+ def copy(x: Int = this.x): ScroogeLikeExample = new Immutable(x)
+
+ override def canEqual(other: Any): Boolean =
other.isInstanceOf[ScroogeLikeExample]
+
+ private def _equals(x: ScroogeLikeExample, y: ScroogeLikeExample):
Boolean =
+ x.productArity == y.productArity &&
--- End diff --
Hm, actually that probably won't work any more or less. OK, it's because
there is an Encoder for Product. You can still simplify the equals() and so on
I think, but looks like that's easier than a new Encoder. Or is it sufficient
to test a Seq of a concrete subtype of ScroogeLikeExample?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234858573
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/ScalaReflectionSuite.scala
---
@@ -109,6 +109,35 @@ object TestingUDT {
}
}
+/** An example derived from Twitter/Scrooge codegen for thrift */
+object ScroogeLikeExample {
+ def apply(x: Int): ScroogeLikeExample = new Immutable(x)
+
+ def unapply(_item: ScroogeLikeExample): Option[Int] = Some(_item.x)
+
+ class Immutable(val x: Int) extends ScroogeLikeExample
+}
+
+trait ScroogeLikeExample extends Product1[Int] with Serializable {
+ import ScroogeLikeExample._
+
+ def x: Int
+
+ override def _1: Int = x
+
+ def copy(x: Int = this.x): ScroogeLikeExample = new Immutable(x)
+
+ override def canEqual(other: Any): Boolean =
other.isInstanceOf[ScroogeLikeExample]
+
+ private def _equals(x: ScroogeLikeExample, y: ScroogeLikeExample):
Boolean =
+ x.productArity == y.productArity &&
--- End diff --
Just use SparkSession.createDataset?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user drewrobb commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234854202
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala
---
@@ -788,12 +788,37 @@ object ScalaReflection extends ScalaReflection {
}
/**
- * Finds an accessible constructor with compatible parameters. This is a
more flexible search
- * than the exact matching algorithm in `Class.getConstructor`. The
first assignment-compatible
- * matching constructor is returned. Otherwise, it returns `None`.
+ * Finds an accessible constructor with compatible parameters. This is a
more flexible search than
+ * the exact matching algorithm in `Class.getConstructor`. The first
assignment-compatible
+ * matching constructor is returned if it exists. Otherwise, we check
for additional compatible
+ * constructors defined in the companion object as `apply` methods.
Otherwise, it returns `None`.
*/
- def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Constructor[_]] = {
-Option(ConstructorUtils.getMatchingAccessibleConstructor(cls,
paramTypes: _*))
+ def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Seq[AnyRef] => Any] = {
--- End diff --
Ok, that makes total sense. I've made this change also
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user drewrobb commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234853788
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/ScalaReflectionSuite.scala
---
@@ -109,6 +109,35 @@ object TestingUDT {
}
}
+/** An example derived from Twitter/Scrooge codegen for thrift */
+object ScroogeLikeExample {
+ def apply(x: Int): ScroogeLikeExample = new Immutable(x)
+
+ def unapply(_item: ScroogeLikeExample): Option[Int] = Some(_item.x)
+
+ class Immutable(val x: Int) extends ScroogeLikeExample
+}
+
+trait ScroogeLikeExample extends Product1[Int] with Serializable {
+ import ScroogeLikeExample._
+
+ def x: Int
+
+ override def _1: Int = x
+
+ def copy(x: Int = this.x): ScroogeLikeExample = new Immutable(x)
+
+ override def canEqual(other: Any): Boolean =
other.isInstanceOf[ScroogeLikeExample]
+
+ private def _equals(x: ScroogeLikeExample, y: ScroogeLikeExample):
Boolean =
+ x.productArity == y.productArity &&
--- End diff --
My previous answer was not complete. `Product1` is also necessary so that
the implicit `Encoders.product[T <: Product : TypeTag]` will work with this
class, if omitted the DatasetSuite test will not compile:
```
[error]
/home/drew/spark/sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala:1577:
value toDS is not a member of
Seq[org.apache.spark.sql.catalyst.ScroogeLikeExample]
[error] val ds = data.toDS
```
I could add some new encoder, but I think that might be worse as the goal
of this PR is for Scrooge classes to work with the provided implicit encoders.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234846910
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/ScalaReflectionSuite.scala
---
@@ -109,6 +109,35 @@ object TestingUDT {
}
}
+/** An example derived from Twitter/Scrooge codegen for thrift */
+object ScroogeLikeExample {
+ def apply(x: Int): ScroogeLikeExample = new Immutable(x)
+
+ def unapply(_item: ScroogeLikeExample): Option[Int] = Some(_item.x)
+
+ class Immutable(val x: Int) extends ScroogeLikeExample
+}
+
+trait ScroogeLikeExample extends Product1[Int] with Serializable {
+ import ScroogeLikeExample._
+
+ def x: Int
+
+ override def _1: Int = x
+
+ def copy(x: Int = this.x): ScroogeLikeExample = new Immutable(x)
+
+ override def canEqual(other: Any): Boolean =
other.isInstanceOf[ScroogeLikeExample]
+
+ private def _equals(x: ScroogeLikeExample, y: ScroogeLikeExample):
Boolean =
+ x.productArity == y.productArity &&
--- End diff --
Sure, but you've already established it's a `ScroogeLikeExample` here. Why
must it be Product1 just to check whether it's also Product1? seems like it's
not adding anything. In fact, why not just compare the one element that this
trait knows about? to the extent it can implement equals() meaningfully, that's
all it is doing already.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234846234
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala
---
@@ -788,12 +788,37 @@ object ScalaReflection extends ScalaReflection {
}
/**
- * Finds an accessible constructor with compatible parameters. This is a
more flexible search
- * than the exact matching algorithm in `Class.getConstructor`. The
first assignment-compatible
- * matching constructor is returned. Otherwise, it returns `None`.
+ * Finds an accessible constructor with compatible parameters. This is a
more flexible search than
+ * the exact matching algorithm in `Class.getConstructor`. The first
assignment-compatible
+ * matching constructor is returned if it exists. Otherwise, we check
for additional compatible
+ * constructors defined in the companion object as `apply` methods.
Otherwise, it returns `None`.
*/
- def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Constructor[_]] = {
-Option(ConstructorUtils.getMatchingAccessibleConstructor(cls,
paramTypes: _*))
+ def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Seq[AnyRef] => Any] = {
--- End diff --
Just cast with: `method.apply(args: _*).asInstanceOf[T]`. That's valid
because that is actually the assumption this code is making. I get no compile
errors with no further changes beyond that. `cls: Class[T]` knows it produces a
T from `newInstance`; did you make that change?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234846491
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala
---
@@ -788,12 +788,37 @@ object ScalaReflection extends ScalaReflection {
}
/**
- * Finds an accessible constructor with compatible parameters. This is a
more flexible search
- * than the exact matching algorithm in `Class.getConstructor`. The
first assignment-compatible
- * matching constructor is returned. Otherwise, it returns `None`.
+ * Finds an accessible constructor with compatible parameters. This is a
more flexible search than
+ * the exact matching algorithm in `Class.getConstructor`. The first
assignment-compatible
+ * matching constructor is returned if it exists. Otherwise, we check
for additional compatible
+ * constructors defined in the companion object as `apply` methods.
Otherwise, it returns `None`.
*/
- def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Constructor[_]] = {
-Option(ConstructorUtils.getMatchingAccessibleConstructor(cls,
paramTypes: _*))
+ def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Seq[AnyRef] => Any] = {
+Option(ConstructorUtils.getMatchingAccessibleConstructor(cls,
paramTypes: _*)) match {
+ case Some(c) => Some((x: Seq[AnyRef]) => c.newInstance(x: _*))
+ case None =>
+val companion = mirror.staticClass(cls.getName).companion
+val moduleMirror = mirror.reflectModule(companion.asModule)
+val applyMethods = companion.asTerm.typeSignature
+ .member(universe.TermName("apply")).asTerm.alternatives
+applyMethods.find { method =>
+ val params = method.typeSignature.paramLists.head
+ // Check that the needed params are the same length and of
matching types
+ params.size == paramTypes.tail.size &&
+ params.zip(paramTypes.tail).forall { case(ps, pc) =>
+ps.typeSignature.typeSymbol == mirror.classSymbol(pc)
+ }
+}.map { applyMethodSymbol =>
+ val expectedArgsCount =
applyMethodSymbol.typeSignature.paramLists.head.size
+ val instanceMirror = mirror.reflect(moduleMirror.instance)
+ val method =
instanceMirror.reflectMethod(applyMethodSymbol.asMethod)
+ (_args: Seq[AnyRef]) => {
+// Drop the "outer" argument if it is provided
+val args = if (_args.size == expectedArgsCount) _args else
_args.tail
--- End diff --
OK, I guess it already works this way, so leave it. But I was suggesting a
check that they're the same Class, not just whether it's a Class. But that's
wrong; 'outer' may be another class. I was somehow thinking of 'this'.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user drewrobb commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234844435
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/ScalaReflectionSuite.scala
---
@@ -109,6 +109,64 @@ object TestingUDT {
}
}
+/** An example derived from Twitter/Scrooge codegen for thrift */
+object ScroogeLikeExample {
+ def apply(
+x: Int
+ ): ScroogeLikeExample =
+new Immutable(
+ x
+)
+
+ def unapply(_item: ScroogeLikeExample): _root_.scala.Option[Int] =
_root_.scala.Some(_item.x)
+
+ class Immutable(val x: Int) extends ScroogeLikeExample
+
+ trait Proxy extends ScroogeLikeExample {
+protected def _underlying_ScroogeLikeExample: ScroogeLikeExample
+override def x: Int = _underlying_ScroogeLikeExample.x
+ }
+}
+
+trait ScroogeLikeExample
+ extends _root_.scala.Product1[Int]
--- End diff --
It provides `productArity` used in `equals` used below.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user drewrobb commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234844425
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/ScalaReflectionSuite.scala
---
@@ -109,6 +109,64 @@ object TestingUDT {
}
}
+/** An example derived from Twitter/Scrooge codegen for thrift */
+object ScroogeLikeExample {
+ def apply(
+x: Int
+ ): ScroogeLikeExample =
+new Immutable(
+ x
+)
+
+ def unapply(_item: ScroogeLikeExample): _root_.scala.Option[Int] =
_root_.scala.Some(_item.x)
+
+ class Immutable(val x: Int) extends ScroogeLikeExample
+
+ trait Proxy extends ScroogeLikeExample {
+protected def _underlying_ScroogeLikeExample: ScroogeLikeExample
+override def x: Int = _underlying_ScroogeLikeExample.x
+ }
+}
+
+trait ScroogeLikeExample
+ extends _root_.scala.Product1[Int]
+ with java.io.Serializable
+{
+ import ScroogeLikeExample._
+
+ def x: Int
+
+ def _1: Int = x
--- End diff --
sure, i can't figure out if it is strictly necessary though
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user drewrobb commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234844322
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -462,12 +462,12 @@ case class NewInstance(
val d = outerObj.getClass +: paramTypes
val c = getConstructor(outerObj.getClass +: paramTypes)
(args: Seq[AnyRef]) => {
-c.newInstance(outerObj +: args: _*)
+c(Seq(outerObj +: args: _*))
--- End diff --
Ah, good catch. I meant to change this before.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user drewrobb commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234844315
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala
---
@@ -973,8 +998,19 @@ trait ScalaReflection extends Logging {
}
}
+ /**
+ * If our type is a Scala trait it may have a companion object that
+ * only defines a constructor via `apply` method.
+ */
+ def getCompanionConstructor(tpe: Type): Symbol = {
--- End diff --
sure
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user drewrobb commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234844306
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala
---
@@ -788,12 +788,37 @@ object ScalaReflection extends ScalaReflection {
}
/**
- * Finds an accessible constructor with compatible parameters. This is a
more flexible search
- * than the exact matching algorithm in `Class.getConstructor`. The
first assignment-compatible
- * matching constructor is returned. Otherwise, it returns `None`.
+ * Finds an accessible constructor with compatible parameters. This is a
more flexible search than
+ * the exact matching algorithm in `Class.getConstructor`. The first
assignment-compatible
+ * matching constructor is returned if it exists. Otherwise, we check
for additional compatible
+ * constructors defined in the companion object as `apply` methods.
Otherwise, it returns `None`.
*/
- def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Constructor[_]] = {
-Option(ConstructorUtils.getMatchingAccessibleConstructor(cls,
paramTypes: _*))
+ def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Seq[AnyRef] => Any] = {
+Option(ConstructorUtils.getMatchingAccessibleConstructor(cls,
paramTypes: _*)) match {
+ case Some(c) => Some((x: Seq[AnyRef]) => c.newInstance(x: _*))
+ case None =>
+val companion = mirror.staticClass(cls.getName).companion
+val moduleMirror = mirror.reflectModule(companion.asModule)
+val applyMethods = companion.asTerm.typeSignature
+ .member(universe.TermName("apply")).asTerm.alternatives
+applyMethods.find { method =>
+ val params = method.typeSignature.paramLists.head
+ // Check that the needed params are the same length and of
matching types
+ params.size == paramTypes.tail.size &&
+ params.zip(paramTypes.tail).forall { case(ps, pc) =>
+ps.typeSignature.typeSymbol == mirror.classSymbol(pc)
+ }
+}.map { applyMethodSymbol =>
+ val expectedArgsCount =
applyMethodSymbol.typeSignature.paramLists.head.size
+ val instanceMirror = mirror.reflect(moduleMirror.instance)
+ val method =
instanceMirror.reflectMethod(applyMethodSymbol.asMethod)
+ (_args: Seq[AnyRef]) => {
+// Drop the "outer" argument if it is provided
+val args = if (_args.size == expectedArgsCount) _args else
_args.tail
--- End diff --
You would still have to check the length, in case the first actual argument
was for some reason also a class.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234833024
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala
---
@@ -788,12 +788,37 @@ object ScalaReflection extends ScalaReflection {
}
/**
- * Finds an accessible constructor with compatible parameters. This is a
more flexible search
- * than the exact matching algorithm in `Class.getConstructor`. The
first assignment-compatible
- * matching constructor is returned. Otherwise, it returns `None`.
+ * Finds an accessible constructor with compatible parameters. This is a
more flexible search than
+ * the exact matching algorithm in `Class.getConstructor`. The first
assignment-compatible
+ * matching constructor is returned if it exists. Otherwise, we check
for additional compatible
+ * constructors defined in the companion object as `apply` methods.
Otherwise, it returns `None`.
*/
- def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Constructor[_]] = {
-Option(ConstructorUtils.getMatchingAccessibleConstructor(cls,
paramTypes: _*))
+ def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Seq[AnyRef] => Any] = {
+Option(ConstructorUtils.getMatchingAccessibleConstructor(cls,
paramTypes: _*)) match {
+ case Some(c) => Some((x: Seq[AnyRef]) => c.newInstance(x: _*))
+ case None =>
+val companion = mirror.staticClass(cls.getName).companion
+val moduleMirror = mirror.reflectModule(companion.asModule)
+val applyMethods = companion.asTerm.typeSignature
+ .member(universe.TermName("apply")).asTerm.alternatives
+applyMethods.find { method =>
+ val params = method.typeSignature.paramLists.head
+ // Check that the needed params are the same length and of
matching types
+ params.size == paramTypes.tail.size &&
+ params.zip(paramTypes.tail).forall { case(ps, pc) =>
+ps.typeSignature.typeSymbol == mirror.classSymbol(pc)
+ }
+}.map { applyMethodSymbol =>
+ val expectedArgsCount =
applyMethodSymbol.typeSignature.paramLists.head.size
+ val instanceMirror = mirror.reflect(moduleMirror.instance)
+ val method =
instanceMirror.reflectMethod(applyMethodSymbol.asMethod)
+ (_args: Seq[AnyRef]) => {
+// Drop the "outer" argument if it is provided
+val args = if (_args.size == expectedArgsCount) _args else
_args.tail
--- End diff --
I guess we could check whether the dropped arg is of the type of the Class
argument to be safer, but this probably works
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234834497
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/ScalaReflectionSuite.scala
---
@@ -109,6 +109,64 @@ object TestingUDT {
}
}
+/** An example derived from Twitter/Scrooge codegen for thrift */
+object ScroogeLikeExample {
+ def apply(
+x: Int
+ ): ScroogeLikeExample =
+new Immutable(
+ x
+)
+
+ def unapply(_item: ScroogeLikeExample): _root_.scala.Option[Int] =
_root_.scala.Some(_item.x)
+
+ class Immutable(val x: Int) extends ScroogeLikeExample
+
+ trait Proxy extends ScroogeLikeExample {
+protected def _underlying_ScroogeLikeExample: ScroogeLikeExample
+override def x: Int = _underlying_ScroogeLikeExample.x
+ }
+}
+
+trait ScroogeLikeExample
+ extends _root_.scala.Product1[Int]
+ with java.io.Serializable
+{
+ import ScroogeLikeExample._
+
+ def x: Int
+
+ def _1: Int = x
--- End diff --
override?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234834269
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/ScalaReflectionSuite.scala
---
@@ -109,6 +109,64 @@ object TestingUDT {
}
}
+/** An example derived from Twitter/Scrooge codegen for thrift */
+object ScroogeLikeExample {
+ def apply(
+x: Int
+ ): ScroogeLikeExample =
+new Immutable(
+ x
+)
+
+ def unapply(_item: ScroogeLikeExample): _root_.scala.Option[Int] =
_root_.scala.Some(_item.x)
--- End diff --
why qualify as `_root_.scala.` here?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234834453
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/ScalaReflectionSuite.scala
---
@@ -109,6 +109,64 @@ object TestingUDT {
}
}
+/** An example derived from Twitter/Scrooge codegen for thrift */
+object ScroogeLikeExample {
+ def apply(
+x: Int
+ ): ScroogeLikeExample =
+new Immutable(
+ x
+)
+
+ def unapply(_item: ScroogeLikeExample): _root_.scala.Option[Int] =
_root_.scala.Some(_item.x)
+
+ class Immutable(val x: Int) extends ScroogeLikeExample
+
+ trait Proxy extends ScroogeLikeExample {
+protected def _underlying_ScroogeLikeExample: ScroogeLikeExample
+override def x: Int = _underlying_ScroogeLikeExample.x
+ }
+}
+
+trait ScroogeLikeExample
+ extends _root_.scala.Product1[Int]
+ with java.io.Serializable
--- End diff --
Import this, unless this is somehow to avoid scala's Serializable
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234833180
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -462,12 +462,12 @@ case class NewInstance(
val d = outerObj.getClass +: paramTypes
val c = getConstructor(outerObj.getClass +: paramTypes)
(args: Seq[AnyRef]) => {
-c.newInstance(outerObj +: args: _*)
+c(Seq(outerObj +: args: _*))
--- End diff --
Do these need to be called as varargs? it's no longer a Constructor but a
function accepting a Seq.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234834318
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/ScalaReflectionSuite.scala
---
@@ -109,6 +109,64 @@ object TestingUDT {
}
}
+/** An example derived from Twitter/Scrooge codegen for thrift */
+object ScroogeLikeExample {
+ def apply(
--- End diff --
I think this is more readable as one liner
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234834550
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/ScalaReflectionSuite.scala
---
@@ -109,6 +109,64 @@ object TestingUDT {
}
}
+/** An example derived from Twitter/Scrooge codegen for thrift */
+object ScroogeLikeExample {
+ def apply(
+x: Int
+ ): ScroogeLikeExample =
+new Immutable(
+ x
+)
+
+ def unapply(_item: ScroogeLikeExample): _root_.scala.Option[Int] =
_root_.scala.Some(_item.x)
+
+ class Immutable(val x: Int) extends ScroogeLikeExample
+
+ trait Proxy extends ScroogeLikeExample {
+protected def _underlying_ScroogeLikeExample: ScroogeLikeExample
+override def x: Int = _underlying_ScroogeLikeExample.x
+ }
+}
+
+trait ScroogeLikeExample
+ extends _root_.scala.Product1[Int]
--- End diff --
Does it really need to extend Product1 for the test? quite possibly, just
didn't see it.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234833053
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala
---
@@ -973,8 +998,19 @@ trait ScalaReflection extends Logging {
}
}
+ /**
+ * If our type is a Scala trait it may have a companion object that
+ * only defines a constructor via `apply` method.
+ */
+ def getCompanionConstructor(tpe: Type): Symbol = {
--- End diff --
Can this be private?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user drewrobb commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234834427
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala
---
@@ -788,12 +788,37 @@ object ScalaReflection extends ScalaReflection {
}
/**
- * Finds an accessible constructor with compatible parameters. This is a
more flexible search
- * than the exact matching algorithm in `Class.getConstructor`. The
first assignment-compatible
- * matching constructor is returned. Otherwise, it returns `None`.
+ * Finds an accessible constructor with compatible parameters. This is a
more flexible search than
+ * the exact matching algorithm in `Class.getConstructor`. The first
assignment-compatible
+ * matching constructor is returned if it exists. Otherwise, we check
for additional compatible
+ * constructors defined in the companion object as `apply` methods.
Otherwise, it returns `None`.
*/
- def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Constructor[_]] = {
-Option(ConstructorUtils.getMatchingAccessibleConstructor(cls,
paramTypes: _*))
+ def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Seq[AnyRef] => Any] = {
--- End diff --
I guess externally `findConstructor` would then be cleaner though? It's too
bad it is only called in one place.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user drewrobb commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234834125
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala
---
@@ -788,12 +788,37 @@ object ScalaReflection extends ScalaReflection {
}
/**
- * Finds an accessible constructor with compatible parameters. This is a
more flexible search
- * than the exact matching algorithm in `Class.getConstructor`. The
first assignment-compatible
- * matching constructor is returned. Otherwise, it returns `None`.
+ * Finds an accessible constructor with compatible parameters. This is a
more flexible search than
+ * the exact matching algorithm in `Class.getConstructor`. The first
assignment-compatible
+ * matching constructor is returned if it exists. Otherwise, we check
for additional compatible
+ * constructors defined in the companion object as `apply` methods.
Otherwise, it returns `None`.
*/
- def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Constructor[_]] = {
-Option(ConstructorUtils.getMatchingAccessibleConstructor(cls,
paramTypes: _*))
+ def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Seq[AnyRef] => Any] = {
--- End diff --
Real fast I get:
```
[error]
/home/drew/spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala:798:
type mismatch;
[error] found : _$31 where type _$31
[error] required: T
[error] case Some(c) => Some((x: Seq[AnyRef]) => c.newInstance(x: _*))
[error] ^
[error]
/home/drew/spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala:818:
type mismatch;
[error] found : Any
[error] required: T
[error] method.apply(args: _*)
[error]
```
`Constructor` has a type parameter, but the signature of `newInstance`
doesn't use it, and in the second case `method.apply` method returned by
reflection has a return type of `Any`. I think that can only be resolved with
adding `asInstanceOf[Seq[AnyRef] => T]`, which I don't think would be any
better (unless I'm 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 #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234832389
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala
---
@@ -788,12 +788,37 @@ object ScalaReflection extends ScalaReflection {
}
/**
- * Finds an accessible constructor with compatible parameters. This is a
more flexible search
- * than the exact matching algorithm in `Class.getConstructor`. The
first assignment-compatible
- * matching constructor is returned. Otherwise, it returns `None`.
+ * Finds an accessible constructor with compatible parameters. This is a
more flexible search than
+ * the exact matching algorithm in `Class.getConstructor`. The first
assignment-compatible
+ * matching constructor is returned if it exists. Otherwise, we check
for additional compatible
+ * constructors defined in the companion object as `apply` methods.
Otherwise, it returns `None`.
*/
- def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Constructor[_]] = {
-Option(ConstructorUtils.getMatchingAccessibleConstructor(cls,
paramTypes: _*))
+ def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Seq[AnyRef] => Any] = {
--- End diff --
A Constructor can only be for an Object, so it was already only returning
something that can make any AnyRef. I think that's better.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user drewrobb commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234828814
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala
---
@@ -788,12 +788,37 @@ object ScalaReflection extends ScalaReflection {
}
/**
- * Finds an accessible constructor with compatible parameters. This is a
more flexible search
- * than the exact matching algorithm in `Class.getConstructor`. The
first assignment-compatible
- * matching constructor is returned. Otherwise, it returns `None`.
+ * Finds an accessible constructor with compatible parameters. This is a
more flexible search than
+ * the exact matching algorithm in `Class.getConstructor`. The first
assignment-compatible
+ * matching constructor is returned if it exists. Otherwise, we check
for additional compatible
+ * constructors defined in the companion object as `apply` methods.
Otherwise, it returns `None`.
*/
- def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Constructor[_]] = {
-Option(ConstructorUtils.getMatchingAccessibleConstructor(cls,
paramTypes: _*))
+ def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Seq[AnyRef] => Any] = {
--- End diff --
I think you are correct, but I was keeping with the existing return type
used in where this is called in `eval` and elsewhere and don't know if I should
make changes not related to my issue. Same thing with changing `_` to `T`.
Would require adding a type parameter to `NewInstance` which would be a very
large change.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user drewrobb commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234828642
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala
---
@@ -788,12 +788,37 @@ object ScalaReflection extends ScalaReflection {
}
/**
- * Finds an accessible constructor with compatible parameters. This is a
more flexible search
- * than the exact matching algorithm in `Class.getConstructor`. The
first assignment-compatible
- * matching constructor is returned. Otherwise, it returns `None`.
+ * Finds an accessible constructor with compatible parameters. This is a
more flexible search than
+ * the exact matching algorithm in `Class.getConstructor`. The first
assignment-compatible
+ * matching constructor is returned if it exists. Otherwise, we check
for additional compatible
+ * constructors defined in the companion object as `apply` methods.
Otherwise, it returns `None`.
*/
- def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Constructor[_]] = {
-Option(ConstructorUtils.getMatchingAccessibleConstructor(cls,
paramTypes: _*))
+ def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Seq[AnyRef] => Any] = {
+Option(ConstructorUtils.getMatchingAccessibleConstructor(cls,
paramTypes: _*)) match {
+ case Some(c) => Some((x: Seq[AnyRef]) => c.newInstance(x: _*))
+ case None =>
+val companion = mirror.staticClass(cls.getName).companion
+val moduleMirror = mirror.reflectModule(companion.asModule)
+val applyMethods = companion.asTerm.typeSignature
+ .member(universe.TermName("apply")).asTerm.alternatives
+applyMethods.filter{ method =>
--- End diff --
thanks, good suggestion
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user drewrobb commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234828661
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala
---
@@ -788,12 +788,37 @@ object ScalaReflection extends ScalaReflection {
}
/**
- * Finds an accessible constructor with compatible parameters. This is a
more flexible search
- * than the exact matching algorithm in `Class.getConstructor`. The
first assignment-compatible
- * matching constructor is returned. Otherwise, it returns `None`.
+ * Finds an accessible constructor with compatible parameters. This is a
more flexible search than
+ * the exact matching algorithm in `Class.getConstructor`. The first
assignment-compatible
+ * matching constructor is returned if it exists. Otherwise, we check
for additional compatible
+ * constructors defined in the companion object as `apply` methods.
Otherwise, it returns `None`.
*/
- def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Constructor[_]] = {
-Option(ConstructorUtils.getMatchingAccessibleConstructor(cls,
paramTypes: _*))
+ def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Seq[AnyRef] => Any] = {
+Option(ConstructorUtils.getMatchingAccessibleConstructor(cls,
paramTypes: _*)) match {
+ case Some(c) => Some((x: Seq[AnyRef]) => c.newInstance(x: _*))
+ case None =>
+val companion = mirror.staticClass(cls.getName).companion
+val moduleMirror = mirror.reflectModule(companion.asModule)
+val applyMethods = companion.asTerm.typeSignature
+ .member(universe.TermName("apply")).asTerm.alternatives
+applyMethods.filter{ method =>
+ val params = method.typeSignature.paramLists.head
+ // Check that the needed params are the same length and of
matching types
+ params.size == paramTypes.tail.size &&
+ params.zip(paramTypes.tail).map{case(ps, pc) =>
--- End diff --
yes!
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user drewrobb commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234828613
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ObjectExpressionsSuite.scala
---
@@ -410,6 +410,16 @@ class ObjectExpressionsSuite extends SparkFunSuite
with ExpressionEvalHelper {
dataType = ObjectType(classOf[outerObj.Inner]),
outerPointer = Some(() => outerObj))
checkObjectExprEvaluation(newInst2, new outerObj.Inner(1))
+
+// SPARK-8288 Test
+import org.apache.spark.sql.catalyst.ScroogeLikeExample
+val newInst3 = NewInstance(
+ cls = classOf[ScroogeLikeExample],
+ arguments = Literal(1) :: Nil,
+ propagateNull = false,
+ dataType = ObjectType(classOf[ScroogeLikeExample]),
+ outerPointer = Some(() => outerObj))
+checkObjectExprEvaluation(newInst3, ScroogeLikeExample(1))
--- End diff --
Thanks, that is a great tip-- I was looking for the place to place tests
like this. Upon starting to add them I uncovered a few more code paths that my
ScroogeExample didn't work with (because the trait was missing methods that
actual scrooge code had to access member elements), so I fixed that also.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234449241
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ObjectExpressionsSuite.scala
---
@@ -410,6 +410,16 @@ class ObjectExpressionsSuite extends SparkFunSuite
with ExpressionEvalHelper {
dataType = ObjectType(classOf[outerObj.Inner]),
outerPointer = Some(() => outerObj))
checkObjectExprEvaluation(newInst2, new outerObj.Inner(1))
+
+// SPARK-8288 Test
+import org.apache.spark.sql.catalyst.ScroogeLikeExample
+val newInst3 = NewInstance(
+ cls = classOf[ScroogeLikeExample],
+ arguments = Literal(1) :: Nil,
+ propagateNull = false,
+ dataType = ObjectType(classOf[ScroogeLikeExample]),
+ outerPointer = Some(() => outerObj))
+checkObjectExprEvaluation(newInst3, ScroogeLikeExample(1))
--- End diff --
Besides unit tests, can you also add the tests on encoding
`ScroogeLikeExample` in Dataset?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234449115
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/ScalaReflectionSuite.scala
---
@@ -362,4 +392,11 @@ class ScalaReflectionSuite extends SparkFunSuite {
assert(numberOfCheckedArguments(deserializerFor[(java.lang.Double,
Int)]) == 1)
assert(numberOfCheckedArguments(deserializerFor[(java.lang.Integer,
java.lang.Integer)]) == 0)
}
+
+ test("SPARK-8288") {
--- End diff --
Can you add a descriptive test name?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234404871
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala
---
@@ -788,12 +788,37 @@ object ScalaReflection extends ScalaReflection {
}
/**
- * Finds an accessible constructor with compatible parameters. This is a
more flexible search
- * than the exact matching algorithm in `Class.getConstructor`. The
first assignment-compatible
- * matching constructor is returned. Otherwise, it returns `None`.
+ * Finds an accessible constructor with compatible parameters. This is a
more flexible search than
+ * the exact matching algorithm in `Class.getConstructor`. The first
assignment-compatible
+ * matching constructor is returned if it exists. Otherwise, we check
for additional compatible
+ * constructors defined in the companion object as `apply` methods.
Otherwise, it returns `None`.
*/
- def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Constructor[_]] = {
-Option(ConstructorUtils.getMatchingAccessibleConstructor(cls,
paramTypes: _*))
+ def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Seq[AnyRef] => Any] = {
+Option(ConstructorUtils.getMatchingAccessibleConstructor(cls,
paramTypes: _*)) match {
+ case Some(c) => Some((x: Seq[AnyRef]) => c.newInstance(x: _*))
+ case None =>
+val companion = mirror.staticClass(cls.getName).companion
+val moduleMirror = mirror.reflectModule(companion.asModule)
+val applyMethods = companion.asTerm.typeSignature
+ .member(universe.TermName("apply")).asTerm.alternatives
+applyMethods.filter{ method =>
+ val params = method.typeSignature.paramLists.head
+ // Check that the needed params are the same length and of
matching types
+ params.size == paramTypes.tail.size &&
+ params.zip(paramTypes.tail).map{case(ps, pc) =>
--- End diff --
Spacing here. Is this simpler with .forall?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234404858
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala
---
@@ -788,12 +788,37 @@ object ScalaReflection extends ScalaReflection {
}
/**
- * Finds an accessible constructor with compatible parameters. This is a
more flexible search
- * than the exact matching algorithm in `Class.getConstructor`. The
first assignment-compatible
- * matching constructor is returned. Otherwise, it returns `None`.
+ * Finds an accessible constructor with compatible parameters. This is a
more flexible search than
+ * the exact matching algorithm in `Class.getConstructor`. The first
assignment-compatible
+ * matching constructor is returned if it exists. Otherwise, we check
for additional compatible
+ * constructors defined in the companion object as `apply` methods.
Otherwise, it returns `None`.
*/
- def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Constructor[_]] = {
-Option(ConstructorUtils.getMatchingAccessibleConstructor(cls,
paramTypes: _*))
+ def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Seq[AnyRef] => Any] = {
+Option(ConstructorUtils.getMatchingAccessibleConstructor(cls,
paramTypes: _*)) match {
+ case Some(c) => Some((x: Seq[AnyRef]) => c.newInstance(x: _*))
+ case None =>
+val companion = mirror.staticClass(cls.getName).companion
+val moduleMirror = mirror.reflectModule(companion.asModule)
+val applyMethods = companion.asTerm.typeSignature
+ .member(universe.TermName("apply")).asTerm.alternatives
+applyMethods.filter{ method =>
--- End diff --
Nit: whitespace around braces, operators, etc
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234404797
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -453,7 +453,7 @@ case class NewInstance(
@transient private lazy val constructor: (Seq[AnyRef]) => Any = {
val paramTypes = ScalaReflection.expressionJavaClasses(arguments)
val getConstructor = (paramClazz: Seq[Class[_]]) => {
- ScalaReflection.findConstructor(cls, paramClazz).getOrElse {
+ ScalaReflection.findConstructor(cls, paramClazz).getOrElse{
--- End diff --
Nit: undo this one
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234404845
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala
---
@@ -788,12 +788,37 @@ object ScalaReflection extends ScalaReflection {
}
/**
- * Finds an accessible constructor with compatible parameters. This is a
more flexible search
- * than the exact matching algorithm in `Class.getConstructor`. The
first assignment-compatible
- * matching constructor is returned. Otherwise, it returns `None`.
+ * Finds an accessible constructor with compatible parameters. This is a
more flexible search than
+ * the exact matching algorithm in `Class.getConstructor`. The first
assignment-compatible
+ * matching constructor is returned if it exists. Otherwise, we check
for additional compatible
+ * constructors defined in the companion object as `apply` methods.
Otherwise, it returns `None`.
*/
- def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Constructor[_]] = {
-Option(ConstructorUtils.getMatchingAccessibleConstructor(cls,
paramTypes: _*))
+ def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Seq[AnyRef] => Any] = {
--- End diff --
Wouldn't the result also be AnyRef, if this produces an Object?
Is it not possible to write this with a type T for the class and its
result? maybe there's a reason.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234404786
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -498,6 +504,7 @@ case class NewInstance(
final $javaType ${ev.value} = ${ev.isNull} ?
${CodeGenerator.defaultValue(dataType)} : $constructorCall;
"""
+
--- End diff --
Nit: revert changes like this if possible
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/23062#discussion_r234404888
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala
---
@@ -788,12 +788,37 @@ object ScalaReflection extends ScalaReflection {
}
/**
- * Finds an accessible constructor with compatible parameters. This is a
more flexible search
- * than the exact matching algorithm in `Class.getConstructor`. The
first assignment-compatible
- * matching constructor is returned. Otherwise, it returns `None`.
+ * Finds an accessible constructor with compatible parameters. This is a
more flexible search than
+ * the exact matching algorithm in `Class.getConstructor`. The first
assignment-compatible
+ * matching constructor is returned if it exists. Otherwise, we check
for additional compatible
+ * constructors defined in the companion object as `apply` methods.
Otherwise, it returns `None`.
*/
- def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Constructor[_]] = {
-Option(ConstructorUtils.getMatchingAccessibleConstructor(cls,
paramTypes: _*))
+ def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]):
Option[Seq[AnyRef] => Any] = {
+Option(ConstructorUtils.getMatchingAccessibleConstructor(cls,
paramTypes: _*)) match {
+ case Some(c) => Some((x: Seq[AnyRef]) => c.newInstance(x: _*))
+ case None =>
+val companion = mirror.staticClass(cls.getName).companion
+val moduleMirror = mirror.reflectModule(companion.asModule)
+val applyMethods = companion.asTerm.typeSignature
+ .member(universe.TermName("apply")).asTerm.alternatives
+applyMethods.filter{ method =>
--- End diff --
.find rather than filter.headOption maybe?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
GitHub user drewrobb opened a pull request: https://github.com/apache/spark/pull/23062 [SPARK-8288][SQL] ScalaReflection can use companion object constructor ## What changes were proposed in this pull request? This change fixes a particular scenario where default spark SQL can't encode (thrift) types that are generated by twitter scrooge. These types are a trait that extends `scala.ProductX` with a constructor defined only in a companion object, rather than a actual case class. The actual case class used is child class, but that type is almost never referred to in code. The type has no corresponding constructor symbol and causes an exception. For all other purposes, these classes act just like case classes, so it is unfortunate that spark SQL can't serialize them nicely as it can actual case classes. For an full example of a scrooge codegen class, see https://gist.github.com/anonymous/ba13d4b612396ca72725eaa989900314. This change catches the case where the type has no constructor but does have an `apply` method on the type's companion object. This allows for thrift types to be serialized/deserialized with implicit encoders the same way as normal case classes. This fix had to be done in three places where the constructor is assumed to be an actual constructor: 1) In serializing, determining the schema for the dataframe relies on inspecting its constructor (`ScalaReflection.constructParams`). Here we fall back to using the companion constructor arguments. 2) In deserializing or evaluating, in the java codegen ( `NewInstance.doGenCode`), the type couldn't be constructed with the new keyword. If there is no constructor, we change the constructor call to try the companion constructor. 3) In deserializing or evaluating, without codegen, the constructor is directly invoked (`NewInstance.constructor`). This was fixed with scala reflection to get the actual companion apply method. The return type of `findConstructor` was changed because the companion apply method constructor can't be represented as a `java.lang.reflect.Constructor`. There might be situations in which this approach would also fail in a new way, but it does at a minimum work for the specific scrooge example and will not impact cases that were already succeeding prior to this change Note: this fix does not enable using scrooge thrift enums, additional work for this is necessary. With this patch, it seems like you could patch `com.twitter.scrooge.ThriftEnum` to extend `_root_.scala.Product1[Int]` with `def _1 = value` to get spark's implicit encoders to handle enums, but I've yet to use this method myself. Note: I previously opened a PR for this issue, but only was able to fix case 1) there: https://github.com/apache/spark/pull/18766 ## How was this patch tested? I've fixed all 3 cases and added two tests that use a case class that is similar to scrooge generated one. The test in ScalaReflectionSuite checks 1), and the additional asserting in ObjectExpressionsSuite checks 2) and 3). You can merge this pull request into a Git repository by running: $ git pull https://github.com/drewrobb/spark SPARK-8288 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/spark/pull/23062.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 #23062 commit e27f933710c289d5bc1ddcad38eecde0e555ac60 Author: Drew Robb Date: 2017-07-29T01:45:00Z ScalaReflection can use companion object constructor --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
