You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "chris-twiner (via GitHub)" <gi...@apache.org> on 2025/02/20 15:39:04 UTC
[PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
chris-twiner opened a new pull request, #50023:
URL: https://github.com/apache/spark/pull/50023
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
4.0.0-preview2 introduced, as part of SPARK-49025 pr #47785, changes which drive ExpressionEncoder derivation purely from AgnosticEncoders. This PR adds a trait:
```scala
@DeveloperApi
trait AgnosticExpressionPathEncoder[T]
extends AgnosticEncoder[T] {
def toCatalyst(input: Expression): Expression
def fromCatalyst(inputPath: Expression): Expression
}
```
and hooks in the De/SerializationBuildHelper matches to allow seamless extension of non-connect custom encoders (such as [frameless](https://github.com/typelevel/frameless) or [sparksql-scalapb](https://github.com/scalapb/sparksql-scalapb)).
SPARK-49960 provides the same information.
Additionally this PR provides fixes necessary to use TransformingEncoder as a root encoder with an OptionalEncoder, use as an ArrayType and MapType entry/key.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
Without this change (or similar) there is no way for custom encoders to integrate with 4.0.0-preview2 derived encoders, something which has worked and devs have benefited from since pre 2.4 days. This stops code such as Dataset.joinWith from deriving a tuple encoder which works (as the provided ExpressionEncoder is now discarded under preview2). Supplying a custom AgnosticEncoder under preview2 also fails as only the preview2 AgnosticEncoders are supported in De/SerializationBuildHelper, triggering a MatchError.
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
No
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
Test was added using a "custom" string encoder and joinWith based on an existing joinWith test. Removing the case statements in either BuildHelper will trigger the MatchError.
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
No
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1970130467
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoder.scala:
##########
@@ -228,7 +236,8 @@ case class ExpressionEncoder[T](
* returns true if `T` is serialized as struct and is not `Option` type.
*/
def isSerializedAsStructForTopLevel: Boolean = {
- isSerializedAsStruct && !classOf[Option[_]].isAssignableFrom(clsTag.runtimeClass)
+ isSerializedAsStruct && !classOf[Option[_]].isAssignableFrom(clsTag.runtimeClass) &&
Review Comment:
I am wondering if we should make these checks part of the AgnosticEncoder api.
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "chris-twiner (via GitHub)" <gi...@apache.org>.
chris-twiner commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1971213487
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/encoders/AgnosticEncoder.scala:
##########
@@ -286,5 +286,6 @@ object AgnosticEncoders {
override def dataType: DataType = transformed.dataType
override def schema: StructType = transformed.schema
override def isStruct: Boolean = transformed.isStruct
+ override def nullable: Boolean = transformed.nullable
Review Comment:
I'll correct that, with a test case directly for it.
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "chris-twiner (via GitHub)" <gi...@apache.org>.
chris-twiner commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1971266224
##########
sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala:
##########
@@ -2841,6 +2860,265 @@ class DatasetSuite extends QueryTest
checkDataset(Seq(seqMutableSet).toDS(), seqMutableSet)
checkDataset(Seq(mapMutableSet).toDS(), mapMutableSet)
}
+
+ // below tests are related to SPARK-49960 and TransformingEncoder usage
+ test("Incorrect derived nullability with TransformingEncoder - non nullable") {
Review Comment:
replaced with a direct test on TransformingEncoder.nullable, per your lower point the seq stuff is better tested in the iterable test
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "chris-twiner (via GitHub)" <gi...@apache.org>.
chris-twiner commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1971266224
##########
sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala:
##########
@@ -2841,6 +2860,265 @@ class DatasetSuite extends QueryTest
checkDataset(Seq(seqMutableSet).toDS(), seqMutableSet)
checkDataset(Seq(mapMutableSet).toDS(), mapMutableSet)
}
+
+ // below tests are related to SPARK-49960 and TransformingEncoder usage
+ test("Incorrect derived nullability with TransformingEncoder - non nullable") {
Review Comment:
rephrased into the positive
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "chris-twiner (via GitHub)" <gi...@apache.org>.
chris-twiner commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1971723345
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/DeserializerBuildHelper.scala:
##########
@@ -458,7 +460,10 @@ object DeserializerBuildHelper {
Literal.create(provider(), ObjectType(classOf[Codec[_, _]])),
"decode",
ObjectType(tag.runtimeClass),
- createDeserializer(encoder, path, walkedTypePath) :: Nil)
+ createDeserializer(encoder, path, walkedTypePath) :: Nil,
+ propagateNull = enc.nullable,
Review Comment:
probably just added for symmetry, it doesn't have an effect on the Spark or frameless test cases, I've reverted the two lines.
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "chris-twiner (via GitHub)" <gi...@apache.org>.
chris-twiner commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1971693941
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/DeserializerBuildHelper.scala:
##########
@@ -270,6 +270,8 @@ object DeserializerBuildHelper {
enc: AgnosticEncoder[_],
path: Expression,
walkedTypePath: WalkedTypePath): Expression = enc match {
+ case ae: AgnosticExpressionPathEncoder[_] =>
+ ae.fromCatalyst(path)
Review Comment:
The "SPARK-49960: joinWith custom encoder" in DatasetSuite tests it
```scala
val ds2 = spark.createDataset[ClassData](Seq(ClassData("a", 1),
ClassData("b", 2)))(CustomPathEncoder.custClassDataEnc)
```
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on PR #50023:
URL: https://github.com/apache/spark/pull/50023#issuecomment-2691389433
@chris-twiner can you fix the style issue?
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r2657055392
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/encoders/EncoderUtils.scala:
##########
@@ -26,6 +27,30 @@ import org.apache.spark.sql.catalyst.util.{ArrayData, MapData}
import org.apache.spark.sql.types.{ArrayType, BinaryType, BooleanType, ByteType, CalendarIntervalType, DataType, DateType, DayTimeIntervalType, Decimal, DecimalType, DoubleType, FloatType, IntegerType, LongType, MapType, ObjectType, ShortType, StringType, StructType, TimestampNTZType, TimestampType, UserDefinedType, VariantType, YearMonthIntervalType}
import org.apache.spark.unsafe.types.{CalendarInterval, UTF8String, VariantVal}
+/**
+ * :: DeveloperApi ::
+ * Extensible [[AgnosticEncoder]] providing conversion extension points over type T
+ * @tparam T over T
+ */
+@DeveloperApi
+@deprecated("This trait is intended only as a migration tool and will be removed in 4.1")
Review Comment:
I've found that we haven't actually removed `AgnosticExpressionPathEncoder` in Apache 4.1, which contradicts the deprecated comment. What do you suggest we do about it? @hvanhovell @HyukjinKwon @chris-twiner
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1970123366
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/encoders/AgnosticEncoder.scala:
##########
@@ -286,5 +286,6 @@ object AgnosticEncoders {
override def dataType: DataType = transformed.dataType
override def schema: StructType = transformed.schema
override def isStruct: Boolean = transformed.isStruct
+ override def nullable: Boolean = transformed.nullable
Review Comment:
That also works for me.
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1969891795
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/encoders/AgnosticEncoder.scala:
##########
@@ -286,5 +286,6 @@ object AgnosticEncoders {
override def dataType: DataType = transformed.dataType
override def schema: StructType = transformed.schema
override def isStruct: Boolean = transformed.isStruct
+ override def nullable: Boolean = transformed.nullable
Review Comment:
Yikes.
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "chris-twiner (via GitHub)" <gi...@apache.org>.
chris-twiner commented on PR #50023:
URL: https://github.com/apache/spark/pull/50023#issuecomment-2672471978
@hvanhovell - per our convo
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "chris-twiner (via GitHub)" <gi...@apache.org>.
chris-twiner commented on PR #50023:
URL: https://github.com/apache/spark/pull/50023#issuecomment-2671895983
replaces https://github.com/apache/spark/pull/48477 with TransformingEncoder fixes.
This allows all of Frameless tests to pass when used either with the backwards compat AgnosticExpressionPathEncoder root _and_ all tests to work with the frameless AgnosticEncoder based [encoder derivation branch](https://github.com/chris-twiner/frameless/tree/temp/spark4_agnosticEncoder_reformated).
One ExpressionEncoderSuite test "transforming encoders as value class - Frameless value class as parameter use case" does not work when using a TransformingEncoder over the string field. I'll raise another issue should this be a valid use case and an actual bug.
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1970803985
##########
sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala:
##########
@@ -2841,6 +2860,265 @@ class DatasetSuite extends QueryTest
checkDataset(Seq(seqMutableSet).toDS(), seqMutableSet)
checkDataset(Seq(mapMutableSet).toDS(), mapMutableSet)
}
+
+ // below tests are related to SPARK-49960 and TransformingEncoder usage
+ test("Incorrect derived nullability with TransformingEncoder - non nullable") {
+ val sparkI = spark
+ type T = Tuple2[Seq[Seq[Int]], Seq[Int]]
+ val data: Seq[T] = Seq( ( Seq( Seq(1, 2, 3) ), Seq(1, 2, 3) ) )
+ // for reference only
+ val sparkDataTypeOG = {
+ import sparkI.implicits._
+ val ds = spark.createDataset[Tuple2[Seq[Seq[Int]], Seq[Int]]](data)
+ ds.schema
+ }
+
+ val provider = () =>
+ new Codec[Seq[Int], Seq[Int]]{
+ override def encode(in: Seq[Int]): Seq[Int] = in
+ override def decode(out: Seq[Int]): Seq[Int] = out
+ }
+
+ val transformingSeq =
+ TransformingEncoder[Seq[Int], Seq[Int]](
+ implicitly[ClassTag[Seq[Int]]],
+ IterableEncoder[Seq[Int], Int](implicitly[ClassTag[Seq[Int]]],
+ PrimitiveIntEncoder, false, false),
+ provider
+ )
+
+ val enc =
+ ProductEncoder(
+ implicitly[ClassTag[T]],
+ Seq(
+ EncoderField("_1",
+ IterableEncoder[Seq[Seq[Int]], Seq[Int]](implicitly[ClassTag[Seq[Seq[Int]]]],
+ transformingSeq, false, false), false, Metadata.empty),
+ EncoderField( "_2", transformingSeq, false, Metadata.empty)
+ ),
+ None
+ )
+ val sparkViaAgnostic = {
+ val ds = spark.createDataset(data)(enc)
+ ds.schema
+ }
+ // the nullability without TransformingEncoder nullability (SerializerBuilderHelper)
+ // is incorrect (_2 is inferred as nullable)
+ assert(enc.dataType === sparkViaAgnostic)
+ }
+
+ def provider[A]: () => Codec[V[A], A] = () =>
+ new Codec[V[A], A]{
+ override def encode(in: V[A]): A = in.v
+ override def decode(out: A): V[A] = V(out)
+ }
+
+ def transforming[A](underlying: AgnosticEncoder[A]): TransformingEncoder[V[A], A] =
+ TransformingEncoder[V[A], A](
+ implicitly[ClassTag[V[A]]],
+ underlying,
+ provider
+ )
+
+ val V_INT = StructType(Seq(StructField("v", IntegerType, nullable = false)))
+
+ // "value" usage for single field, a wrapping nullable type is required
+ val OPTION_OF_V_INT = StructType(Seq(StructField("value",
+ V_INT, nullable = true)))
+
+ // product encoder for a non-nullable V
+ val V_OF_INT =
+ ProductEncoder(
+ classTag[V[Int]],
+ Seq(EncoderField("v", PrimitiveIntEncoder, nullable = false, Metadata.empty)),
+ None
+ )
+
+ test("""Encoder derivation with nested TransformingEncoder of OptionEncoder""".stripMargin) {
+ val sparkI = spark
+ type T = V[V[Option[V[Int]]]]
+ val data: Seq[T] = Seq(V(V(None)), V(V(Some(V(1)))))
+ // for reference - datatype will introduce nested classes as expected
+ val sparkDataTypeOG = {
+ import sparkI.implicits._
+ val ds = spark.createDataset[V[V[Option[V[Int]]]]](data)
+ ds.schema
+ }
+
+ /* attempt to behave as if value class semantics except the last product,
+ using a final transforming instead of a product serializes */
+ val enc =
+ transforming(
+ transforming(
+ OptionEncoder(
+ // works
+ // transforming(PrimitiveIntEncoder)
+ // does not work
+ V_OF_INT
+ )
+ )
+ )
+
+ assert(enc.schema === OPTION_OF_V_INT)
+
+ val sparkViaAgnostic = {
+ val ds = spark.createDataset(data)(enc)
+ ds.schema
+ }
+
+ /* The schema has been changed to just the Product V[Int], the wrapping Option value
+ struct has been removed - it should not have been */
+ assert(sparkViaAgnostic === enc.schema)
+
+ val ds = spark.createDataset(data)(enc)
+ assert(ds.collect().toVector === data.toVector)
+ }
+
+ test("""Encoder derivation with TransformingEncoder of OptionEncoder""".stripMargin) {
Review Comment:
Is this illustrating a different issue than above?
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1970811408
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/DeserializerBuildHelper.scala:
##########
@@ -458,7 +460,10 @@ object DeserializerBuildHelper {
Literal.create(provider(), ObjectType(classOf[Codec[_, _]])),
"decode",
ObjectType(tag.runtimeClass),
- createDeserializer(encoder, path, walkedTypePath) :: Nil)
+ createDeserializer(encoder, path, walkedTypePath) :: Nil,
+ propagateNull = enc.nullable,
Review Comment:
Besides the fact that this produces more efficient code. Is there anyway this influences behavior? I am trying figure out if this needs testing.
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1970705482
##########
sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala:
##########
@@ -2804,6 +2808,21 @@ class DatasetSuite extends QueryTest
}
}
+ test("SPARK-49960: joinWith custom encoder") {
+ /*
+ test based on "joinWith class with primitive, toDF"
+ with "custom" encoder. Removing the use of AgnosticExpressionPathEncoder
+ within SerializerBuildHelper and DeserializerBuildHelper will trigger MatchErrors
+ */
+ val ds1 = Seq(1, 1, 2).toDS()
+ val ds2 = SparkSession.active.createDataset[ClassData](Seq(ClassData("a", 1),
Review Comment:
Nit: use `spark` instead of `SparkSession.active`.
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "chris-twiner (via GitHub)" <gi...@apache.org>.
chris-twiner commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1971704899
##########
sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala:
##########
@@ -2841,6 +2860,265 @@ class DatasetSuite extends QueryTest
checkDataset(Seq(seqMutableSet).toDS(), seqMutableSet)
checkDataset(Seq(mapMutableSet).toDS(), mapMutableSet)
}
+
+ // below tests are related to SPARK-49960 and TransformingEncoder usage
+ test("Incorrect derived nullability with TransformingEncoder - non nullable") {
+ val sparkI = spark
+ type T = Tuple2[Seq[Seq[Int]], Seq[Int]]
+ val data: Seq[T] = Seq( ( Seq( Seq(1, 2, 3) ), Seq(1, 2, 3) ) )
+ // for reference only
+ val sparkDataTypeOG = {
+ import sparkI.implicits._
+ val ds = spark.createDataset[Tuple2[Seq[Seq[Int]], Seq[Int]]](data)
+ ds.schema
+ }
+
+ val provider = () =>
+ new Codec[Seq[Int], Seq[Int]]{
+ override def encode(in: Seq[Int]): Seq[Int] = in
+ override def decode(out: Seq[Int]): Seq[Int] = out
+ }
+
+ val transformingSeq =
+ TransformingEncoder[Seq[Int], Seq[Int]](
+ implicitly[ClassTag[Seq[Int]]],
+ IterableEncoder[Seq[Int], Int](implicitly[ClassTag[Seq[Int]]],
+ PrimitiveIntEncoder, false, false),
+ provider
+ )
+
+ val enc =
+ ProductEncoder(
+ implicitly[ClassTag[T]],
+ Seq(
+ EncoderField("_1",
+ IterableEncoder[Seq[Seq[Int]], Seq[Int]](implicitly[ClassTag[Seq[Seq[Int]]]],
+ transformingSeq, false, false), false, Metadata.empty),
+ EncoderField( "_2", transformingSeq, false, Metadata.empty)
+ ),
+ None
+ )
+ val sparkViaAgnostic = {
+ val ds = spark.createDataset(data)(enc)
+ ds.schema
+ }
+ // the nullability without TransformingEncoder nullability (SerializerBuilderHelper)
+ // is incorrect (_2 is inferred as nullable)
+ assert(enc.dataType === sparkViaAgnostic)
+ }
+
+ def provider[A]: () => Codec[V[A], A] = () =>
+ new Codec[V[A], A]{
+ override def encode(in: V[A]): A = in.v
+ override def decode(out: A): V[A] = V(out)
+ }
+
+ def transforming[A](underlying: AgnosticEncoder[A]): TransformingEncoder[V[A], A] =
+ TransformingEncoder[V[A], A](
+ implicitly[ClassTag[V[A]]],
+ underlying,
+ provider
+ )
+
+ val V_INT = StructType(Seq(StructField("v", IntegerType, nullable = false)))
+
+ // "value" usage for single field, a wrapping nullable type is required
+ val OPTION_OF_V_INT = StructType(Seq(StructField("value",
+ V_INT, nullable = true)))
+
+ // product encoder for a non-nullable V
+ val V_OF_INT =
+ ProductEncoder(
+ classTag[V[Int]],
+ Seq(EncoderField("v", PrimitiveIntEncoder, nullable = false, Metadata.empty)),
+ None
+ )
+
+ test("""Encoder derivation with nested TransformingEncoder of OptionEncoder""".stripMargin) {
+ val sparkI = spark
+ type T = V[V[Option[V[Int]]]]
+ val data: Seq[T] = Seq(V(V(None)), V(V(Some(V(1)))))
+ // for reference - datatype will introduce nested classes as expected
+ val sparkDataTypeOG = {
+ import sparkI.implicits._
+ val ds = spark.createDataset[V[V[Option[V[Int]]]]](data)
+ ds.schema
+ }
+
+ /* attempt to behave as if value class semantics except the last product,
+ using a final transforming instead of a product serializes */
+ val enc =
+ transforming(
+ transforming(
+ OptionEncoder(
+ // works
+ // transforming(PrimitiveIntEncoder)
+ // does not work
+ V_OF_INT
+ )
+ )
+ )
+
+ assert(enc.schema === OPTION_OF_V_INT)
+
+ val sparkViaAgnostic = {
+ val ds = spark.createDataset(data)(enc)
+ ds.schema
+ }
+
+ /* The schema has been changed to just the Product V[Int], the wrapping Option value
+ struct has been removed - it should not have been */
+ assert(sparkViaAgnostic === enc.schema)
+
+ val ds = spark.createDataset(data)(enc)
+ assert(ds.collect().toVector === data.toVector)
+ }
+
+ test("""Encoder derivation with TransformingEncoder of OptionEncoder""".stripMargin) {
Review Comment:
not really, it's only testing the recursion works in the ExpressionEncoder() detection. happy to remove
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "chris-twiner (via GitHub)" <gi...@apache.org>.
chris-twiner commented on PR #50023:
URL: https://github.com/apache/spark/pull/50023#issuecomment-2685258037
> Looks good overall.
>
> I am fine with merging it as is. Or a I can wait a bit so you can address the comments. I would like to get this in by RC2 (this will be cut end of this week). Please let me know what works for you.
fyi as you are coming on line now - I've got every point of the feedback implemented (excluding the two post rc2 notes I mentioned above - nullable top level field and union), bar the DeserializerBuildHelper decode invoke nulls point - it'll be another couple of hours to test that out.
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "chris-twiner (via GitHub)" <gi...@apache.org>.
chris-twiner commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1971629784
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/SerializerBuildHelper.scala:
##########
@@ -429,7 +430,10 @@ object SerializerBuildHelper {
Literal(codecProvider(), ObjectType(classOf[Codec[_, _]])),
"encode",
externalDataTypeFor(encoder),
- input :: Nil)
+ input :: Nil,
Review Comment:
I replaced the duplicative test with an explicit test for TransformingEncoder.nullable the catchily named "Encoder with OptionEncoder of transformation". Encoding itself "works" but the output is incorrect with Some(null) vs None. (NB in frameless this equates to 15 test failures)
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1970123366
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/encoders/AgnosticEncoder.scala:
##########
@@ -286,5 +286,6 @@ object AgnosticEncoders {
override def dataType: DataType = transformed.dataType
override def schema: StructType = transformed.schema
override def isStruct: Boolean = transformed.isStruct
+ override def nullable: Boolean = transformed.nullable
Review Comment:
<del>That also works for me.</del>
Actually having a nullable param is the only correct solution. It is up to the codec to decide how it deals with nulls.
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "chris-twiner (via GitHub)" <gi...@apache.org>.
chris-twiner commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1971213487
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/encoders/AgnosticEncoder.scala:
##########
@@ -286,5 +286,6 @@ object AgnosticEncoders {
override def dataType: DataType = transformed.dataType
override def schema: StructType = transformed.schema
override def isStruct: Boolean = transformed.isStruct
+ override def nullable: Boolean = transformed.nullable
Review Comment:
I'll correct that
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on PR #50023:
URL: https://github.com/apache/spark/pull/50023#issuecomment-2691797769
Merging to master/4.0. Thanks!
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1970795330
##########
sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala:
##########
@@ -2841,6 +2860,265 @@ class DatasetSuite extends QueryTest
checkDataset(Seq(seqMutableSet).toDS(), seqMutableSet)
checkDataset(Seq(mapMutableSet).toDS(), mapMutableSet)
}
+
+ // below tests are related to SPARK-49960 and TransformingEncoder usage
+ test("Incorrect derived nullability with TransformingEncoder - non nullable") {
+ val sparkI = spark
+ type T = Tuple2[Seq[Seq[Int]], Seq[Int]]
+ val data: Seq[T] = Seq( ( Seq( Seq(1, 2, 3) ), Seq(1, 2, 3) ) )
+ // for reference only
+ val sparkDataTypeOG = {
+ import sparkI.implicits._
+ val ds = spark.createDataset[Tuple2[Seq[Seq[Int]], Seq[Int]]](data)
+ ds.schema
+ }
+
+ val provider = () =>
+ new Codec[Seq[Int], Seq[Int]]{
+ override def encode(in: Seq[Int]): Seq[Int] = in
+ override def decode(out: Seq[Int]): Seq[Int] = out
+ }
+
+ val transformingSeq =
+ TransformingEncoder[Seq[Int], Seq[Int]](
+ implicitly[ClassTag[Seq[Int]]],
+ IterableEncoder[Seq[Int], Int](implicitly[ClassTag[Seq[Int]]],
+ PrimitiveIntEncoder, false, false),
+ provider
+ )
+
+ val enc =
+ ProductEncoder(
+ implicitly[ClassTag[T]],
+ Seq(
+ EncoderField("_1",
+ IterableEncoder[Seq[Seq[Int]], Seq[Int]](implicitly[ClassTag[Seq[Seq[Int]]]],
+ transformingSeq, false, false), false, Metadata.empty),
+ EncoderField( "_2", transformingSeq, false, Metadata.empty)
+ ),
+ None
+ )
+ val sparkViaAgnostic = {
+ val ds = spark.createDataset(data)(enc)
+ ds.schema
+ }
+ // the nullability without TransformingEncoder nullability (SerializerBuilderHelper)
+ // is incorrect (_2 is inferred as nullable)
+ assert(enc.dataType === sparkViaAgnostic)
+ }
+
+ def provider[A]: () => Codec[V[A], A] = () =>
+ new Codec[V[A], A]{
+ override def encode(in: V[A]): A = in.v
+ override def decode(out: A): V[A] = V(out)
+ }
+
+ def transforming[A](underlying: AgnosticEncoder[A]): TransformingEncoder[V[A], A] =
+ TransformingEncoder[V[A], A](
+ implicitly[ClassTag[V[A]]],
+ underlying,
+ provider
+ )
+
+ val V_INT = StructType(Seq(StructField("v", IntegerType, nullable = false)))
+
+ // "value" usage for single field, a wrapping nullable type is required
+ val OPTION_OF_V_INT = StructType(Seq(StructField("value",
+ V_INT, nullable = true)))
+
+ // product encoder for a non-nullable V
+ val V_OF_INT =
+ ProductEncoder(
+ classTag[V[Int]],
+ Seq(EncoderField("v", PrimitiveIntEncoder, nullable = false, Metadata.empty)),
+ None
+ )
+
+ test("""Encoder derivation with nested TransformingEncoder of OptionEncoder""".stripMargin) {
+ val sparkI = spark
+ type T = V[V[Option[V[Int]]]]
+ val data: Seq[T] = Seq(V(V(None)), V(V(Some(V(1)))))
+ // for reference - datatype will introduce nested classes as expected
+ val sparkDataTypeOG = {
+ import sparkI.implicits._
+ val ds = spark.createDataset[V[V[Option[V[Int]]]]](data)
+ ds.schema
+ }
+
+ /* attempt to behave as if value class semantics except the last product,
+ using a final transforming instead of a product serializes */
+ val enc =
+ transforming(
+ transforming(
+ OptionEncoder(
+ // works
+ // transforming(PrimitiveIntEncoder)
Review Comment:
Bug?
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1970792401
##########
sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala:
##########
@@ -2841,6 +2860,265 @@ class DatasetSuite extends QueryTest
checkDataset(Seq(seqMutableSet).toDS(), seqMutableSet)
checkDataset(Seq(mapMutableSet).toDS(), mapMutableSet)
}
+
+ // below tests are related to SPARK-49960 and TransformingEncoder usage
+ test("Incorrect derived nullability with TransformingEncoder - non nullable") {
+ val sparkI = spark
+ type T = Tuple2[Seq[Seq[Int]], Seq[Int]]
+ val data: Seq[T] = Seq( ( Seq( Seq(1, 2, 3) ), Seq(1, 2, 3) ) )
+ // for reference only
+ val sparkDataTypeOG = {
Review Comment:
It is a bit bit distracting to add code that is not used. I understand that this represents the automated way of encoder inference.
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "chris-twiner (via GitHub)" <gi...@apache.org>.
chris-twiner commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1971232668
##########
sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala:
##########
@@ -2841,6 +2860,265 @@ class DatasetSuite extends QueryTest
checkDataset(Seq(seqMutableSet).toDS(), seqMutableSet)
checkDataset(Seq(mapMutableSet).toDS(), mapMutableSet)
}
+
+ // below tests are related to SPARK-49960 and TransformingEncoder usage
+ test("Incorrect derived nullability with TransformingEncoder - non nullable") {
+ val sparkI = spark
+ type T = Tuple2[Seq[Seq[Int]], Seq[Int]]
+ val data: Seq[T] = Seq( ( Seq( Seq(1, 2, 3) ), Seq(1, 2, 3) ) )
+ // for reference only
+ val sparkDataTypeOG = {
Review Comment:
agreed, per above I've removed
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1970706421
##########
sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala:
##########
@@ -2841,6 +2860,265 @@ class DatasetSuite extends QueryTest
checkDataset(Seq(seqMutableSet).toDS(), seqMutableSet)
checkDataset(Seq(mapMutableSet).toDS(), mapMutableSet)
}
+
+ // below tests are related to SPARK-49960 and TransformingEncoder usage
+ test("Incorrect derived nullability with TransformingEncoder - non nullable") {
Review Comment:
I am a bit confused by the phrasing here. Did we fix the issue? Or are you showing it is broken?
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "chris-twiner (via GitHub)" <gi...@apache.org>.
chris-twiner commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1971695592
##########
sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala:
##########
@@ -2841,6 +2860,265 @@ class DatasetSuite extends QueryTest
checkDataset(Seq(seqMutableSet).toDS(), seqMutableSet)
checkDataset(Seq(mapMutableSet).toDS(), mapMutableSet)
}
+
+ // below tests are related to SPARK-49960 and TransformingEncoder usage
+ test("Incorrect derived nullability with TransformingEncoder - non nullable") {
+ val sparkI = spark
+ type T = Tuple2[Seq[Seq[Int]], Seq[Int]]
+ val data: Seq[T] = Seq( ( Seq( Seq(1, 2, 3) ), Seq(1, 2, 3) ) )
+ // for reference only
+ val sparkDataTypeOG = {
+ import sparkI.implicits._
+ val ds = spark.createDataset[Tuple2[Seq[Seq[Int]], Seq[Int]]](data)
+ ds.schema
+ }
+
+ val provider = () =>
+ new Codec[Seq[Int], Seq[Int]]{
+ override def encode(in: Seq[Int]): Seq[Int] = in
+ override def decode(out: Seq[Int]): Seq[Int] = out
+ }
+
+ val transformingSeq =
+ TransformingEncoder[Seq[Int], Seq[Int]](
+ implicitly[ClassTag[Seq[Int]]],
+ IterableEncoder[Seq[Int], Int](implicitly[ClassTag[Seq[Int]]],
+ PrimitiveIntEncoder, false, false),
+ provider
+ )
+
+ val enc =
+ ProductEncoder(
+ implicitly[ClassTag[T]],
+ Seq(
+ EncoderField("_1",
+ IterableEncoder[Seq[Seq[Int]], Seq[Int]](implicitly[ClassTag[Seq[Seq[Int]]]],
+ transformingSeq, false, false), false, Metadata.empty),
+ EncoderField( "_2", transformingSeq, false, Metadata.empty)
+ ),
+ None
+ )
+ val sparkViaAgnostic = {
+ val ds = spark.createDataset(data)(enc)
+ ds.schema
+ }
+ // the nullability without TransformingEncoder nullability (SerializerBuilderHelper)
+ // is incorrect (_2 is inferred as nullable)
+ assert(enc.dataType === sparkViaAgnostic)
+ }
+
+ def provider[A]: () => Codec[V[A], A] = () =>
+ new Codec[V[A], A]{
+ override def encode(in: V[A]): A = in.v
+ override def decode(out: A): V[A] = V(out)
+ }
+
+ def transforming[A](underlying: AgnosticEncoder[A]): TransformingEncoder[V[A], A] =
+ TransformingEncoder[V[A], A](
+ implicitly[ClassTag[V[A]]],
+ underlying,
+ provider
+ )
+
+ val V_INT = StructType(Seq(StructField("v", IntegerType, nullable = false)))
+
+ // "value" usage for single field, a wrapping nullable type is required
+ val OPTION_OF_V_INT = StructType(Seq(StructField("value",
+ V_INT, nullable = true)))
+
+ // product encoder for a non-nullable V
+ val V_OF_INT =
+ ProductEncoder(
+ classTag[V[Int]],
+ Seq(EncoderField("v", PrimitiveIntEncoder, nullable = false, Metadata.empty)),
+ None
+ )
+
+ test("""Encoder derivation with nested TransformingEncoder of OptionEncoder""".stripMargin) {
+ val sparkI = spark
+ type T = V[V[Option[V[Int]]]]
+ val data: Seq[T] = Seq(V(V(None)), V(V(Some(V(1)))))
+ // for reference - datatype will introduce nested classes as expected
+ val sparkDataTypeOG = {
+ import sparkI.implicits._
+ val ds = spark.createDataset[V[V[Option[V[Int]]]]](data)
+ ds.schema
+ }
+
+ /* attempt to behave as if value class semantics except the last product,
+ using a final transforming instead of a product serializes */
+ val enc =
+ transforming(
+ transforming(
+ OptionEncoder(
+ // works
+ // transforming(PrimitiveIntEncoder)
Review Comment:
no, I've removed that along with the other visual cruft
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1970705855
##########
sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala:
##########
@@ -2841,6 +2860,265 @@ class DatasetSuite extends QueryTest
checkDataset(Seq(seqMutableSet).toDS(), seqMutableSet)
checkDataset(Seq(mapMutableSet).toDS(), mapMutableSet)
}
+
+ // below tests are related to SPARK-49960 and TransformingEncoder usage
+ test("Incorrect derived nullability with TransformingEncoder - non nullable") {
+ val sparkI = spark
+ type T = Tuple2[Seq[Seq[Int]], Seq[Int]]
+ val data: Seq[T] = Seq( ( Seq( Seq(1, 2, 3) ), Seq(1, 2, 3) ) )
+ // for reference only
+ val sparkDataTypeOG = {
+ import sparkI.implicits._
Review Comment:
NIT: Don't we import `testImplicits._` here?
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "chris-twiner (via GitHub)" <gi...@apache.org>.
chris-twiner commented on PR #50023:
URL: https://github.com/apache/spark/pull/50023#issuecomment-2691506401
> @chris-twiner can you fix the style issue?
yeah done, I'll keep an eye on the progress over the next hour or so incase any restarts are needed. Odd though, no files mentioned as incorrectly formatted in the build (just that there are some), maven run locally to check says it's good, intellij says the files are good - actually running the mvn reformat instead of validate did the trick :-)
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "chris-twiner (via GitHub)" <gi...@apache.org>.
chris-twiner commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1971206736
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoder.scala:
##########
@@ -228,7 +236,8 @@ case class ExpressionEncoder[T](
* returns true if `T` is serialized as struct and is not `Option` type.
*/
def isSerializedAsStructForTopLevel: Boolean = {
- isSerializedAsStruct && !classOf[Option[_]].isAssignableFrom(clsTag.runtimeClass)
+ isSerializedAsStruct && !classOf[Option[_]].isAssignableFrom(clsTag.runtimeClass) &&
Review Comment:
I think it'd make sense, for the path encoder backwards compat logic I can embed / document that in shim. The Builders could embed that. I can take a stab at that post rc2.
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "chris-twiner (via GitHub)" <gi...@apache.org>.
chris-twiner commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r2665525133
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/encoders/EncoderUtils.scala:
##########
@@ -26,6 +27,30 @@ import org.apache.spark.sql.catalyst.util.{ArrayData, MapData}
import org.apache.spark.sql.types.{ArrayType, BinaryType, BooleanType, ByteType, CalendarIntervalType, DataType, DateType, DayTimeIntervalType, Decimal, DecimalType, DoubleType, FloatType, IntegerType, LongType, MapType, ObjectType, ShortType, StringType, StructType, TimestampNTZType, TimestampType, UserDefinedType, VariantType, YearMonthIntervalType}
import org.apache.spark.unsafe.types.{CalendarInterval, UTF8String, VariantVal}
+/**
+ * :: DeveloperApi ::
+ * Extensible [[AgnosticEncoder]] providing conversion extension points over type T
+ * @tparam T over T
+ */
+@DeveloperApi
+@deprecated("This trait is intended only as a migration tool and will be removed in 4.1")
Review Comment:
[SPARK-54919](https://issues.apache.org/jira/browse/SPARK-54919) raised to track.
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "chris-twiner (via GitHub)" <gi...@apache.org>.
chris-twiner commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r2664173469
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/encoders/EncoderUtils.scala:
##########
@@ -26,6 +27,30 @@ import org.apache.spark.sql.catalyst.util.{ArrayData, MapData}
import org.apache.spark.sql.types.{ArrayType, BinaryType, BooleanType, ByteType, CalendarIntervalType, DataType, DateType, DayTimeIntervalType, Decimal, DecimalType, DoubleType, FloatType, IntegerType, LongType, MapType, ObjectType, ShortType, StringType, StructType, TimestampNTZType, TimestampType, UserDefinedType, VariantType, YearMonthIntervalType}
import org.apache.spark.unsafe.types.{CalendarInterval, UTF8String, VariantVal}
+/**
+ * :: DeveloperApi ::
+ * Extensible [[AgnosticEncoder]] providing conversion extension points over type T
+ * @tparam T over T
+ */
+@DeveloperApi
+@deprecated("This trait is intended only as a migration tool and will be removed in 4.1")
Review Comment:
hihi, that's on me, I committed to Herman to get the PR in order to allow this. I've got all the derivation logic for Frameless working with AgnosticEncoder's and I've started the work locally, I'll raise a jira and PR within a week.
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1970795330
##########
sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala:
##########
@@ -2841,6 +2860,265 @@ class DatasetSuite extends QueryTest
checkDataset(Seq(seqMutableSet).toDS(), seqMutableSet)
checkDataset(Seq(mapMutableSet).toDS(), mapMutableSet)
}
+
+ // below tests are related to SPARK-49960 and TransformingEncoder usage
+ test("Incorrect derived nullability with TransformingEncoder - non nullable") {
+ val sparkI = spark
+ type T = Tuple2[Seq[Seq[Int]], Seq[Int]]
+ val data: Seq[T] = Seq( ( Seq( Seq(1, 2, 3) ), Seq(1, 2, 3) ) )
+ // for reference only
+ val sparkDataTypeOG = {
+ import sparkI.implicits._
+ val ds = spark.createDataset[Tuple2[Seq[Seq[Int]], Seq[Int]]](data)
+ ds.schema
+ }
+
+ val provider = () =>
+ new Codec[Seq[Int], Seq[Int]]{
+ override def encode(in: Seq[Int]): Seq[Int] = in
+ override def decode(out: Seq[Int]): Seq[Int] = out
+ }
+
+ val transformingSeq =
+ TransformingEncoder[Seq[Int], Seq[Int]](
+ implicitly[ClassTag[Seq[Int]]],
+ IterableEncoder[Seq[Int], Int](implicitly[ClassTag[Seq[Int]]],
+ PrimitiveIntEncoder, false, false),
+ provider
+ )
+
+ val enc =
+ ProductEncoder(
+ implicitly[ClassTag[T]],
+ Seq(
+ EncoderField("_1",
+ IterableEncoder[Seq[Seq[Int]], Seq[Int]](implicitly[ClassTag[Seq[Seq[Int]]]],
+ transformingSeq, false, false), false, Metadata.empty),
+ EncoderField( "_2", transformingSeq, false, Metadata.empty)
+ ),
+ None
+ )
+ val sparkViaAgnostic = {
+ val ds = spark.createDataset(data)(enc)
+ ds.schema
+ }
+ // the nullability without TransformingEncoder nullability (SerializerBuilderHelper)
+ // is incorrect (_2 is inferred as nullable)
+ assert(enc.dataType === sparkViaAgnostic)
+ }
+
+ def provider[A]: () => Codec[V[A], A] = () =>
+ new Codec[V[A], A]{
+ override def encode(in: V[A]): A = in.v
+ override def decode(out: A): V[A] = V(out)
+ }
+
+ def transforming[A](underlying: AgnosticEncoder[A]): TransformingEncoder[V[A], A] =
+ TransformingEncoder[V[A], A](
+ implicitly[ClassTag[V[A]]],
+ underlying,
+ provider
+ )
+
+ val V_INT = StructType(Seq(StructField("v", IntegerType, nullable = false)))
+
+ // "value" usage for single field, a wrapping nullable type is required
+ val OPTION_OF_V_INT = StructType(Seq(StructField("value",
+ V_INT, nullable = true)))
+
+ // product encoder for a non-nullable V
+ val V_OF_INT =
+ ProductEncoder(
+ classTag[V[Int]],
+ Seq(EncoderField("v", PrimitiveIntEncoder, nullable = false, Metadata.empty)),
+ None
+ )
+
+ test("""Encoder derivation with nested TransformingEncoder of OptionEncoder""".stripMargin) {
+ val sparkI = spark
+ type T = V[V[Option[V[Int]]]]
+ val data: Seq[T] = Seq(V(V(None)), V(V(Some(V(1)))))
+ // for reference - datatype will introduce nested classes as expected
+ val sparkDataTypeOG = {
+ import sparkI.implicits._
+ val ds = spark.createDataset[V[V[Option[V[Int]]]]](data)
+ ds.schema
+ }
+
+ /* attempt to behave as if value class semantics except the last product,
+ using a final transforming instead of a product serializes */
+ val enc =
+ transforming(
+ transforming(
+ OptionEncoder(
+ // works
+ // transforming(PrimitiveIntEncoder)
Review Comment:
Is this an open bug?
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "chris-twiner (via GitHub)" <gi...@apache.org>.
chris-twiner commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1971723345
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/DeserializerBuildHelper.scala:
##########
@@ -458,7 +460,10 @@ object DeserializerBuildHelper {
Literal.create(provider(), ObjectType(classOf[Codec[_, _]])),
"decode",
ObjectType(tag.runtimeClass),
- createDeserializer(encoder, path, walkedTypePath) :: Nil)
+ createDeserializer(encoder, path, walkedTypePath) :: Nil,
+ propagateNull = enc.nullable,
Review Comment:
it's possible I just added for symmetry, it doesn't have an effect on the Spark test cases, I'll report back in a couple of hours if it affects any frameless ones - either adding a test to cover those two lines or reverting them.
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1970783242
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/SerializerBuildHelper.scala:
##########
@@ -429,7 +430,10 @@ object SerializerBuildHelper {
Literal(codecProvider(), ObjectType(classOf[Codec[_, _]])),
"encode",
externalDataTypeFor(encoder),
- input :: Nil)
+ input :: Nil,
Review Comment:
It took me a while to realize that this fixes the nullability issue, and not the TransformingEncoder fix. Based on the nullability of the struct field we insert an AssertNotNull and a KnowNotNull; the correct nullability was not propagated by Invoke.
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "chris-twiner (via GitHub)" <gi...@apache.org>.
chris-twiner commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1971723345
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/DeserializerBuildHelper.scala:
##########
@@ -458,7 +460,10 @@ object DeserializerBuildHelper {
Literal.create(provider(), ObjectType(classOf[Codec[_, _]])),
"decode",
ObjectType(tag.runtimeClass),
- createDeserializer(encoder, path, walkedTypePath) :: Nil)
+ createDeserializer(encoder, path, walkedTypePath) :: Nil,
+ propagateNull = enc.nullable,
Review Comment:
probably just added for symmetry, it doesn't have an effect on the Spark or frameless test cases, I've reverting them.
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "asfgit (via GitHub)" <gi...@apache.org>.
asfgit closed pull request #50023: [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes
URL: https://github.com/apache/spark/pull/50023
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "chris-twiner (via GitHub)" <gi...@apache.org>.
chris-twiner commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1969966712
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/encoders/AgnosticEncoder.scala:
##########
@@ -286,5 +286,6 @@ object AgnosticEncoders {
override def dataType: DataType = transformed.dataType
override def schema: StructType = transformed.schema
override def isStruct: Boolean = transformed.isStruct
+ override def nullable: Boolean = transformed.nullable
Review Comment:
Given this is only for Spark 4 I'd be happier adding nullable as a param, this would allow signalling to the builders if the codec can handle nulls or not for the general case. Let me know if that's an option and I'll do a full suite of testing again.
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1970126078
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoder.scala:
##########
@@ -228,7 +236,8 @@ case class ExpressionEncoder[T](
* returns true if `T` is serialized as struct and is not `Option` type.
*/
def isSerializedAsStructForTopLevel: Boolean = {
- isSerializedAsStruct && !classOf[Option[_]].isAssignableFrom(clsTag.runtimeClass)
+ isSerializedAsStruct && !classOf[Option[_]].isAssignableFrom(clsTag.runtimeClass) &&
Review Comment:
`isSerializedAsStruct && !transformerOfOption(encoder)`?
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1970135148
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoderSuite.scala:
##########
@@ -142,6 +142,19 @@ case class OptionNestedGeneric[T](list: Option[T])
case class MapNestedGenericKey[T](list: Map[T, Int])
case class MapNestedGenericValue[T](list: Map[Int, T])
+// ADT encoding for TransformingEncoder test
+trait Base {
Review Comment:
For this case I really want to add some sort of a UnionEncoder. That either nests the individual implementations, or flattens them (+ a discriminator field).
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "chris-twiner (via GitHub)" <gi...@apache.org>.
chris-twiner commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1971266224
##########
sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala:
##########
@@ -2841,6 +2860,265 @@ class DatasetSuite extends QueryTest
checkDataset(Seq(seqMutableSet).toDS(), seqMutableSet)
checkDataset(Seq(mapMutableSet).toDS(), mapMutableSet)
}
+
+ // below tests are related to SPARK-49960 and TransformingEncoder usage
+ test("Incorrect derived nullability with TransformingEncoder - non nullable") {
Review Comment:
removed, per your lower point it's better tested in the iterable test
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1969891795
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/encoders/AgnosticEncoder.scala:
##########
@@ -286,5 +286,6 @@ object AgnosticEncoders {
override def dataType: DataType = transformed.dataType
override def schema: StructType = transformed.schema
override def isStruct: Boolean = transformed.isStruct
+ override def nullable: Boolean = transformed.nullable
Review Comment:
yikes...
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1970791525
##########
sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala:
##########
Review Comment:
I think most of these test cases could also be tested in ExpressionEncoderSuite. For example for the nullability tests we should just check if the schema produced by the agnostic encoder is the same as the schema produced by the expression encoder.
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1970806608
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/DeserializerBuildHelper.scala:
##########
@@ -270,6 +270,8 @@ object DeserializerBuildHelper {
enc: AgnosticEncoder[_],
path: Expression,
walkedTypePath: WalkedTypePath): Expression = enc match {
+ case ae: AgnosticExpressionPathEncoder[_] =>
+ ae.fromCatalyst(path)
Review Comment:
Is this tested anywhere? I know you have a test for` toCatalyst`.
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "chris-twiner (via GitHub)" <gi...@apache.org>.
chris-twiner commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1971221056
##########
sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala:
##########
@@ -2841,6 +2860,265 @@ class DatasetSuite extends QueryTest
checkDataset(Seq(seqMutableSet).toDS(), seqMutableSet)
checkDataset(Seq(mapMutableSet).toDS(), mapMutableSet)
}
+
+ // below tests are related to SPARK-49960 and TransformingEncoder usage
+ test("Incorrect derived nullability with TransformingEncoder - non nullable") {
+ val sparkI = spark
+ type T = Tuple2[Seq[Seq[Int]], Seq[Int]]
+ val data: Seq[T] = Seq( ( Seq( Seq(1, 2, 3) ), Seq(1, 2, 3) ) )
+ // for reference only
+ val sparkDataTypeOG = {
+ import sparkI.implicits._
Review Comment:
I've removed all of the "for reference only" sections, they were only for debug and I should have removed the beforehand
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49960][SQL] Custom ExpressionEncoder support and TransformingEncoder fixes [spark]
Posted by "chris-twiner (via GitHub)" <gi...@apache.org>.
chris-twiner commented on code in PR #50023:
URL: https://github.com/apache/spark/pull/50023#discussion_r1971211739
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoderSuite.scala:
##########
@@ -142,6 +142,19 @@ case class OptionNestedGeneric[T](list: Option[T])
case class MapNestedGenericKey[T](list: Map[T, Int])
case class MapNestedGenericValue[T](list: Map[Int, T])
+// ADT encoding for TransformingEncoder test
+trait Base {
Review Comment:
It would save an intermediary representation, the problem is the scope of the field, locking it down to one field doesn't seem right but if the field could be optionally dropped then it could be a generated Column by the users. I'll take a stab at it post rc2
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org