[SPARK-48898][SQL] Set nullability correctly in the Variant schema #49151
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…schema" This reverts commit b2c8b30.
dongjoon-hyun
approved these changes
Dec 11, 2024
chenhao-db
approved these changes
Dec 11, 2024
HyukjinKwon
approved these changes
Dec 12, 2024
cloud-fan
approved these changes
Dec 12, 2024
|
Could you re-trigger once more, @cashmand ? |
|
Hi @dongjoon-hyun, it looks like the tests are passing now. |
|
Actually @dongjoon-hyun, I just realized that ArrayType has the same nullability issue. Let me update the PR to also set the nullability correctly on ArrayType. |
|
Thank you! |
chenhao-db
approved these changes
Dec 12, 2024
|
@dongjoon-hyun, the PR should be good to merge now. Thanks for your patience! |
|
thanks, merging to master! |
ericm-db
pushed a commit
to ericm-db/spark
that referenced
this pull request
Dec 17, 2024
### What changes were proposed in this pull request? The variantShreddingSchema method converts a human-readable schema for Variant to one that's a valid shredding schema. According to the shredding schema in apache/parquet-format#461, each shredded field in an object should be a required group - i.e. a non-nullable struct. This PR fixes the variantShreddingSchema to mark that struct as non-nullable. ### Why are the changes needed? If we use variantShreddingSchema to construct a schema for Parquet, the schema would be technically non-conformant with the spec by setting the group as optional. I don't think this should really matter to readers, but it would waste a bit of space in the Parquet file by adding an extra definition level. ### Does this PR introduce _any_ user-facing change? No, this code is not used yet. ### How was this patch tested? Added a test to do some minimal validation of the variantShreddingSchema function. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#49151 from cashmand/SPARK-48898-nullability-again. Authored-by: cashmand <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
The variantShreddingSchema method converts a human-readable schema for Variant to one that's a valid shredding schema. According to the shredding schema in apache/parquet-format#461, each shredded field in an object should be a required group - i.e. a non-nullable struct. This PR fixes the variantShreddingSchema to mark that struct as non-nullable.
Why are the changes needed?
If we use variantShreddingSchema to construct a schema for Parquet, the schema would be technically non-conformant with the spec by setting the group as optional. I don't think this should really matter to readers, but it would waste a bit of space in the Parquet file by adding an extra definition level.
Does this PR introduce any user-facing change?
No, this code is not used yet.
How was this patch tested?
Added a test to do some minimal validation of the variantShreddingSchema function.
Was this patch authored or co-authored using generative AI tooling?
No.