-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[Variant] Add variant to arrow for Date64/Timestamp(Second/Millisecond)/Time32/Time64 #8950
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Variant] Add variant to arrow for Date64/Timestamp(Second/Millisecond)/Time32/Time64 #8950
Conversation
69bb43b to
1af7a22
Compare
klion26
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| }); | ||
| impl_primitive_from_variant!(datatypes::Time64MicrosecondType, as_time_utc, |v| { | ||
| (v.num_seconds_from_midnight() * 1_000_000 + v.nanosecond() / 1_000) as i64 | ||
| Some((v.num_seconds_from_midnight() * 1_000_000 + v.nanosecond() / 1_000) as i64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to handle the case v.nanosecond() % 1000 != 0 here. Thanks for variant_array::canonicalize_and_verify_data_type, we can assume that the input here is always Time64(TimeUnit::Microsecond)(No Time32 or Time64(TimeUnitNanosecond))
alamb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @klion26 -- I went though this PR and it looks good to me
| Some(datatypes::Date64Type::from_naive_date(v)) | ||
| }); | ||
| impl_primitive_from_variant!(datatypes::Time32SecondType, as_time_utc, |v| { | ||
| // Return None if there are leftover nanoseconds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this makes sense to me -- we return None because we can't cast without losing precision
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| perfectly_shredded_variant_array_fn!(perfectly_shredded_time_variant_array_for_time32, || { | ||
| Time64MicrosecondArray::from(vec![ | ||
| Some(1234), // This can't be cast to Time32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it can cast, but the cast is not lossless
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, updated the comment to make it more clearly.
|
Thanks @klion26 |
|
@alamb Thanks for the review and merging! |
Which issue does this PR close?
What changes are included in this PR?
Add support for variant to arrow primitive types(for the remaining arrow primitive types), and some tests to cover them.
For the behavior that can't be cast safely, I'll continue to track them in #8086 and #8873
- else None
- else None
- else None
- else Nnoe
Are these changes tested?
Added some new tests
Are there any user-facing changes?
No