You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by drewrobb <gi...@git.apache.org> on 2018/11/16 23:46:54 UTC
[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 <dr...@...>
Date: 2017-07-29T01:45:00Z
ScalaReflection can use companion object constructor
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23062
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23062
**[Test build #98940 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/98940/testReport)** for PR 23062 at commit [`e27f933`](https://github.com/apache/spark/commit/e27f933710c289d5bc1ddcad38eecde0e555ac60).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by srowen <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23062
**[Test build #98940 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/98940/testReport)** for PR 23062 at commit [`e27f933`](https://github.com/apache/spark/commit/e27f933710c289d5bc1ddcad38eecde0e555ac60).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by srowen <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by srowen <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23062
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by drewrobb <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23062
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/99033/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23062
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/5172/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23062
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by drewrobb <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by srowen <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by drewrobb <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by srowen <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by drewrobb <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by srowen <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/23062
Merged to master
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by srowen <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23062
**[Test build #99033 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/99033/testReport)** for PR 23062 at commit [`0074693`](https://github.com/apache/spark/commit/0074693d6da1e50bcc5b745d42da1f72cc54bdaf).
* This patch **fails Scala style tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by drewrobb <gi...@git.apache.org>.
Github user drewrobb commented on the issue:
https://github.com/apache/spark/pull/23062
Thanks much for the prompt feedback. PTAL at changes. I had to modify the ScroogeLikeExample class to make the additional DatasetSuite test work-- my prior example had removed methods from the scrooge code that were actually necessary. Kept as separate commits for review, but can squash later?
RE Scala 2.12 are there any specific things you are worried about I could look at? I'm not super familiar with changes in 2.12, and I think 2.11 should be fine, as this was developed using 2.11. And I assume the QA build is testing on 2.12 also?
Also this might not be the place to ask but it is rather ungreppable-- have you encountered compilation generated java files in ./org/ (that directory is also not in .gitignore!) breaking compilation in sbt? In my development environment I'll frequently have to delete that entire directory for compilation to work again.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23062
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/5158/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by srowen <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by srowen <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23062
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by srowen <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23062
**[Test build #99033 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/99033/testReport)** for PR 23062 at commit [`0074693`](https://github.com/apache/spark/commit/0074693d6da1e50bcc5b745d42da1f72cc54bdaf).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23062
**[Test build #99029 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/99029/testReport)** for PR 23062 at commit [`39979e4`](https://github.com/apache/spark/commit/39979e43add48c9f1597ff8314434ff679ac5483).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by drewrobb <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23062
**[Test build #99043 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/99043/testReport)** for PR 23062 at commit [`8c25fd0`](https://github.com/apache/spark/commit/8c25fd0c843576fb86b1881b57a6eb445e1c5461).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23062
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23062
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23062
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/5167/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23062
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/98940/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by srowen <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by drewrobb <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23062
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23062
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by drewrobb <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by srowen <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23062
**[Test build #99031 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/99031/testReport)** for PR 23062 at commit [`4cba3cc`](https://github.com/apache/spark/commit/4cba3ccaaa39f7e2103ce7d4f953aba49e942a28).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23062
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/5092/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23062
**[Test build #99043 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/99043/testReport)** for PR 23062 at commit [`8c25fd0`](https://github.com/apache/spark/commit/8c25fd0c843576fb86b1881b57a6eb445e1c5461).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by srowen <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23062
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/99029/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23062
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/99043/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by srowen <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by srowen <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by srowen <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by drewrobb <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23062
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/99039/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23062
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/99044/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by srowen <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by srowen <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23062
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/5162/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by drewrobb <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23062
**[Test build #99039 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/99039/testReport)** for PR 23062 at commit [`83a1987`](https://github.com/apache/spark/commit/83a19876e76d8ec4afb36d023e37575586e1d19e).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by srowen <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by srowen <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23062
**[Test build #99029 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/99029/testReport)** for PR 23062 at commit [`39979e4`](https://github.com/apache/spark/commit/39979e43add48c9f1597ff8314434ff679ac5483).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by viirya <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by srowen <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23062
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23062
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23062
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/99031/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by drewrobb <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by drewrobb <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23062
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23062
**[Test build #99044 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/99044/testReport)** for PR 23062 at commit [`04a34c4`](https://github.com/apache/spark/commit/04a34c4ee7c43120721659713fb8024e90282a0c).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23062
**[Test build #99031 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/99031/testReport)** for PR 23062 at commit [`4cba3cc`](https://github.com/apache/spark/commit/4cba3ccaaa39f7e2103ce7d4f953aba49e942a28).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23062
**[Test build #99044 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/99044/testReport)** for PR 23062 at commit [`04a34c4`](https://github.com/apache/spark/commit/04a34c4ee7c43120721659713fb8024e90282a0c).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23062
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/5171/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23062
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23062
**[Test build #99039 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/99039/testReport)** for PR 23062 at commit [`83a1987`](https://github.com/apache/spark/commit/83a19876e76d8ec4afb36d023e37575586e1d19e).
* This patch **fails Scala style tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by drewrobb <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by srowen <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by viirya <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by drewrobb <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by drewrobb <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23062: [SPARK-8288][SQL] ScalaReflection can use compani...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/23062
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23062: [SPARK-8288][SQL] ScalaReflection can use companion obje...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23062
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org