feat: cast between `Intervals` #4182

izveigor · 2023-05-08T19:44:18Z

Which issue does this PR close?

Closes #4181

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Yes

tustvold

I think this needs some review of the overflow behaviour

tustvold · 2023-05-09T16:03:05Z

arrow-cast/src/cast.rs

+    let array = array
+        .as_any()
+        .downcast_ref::<PrimitiveArray<IntervalYearMonthType>>()
+        .ok_or_else(|| {
+            ArrowError::ComputeError(
+                "Internal Error: Cannot cast interval to IntervalArray of expected type"
+                    .to_string(),
+            )
+        })?;


Suggested change

let array = array

.as_any()

.downcast_ref::<PrimitiveArray<IntervalYearMonthType>>()

.ok_or_else(|| {

ArrowError::ComputeError(

"Internal Error: Cannot cast interval to IntervalArray of expected type"

.to_string(),

)

})?;

let array = array.as_primitive::<IntervalYearMonthType>();

tustvold · 2023-05-09T16:03:35Z

arrow-cast/src/cast.rs

+    let array = array
+        .as_any()
+        .downcast_ref::<PrimitiveArray<IntervalDayTimeType>>()
+        .ok_or_else(|| {
+            ArrowError::ComputeError(
+                "Internal Error: Cannot cast interval to IntervalArray of expected type"
+                    .to_string(),
+            )
+        })?;


Suggested change

let array = array

.as_any()

.downcast_ref::<PrimitiveArray<IntervalDayTimeType>>()

.ok_or_else(|| {

ArrowError::ComputeError(

"Internal Error: Cannot cast interval to IntervalArray of expected type"

.to_string(),

)

})?;

let array = array.as_primitive::<IntervalDayTimeType>();

tustvold · 2023-05-09T16:04:14Z

arrow-cast/src/cast.rs

+            )
+        })?;
+
+    let iter = array.iter().map(|v| {


I think this needs to perform checked conversion, and can make use of unary_opt and try_unary to achieve this based on the cast options

tustvold · 2023-05-09T16:04:39Z

arrow-cast/src/cast.rs

+        casted_array
+            .as_any()
+            .downcast_ref::<IntervalMonthDayNanoArray>()
+            .ok_or_else(|| {
+                ArrowError::ComputeError(
+                    "Failed to downcast to IntervalMonthDayNanoArray".to_string(),
+                )
+            })


Suggested change

casted_array

.as_any()

.downcast_ref::<IntervalMonthDayNanoArray>()

.ok_or_else(|| {

ArrowError::ComputeError(

"Failed to downcast to IntervalMonthDayNanoArray".to_string(),

)

})

Ok(casted_array.as_primitive::<IntervalMonthDataNanoType>().clone())

arrow-cast/src/cast.rs

alamb · 2023-05-09T16:42:30Z

Thank you for taking this on @izveigor

… IntervalUnit::DayTime -> IntervalUnit::MonthDayNano

izveigor · 2023-05-30T20:24:15Z

arrow-cast/src/cast.rs

+        )))
+    } else {
+        Ok(Arc::new(
+            array.try_unary::<_, IntervalMonthDayNanoType, ArrowError>(|v| {


Is an unsafe way needed in this case, if the overflow never happens?

If the method is infallible you can just use unary without needing to use either unary_opt or try_unary

izveigor · 2023-05-30T20:25:28Z

arrow-cast/src/cast.rs

+            |v| {
+                let (days, ms) = IntervalDayTimeType::to_parts(v);
+                Some(IntervalMonthDayNanoType::make_value(
+                    0,


I think the method: months = days * 30 is not entirely accurate?

izveigor · 2023-05-30T20:27:14Z

@tustvold Sorry for the long delay. I left a few questions on this PR. Can you pay attention to them if you have time.

tustvold

I left some comments, thinking about this further I think the only well-formed interval casts are from a smaller representation to a larger representation.

I'm not sure there is a "correct" way to go from IntervalUnit::MonthDayNano to IntervalUnit::DayTime.

However, I think just providing the infallible "widening" casts, is good enough, and will be sufficient for most use-cases. At the very least they seem like a good place to start

tustvold · 2023-06-01T11:32:28Z

arrow-cast/src/cast.rs

+                Some(IntervalMonthDayNanoType::make_value(
+                    0,
+                    days,
+                    (ms * mul).into(),


Suggested change

(ms * mul).into(),

ms as i64 * mil,

tustvold · 2023-06-01T11:32:45Z

arrow-cast/src/cast.rs

+    cast_options: &CastOptions,
+) -> Result<ArrayRef, ArrowError> {
+    let array = array.as_primitive::<IntervalDayTimeType>();
+    let mul = 10_i32.pow(6);


Suggested change

let mul = 10_i32.pow(6);

let mul = 1_000_000;

tustvold · 2023-06-01T11:33:39Z

arrow-cast/src/cast.rs

+    let array = array.as_primitive::<IntervalDayTimeType>();
+    let mul = 10_i32.pow(6);
+
+    if cast_options.safe {


It occurs to me that this actually cannot overflow, and so can just use unary.

It is performing a widening multiplication of i32 milliseconds to i64 nanoseconds

tustvold · 2023-06-01T11:36:14Z

arrow-cast/src/cast.rs

+            let (_, days, nanos) = IntervalMonthDayNanoType::to_parts(v);
+            Some(IntervalDayTimeType::make_value(
+                days,
+                (nanos / mul).try_into().unwrap(),


Suggested change

(nanos / mul).try_into().unwrap(),

(nanos / mul).try_into().ok()?,

tustvold · 2023-06-01T11:37:22Z

arrow-cast/src/cast.rs

+
+    if cast_options.safe {
+        Ok(Arc::new(array.unary_opt::<_, IntervalDayTimeType>(|v| {
+            let (_, days, nanos) = IntervalMonthDayNanoType::to_parts(v);


This appears to discard the months component?

tustvold · 2023-06-01T11:39:29Z

arrow-cast/src/cast.rs

+    array: &dyn Array,
+    cast_options: &CastOptions,
+) -> Result<ArrayRef, ArrowError> {
+    let array = array.as_primitive::<IntervalMonthDayNanoType>();


I think people would find it surprising that this discards any days or nanoseconds. I suspect we either need to error if they are anything other than 0, or make an approximation about the number of hours in a day and the number of days in a month...

alamb · 2023-06-01T19:54:15Z

I wonder if this PR will make the next release (so we can use it in datafusion) 🤔

tustvold · 2023-06-02T13:32:15Z

Thank you

See apache@863d599 See apache#4182

See apache@863d599 See apache#4182 Can drop this after rebase on commit 863d599 "feat: cast between Intervals (apache#4182)", first released in 41.0.0

feat: cast between Intervals

3ce4b12

github-actions bot added the arrow Changes to the arrow crate label May 8, 2023

tustvold mentioned this pull request May 8, 2023

feat: support Interval +/- Interval #4179

Closed

tustvold reviewed May 9, 2023

View reviewed changes

izveigor marked this pull request as draft May 10, 2023 18:05

izveigor added 2 commits May 30, 2023 13:51

Merge branch 'master' into cast_intervals

43a4af2

feat: cast: IntervalUnit::YearMonth -> IntervalUnit::MonthDayNano and…

d2fa575

… IntervalUnit::DayTime -> IntervalUnit::MonthDayNano

izveigor commented May 30, 2023

View reviewed changes

izveigor marked this pull request as ready for review May 30, 2023 20:27

tustvold mentioned this pull request Jun 1, 2023

fix: allow cast between different Interval type #4329

Closed

tustvold reviewed Jun 1, 2023

View reviewed changes

izveigor added 2 commits June 2, 2023 14:31

Merge branch 'master' into cast_intervals

d951154

refactoring

aa6a9c1

tustvold approved these changes Jun 2, 2023

View reviewed changes

tustvold merged commit 863d599 into apache:master Jun 2, 2023

mr-brobot mentioned this pull request Jun 4, 2023

Support casting to/from different interval units (eg YearDay --> MonthDayNano, etc) #3959

Closed

mcheshkov added a commit to cube-js/arrow-rs that referenced this pull request Jul 18, 2024

Backport "feat: cast between Intervals"

9a538de

See apache@863d599 See apache#4182

mcheshkov mentioned this pull request Jul 18, 2024

Backport "feat: cast between Intervals" cube-js/arrow-rs#38

Merged

mcheshkov added a commit to cube-js/arrow-rs that referenced this pull request Jul 22, 2024

Backport "feat: cast between Intervals"

8fd2aa8

See apache@863d599 See apache#4182

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: cast between `Intervals` #4182

feat: cast between `Intervals` #4182

izveigor commented May 8, 2023

tustvold left a comment

tustvold May 9, 2023

tustvold May 9, 2023

tustvold May 9, 2023

tustvold May 9, 2023

alamb commented May 9, 2023

izveigor May 30, 2023

tustvold Jun 1, 2023 •

edited

Loading

izveigor May 30, 2023

izveigor commented May 30, 2023

tustvold left a comment •

edited

Loading

tustvold Jun 1, 2023

tustvold Jun 1, 2023

tustvold Jun 1, 2023

tustvold Jun 1, 2023

tustvold Jun 1, 2023

tustvold Jun 1, 2023

alamb commented Jun 1, 2023

tustvold commented Jun 2, 2023

	(nanos / mul).try_into().unwrap(),
	(nanos / mul).try_into().ok()?,

feat: cast between Intervals #4182

feat: cast between Intervals #4182

Conversation

izveigor commented May 8, 2023

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

tustvold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented May 9, 2023

Choose a reason for hiding this comment

tustvold Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

izveigor commented May 30, 2023

tustvold left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Jun 1, 2023

tustvold commented Jun 2, 2023

feat: cast between `Intervals` #4182

feat: cast between `Intervals` #4182

tustvold Jun 1, 2023 •

edited

Loading

tustvold left a comment •

edited

Loading