mickjermsurawong-stripe opened a new pull request #33205:
URL: https://github.com/apache/spark/pull/33205
<!--
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 message, please read the
guideline first:
https://spark.apache.org/error-message-guidelines.html
-->
### 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.
-->
- This PR revisits https://github.com/apache/spark/pull/22309, and
[SPARK-20384](https://issues.apache.org/jira/browse/SPARK-20384) solving the
original problem, but additionally will prevent backward-compat break on schema
of top-level `AnyVal` value class.
- Why previous break? We currently support top-level value classes just as
any other case class; field of the underlying type is present in schema. This
means any dataframe SQL filtering on this expects the field name to be present.
The previous PR changes this schema and would result in breaking current usage.
See test `"schema for case class that is a value class"`. This PR keeps the
schema.
- We actually currently support collection of value classes prior to this
change, but not case class of nested value class. This means the schema of
these classes shouldn't change to prevent breaking too.
- However, what we can change, without breaking, is schema of nested value
class, which will fails due to the compile problem, and thus its schema now
isn't actually valid. After the change, the schema of this nested value class
is now flattened
- With this PR, there's flattening only for nested value class (new), but
not for top-level and collection classes (existing behavior)
- This PR revisits https://github.com/apache/spark/pull/27153 by handling
tuple `Tuple2[AnyVal, AnyVal]` which is a constructor ("nested class") but is a
generic type, so it should not be flattened behaving similarly to `Seq[AnyVal]`
### 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.
-->
- Currently, nested value class isn't supported. This is because when the
generated code treats `anyVal` class in its unwrapped form, but we encode the
type to be the wrapped case class. This results in compile of generated code
For example,
For a given `AnyVal` wrapper and its root-level class container
```
case class IntWrapper(i: Int) extends AnyVal
case class ComplexValueClassContainer(c: IntWrapper)
```
The problematic part of generated code:
```
private InternalRow If_1(InternalRow i) {
boolean isNull_42 = i.isNullAt(0);
// 1) ******** The root-level case class we care
org.apache.spark.sql.catalyst.encoders.ComplexValueClassContainer
value_46 = isNull_42 ?
null :
((org.apache.spark.sql.catalyst.encoders.ComplexValueClassContainer) i.get(0,
null));
if (isNull_42) {
throw new NullPointerException(((java.lang.String) references[5]
/* errMsg */ ));
}
boolean isNull_39 = true;
// 2) ******** We specify its member to be unwrapped case class
extending `AnyVal`
org.apache.spark.sql.catalyst.encoders.IntWrapper value_43 = null;
if (!false) {
isNull_39 = false;
if (!isNull_39) {
// 3) ******** ERROR: `c()` compiled however is of type
`int` and thus we see error
value_43 = value_46.c();
}
}
```
We get this errror: Assignment conversion not possible from type "int" to
type "org.apache.spark.sql.catalyst.encoders.IntWrapper"
```
java.util.concurrent.ExecutionException:
org.codehaus.commons.compiler.CompileException:
File 'generated.java', Line 159, Column 1: failed to compile:
org.codehaus.commons.compiler.CompileException: File 'generated.java', Line
159, Column 1: Assignment conversion not possible from type "int" to type
"org.apache.spark.sql.catalyst.encoders.IntWrapper"
```
From [doc](https://docs.scala-lang.org/overviews/core/value-classes.html) on
value class: , 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`.
### 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'.
-->
- Yes, this will allow support for the nested value class.
### 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.
-->
- Added unit tests to illustrate
- raw schema
- projection
- round-trip encode/decode
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
