[SPARK-49985][SQL] Remove support for interval types in Variant #48215
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
harshmotw-db
commented
Sep 23, 2024
common/variant/README.md
Outdated
| Type IDs 17 and 18 were originally reserved for a prototype feature (string-from-metadata) that was never implemented. These IDs are available for use by new types. | ||
|
|
||
| [1] The parquet format does not have pure equivalents for the year-month and day-time interval types. Year-month intervals are usually represented using int32 values and the day-time intervals are usually represented using int64 values. However, these values don't include the start and end fields of these types. Therefore, Spark stores them in the column metadata. | ||
| Type IDs 17 and 18 were originally reserved for a prototype feature (string-from-metadata) that was never implemented. These IDs are available for use by new types. Type IDs 19 and 20 were used to represent interval types for whom support has been temporarily disabled, and therefore, these type IDs should not be used by new types. |
There was a problem hiding this comment.
NOTE TO REVIEWERS: Please see if you approve of this comment.
There was a problem hiding this comment.
Why can't future types use 19 and 20? Shouldn't intervals just be removed completely, and added later when the exact encoded format is agreed upon?
|
cc @cashmand For changes in the README file. |
gene-db
reviewed
Sep 24, 2024
There was a problem hiding this comment.
@harshmotw-db I have a high-level question about the interval. Shouldn't we just remove the interval type, and "release" the ids?
common/variant/README.md
Outdated
| Type IDs 17 and 18 were originally reserved for a prototype feature (string-from-metadata) that was never implemented. These IDs are available for use by new types. | ||
|
|
||
| [1] The parquet format does not have pure equivalents for the year-month and day-time interval types. Year-month intervals are usually represented using int32 values and the day-time intervals are usually represented using int64 values. However, these values don't include the start and end fields of these types. Therefore, Spark stores them in the column metadata. | ||
| Type IDs 17 and 18 were originally reserved for a prototype feature (string-from-metadata) that was never implemented. These IDs are available for use by new types. Type IDs 19 and 20 were used to represent interval types for whom support has been temporarily disabled, and therefore, these type IDs should not be used by new types. |
There was a problem hiding this comment.
Why can't future types use 19 and 20? Shouldn't intervals just be removed completely, and added later when the exact encoded format is agreed upon?
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala
Outdated
Show resolved
Hide resolved
…on't work with Variants
gene-db
approved these changes
Oct 15, 2024
|
@cloud-fan @HyukjinKwon can you please look at this PR? Thanks! |
HyukjinKwon
approved these changes
Oct 23, 2024
|
Merged to master. |
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?
Support for interval types was added to the variant spec. This PR removes this support and removes the ability to cast from interval types to variant and vice versa.
Why are the changes needed?
I implemented interval support for Variant before, but because the Variant spec type is supposed to be open and compatible with other engines which may not support all the ANSI Interval types, more thought needs to be put into the design of these intervals in Variant.
Does this PR introduce any user-facing change?
Yes, after this change, users would no longer be able to cast between variants and intervals.
How was this patch tested?
Unit tests making sure that
Was this patch authored or co-authored using generative AI tooling?
No.