perf: remove per-row String allocation from Spark soundex and quote - #23880
Open
andygrove wants to merge 3 commits into
Open
perf: remove per-row String allocation from Spark soundex and quote#23880andygrove wants to merge 3 commits into
andygrove wants to merge 3 commits into
Conversation
Both functions built a fresh String for every row and collected the results into a StringArray. soundex allocated twice per row -- once for the code buffer and once more for the format! that zero-pads it -- and quote allocated a String sized to the input before copying it in character at a time. Neither needs to allocate. A soundex code is always exactly four ASCII characters, so it is built in a stack buffer. quote writes straight into the builder and copies the runs between quotes rather than one char at a time. soundex -50%, quote -61% against the benchmarks added in apache#23882.
andygrove
force-pushed
the
opt/spark-string-per-row-alloc
branch
from
July 25, 2026 14:08
812da2f to
3e80e81
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23880 +/- ##
==========================================
+ Coverage 80.65% 81.17% +0.52%
==========================================
Files 1091 1109 +18
Lines 371031 388161 +17130
Branches 371031 388161 +17130
==========================================
+ Hits 299256 315109 +15853
- Misses 53935 54508 +573
- Partials 17840 18544 +704 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
neilconway
reviewed
Jul 25, 2026
neilconway
left a comment
Contributor
There was a problem hiding this comment.
Overall looks reasonable!
`value_data().len()` spans the entire values buffer even when the array is a slice, and `get_buffer_memory_size()` reports buffer capacities while ignoring inlined view values. Take the length from the sliced offsets and from `total_bytes_len()` instead.
Jefffrey
reviewed
Jul 28, 2026
Comment on lines
+96
to
+99
| let data_len = match (offsets.first(), offsets.last()) { | ||
| (Some(first), Some(last)) => last.as_usize() - first.as_usize(), | ||
| _ => 0, | ||
| }; |
Contributor
There was a problem hiding this comment.
offset buffers are guaranteed to be non-empty so we can unwrap instead of matching here
comphead
reviewed
Aug 12, 2026
comphead
left a comment
Contributor
There was a problem hiding this comment.
Thanks @andygrove the PR looks solid to me.
Couple of tests could be added though
quote. NoLargeUtf8case (e.g.arrow_cast(..., 'LargeUtf8')). Thequote_array::<i64>branch ofSparkQuote::return_typeis unreached byquote.slt.quote. NoUtf8Viewcase exercisesquote_view, whose capacity hint usestotal_bytes_len(). That is a different path fromquote_array'slast - firstcomputation.quote. NoSELECT quote(NULL)and noSELECT quote('')appear inquote.slt. The rewrittenquote_impl's newappend_null()branch and empty-run fallback are unexercised. Siblingsoundex.sltdoes cover both.quote. All 29 existing cases areSELECT quote(<scalar>);(single-row arrays). Cross-rowStringBuilderreuse across many rows is not directly exercised. Add aSELECT quote(col) FROM (VALUES ...)batch test.soundex. NoLargeUtf8orUtf8Viewcase exercises thesoundex_array::<i64>orsoundex_viewbranches.soundex. No non-ASCII alphabetic first-character case (e.g.Ñoño,Éclair) confirms the passthroughappend_value(s)route. Thefrom_utf8_uncheckedSAFETY invariant is not test-guarded against future first-char broadening.soundex. No sliced-array, empty-array, or multi-row batch case. All existing tests are single-row scalar.
- `quote`/`soundex` now preserve the input's `LargeUtf8` type instead of narrowing to `Utf8`, which had triggered a planner assertion. The `soundex.slt` and `quote.slt` files exercised only `Utf8` inputs, so the mismatch went unnoticed. - Drop the `from_utf8_unchecked` in `soundex`; the four-byte validation is cheap and no longer relies on an invariant that only tests could guard. - Unwrap the offset first/last directly, per @Jefffrey. - Add unit tests and sqllogictests for `LargeUtf8`, `Utf8View`, `NULL`, empty values, non-ASCII alphabetic first characters, sliced arrays, and multi-row batches.
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
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?
N/A
Rationale for this change
Spark's
soundexandquoteboth allocated a freshStringfor every row andcollected the results into a
StringArray.soundexallocated twice per row: once for the code buffer, and again for theformat!("{soundex_code:0<4}")that zero-pads it.quoteallocated aStringsized to the input, then copied the input into it one
charat a time.Neither function needs to allocate per row. A soundex code is always exactly
four ASCII characters, so it fits in a stack buffer.
quoteonly ever wraps theinput and escapes embedded quotes, so it can write straight into the output
buffer and copy the runs between quotes rather than character by character.
What changes are included in this PR?
soundex.rs:compute_soundex(&str) -> Stringbecomesappend_soundex(&mut StringBuilder, &str),building the four-character code in a
[u8; 4]initialised tob'0'— which isthe zero-padding, so the trailing
format!disappears.previously this path called
s.to_string(), now it appends by reference.Utf8/LargeUtf8andUtf8Viewentry points share onesoundex_implover
Option<&str>, pre-sizing the builder at 4 bytes per row.quote.rs:compute_quote(&str) -> Stringbecomesappend_quoted(&mut StringBuilder, &str),writing into the builder's buffer via the
fmt::Writeimpl and finalising withappend_value("").str::split('\'')to copy the runs between quotes in one memcpyeach, instead of pushing every
charindividually.value_data()length plus two bytesper row for the surrounding quotes.
Output is unchanged in both cases.
Are these changes tested?
Existing coverage pins the behaviour:
spark/string/soundex.sltandspark/string/quote.sltassert concrete outputs, including the non-alphabeticpassthrough, codes shorter than four characters (padding), codes truncated at
four, and strings with and without embedded quotes. All 59
spark/stringsqllogictest files and the 258
datafusion-sparkunit tests pass.The benchmark used below,
datafusion/spark/benches/soundex_quote.rs, is addedseparately in #23882 so the baseline can be measured on
mainbefore thischange lands. It covers both functions over
Utf8andUtf8Viewat 1024 and8192 rows with 20% nulls; the
quoteinput is a mix of strings with and withoutembedded quotes so the escaping path is exercised without dominating.
Benchmarks
Criterion,
apache/main@f1ab86dadas baseline. Median of the reportedchange interval.
soundex/utf8soundex/utf8viewquote/utf8quote/utf8viewAre there any user-facing changes?
No. Both functions produce byte-identical output; this is purely an allocation
change.