Support for casting `Utf8` and `LargeUtf8` --> `Interval` by doki23 · Pull Request #3762 · apache/arrow-rs

doki23 · 2023-02-25T08:34:42Z

Which issue does this PR close?

Closes #3643.

doki23 · 2023-02-27T15:51:30Z

It seems that the parse function is too complex to get auto-vectorized.

alamb

Thank you @doki23 -- this is looking very close.

I think this PR needs a few more tests, and I suggest removing the assert_contains but otherwise I think it looks 👌

Thank you very much

arrow-cast/src/cast.rs

arrow-cast/src/parse.rs

arrow-cast/src/test_util.rs

arrow-cast/src/parse.rs

doki23 · 2023-03-04T12:53:01Z

arrow-cast/src/parse.rs

+        * SECONDS_PER_HOUR
+        * NANOS_PER_SECOND;
+
+    // Convert to higher units as much as possible


@alamb Thank you for your review and I made some changes here. For instance, it converts 31 days to 31 days before, but now, it will return 1 month 1 day. I think it's more ergonomic.

I don't think this is correct, the number of days in a month is not fixed. The postgres docs make an exception for the case of fractional dates, but I don't see any indication it does this in the general case

alamb

Thank you @doki23 -- I think this PR looks great and is ready to go. ❤️

arrow-cast/src/parse.rs

tustvold · 2023-03-06T10:05:33Z

arrow-cast/src/parse.rs

+    mut day_part: f64,
+    mut nanos_part: f64,
+) -> (i64, i64, f64) {
+    // Convert fractional month to days, It's not supported by Arrow types, but anyway


Is this correct? It seems to assume a month has 30 days, which isn't true?

According to this doc it's correct. But you remind me that maybe it's incorrect to spill smaller units to larger units.

In particular

Field values can have fractional parts: for example, '1.5 weeks' or '01:02:03.45'. However, because interval internally stores only three integer units (months, days, microseconds), fractional units must be spilled to smaller units. Fractional parts of units greater than months are rounded to be an integer number of months, e.g. '1.5 years' becomes '1 year 6 mons'. Fractional parts of weeks and days are computed to be an integer number of days and microseconds, assuming 30 days per month and 24 hours per day, e.g., '1.75 months' becomes 1 mon 22 days 12:00:00. Only seconds will ever be shown as fractional on output.

Perhaps we could add a note, with a link?

tustvold · 2023-03-06T10:06:52Z

arrow-cast/src/parse.rs

+    day_part += (month_part - (month_part as i64) as f64) * 30_f64;
+
+    // Convert fractional days to hours
+    nanos_part += (day_part - ((day_part as i64) as f64))


I'm not sure this truncation logic is correct for negative quantities

It's ok, I've added an unit test for it.

I can't see a test that has a negative fractional number of days or months? Am I being blind?

tustvold · 2023-03-06T10:07:44Z

arrow-cast/src/parse.rs

    }

+    #[test]
+    fn test_parse_interval() {


I think it would be good to have a test of the negative fractional quantities e.g. -1.1 month

arrow-cast/src/parse.rs

Co-authored-by: Raphael Taylor-Davies <[email protected]>

…val_parts

tustvold · 2023-03-06T17:08:47Z

arrow-cast/src/parse.rs

+        // @todo It's better to use Decimal in order to protect rounding errors
+        // Wait https://github.com/apache/arrow/pull/9232


Suggested change

// @todo It's better to use Decimal in order to protect rounding errors

// Wait https://github.com/apache/arrow/pull/9232

// TODO: Use fixed-point arithmetic to avoid truncation and rounding errors (#3809)

tustvold

I think other than a test of negative fractional months and days, this looks good to me. I've filed #3809 to track altering this to use fixed-point arithmetic to avoid the current issues around truncation and overflow

Edit: I think we also need to remove the logic that "promotes" from days to months

tustvold · 2023-03-06T17:37:21Z

arrow-cast/src/parse.rs

+    day_part += ((nanos_part as i64) / (NANOS_PER_DAY as i64)) as f64;
+    month_part += ((day_part as i64) / 30_i64) as f64;
+    nanos_part %= NANOS_PER_DAY;
+    day_part %= 30_f64;


Suggested change

day_part += ((nanos_part as i64) / (NANOS_PER_DAY as i64)) as f64;

month_part += ((day_part as i64) / 30_i64) as f64;

nanos_part %= NANOS_PER_DAY;

day_part %= 30_f64;

I would suggest removing this, not only is it potentially incorrect as months don't have a fixed number of days, but also integer division is very slow (although LLVM may be smart enough to convert this to fixed point multiplication)

I like the idea of removing the "convert to higher units" logic and and file a ticket to (potentially) support it

I am happy to file such a ticket

I agree the idea of 40 days --> 1 month and 10 days that this PR will do, doesn't seem correct

alamb · 2023-03-06T19:43:31Z

BTW thank you for sticking with this @doki23 -- I didn't realize how much more work would be needed after a more thorough review in arrow-rs 😓

alamb · 2023-03-06T19:43:46Z

Also, thank you @tustvold for the thorough review!

doki23 · 2023-03-07T00:16:03Z

arrow-cast/src/parse.rs

+
+        assert_eq!(
+            (-1i32, -18i32, (-0.2 * NANOS_PER_DAY) as i64),
+            parse_interval("months", "-1.5 months -3.2 days").unwrap(),


@tustvold Here it is.

doki23 · 2023-03-07T00:24:36Z

@alamb I'm glad to stick with this because your(you and @tustvold) reviews bring me some programing inspirations，it is fun to me.

…_string_interval

tustvold · 2023-03-07T10:37:34Z

Thank you for this 👍

alamb · 2023-03-07T11:17:02Z

Thanks again @doki23 and @tustvold

* cast string to interval * cast string to interval * unit tests * fix * update * code clean * update unit tests and align_interval_parts * fix ut * make clippy happy * Update arrow-cast/src/parse.rs Co-authored-by: Raphael Taylor-Davies <[email protected]> * change return types of calculate_from_part and fix bug of align_interval_parts * make clippy happy * remote useless overflow check * remove the "convert to higher units" logic --------- Co-authored-by: Raphael Taylor-Davies <[email protected]>

* cast string to interval * cast string to interval * unit tests * fix * update * code clean * update unit tests and align_interval_parts * fix ut * make clippy happy * Update arrow-cast/src/parse.rs Co-authored-by: Raphael Taylor-Davies <[email protected]> * change return types of calculate_from_part and fix bug of align_interval_parts * make clippy happy * remote useless overflow check * remove the "convert to higher units" logic --------- Co-authored-by: Raphael Taylor-Davies <[email protected]> Can drop this after rebase on commit 14544fb "Support for casting Utf8 and LargeUtf8 --> Interval (apache#3762)", first released in 35.0.0

doki23 added 3 commits February 25, 2023 16:13

cast string to interval

258b4ff

cast string to interval

33ed551

unit tests

45e4c30

doki23 changed the title ~~Cast string interval~~ Cast string to interval Feb 25, 2023

fix

f84ce78

github-actions bot added the arrow Changes to the arrow crate label Feb 25, 2023

update

8f33ab1

doki23 marked this pull request as ready for review February 27, 2023 15:34

code clean

7cae129

alamb reviewed Mar 2, 2023

View reviewed changes

update unit tests and align_interval_parts

7a15624

doki23 commented Mar 4, 2023

View reviewed changes

doki23 added 2 commits March 4, 2023 20:58

fix ut

33ca780

make clippy happy

f0d6953

alamb changed the title ~~Cast string to interval~~ Add support for casting Utf8 and LargeUtf8 --> Interval Mar 4, 2023

alamb approved these changes Mar 4, 2023

View reviewed changes

alamb changed the title ~~Add support for casting Utf8 and LargeUtf8 --> Interval~~ Support for casting Utf8 and LargeUtf8 --> Interval Mar 4, 2023

tustvold reviewed Mar 6, 2023

View reviewed changes

arrow-cast/src/parse.rs Outdated Show resolved Hide resolved

tustvold reviewed Mar 6, 2023

View reviewed changes

arrow-cast/src/parse.rs Outdated Show resolved Hide resolved

tustvold reviewed Mar 6, 2023

View reviewed changes

arrow-cast/src/parse.rs Outdated Show resolved Hide resolved

tustvold reviewed Mar 6, 2023

View reviewed changes

arrow-cast/src/parse.rs Outdated Show resolved Hide resolved

tustvold reviewed Mar 6, 2023

View reviewed changes

arrow-cast/src/parse.rs Outdated Show resolved Hide resolved

doki23 and others added 4 commits March 6, 2023 21:23

Update arrow-cast/src/parse.rs

f0da097

Co-authored-by: Raphael Taylor-Davies <[email protected]>

change return types of calculate_from_part and fix bug of align_inter…

6c085a1

…val_parts

make clippy happy

e376e5f

remote useless overflow check

cc10172

tustvold mentioned this pull request Mar 6, 2023

Use Fixed Point Arithmetic in Interval Parsing #3809

Closed

tustvold reviewed Mar 6, 2023

View reviewed changes

tustvold approved these changes Mar 6, 2023

View reviewed changes

tustvold reviewed Mar 6, 2023

View reviewed changes

doki23 commented Mar 7, 2023

View reviewed changes

doki23 added 2 commits March 7, 2023 13:02

remove the "convert to higher units" logic

2446be9

Merge branch 'master' of https://github.com/apache/arrow-rs into cast…

53fd81d

…_string_interval

tustvold merged commit 14544fb into apache:master Mar 7, 2023

This was referenced Mar 20, 2023

Support ::interval casting / INTERVAL SQL datatype apache/datafusion#5651

Closed

Support microsecond and nanosecond in interval parsing #3916

Merged

This was referenced Mar 27, 2023

[Epic] A collection of issues related to Intervals #3958

Closed

Fix interval to use consistent units and arrow parser apache/datafusion#5806

Merged

		// @todo It's better to use Decimal in order to protect rounding errors
		// Wait https://github.com/apache/arrow/pull/9232

	// @todo It's better to use Decimal in order to protect rounding errors
	// Wait https://github.com/apache/arrow/pull/9232
	// TODO: Use fixed-point arithmetic to avoid truncation and rounding errors (#3809)

Comments

Conversation

doki23 commented Feb 25, 2023

Which issue does this PR close?

Uh oh!

doki23 commented Feb 27, 2023

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

doki23 Mar 4, 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!

tustvold Mar 6, 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 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tustvold left a comment • 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!

tustvold Mar 6, 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!

Choose a reason for hiding this comment

Uh oh!

doki23 Mar 7, 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 Mar 6, 2023

Uh oh!

alamb commented Mar 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

doki23 commented Mar 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tustvold commented Mar 7, 2023

Uh oh!

alamb commented Mar 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

doki23 Mar 4, 2023 •

edited

Loading

tustvold Mar 6, 2023 •

edited

Loading

tustvold left a comment •

edited

Loading

tustvold Mar 6, 2023 •

edited

Loading

doki23 Mar 7, 2023 •

edited

Loading

doki23 commented Mar 7, 2023 •

edited

Loading