feat: cast from/to interval and duration by Weijun-H · Pull Request #4020 · apache/arrow-rs

Weijun-H · 2023-04-05T09:56:09Z

Which issue does this PR close?

Closes #3998

Rationale for this change

Explain #3998

What changes are included in this PR?

Are there any user-facing changes?

viirya · 2023-04-05T18:49:12Z

arrow-cast/src/cast.rs

+    array: &dyn Array,
+    cast_options: &CastOptions,
+) -> Result<ArrayRef, ArrowError> {
+    let array = array.as_any().downcast_ref::<PrimitiveArray<D>>().unwrap();


Can we propagate the error?

viirya · 2023-04-05T18:50:55Z

arrow-cast/src/cast.rs

+    for i in 0..array.len() {
+        if array.is_null(i) {
+            builder.append_null();
+        } else {


For array with no nulls, I think we can have a faster path?

PrimitiveArray::from_trusted_len_iter may fit here.

viirya · 2023-04-05T18:51:27Z

arrow-cast/src/cast.rs

+            builder.append_null();
+        } else {
+            let v = array.value(i) as i128;
+            let v = v.mul_checked(scale);


For scale 1 case, maybe we can skip this call?

viirya · 2023-04-05T18:54:26Z

arrow-cast/src/cast.rs

+                        return Err(ArrowError::ComputeError(format!(
+                            "Cannot cast to {:?}. Overflowing on {:?}",
+                            D::DATA_TYPE,
+                            e


e is ArrowError, so you will get nested ArrowError.

metesynnada · 2023-04-11T15:27:56Z

LGTM, thanks for the effort.

alamb

Thanks as well for this PR @Weijun-H -- it is getting there. I am not sure the overflow handling is handled correctly. I left some comments and suggested tests that I think will help get this PR ready to go

alamb · 2023-04-14T20:27:32Z

arrow-cast/src/cast.rs

+    if cast_options.safe {
+        let iter = array
+            .iter()
+            .map(|v| v.and_then(|v| ((v as i128).checked_mul(scale))));


I am not sure this code correctly converts v into an IntervalMonthDayNanoType -- the IntervalMonthDayNanoType is represented as two fields but this is treating it as a single native type)

I think we should construct IntervalMonthDayNano with this method:

https://docs.rs/arrow/37.0.0/arrow/datatypes/struct.IntervalMonthDayNanoType.html#method.make_value

So something like

let months = 0; let days = 0; let nanos: i64 = v.checked_mul(scale)?; Ok(IntervalMonthDayNanoType::make_value(months, days, nanos))

alamb · 2023-04-14T20:28:22Z

arrow-cast/src/cast.rs

+    #[test]
+    fn test_cast_from_duration_to_interval() {
+        // from duration second to interval month day nano
+        let array = vec![1234567];


Can you also please test a value if i64::MAX here and ensure an error is returned?

alamb · 2023-04-14T20:30:51Z

arrow-cast/src/cast.rs

+        assert_eq!(casted_array.value(0), 0);
+
+        // from interval month day nano to duration millisecond
+        let array = vec![1234567];


You could probably refactor this repetition into a function and avoid so much boiler plate in the tests

alamb · 2023-04-15T12:10:40Z

arrow-cast/src/cast.rs

+            &CastOptions { safe: false },
+        )
+        .unwrap();
+        assert_eq!(casted_array.value(0), 9223372036854775807000000);


So if I convert this value back into fields:

let (months, days, nanos) = IntervalMonthDayNanoType::to_parts(9223372036854775807000000); println!("months: {months}, days: {days}, nanos: {nanos}");

it prints out:

months: 0, days: 499999, nanos: -1000000

Which doesn't seem right for 9223372036854775807 milliseconds

I think what is happening can be explained here (the nanoseconds fields have overflowed into the days)

https://docs.rs/arrow-array/37.0.0/src/arrow_array/types.rs.html#435-445

/* https://github.com/apache/arrow/blob/02c8598d264c839a5b5cf3109bfd406f3b8a6ba5/cpp/src/arrow/type.h#L1475 struct MonthDayNanos { int32_t months; int32_t days; int64_t nanoseconds; } 128 112 96 80 64 48 32 16 0 +-------+-------+-------+-------+-------+-------+-------+-------+ | months | days | nanos | +-------+-------+-------+-------+-------+-------+-------+-------+ */

I think what should happen is

(preferred): the cast should return an error if the nanosecond field would have overflowed

(secondarily): if the nanosecond field overflows the overflow should be converted into days

I think erroring (at least at first) is preferred because it avoids questions like "how many seconds are there in a day" which is not actually a fixed quantity given leap seconds, etc.

If someone has need of casting such durations to intervals, we can then sort out the mechanics

alamb · 2023-04-15T12:12:31Z

Thanks @Weijun-H -- I think this is getting close. From my perspecitive all that is left is handling nanosecond overflow correctly and this will be good to go

alamb

Thank you for bearing with my @Weijun-H -- sorry for the back and forth

alamb · 2023-04-15T16:35:19Z

arrow-cast/src/cast.rs

+    if cast_options.safe {
+        let iter = array
+            .iter()
+            .map(|v| v.and_then(|v| v.checked_mul(scale).map(|v| v as i128)));


I think we need to check the overflow in the safe case as well -- and and set the result to Null / NONE when overflow happened

alamb · 2023-04-15T16:37:01Z

arrow-cast/src/cast.rs

+        );
+        assert_eq!(casted_array.value(0), 1234567000);
+
+        let array = vec![i64::MAX];


Can you also please add a test here (and the cases below) showing what happens when an overflow happens with DEFAULT_CAST_OPTIONS? I would expect the result to be Null

alamb · 2023-04-15T17:04:02Z

arrow-cast/src/cast.rs

+            &DEFAULT_CAST_OPTIONS,
+        )
+        .unwrap();
+        assert_eq!(casted_array.value(0), 0);


I think this check should be that the casted_array.valid(0) is false, right?

alamb

I think this PR is ready to go. I don't totally understand why https://github.com/apache/arrow-rs/pull/4020/files#r1167579187 is not needed, but the test coverage

        let array = vec![i64::MAX];
        let casted_array = cast_from_duration_to_interval::<DurationMillisecondType>(
            array.clone(),
            &DEFAULT_CAST_OPTIONS,
        )
        .unwrap();
        assert!(!casted_array.is_valid(0));

covers the case I was thinking of so 👍

Thank you @Weijun-H

Weijun-H · 2023-04-16T11:08:01Z

I think this PR is ready to go. I don't totally understand why #4020 (files) is not needed, but the test coverage
        let array = vec![i64::MAX];
        let casted_array = cast_from_duration_to_interval::<DurationMillisecondType>(
            array.clone(),
            &DEFAULT_CAST_OPTIONS,
        )
        .unwrap();
        assert!(!casted_array.is_valid(0));
covers the case I was thinking of so 👍

Thank you @Weijun-H

Because v is i64 in #4020 (files), checked_mul will return None when overflowing.

github-actions bot added the arrow Changes to the arrow crate label Apr 5, 2023

feat: cast from/to interval and duration

6d58d1c

Weijun-H force-pushed the cast-between-duration-interval branch from 9082dff to 6d58d1c Compare April 5, 2023 10:01

update can_cast_type

e039ce3

viirya reviewed Apr 5, 2023

View reviewed changes

Weijun-H added 3 commits April 6, 2023 11:22

refactor the function

16d7353

skip the computation when scale is 1

47010b0

speed up process

c4a3e24

alamb reviewed Apr 14, 2023

View reviewed changes

update tests

93ff370

alamb reviewed Apr 15, 2023

View reviewed changes

handle nanosecond overflow

b0a7e29

alamb reviewed Apr 15, 2023

View reviewed changes

update tests for safe cast_option when overflowing

c3bd35a

alamb reviewed Apr 15, 2023

View reviewed changes

update tests to check null when overflowing

f2f6b95

alamb approved these changes Apr 16, 2023

View reviewed changes

alamb merged commit 682231c into apache:master Apr 16, 2023

This was referenced Apr 21, 2023

Support casting to/from Interval and Duration #3998

Closed

Update to arrow 38 apache/datafusion#6115

Merged

tustvold mentioned this pull request Jul 19, 2023

Update Arrow 45.0.0 And Datum Arithmetic, change Decimal Division semantics apache/datafusion#6832

Merged

Conversation

Weijun-H commented Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metesynnada commented Apr 11, 2023

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb Apr 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Apr 15, 2023

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Weijun-H commented Apr 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Weijun-H commented Apr 5, 2023 •

edited

Loading

alamb Apr 15, 2023 •

edited

Loading