Repository navigation
perf: Use sub_view in trim, substr, split_part, and substring_index - #26121
Merged
Merged
Conversation
The trim benchmarks gave every row the same content and padding length, so per-row work that depends on those lengths was perfectly predictable. They also passed a `Utf8` return field for every input type and discarded the result, so with debug assertions enabled, the Utf8View and LargeUtf8 cases returned an internal error that went unnoticed. Draw each row's content and padding lengths from a range, add cases for values with nothing to trim and for padding made of several characters, resolve the return field with `return_field_from_args`, and check the result. Drop the LargeUtf8 cases, which run the same code as Utf8.
Every string in the substr benchmarks had the same length, and every row used the same start and count, so every result had the same length too. The benchmarks also passed a `Utf8View` return field for every input type and discarded the result, so with debug assertions enabled, the Utf8 and LargeUtf8 cases returned an internal error that went unnoticed. Rewrite them like the `left` and `right` benchmarks: draw each input's length from a range; cover short and long results, results without a count, and a start computed per row; resolve the return field with `return_field_from_args`; and check the result. Drop the LargeUtf8 cases, which run the same code as Utf8, and use a single batch size.
Every string in the split_part benchmarks had the same number of fields, each of the same length, so every result had the same length. Only two cases used Utf8View, and both had long results stored out of line. Rewrite them like the `left` and `right` benchmarks: draw each row's number of fields and each field's length from a range, and run every case on both Utf8 and Utf8View. Cover short and long fields, a field among many, a negative position, a multi-character delimiter, and a position computed per row, which replaces the cases that passed the delimiter and position as arrays.
Run each substring_index benchmark on 8192 rows, the batch size the other string benchmarks use, instead of on 100, 1000, and 10000 rows.
`trim`, `substr`, `split_part`, and `substring_index` built Utf8View results with `append_view` or one of two private copies of it, all of which call `make_view`. `make_view` picks copy code with a jump on the result's length, which the CPU often mispredicts when result lengths vary from row to row. `left` and `right` already avoid this with `sub_view`. Use `sub_view` in these functions too, and remove `append_view` and the private copies. `trim`, `split_part`, and `substring_index` find their results as `&str` slices of the input, so add `substr_view`, which builds the view for such a slice with `sub_view`. `Trimmer` now returns just the trimmed slice; the byte offset it also returned was only needed by `append_view`. Also have `sub_view` check the result's length first: results of more than 12 bytes only need their 4-byte prefix, so reading a 12-byte window and shifting it is wasted work for them.
Contributor
Author
|
Benchmark PR has the benchmark commits for merging separately: #26114 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #26121 +/- ##
=========================================
Coverage 82.72% 82.72%
=========================================
Files 1147 1147
Lines 448093 449346 +1253
Branches 448093 449346 +1253
=========================================
+ Hits 370689 371741 +1052
- Misses 54892 54945 +53
- Partials 22512 22660 +148 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
comphead
approved these changes
Oct 7, 2026
comphead
left a comment
comphead
left a comment
Contributor
There was a problem hiding this comment.
Thanks @neilconway the CI is green and LOC is negative, it already sounds good to me
Omega359
pushed a commit
to Omega359/arrow-datafusion
that referenced
this pull request
Oct 11, 2026
…g_index` (apache#26121) ## Which issue does this PR close? - N/A ## Rationale for this change `sub_view` was introduced in apache#26039; it efficiently builds a view that is a substring of another view. Several string functions had private helpers that did the same thing or used `append_view`. This PR refactors those places to use `sub_view` instead. Both `append_view` and the private helpers were built on Arrow's `make_view`. `make_view` has some shortcomings: * For strings of up to 12 bytes, it does an indirect jump on the string's length. For real-world data with variable length strings, this jump is often poorly predicted. * Perhaps because it has a 13-arm match statement, `make_view` was marked as `inline(never)`, to avoid bloating the code at each call-site In contrast, `sub_view` replaces the indirect jump with shifts and masks, and is small enough to inline. Benchmarks show that adopting `sub_view` improves performance by 5-40%, depending on the workload. Along the way, simplify the `Trimmer` interface because `append_view` is no longer used. Benchmarks: (x86, AMD EPYC Milan): - ltrim/spaces: 52.4µs → 37.5µs, −28% - ltrim/heavy_padding: 285.2µs → 215.6µs, −24% - ltrim/nothing_to_trim: 51.8µs → 29.3µs, −43% - ltrim/char_set: 2.80ms → 2.80ms, 0% - rtrim/spaces: 53.6µs → 37.6µs, −30% - rtrim/heavy_padding: 322.0µs → 253.0µs, −21% - rtrim/nothing_to_trim: 51.4µs → 29.0µs, −44% - rtrim/char_set: 2.90ms → 2.80ms, −3% - btrim/spaces: 87.3µs → 56.6µs, −35% - btrim/heavy_padding: 458.6µs → 398.3µs, −13% - btrim/nothing_to_trim: 58.1µs → 42.0µs, −28% - btrim/char_set: 5.70ms → 5.50ms, −4% - substr/short_result: 114.6µs → 100.5µs, −12% - substr/long_result: 198.4µs → 181.8µs, −8% - substr/short_result_long_input: 93.5µs → 95.5µs, +2% - substr/no_count: 84.0µs → 62.3µs, −26% - substr/per_row_start: 149.0µs → 119.5µs, −20% - split_part/short_fields: 140.9µs → 113.7µs, −19% - split_part/long_fields: 137.6µs → 113.3µs, −18% - split_part/many_fields: 497.9µs → 427.6µs, −14% - split_part/negative_position: 98.6µs → 89.0µs, −10% - split_part/multi_char_delimiter: 163.8µs → 152.6µs, −7% - split_part/per_row_position: 458.5µs → 460.2µs, 0% - substr_index/array_single_delimiter: 295.1µs → 292.7µs, −1% - substr_index/array_long_delimiter: 471.7µs → 466.5µs, −1% - substr_index/scalar_single_delimiter_pos: 75.0µs → 72.0µs, −4% - substr_index/scalar_single_delimiter_neg: 79.3µs → 75.7µs, −5% - substr_index/scalar_long_delimiter_pos: 103.1µs → 104.5µs, +1% - substr_index/scalar_long_delimiter_neg: 219.2µs → 207.3µs, −5% - left/short_result: 37.2µs → 37.7µs, +1% - left/long_result: 38.2µs → 38.2µs, 0% - left/short_result_long_input: 50.4µs → 50.3µs, 0% - left/n_exceeds_len: 36.4µs → 34.6µs, −5% - left/negative_n: 34.7µs → 34.6µs, 0% - left/per_row_n: 33.5µs → 33.8µs, +1% - right/short_result: 48.1µs → 48.3µs, 0% - right/long_result: 52.2µs → 45.0µs, −14% - right/short_result_long_input: 63.1µs → 61.1µs, −3% - right/n_exceeds_len: 46.4µs → 41.9µs, −10% - right/negative_n: 45.7µs → 41.0µs, −10% - right/per_row_n: 45.3µs → 43.7µs, −4% ## What changes are included in this PR? * Refactor `trim`, `substr`, `split_part`, and `substring_index` to use `sub_view` * Remove `append_view` (no longer used) * Simplify `Trimmer` interface (byte offset no longer needed) * Optimize `sub_view` for long result strings ## What is the testing strategy for this PR? Covered by existing tests. ## Are there any user-facing changes? No.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
sub_viewwas introduced in #26039; it efficiently builds a view that is a substring of another view. Several string functions had private helpers that did the same thing or usedappend_view. This PR refactors those places to usesub_viewinstead.Both
append_viewand the private helpers were built on Arrow'smake_view.make_viewhas some shortcomings:make_viewwas marked asinline(never), to avoid bloating the code at each call-siteIn contrast,
sub_viewreplaces the indirect jump with shifts and masks, and is small enough to inline. Benchmarks show that adoptingsub_viewimproves performance by 5-40%, depending on the workload.Along the way, simplify the
Trimmerinterface becauseappend_viewis no longer used.Benchmarks: (x86, AMD EPYC Milan):
What changes are included in this PR?
trim,substr,split_part, andsubstring_indexto usesub_viewappend_view(no longer used)Trimmerinterface (byte offset no longer needed)sub_viewfor long result stringsWhat is the testing strategy for this PR?
Covered by existing tests.
Are there any user-facing changes?
No.