Visitar URL original
bench: Various improvements to `trim`, `substr`, `split_part` benchmarks by neilconway · Pull Request #26114 · apache/datafusion · GitHub
Skip to content

bench: Various improvements to trim, substr, split_part benchmarks - #26114

Closed
neilconway wants to merge 5 commits into
apache:mainfrom
neilconway:neilc/bench-string-variable-lengths
Closed

neilconway wants to merge 5 commits into
apache:mainfrom
neilconway:neilc/bench-string-variable-lengths

Conversation

@neilconway

@neilconway neilconway commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

Various cleanups and improvements for these string benchmarks:

  • The benchmarks all used fixed-length strings, which is atypical for real-world workloads. Fixed-length inputs make life much easier for the branch predictor, and that can have a very significant impact when microbenchmarking and tuning these functions.
  • Add/remove benchmark cases to improve coverage
  • The return type of several UDF invocations was incorrect (e.g., Utf8 vs Utf8View).
  • Make benchmark coverage consistent between Utf8 and Utf8View
  • Use 8k batch size consistently
  • Refactoring and code cleanup

This PR also adjusts the substr_index benchmarks to use an 8k batch size for consistency, but it didn't suffer from the other issues described above.

What changes are included in this PR?

See above.

What is the testing strategy for this PR?

Benchmark changes only.

Are there any user-facing changes?

No.

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.
@github-actions github-actions Bot added the functions Changes to functions implementation label Oct 7, 2026
@neilconway neilconway changed the title bench: Various improvements to trim, substr, split_part benchmarks bench: Various improvements to trim, substr, split_part benchmarks Oct 7, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.73%. Comparing base (720c5df) to head (05baa5f).
⚠️ Report is 14 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##             main   #26114     +/-   ##
=========================================
  Coverage   82.72%   82.73%             
=========================================
  Files        1147     1147             
  Lines      448093   449353   +1260     
  Branches   448093   449353   +1260     
=========================================
+ Hits       370689   371776   +1087     
- Misses      54892    54918     +26     
- Partials    22512    22659    +147     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@neilconway

Copy link
Copy Markdown
Contributor Author

Landing as part of #26121 instead

@neilconway neilconway closed this Oct 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

functions Changes to functions implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants