Repository navigation
bench: Various improvements to trim, substr, split_part benchmarks - #26114
Closed
neilconway wants to merge 5 commits into
Closed
neilconway wants to merge 5 commits into
neilconway wants to merge 5 commits into
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.
neilconway
changed the title
trim, substr, split_part benchmarks
Codecov Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
Contributor
Author
|
Landing as part of #26121 instead |
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
2 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?
left,right#26026Rationale for this change
Various cleanups and improvements for these string benchmarks:
This PR also adjusts the
substr_indexbenchmarks 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.