feat: use Substrait's PrecisionTimestamp and PrecisionTimestampTz instead of deprecated Timestamp#11597
Merged
alamb merged 5 commits intoapache:mainfrom Aug 22, 2024
Merged
Conversation
Blizzara
force-pushed
the
avo/precision-timestamp
branch
from
49d90c8 to
350e00f
Compare
Blizzara
force-pushed
the
avo/precision-timestamp
branch
from
350e00f to
e8753f6
Compare
Blizzara
commented
Aug 20, 2024
| r#type::Kind::IntervalYear(_) => { | ||
| Ok(DataType::Interval(IntervalUnit::YearMonth)) | ||
| } | ||
| r#type::Kind::IntervalDay(_) => Ok(DataType::Interval(IntervalUnit::DayTime)), |
Contributor
Author
There was a problem hiding this comment.
this was just cleanup - we don't check type variations for types where we don't use them
Blizzara
commented
Aug 20, 2024
| })) => { | ||
| // DF only supports millisecond precision, so we lose the micros here | ||
| ScalarValue::new_interval_dt(*days, (seconds * 1000) + (microseconds / 1000)) | ||
| // DF only supports millisecond precision, so for any more granular type we lose precision |
Contributor
Author
There was a problem hiding this comment.
these changes were needed as part of bumping substrait (substrait-io/substrait#665)
Blizzara
commented
Aug 20, 2024
| kind: Some(r#type::Kind::IntervalDay(r#type::IntervalDay { | ||
| type_variation_reference: DEFAULT_TYPE_VARIATION_REF, | ||
| nullability, | ||
| precision: Some(3), // DayTime precision is always milliseconds |
Contributor
Author
There was a problem hiding this comment.
required due to bumping substrait substrait-io/substrait#665
Blizzara
force-pushed
the
avo/precision-timestamp
branch
from
e8753f6 to
a639feb
Compare
Blizzara
commented
Aug 20, 2024
| substrait = { version = "0.36.0", features = ["serde"] } | ||
| pbjson-types = "0.7" | ||
| prost = "0.13" | ||
| substrait = { version = "0.41", features = ["serde"] } |
Contributor
Author
There was a problem hiding this comment.
need a bump for precision timestamp types to have precision, and their value as i64 instead of u64
pbjson and prost need to be bumped to match substrait's deps
Contributor
Blizzara
commented
Aug 20, 2024
| join_rel::JoinType::Anti => Ok(JoinType::LeftAnti), | ||
| join_rel::JoinType::Semi => Ok(JoinType::LeftSemi), | ||
| join_rel::JoinType::LeftAnti => Ok(JoinType::LeftAnti), | ||
| join_rel::JoinType::LeftSemi => Ok(JoinType::LeftSemi), |
Contributor
Author
There was a problem hiding this comment.
needed due to bumping substrait - I think this is just a compile-time break tho, the actual protobuf values stay the same
Blizzara
force-pushed
the
avo/precision-timestamp
branch
from
a639feb to
5347f30
Compare
alamb
approved these changes
Aug 21, 2024
| substrait = { version = "0.36.0", features = ["serde"] } | ||
| pbjson-types = "0.7" | ||
| prost = "0.13" | ||
| substrait = { version = "0.41", features = ["serde"] } |
Contributor
| pub const DEFAULT_TYPE_VARIATION_REF: u32 = 0; | ||
| pub const UNSIGNED_INTEGER_TYPE_VARIATION_REF: u32 = 1; | ||
|
|
||
| #[deprecated(since = "41.0.0", note = "Use `PrecisionTimestamp(Tz)` type instead")] |
Contributor
There was a problem hiding this comment.
SInce we have already released 41 https://crates.io/crates/datafusion/41.0.0 I think we should probably label this with the next version. For example:
Suggested change
| #[deprecated(since = "41.0.0", note = "Use `PrecisionTimestamp(Tz)` type instead")] | |
| #[deprecated(since = "42.0.0", note = "Use `PrecisionTimestamp(Tz)` type instead")] |
Contributor
Author
There was a problem hiding this comment.
Hah yeah, this PR was a long time in the making 😅 fixed in a05637b
Contributor
|
Thanks again @Blizzara |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Which issue does this PR close?
N/A
closes #12074
Rationale for this change
DF was using the Substrait type_variations on a Substrait Timestamp to indicate whether a timestamp is seconds/millis/micros/nanos, while that works within DF that's not interoperable with other systems. Substrait has since introduced a PrecisionTimestamp type which includes the precision as a first class option, so we should use that instead.
What changes are included in this PR?
Are these changes tested?
Yes, with unit tests
Are there any user-facing changes?