You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2021/07/13 06:37:35 UTC
[GitHub] [spark] eejbyfeldt opened a new pull request #33316: [WIP][SPARK-20384][SQL] Support value classes and always encoded as underlying type
eejbyfeldt opened a new pull request #33316:
URL: https://github.com/apache/spark/pull/33316
### What changes were proposed in this pull request?
This PR adds support for using value class in nested case classes. This has previously been proposed in the following PRs: https://github.com/apache/spark/pull/22309 https://github.com/apache/spark/pull/27153 https://github.com/apache/spark/pull/33205
The first PR had the same approach as in this one of always encoding the value class as the underlying type. But as noted by @cloud-fan: https://github.com/apache/spark/pull/22309#issuecomment-433913001 the implementation was quite spread out in `ScalaReflection.scala` this is addressed in this PR by "unwrapping" the value class only when used inside a case class. This removes the need for the extra argument `instantiateValueClass` to `deserializerFor` and makes the implementation less spread out.
The other two are both by @mickjermsurawong-stripe and takes the approach that value class are only "unwrapped" is cases where there there will be no runtime object. This has the advantage of being fully backwards compatible with cases where case class currently work. The partial support for value classes was original added in: https://github.com/apache/spark/pull/15284
does only add test cases for single column value classes. Therefore it seems like other cases where value classes happen to work like `Option[ValueClass]` or `Array[ValueClass]` are mostly accidental and where not thoughtfully designed to have the current behavior.
This PR is meant for discussion and alternative approach to https://github.com/apache/spark/pull/33205 if there is no need for schema backwards compatibility.
<!--
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.
-->
### Why are the changes needed?
Support value classes in Datasets, so that they can be used in code bases that currently use them in their modeling.
<!--
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.
-->
### Does this PR introduce _any_ user-facing change?
This add supports for using value class in DataFrames and Datasets that would previously led to exceptions.
The main change the can effect existing code is that previously single column value classes would be encoded as they where simple case class.
To avoid extra complexity this patch always treats value classes as their underlying type.
Therefore given the following case class:
```
case class MyInt(wrapped: Int) extends AnyVal
```
this means that both `Dataset[Int]` and `Dataset[MyInt]` will have the same schema
```
root
|-- value: integer (nullable = false)
```
Before this patch the schema of `Dataset[MyInt]` would instead have been
```
root
|-- wrapped: integer (nullable = false)
```
### How was this patch tested?
New unit tests in: `ScalaReflectionSuite.scala`, `ExpressionEncoderSuite.scala`, `DataFrameSuite.scala` and `DatasetSuite.scala`.
--
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
[GitHub] [spark] eejbyfeldt closed pull request #33316: [WIP][SPARK-20384][SQL] Support value classes and always encoded as underlying type
Posted by GitBox <gi...@apache.org>.
eejbyfeldt closed pull request #33316:
URL: https://github.com/apache/spark/pull/33316
--
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
[GitHub] [spark] eejbyfeldt commented on pull request #33316: [WIP][SPARK-20384][SQL] Support value classes and always encoded as underlying type
Posted by GitBox <gi...@apache.org>.
eejbyfeldt commented on pull request #33316:
URL: https://github.com/apache/spark/pull/33316#issuecomment-890778829
Closing and hoping we can have: https://github.com/apache/spark/pull/33205 merged instead.
--
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
[GitHub] [spark] AmplabJenkins commented on pull request #33316: [WIP][SPARK-20384][SQL] Support value classes and always encoded as underlying type
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33316:
URL: https://github.com/apache/spark/pull/33316#issuecomment-878820358
Can one of the admins verify this patch?
--
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