eejbyfeldt commented on a change in pull request #33205:
URL: https://github.com/apache/spark/pull/33205#discussion_r663891046
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala
##########
@@ -155,6 +155,65 @@ object ScalaReflection extends ScalaReflection {
}
}
+ /**
+ * [SPARK-20384] Determine params for a give constructor type, handling
unwrapping param
+ * types for value class.
+ *
+ * From doc on value class:
https://docs.scala-lang.org/overviews/core/value-classes.html
+ * Given: `class Wrapper(val underlying: Int) extends AnyVal`,
+ *
+ * 1) "The type at compile time is `Wrapper`, but at runtime, the
representation is an `Int`"
+ * This implies that when our struct has a field of value class, the
generated code
+ * should support the underlying type during runtime execution.
+ *
+ * 2) `Wrapper` "must be instantiated... when a value class is used as a
type argument".
+ * This implies that scala.Tuple[Wrapper, ...], Seq[Wrapper], Map[String,
Wrapper],
+ * Option[Wrapper] will still contain Wrapper` as-is in during runtime
instead of `Int`.
+ *
+ * From 2), out of those generic types, only Tuple is considered a type with
constructor, so
+ * no unwrapping is done.
+ *
+ * @param tpe field type of this constructor
+ * @param isTupleType whether the constructor is tuple
+ */
+ private def getConstructorUnwrappedParameters(tpe: Type, isTupleType:
Boolean):
+ Seq[(String, Type)] = {
+ val params = getConstructorParameters(tpe)
+ if (isTupleType) {
Review comment:
A scala `Tuple` is just a case class with generics:
https://github.com/scala/scala/blob/2.13.x/src/library/scala/Tuple2.scala#L24
But since the current code looks specifically for `scala.Tuple` this means
that it will not handle user defined case classes with generics.
For example I think adding the following test in the `ExpressionEncodeSuite`
case on this branch will fail:
```
case class CaseClassWithGeneric[T](generic: T, value: IntWrapper)
encodeDecodeTest(
CaseClassWithGeneric[IntWrapper](IntWrapper(1), IntWrapper(2)),
"case class with generic fields")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
