Improve `trim` for string view #12395

Rachelint · 2024-09-09T13:51:08Z

Which issue does this PR close?

Closes #12387

Rationale for this change

Similar as the string view version substr, we can impl the string view version trim to improve performance.

What changes are included in this PR?

Impl a string view version trim which can avoid copying the whole long(> 12) string when performing trim.
Introduce the basic unit tests for trim.

Are these changes tested?

Test by new unit test and exist other tests.

Are there any user-facing changes?

No.

Kev1n8 · 2024-09-09T16:03:36Z

FYI @Rachelint that #12383 is modifying make_and_append_view, the original implementation is not correct, which is my fault.

Rachelint · 2024-09-09T16:16:36Z

FYI @Rachelint that #12383 is modifying make_and_append_view, the original implementation is not correct, which is my fault.

Thanks! I will push forward this until #12383 merged.

alamb

Thank you @Rachelint -- this looks really nice and quite close 🙏

I left some comments, but I don't think they are required to merge this.

I do think we should have benchmark numbers showing this makes things faster in order to merge it. Could you please make a StringView based benchmark for trim -- perhaps in

datafusion/datafusion/functions/benches/ltrim.rs

Line 4 in a08f923

// regarding copyright ownership. The ASF licenses this file

?

Then we can run that benchmark and show that this PR improves the performance.

Thanks again!

alamb · 2024-09-16T19:15:41Z

datafusion/functions/src/string/btrim.rs

@@ -82,7 +82,11 @@ impl ScalarUDFImpl for BTrimFunc {
    }

    fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
-        utf8_to_str_type(&arg_types[0], "btrim")
+        if arg_types[0] == DataType::Utf8View {
+            Ok(DataType::Utf8View)


👍

Also eventually it would also be possible to return Utf8View when the input was Utf8 and save a copy as well

alamb · 2024-09-16T19:21:03Z

datafusion/functions/src/string/common.rs

 use datafusion_common::cast::{as_generic_string_array, as_string_view_array};
 use datafusion_common::Result;
 use datafusion_common::{exec_err, ScalarValue};
 use datafusion_expr::ColumnarValue;

+/// Make a `u128` based on the given substr, start(offset to view.offset), and
+/// push into to the given buffers
+pub(crate) fn make_and_append_view(


🤔 I wonder if we should (as a follow on PR) propose adding this upstream to arrow-rs as it seems valuable for any trim related kernels on stringview

It sounds great! and #12383 (comment) can be solved if it is function in arrow-rs.

alamb · 2024-09-16T19:27:32Z

datafusion/functions/src/string/ltrim.rs

@@ -81,7 +81,11 @@ impl ScalarUDFImpl for LtrimFunc {
    }

    fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
-        utf8_to_str_type(&arg_types[0], "ltrim")
+        if arg_types[0] == DataType::Utf8View {
+            Ok(DataType::Utf8View)


Can we possibly add a .slt test to cover this (showing that the output type is now a view and some basic end to end tests (if not already done)?)

Perhaps in https://github.com/apache/datafusion/blob/main/datafusion/sqllogictest/test_files/string_view.slt ?

Ok, I am fixing tests and benchmarks now.

Fixed test, and introduced a benchamrk in #12513 .
#12395 (comment) shows number about improvement.

The benchmark pr still need to be sorted out, I will make it later today.

Rachelint · 2024-09-17T12:04:50Z

I think maybe we should place the LTrim/RTrim/BTrim into a same place(like trim.rs)?

Kev1n8 · 2024-09-17T15:40:06Z

For benchmarking, I would recommend this PR #12111. for what it's worth

Rachelint · 2024-09-17T16:59:57Z

For benchmarking, I would recommend this PR #12111. for what it's worth

Thanks, it is really helpful!

Rachelint · 2024-09-17T19:02:18Z

Run benchmark introduced in #12513, about 10~20% improvement for the long string(64 bytes).

Highlights, as we expected, the string view trim mainly reduces copyings when the trimmed result > 12:

group                                                                                    main                                   string-view-trim
-----                                                                                    ----                                   ----------------
INPUT LEN > 12, OUTPUT LEN > 12/string_view [size=1024, len_before=64, len_after=60]     1.16     41.2±0.19µs        ? ?/sec    1.00     35.6±0.21µs        ? ?/sec
INPUT LEN > 12, OUTPUT LEN > 12/string_view [size=4096, len_before=64, len_after=60]     1.25    173.1±5.68µs        ? ?/sec    1.00    138.5±0.78µs        ? ?/sec
INPUT LEN > 12, OUTPUT LEN > 12/string_view [size=8192, len_before=64, len_after=60]     1.24    341.3±3.67µs        ? ?/sec    1.00    276.1±1.17µs        ? ?/sec

The detailed sorted out benchmark result:

group                                                                                    main                                   string-view-trim
-----                                                                                    ----                                   ----------------
INPUT LEN <= 12/large_string [size=1024, len_before=12, len_after=8]                     1.00     35.9±0.07µs        ? ?/sec    1.01     36.1±0.36µs        ? ?/sec
INPUT LEN <= 12/large_string [size=4096, len_before=12, len_after=8]                     1.00    139.6±0.51µs        ? ?/sec    1.00    139.1±0.49µs        ? ?/sec
INPUT LEN <= 12/large_string [size=8192, len_before=12, len_after=8]                     1.01    281.2±2.01µs        ? ?/sec    1.00    278.4±2.06µs        ? ?/sec
INPUT LEN <= 12/string [size=1024, len_before=12, len_after=8]                           1.00     35.9±0.31µs        ? ?/sec    1.00     35.9±0.14µs        ? ?/sec
INPUT LEN <= 12/string [size=4096, len_before=12, len_after=8]                           1.00    138.5±0.41µs        ? ?/sec    1.01    139.4±0.52µs        ? ?/sec
INPUT LEN <= 12/string [size=8192, len_before=12, len_after=8]                           1.00    279.1±3.72µs        ? ?/sec    1.00    278.6±1.07µs        ? ?/sec
INPUT LEN <= 12/string_view [size=1024, len_before=12, len_after=8]                      1.00     36.2±1.13µs        ? ?/sec    1.00     36.1±1.98µs        ? ?/sec
INPUT LEN <= 12/string_view [size=4096, len_before=12, len_after=8]                      1.00    139.7±1.54µs        ? ?/sec    1.00    139.0±2.41µs        ? ?/sec
INPUT LEN <= 12/string_view [size=8192, len_before=12, len_after=8]                      1.01    277.5±1.31µs        ? ?/sec    1.00    275.5±2.25µs        ? ?/sec
INPUT LEN > 12, OUTPUT LEN <= 12/large_string [size=1024, len_before=64, len_after=4]    1.03    135.5±4.86µs        ? ?/sec    1.00    131.6±1.33µs        ? ?/sec
INPUT LEN > 12, OUTPUT LEN <= 12/large_string [size=4096, len_before=64, len_after=4]    1.00    522.5±2.32µs        ? ?/sec    1.00    522.1±2.30µs        ? ?/sec
INPUT LEN > 12, OUTPUT LEN <= 12/large_string [size=8192, len_before=64, len_after=4]    1.00   1039.3±3.48µs        ? ?/sec    1.00   1040.9±3.07µs        ? ?/sec
INPUT LEN > 12, OUTPUT LEN <= 12/string [size=1024, len_before=64, len_after=4]          1.01    132.5±1.17µs        ? ?/sec    1.00    131.3±0.92µs        ? ?/sec
INPUT LEN > 12, OUTPUT LEN <= 12/string [size=4096, len_before=64, len_after=4]          1.01    527.6±3.43µs        ? ?/sec    1.00    522.2±1.72µs        ? ?/sec
INPUT LEN > 12, OUTPUT LEN <= 12/string [size=8192, len_before=64, len_after=4]          1.00   1043.3±2.28µs        ? ?/sec    1.00   1040.7±3.50µs        ? ?/sec
INPUT LEN > 12, OUTPUT LEN <= 12/string_view [size=1024, len_before=64, len_after=4]     1.01    131.3±0.40µs        ? ?/sec    1.00    130.5±0.60µs        ? ?/sec
INPUT LEN > 12, OUTPUT LEN <= 12/string_view [size=4096, len_before=64, len_after=4]     1.01    524.0±2.79µs        ? ?/sec    1.00    519.3±2.52µs        ? ?/sec
INPUT LEN > 12, OUTPUT LEN <= 12/string_view [size=8192, len_before=64, len_after=4]     1.00   1041.1±3.21µs        ? ?/sec    1.00   1040.1±9.73µs        ? ?/sec
INPUT LEN > 12, OUTPUT LEN > 12/large_string [size=1024, len_before=64, len_after=60]    1.00     41.2±0.30µs        ? ?/sec    1.00     41.2±0.16µs        ? ?/sec
INPUT LEN > 12, OUTPUT LEN > 12/large_string [size=4096, len_before=64, len_after=60]    1.01    169.9±4.30µs        ? ?/sec    1.00    168.1±1.83µs        ? ?/sec
INPUT LEN > 12, OUTPUT LEN > 12/large_string [size=8192, len_before=64, len_after=60]    1.01   345.1±10.96µs        ? ?/sec    1.00    342.5±4.26µs        ? ?/sec
INPUT LEN > 12, OUTPUT LEN > 12/string [size=1024, len_before=64, len_after=60]          1.02     41.8±0.62µs        ? ?/sec    1.00     41.0±0.12µs        ? ?/sec
INPUT LEN > 12, OUTPUT LEN > 12/string [size=4096, len_before=64, len_after=60]          1.01    171.6±1.73µs        ? ?/sec    1.00    169.2±2.07µs        ? ?/sec
INPUT LEN > 12, OUTPUT LEN > 12/string [size=8192, len_before=64, len_after=60]          1.00    343.0±6.30µs        ? ?/sec    1.00    341.8±6.00µs        ? ?/sec
INPUT LEN > 12, OUTPUT LEN > 12/string_view [size=1024, len_before=64, len_after=60]     1.16     41.2±0.19µs        ? ?/sec    1.00     35.6±0.21µs        ? ?/sec
INPUT LEN > 12, OUTPUT LEN > 12/string_view [size=4096, len_before=64, len_after=60]     1.25    173.1±5.68µs        ? ?/sec    1.00    138.5±0.78µs        ? ?/sec
INPUT LEN > 12, OUTPUT LEN > 12/string_view [size=8192, len_before=64, len_after=60]     1.24    341.3±3.67µs        ? ?/sec    1.00    276.1±1.17µs        ? ?/sec

github-actions bot added the functions label Sep 9, 2024

Rachelint mentioned this pull request Sep 9, 2024

Fix: substr() on StringView column's behavior is inconsistent with the old version #12383

Merged

Rachelint force-pushed the string-view-trim branch from f6c83bf to 325fac6 Compare September 9, 2024 17:18

Rachelint added 5 commits September 10, 2024 23:44

draft.

736eb11

add unit tests for xTrim.

a0da2d0

fix fmt.

3c8b035

tmp copy for ci.

06d104d

move make_and_append_view to common.

48cb4db

Rachelint force-pushed the string-view-trim branch from 325fac6 to 48cb4db Compare September 11, 2024 01:38

Rachelint added 2 commits September 11, 2024 21:08

fix sting view trim about the process of empty string.

863e9b7

fix compile.

36a8125

Rachelint marked this pull request as ready for review September 11, 2024 13:13

eliminate some repeated codes.

aa2c131

This was referenced Sep 11, 2024

DataFusion weekly project plan (Andrew Lamb) - Sep 9, 2024 #12391

Closed

DataFusion weekly project plan (Andrew Lamb) - Sep 16, 2024 #12494

Open

alamb reviewed Sep 16, 2024

View reviewed changes

add sql test case about string view trim.

e3e9b53

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Sep 17, 2024

Merge branch 'main' into string-view-trim

dbd0f25

Rachelint force-pushed the string-view-trim branch from 4e092d4 to dbd0f25 Compare September 17, 2024 18:45

remove unused imports.

6d5660f

Rachelint mentioned this pull request Sep 18, 2024

Improve benchmark for ltrim #12513

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `trim` for string view #12395

Improve `trim` for string view #12395

Rachelint commented Sep 9, 2024 •

edited

Loading

Kev1n8 commented Sep 9, 2024

Rachelint commented Sep 9, 2024 •

edited

Loading

alamb left a comment

alamb Sep 16, 2024

alamb Sep 16, 2024

Rachelint Sep 17, 2024 •

edited

Loading

alamb Sep 16, 2024

Rachelint Sep 17, 2024 •

edited

Loading

Rachelint Sep 17, 2024 •

edited

Loading

Rachelint commented Sep 17, 2024 •

edited

Loading

Kev1n8 commented Sep 17, 2024

Rachelint commented Sep 17, 2024

Rachelint commented Sep 17, 2024 •

edited

Loading

Improve trim for string view #12395

Are you sure you want to change the base?

Improve trim for string view #12395

Conversation

Rachelint commented Sep 9, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Kev1n8 commented Sep 9, 2024

Rachelint commented Sep 9, 2024 • edited Loading

alamb left a comment

Choose a reason for hiding this comment

alamb Sep 16, 2024

Choose a reason for hiding this comment

alamb Sep 16, 2024

Choose a reason for hiding this comment

Rachelint Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

alamb Sep 16, 2024

Choose a reason for hiding this comment

Rachelint Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Rachelint Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Rachelint commented Sep 17, 2024 • edited Loading

Kev1n8 commented Sep 17, 2024

Rachelint commented Sep 17, 2024

Rachelint commented Sep 17, 2024 • edited Loading

Improve `trim` for string view #12395

Improve `trim` for string view #12395

Rachelint commented Sep 9, 2024 •

edited

Loading

Rachelint commented Sep 9, 2024 •

edited

Loading

Rachelint Sep 17, 2024 •

edited

Loading

Rachelint Sep 17, 2024 •

edited

Loading

Rachelint Sep 17, 2024 •

edited

Loading

Rachelint commented Sep 17, 2024 •

edited

Loading

Rachelint commented Sep 17, 2024 •

edited

Loading