feat: Implement string cast operations for Time32 and Time64 #2251

stuartcarnie · 2022-08-01T03:23:36Z

Which issue does this PR close?

Closes #2053 and helps apache/datafusion#2883.

Rationale for this change

N/A

What changes are included in this PR?

Implements cast operations following precedence of existing implementations.

Are there any user-facing changes?

The cast API now supports string -> Time32 and Time64 transformations.

alamb

Thank you @stuartcarnie -- this is looking great. I left some feedback but I ran out of time today -- I will complete my review first thing tomorrow.

cc @avantgardnerio

alamb · 2022-08-01T20:36:02Z

arrow/src/compute/kernels/cast.rs

+        (Utf8,
+            Date32
+            | Date64
+            | Time32(TimeUnit::Second)


alamb · 2022-08-01T20:40:53Z

arrow/src/compute/kernels/cast.rs

+fn seconds_since_midnight(time: &chrono::NaiveTime) -> i32 {
+    let sec = time.num_seconds_from_midnight();
+    let frac = time.nanosecond();
+    let adjust = if frac < 1_000_000_000 { 0 } else { 1 };


It took me a while to grok this -- I think it is leap second handling
https://docs.rs/chrono/0.4.19/chrono/trait.Timelike.html#tymethod.nanosecond

Maybe we could add a comment explaining what was going on

Suggested change

let adjust = if frac < 1_000_000_000 { 0 } else { 1 };

// handle leap second

// see https://docs.rs/chrono/0.4.19/chrono/trait.Timelike.html#tymethod.nanosecond

let adjust = if frac < 1_000_000_000 { 0 } else { 1 };

I'm a bit confused about this myself. It was my understanding that these were added manually by timekeepers? Does chrono keep a list of historical leap seconds? https://en.wikipedia.org/wiki/Leap_second#:~:text=Between%201972%20and%202020%2C%20a,every%2021%20months%2C%20on%20average.

Chrono docs https://docs.rs/chrono/0.3.1/chrono/naive/time/index.html

Looks like they can model them, but don't keep a database of them? https://github.com/chronotope/chrono/blob/ab688c384f7c797466fca69d69bfd5ee3c2cba96/src/naive/time/mod.rs#L480

Ah, it can read unix TZif files to get the historical data https://github.com/chronotope/chrono/blob/ab688c384f7c797466fca69d69bfd5ee3c2cba96/src/offset/local/tz_info/timezone.rs#L662

https://manpages.ubuntu.com/manpages/xenial/man5/tzfile.5.html

It is possible to parse a time with a leap second, using the 60th second:

https://docs.rs/chrono/0.3.1/chrono/naive/time/index.html#reading-and-writing-leap-seconds

The leap second is stored as a fractional second of 1_000_000_000 nanoseconds, but I realise now that I don't need any logic to handle it. The code is much simpler 😂

alamb · 2022-08-01T20:43:48Z

arrow/src/compute/kernels/cast.rs

+    let (frac, adjust) = if frac < 1_000_000_000 {
+        (frac, 0)
+    } else {
+        (frac - 1_000_000_000, MILLIS_PER_SEC)
+    };
+    (sec + adjust + frac / NANOS_PER_MILLI) as i32


There is probably a good reason, but my feeble mind can't figure it out. Why is it important to break up frac and adjust?

For example, isn't this equivalent?

Suggested change

let (frac, adjust) = if frac < 1_000_000_000 {

(frac, 0)

} else {

(frac - 1_000_000_000, MILLIS_PER_SEC)

};

(sec + adjust + frac / NANOS_PER_MILLI) as i32

(sec + frac / NANOS_PER_MILLI) as i32

Yes indeed, I concluded the same thing and pushed up the changes, thanks! I will move the functions back inline as I originally had them, before I overcomplicated it 😂

alamb · 2022-08-01T20:44:39Z

arrow/src/compute/kernels/cast.rs

@@ -1584,6 +1625,303 @@ fn cast_string_to_date64<Offset: OffsetSizeTrait>(
    Ok(Arc::new(array) as ArrayRef)
 }

+fn seconds_since_midnight(time: &chrono::NaiveTime) -> i32 {


https://github.com/apache/arrow-rs/blob/master/arrow/src/temporal_conversions.rs may be a good place to put these functions too (so they have a chance of being found / reused)

alamb · 2022-08-01T20:50:15Z

arrow/src/compute/kernels/cast.rs

+        let iter = (0..string_array.len()).map(|i| {
+            if string_array.is_null(i) {
+                None
+            } else {
+                string_array
+                    .value(i)
+                    .parse::<chrono::NaiveTime>()
+                    .map(|time| seconds_since_midnight(&time))
+                    .ok()
+            }
+        });


I think this will be faster as it will not require checking bounds for calls to is_null or value:

Suggested change

let iter = (0..string_array.len()).map(|i| {

if string_array.is_null(i) {

None

} else {

string_array

.value(i)

.parse::<chrono::NaiveTime>()

.map(|time| seconds_since_midnight(&time))

.ok()

}

});

let iter = string_array

.iter()

.flat_map(|v| {

v.map(|v| {

v.parse::<chrono::NaiveTime>()

.map(|time| seconds_since_midnight(&time))

.ok()

})

});

I think the same type of transformation can be applied to the iterator below this as well.

Sounds good – it looks like the string to date transformation functions would benefit from the same treatment, but I'll leave that to another PR so as to not cloud this one.

I tried, but unfortunately it fails at runtime with a panic:

thread 'compute::kernels::cast::tests::test_cast_string_to_time32second' panicked at 'trusted_len_unzip requires an upper limit', arrow/src/array/array_primitive.rs:470:25

at

arrow-rs/arrow/src/array/array_primitive.rs

Lines 469 to 470 in 91b2d7e

let (_, upper) = iterator.size_hint();

let len = upper.expect("trusted_len_unzip requires an upper limit");

The previous iter was ultimately of type Range<usize>, which returns Some(_) for the size_hint:

https://github.com/rust-lang/rust/blob/fe3342816a282949f014caa05ea2e669ff9d3d3c/library/core/src/iter/range.rs#L714-L722

whereas our new iter does not, as it does not know the size.

There are probably good reasons, but it is unfortunate that size_hint was not a separate trait, so this could be caught at compile time.

Thanks for checking @stuartcarnie -- I wonder if flat_map is the problem -- let me take another crack at this

I found a way to do this cleanly -- in #2284

alamb · 2022-08-01T20:51:24Z

arrow/src/compute/kernels/cast.rs

+                } else {
+                    let string = string_array
+                        .value(i);
+                    chrono::Duration::days(3);


The

chrono::Duration::days(3);

Seems like a leftover?

Indeed – it is gone now, thanks for the spot

Remove the unnecessary conditionals to extract the leap second, as it is already handled when converting to a time unit relative to midnight 🤦🏻‍♂️

avantgardnerio

Thanks for the clarifications! Hopefully having them on this PR will help someone in the future - they definitely helped me.

LGTM

codecov-commenter · 2022-08-01T22:33:51Z

Codecov Report

Attention: Patch coverage is 73.44633% with 47 lines in your changes missing coverage. Please review.

Project coverage is 82.26%. Comparing base (3032a52) to head (7340b19).
Report is 2123 commits behind head on master.

Files	Patch %	Lines
arrow/src/compute/kernels/cast.rs	73.44%	47 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2251      +/-   ##
==========================================
- Coverage   82.29%   82.26%   -0.04%     
==========================================
  Files         243      245       +2     
  Lines       62443    62863     +420     
==========================================
+ Hits        51387    51713     +326     
- Misses      11056    11150      +94

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

alamb

I think this code's logic and tests looks good to me. I do think it would be nice to clean up the iterator logic a bit, though I think we can do that as a follow on PR.

alamb · 2022-08-02T11:54:19Z

arrow/src/compute/kernels/cast.rs

+        ])) as ArrayRef;
+        let a2 = Arc::new(LargeStringArray::from(vec![
+            Some("08:08:35.091323414"),
+            Some("08:08:60.091323414"), // leap second


💯 for the leap second

alamb · 2022-08-02T11:54:51Z

arrow/src/compute/kernels/cast.rs

@@ -2854,6 +3172,102 @@ mod tests {
        }
    }

+    #[test]
+    fn test_cast_string_to_time32second() {


👍 these are great tests.

alamb · 2022-08-02T12:09:40Z

arrow/src/compute/kernels/cast.rs

+            assert!(c.is_null(2));
+            assert!(c.is_null(3));
+            assert!(c.is_null(4));
+        }


I didn't see any tests for the fallable path (aka that throws an error). I want to have another crack at using the iterators so I'll add those tests in a follow up

ursabot · 2022-08-02T12:11:41Z

Benchmark runs are scheduled for baseline = ed9fc56 and contender = 9a4b1c9. 9a4b1c9 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

feat: Implement string cast operations for Time32 and Time64

970b46e

github-actions bot added the arrow Changes to the arrow crate label Aug 1, 2022

alamb reviewed Aug 1, 2022

View reviewed changes

chore: Remove unnecessary leap second handling

490f0b3

Remove the unnecessary conditionals to extract the leap second, as it is already handled when converting to a time unit relative to midnight 🤦🏻‍♂️

avantgardnerio approved these changes Aug 1, 2022

View reviewed changes

chore: Inline trivial functions

7340b19

alamb approved these changes Aug 2, 2022

View reviewed changes

alamb reviewed Aug 2, 2022

View reviewed changes

alamb merged commit 9a4b1c9 into apache:master Aug 2, 2022

This was referenced Aug 2, 2022

[Minor] Add tests for temporal cast error paths #2283

Merged

Clean up string casts and improve performance #2284

Merged

stuartcarnie deleted the sgc/issues/2053 branch August 3, 2022 00:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement string cast operations for Time32 and Time64 #2251

feat: Implement string cast operations for Time32 and Time64 #2251

stuartcarnie commented Aug 1, 2022

alamb left a comment

alamb Aug 1, 2022

alamb Aug 1, 2022

avantgardnerio Aug 1, 2022

avantgardnerio Aug 1, 2022

avantgardnerio Aug 1, 2022

avantgardnerio Aug 1, 2022

stuartcarnie Aug 1, 2022

alamb Aug 1, 2022

stuartcarnie Aug 1, 2022

alamb Aug 1, 2022

alamb Aug 1, 2022

alamb Aug 1, 2022

stuartcarnie Aug 1, 2022 •

edited

Loading

stuartcarnie Aug 1, 2022 •

edited

Loading

alamb Aug 2, 2022

alamb Aug 2, 2022

alamb Aug 1, 2022

stuartcarnie Aug 1, 2022

avantgardnerio left a comment

codecov-commenter commented Aug 1, 2022 •

edited

Loading

alamb left a comment

alamb Aug 2, 2022

alamb Aug 2, 2022

alamb Aug 2, 2022

ursabot commented Aug 2, 2022

	let (_, upper) = iterator.size_hint();
	let len = upper.expect("trusted_len_unzip requires an upper limit");

feat: Implement string cast operations for Time32 and Time64 #2251

feat: Implement string cast operations for Time32 and Time64 #2251

Conversation

stuartcarnie commented Aug 1, 2022

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

alamb 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

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

Choose a reason for hiding this comment

stuartcarnie Aug 1, 2022 • edited Loading

Choose a reason for hiding this comment

stuartcarnie Aug 1, 2022 • 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

avantgardnerio left a comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 1, 2022 • edited Loading

Codecov Report

alamb 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

ursabot commented Aug 2, 2022

stuartcarnie Aug 1, 2022 •

edited

Loading

stuartcarnie Aug 1, 2022 •

edited

Loading

codecov-commenter commented Aug 1, 2022 •

edited

Loading