Clean up string casts and improve performance #2284

alamb · 2022-08-02T13:01:01Z

~~Draft until #2283 is merged~~

Which issue does this PR close?

Rationale for this change

Here are some cleanups I suggested to @stuartcarnie of while reviewing #2251
-- #2251 (comment). Turns out not only did I give a bad suggestion (🤦 ), he was following the existing pattern in this file.

Since I was already working on this, I figured I would finish up the change so that new kernels don't get the same copy/paste code.

I think this both makes the code easier to read (it is less verbose) and it should improve performance by removing bounds checks (and unnecessary String construction and allocation)

What changes are included in this PR?

Use Iterators rather than value() and is_null and remove several to_string() calls
Move duplicated constants into temporal_conversion.rs

Are there any user-facing changes?

No (maybe faster cast kernels)

alamb · 2022-08-02T13:04:03Z

arrow/src/compute/kernels/cast.rs

@@ -1247,19 +1251,6 @@ const fn time_unit_multiple(unit: &TimeUnit) -> i64 {
    }
 }

-/// Number of seconds in a day


these were already defined in temporal_conversion.rs so use those definitions instead of duplicating them)

alamb · 2022-08-02T13:05:10Z

arrow/src/compute/kernels/cast.rs

@@ -1463,35 +1454,28 @@ where
    <T as ArrowPrimitiveType>::Native: lexical_core::FromLexical,
 {
    if cast_options.safe {


I basically applied the same transformation to all the kernels I modified -- I changed the safe path to use iter().map(|v| v.and_then) and I changed the not safe path to use iter().map(|v| v.map().transpose)

alamb · 2022-08-02T13:06:00Z

arrow/src/compute/kernels/cast.rs

-                if from.is_null(i) {
-                    Ok(None)
-                } else {
-                    let string = from.value(i);


I don't know why this code was creating an owned String for each element, so I removed it. The same pattern was repeated in all the other kernels I modified in this PR

alamb · 2022-08-02T17:19:29Z

arrow/src/compute/kernels/cast.rs

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


this seems to be an unused random chrono::Duration::days(3); construction 🤷

Gosh, I thought that was removed from my branch, but there were multiple instances. Sorry about that!

stuartcarnie

These are nice, concise improvements to the previous implementation and will provide good examples for future developers.

viirya · 2022-08-03T01:27:37Z

arrow/src/compute/kernels/cast.rs

+                             + time.nanosecond() as i64 / NANOS_PER_MICRO)
+                        .map_err(|_| {
+                            ArrowError::CastError(
+                                format!("Cannot cast string '{}' to value of arrow::datatypes::types::Time64MicrosecondType type", v),


Personally I think Time64MicrosecondType is enough. But just nitpicking.

I agree -- they are heinous -- here is a proposal to fix them: #2295

HaoYang670 · 2022-08-03T01:36:05Z

As the title says, there should be performance improvement. However, I don't see it when doing benchmarks on my laptop:
Tested on Intel Ubuntu

cargo bench --bench cast_kernels -- --baseline master

...
cast utf8 to date32 512 time:   [22.682 us 22.698 us 22.716 us]                                     
                        change: [-0.3103% -0.2050% -0.1135%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 12 outliers among 100 measurements (12.00%)
  4 (4.00%) high mild
  8 (8.00%) high severe

cast utf8 to date64 512 time:   [39.237 us 39.258 us 39.285 us]                                     
                        change: [+2.0238% +2.2611% +2.4840%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 13 outliers among 100 measurements (13.00%)
  1 (1.00%) low mild
  3 (3.00%) high mild
  9 (9.00%) high severe
...

tustvold

It's worth noting this will in most cases boil down to the same thing, unfortunately ArrayIter has the same naive null mask handling, but once we fix that this code will benefit 👍

The code is also more concise and easier to read so

ursabot · 2022-08-03T08:32:39Z

Benchmark runs are scheduled for baseline = 1f9973c and contender = ec83638. ec83638 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

alamb · 2022-08-03T10:27:16Z

As the title says, there should be performance improvement. However, I don't see it when doing benchmarks on my laptop:

Thanks for checking @HaoYang670

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

alamb commented Aug 2, 2022

View reviewed changes

alamb mentioned this pull request Aug 2, 2022

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

Merged

alamb changed the title ~~[Minor] Clean up string casts~~ Clean up string casts and improve performance Aug 2, 2022

Reduce duplication and bounds checks in cast kernels

68df4fa

alamb force-pushed the alamb/cleaner_cast branch from 5f14220 to 68df4fa Compare August 2, 2022 17:18

alamb marked this pull request as ready for review August 2, 2022 17:18

alamb commented Aug 2, 2022

View reviewed changes

stuartcarnie approved these changes Aug 3, 2022

View reviewed changes

viirya reviewed Aug 3, 2022

View reviewed changes

viirya approved these changes Aug 3, 2022

View reviewed changes

tustvold approved these changes Aug 3, 2022

View reviewed changes

tustvold merged commit ec83638 into apache:master Aug 3, 2022

alamb mentioned this pull request Aug 3, 2022

[Minor] Improve types shown in cast error messages #2295

Merged

alamb deleted the alamb/cleaner_cast branch August 3, 2022 10:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up string casts and improve performance #2284

Clean up string casts and improve performance #2284

alamb commented Aug 2, 2022 •

edited

alamb Aug 2, 2022

alamb Aug 2, 2022

alamb Aug 2, 2022 •

edited

alamb Aug 2, 2022

stuartcarnie Aug 3, 2022

stuartcarnie left a comment

viirya Aug 3, 2022

alamb Aug 3, 2022

HaoYang670 commented Aug 3, 2022

tustvold left a comment •

edited

ursabot commented Aug 3, 2022

alamb commented Aug 3, 2022

Clean up string casts and improve performance #2284

Clean up string casts and improve performance #2284

Conversation

alamb commented Aug 2, 2022 • edited

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

alamb Aug 2, 2022

Choose a reason for hiding this comment

alamb Aug 2, 2022

Choose a reason for hiding this comment

alamb Aug 2, 2022 • edited

Choose a reason for hiding this comment

alamb Aug 2, 2022

Choose a reason for hiding this comment

stuartcarnie Aug 3, 2022

Choose a reason for hiding this comment

stuartcarnie left a comment

Choose a reason for hiding this comment

viirya Aug 3, 2022

Choose a reason for hiding this comment

alamb Aug 3, 2022

Choose a reason for hiding this comment

HaoYang670 commented Aug 3, 2022

tustvold left a comment • edited

Choose a reason for hiding this comment

ursabot commented Aug 3, 2022

alamb commented Aug 3, 2022

alamb commented Aug 2, 2022 •

edited

alamb Aug 2, 2022 •

edited

tustvold left a comment •

edited