-
Notifications
You must be signed in to change notification settings - Fork 786
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
feat: add support for casting Duration/Interval to Int64Array #1196
Conversation
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 @e-dard -- I want to sort out the CI failures before merging any more PRs, and I will do so tomorrow morning
match from_type{ | ||
IntervalUnit::YearMonth => true, | ||
IntervalUnit::DayTime => true, | ||
IntervalUnit::MonthDayNano => false, // Native type is i128 |
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.
One possible way to support this would be to cast to Decimal
which can represent the full type
Though that is not very ideal for various reasons either.
So for now, this looks good 👍
IntervalUnit::YearMonth => { | ||
cast_numeric_arrays::<IntervalYearMonthType, Int64Type>(array) | ||
} | ||
IntervalUnit::DayTime => cast_array_data::<Int64Type>(array, to_type.clone()), |
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 guess this makes sense (to pass back the underlying data). The DayTime value is actually made up of two fields (days / milliseconds): https://github.com/apache/arrow/blob/master/format/Schema.fbs#L361-L365
I wonder if using one of the temporal kernels might make more sense for your usecase: https://docs.rs/arrow/7.0.0/arrow/compute/kernels/temporal/index.html#
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 terms of the DayTime
value I was thinking along the lines of this comment #685 (comment) (emit it as it is and then the user can optionally do another operation on it if they need to).
44600b8
to
535b684
Compare
Codecov Report
@@ Coverage Diff @@
## master #1196 +/- ##
=======================================
Coverage 82.67% 82.68%
=======================================
Files 175 175
Lines 51561 51595 +34
=======================================
+ Hits 42630 42661 +31
- Misses 8931 8934 +3
Continue to review full report at Codecov.
|
Which issue does this PR close?
Closes #685.
What changes are included in this PR?
This PR adds support for casting from all the
Duration
types toInt64Array
. It also adds support for casting from the followingInterval
types:Interval(IntervalUnit::YearMonth)
(Native=i64
)Interval(IntervalUnit::DayTime)
(Native=i32
)However, I punted on
Interval(IntervalUnit::MonthDayNano)
, which has a native typei128
. Attempting to cast between that and anInt64Array
will result in a cast error.I'm happy to change that behaviour if there is a preferable alternative.
Are there any user-facing changes?
No.