-
Notifications
You must be signed in to change notification settings - Fork 816
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
implement second/minute helpers for temporal #493
Conversation
Co-authored-by: Daniël Heres <danielheres@gmail.com>
Co-authored-by: Daniël Heres <danielheres@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #493 +/- ##
==========================================
- Coverage 82.65% 82.61% -0.04%
==========================================
Files 165 165
Lines 45524 45652 +128
==========================================
+ Hits 37628 37717 +89
- Misses 7896 7935 +39
Continue to review full report at Codecov.
|
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.
Thank you @ovr - this looks great.
BTW I plan to cherry pick this into the 4.4 release of arrow-rs, which I hope will be released this weekend or early next week (and then automatically be available to datafusion too) |
* implement second/minute helpers for temporal * Update arrow/src/compute/kernels/temporal.rs Co-authored-by: Daniël Heres <danielheres@gmail.com> * Update arrow/src/compute/kernels/temporal.rs Co-authored-by: Daniël Heres <danielheres@gmail.com> Co-authored-by: Daniël Heres <danielheres@gmail.com>
{ | ||
let mut b = Int32Builder::new(array.len()); | ||
match array.data_type() { | ||
&DataType::Date64 | &DataType::Timestamp(_, _) => { |
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.
I am not sure this is correct: when the timestamp is timezone-aware can we extract the second from its corresponding timezone-unaware equivalent?
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.
🤔 In general I think Rust/Arrow and DataFusion's support for timezone aware timestamps is pretty poor. I wonder if we could perhaps change this to DataType::Timestamp(_, None)
and error if DataType::Timestamp(_, Some(tz))
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.
Filed #500 to track
* implement second/minute helpers for temporal * Update arrow/src/compute/kernels/temporal.rs Co-authored-by: Daniël Heres <danielheres@gmail.com> * Update arrow/src/compute/kernels/temporal.rs Co-authored-by: Daniël Heres <danielheres@gmail.com> Co-authored-by: Daniël Heres <danielheres@gmail.com> Co-authored-by: Dmitry Patsura <zaets28rus@gmail.com> Co-authored-by: Daniël Heres <danielheres@gmail.com>
Hello!
Which issue does this PR close?
I am interested in the implementation of:
in the DF.
Thanks