Fix interval to duration casting (#4553) #4562
Conversation
| let iter = array.iter().map(|v| { | ||
| v.and_then(|v| { | ||
| let v = v / scale; | ||
| if v > i64::MAX as i128 { |
There was a problem hiding this comment.
This formulation failed to handle negatives correctly
arrow-cast/src/cast.rs
Outdated
| }); | ||
| let iter = array | ||
| .iter() | ||
| .map(|v| v.and_then(|v| (v >> 64 == 0).then(|| (v as i64) / scale))); |
There was a problem hiding this comment.
As an added bonus we are now doing 64-bit division which should be cheaper than 128-bit
arrow-cast/src/cast.rs
Outdated
| @@ -8658,10 +8621,6 @@ mod tests { | |||
| &CastOptions::default(), | |||
| ) | |||
| .unwrap(); | |||
| assert_eq!( | |||
There was a problem hiding this comment.
These tests are redundant as they're already encoded in the types returned
There was a problem hiding this comment.
I don't understand this rationale. While the types are encoded in the return type (e,.g IntervalMonthDayNanoArray) they aren't checked by the test. Thus, if they changed to IntervalDayTime or something I don't think the compiler would fail.
Maybe we could add explicit types, like
let casted_array: IntervalMonthDayNanoArray = cast_from_duration_to_interval::<DurationSecondType>(...)
arrow-cast/src/cast.rs
Outdated
| @@ -447,20 +447,11 @@ fn cast_interval_day_time_to_interval_month_day_nano( | |||
| } | |||
|
|
|||
| /// Cast the array from interval to duration | |||
| fn cast_interval_to_duration<D: ArrowTemporalType<Native = i64>>( | |||
| fn cast_month_day_none_to_duration<D: ArrowTemporalType<Native = i64>>( | |||
There was a problem hiding this comment.
What is the meaning of the _none_ in this name? I couldn't figure it out. Maybe it should be_nano_ 🤔
| } | ||
| }) | ||
| }); | ||
| let iter = array |
arrow-cast/src/cast.rs
Outdated
| @@ -8658,10 +8621,6 @@ mod tests { | |||
| &CastOptions::default(), | |||
| ) | |||
| .unwrap(); | |||
| assert_eq!( | |||
There was a problem hiding this comment.
I don't understand this rationale. While the types are encoded in the return type (e,.g IntervalMonthDayNanoArray) they aren't checked by the test. Thus, if they changed to IntervalDayTime or something I don't think the compiler would fail.
Maybe we could add explicit types, like
let casted_array: IntervalMonthDayNanoArray = cast_from_duration_to_interval::<DurationSecondType>(...)| let casted_array = | ||
| cast_from_interval_to_duration::<DurationNanosecondType>(&array, &nullable) | ||
| .unwrap(); | ||
| assert!(!casted_array.is_valid(0)); |
There was a problem hiding this comment.
I think this test should also explicitly verify the values for .values(4) (not just that they are valid or not)
Which issue does this PR close?
Closes #4553
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?