perf: remove per-row String allocation from Spark soundex and quote by andygrove · Pull Request #23880 · apache/datafusion

andygrove · 2026-07-25T13:52:54Z

Which issue does this PR close?

N/A

Rationale for this change

Spark's soundex and quote both allocated a fresh String for every row and
collected the results into a StringArray.

soundex allocated twice per row: once for the code buffer, and again for the
format!("{soundex_code:0<4}") that zero-pads it. quote allocated a String
sized to the input, then copied the input into it one char at 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. quote only ever wraps the
input 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) -> String becomes append_soundex(&mut StringBuilder, &str),
building the four-character code in a [u8; 4] initialised to b'0' — which is
the zero-padding, so the trailing format! disappears.
Strings that do not start with an ASCII letter are passed through unchanged;
previously this path called s.to_string(), now it appends by reference.
The Utf8/LargeUtf8 and Utf8View entry points share one soundex_impl
over Option<&str>, pre-sizing the builder at 4 bytes per row.

quote.rs:

compute_quote(&str) -> String becomes append_quoted(&mut StringBuilder, &str),
writing into the builder's buffer via the fmt::Write impl and finalising with
append_value("").
Escaping uses str::split('\'') to copy the runs between quotes in one memcpy
each, instead of pushing every char individually.
The builder is pre-sized from the input's value_data() length plus two bytes
per row for the surrounding quotes.

Output is unchanged in both cases.

Are these changes tested?

Existing coverage pins the behaviour: spark/string/soundex.slt and
spark/string/quote.slt assert concrete outputs, including the non-alphabetic
passthrough, codes shorter than four characters (padding), codes truncated at
four, and strings with and without embedded quotes. All 59 spark/string
sqllogictest files and the 258 datafusion-spark unit tests pass.

The benchmark used below, datafusion/spark/benches/soundex_quote.rs, is added
separately in #23882 so the baseline can be measured on main before this
change lands. It covers both functions over Utf8 and Utf8View at 1024 and
8192 rows with 20% nulls; the quote input is a mix of strings with and without
embedded quotes so the escaping path is exercised without dominating.

Benchmarks

Criterion, apache/main @ f1ab86dad as baseline. Median of the reported
change interval.

Benchmark	1024	8192
`soundex/utf8`	−52.9%	−49.5%
`soundex/utf8view`	−54.5%	−50.2%
`quote/utf8`	−60.7%	−61.5%
`quote/utf8view`	−60.5%	−61.3%

Are there any user-facing changes?

No. Both functions produce byte-identical output; this is purely an allocation
change.

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.

codecov-commenter · 2026-07-25T14:35:30Z

Codecov Report

❌ Patch coverage is 98.81657% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.65%. Comparing base (20d1c56) to head (211db45).

Files with missing lines	Patch %	Lines
datafusion/spark/src/function/string/soundex.rs	97.72%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #23880    +/-   ##
========================================
  Coverage   81.64%   81.65%            
========================================
  Files        1123     1123            
  Lines      410248   410376   +128     
  Branches   410248   410376   +128     
========================================
+ Hits       334940   335076   +136     
+ Misses      55617    55608     -9     
- Partials    19691    19692     +1

☔ 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

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.

comphead

Thanks @andygrove the PR looks solid to me.

Couple of tests could be added though

quote. No LargeUtf8 case (e.g. arrow_cast(..., 'LargeUtf8')). The quote_array::<i64> branch of SparkQuote::return_type is unreached by quote.slt.
quote. No Utf8View case exercises quote_view, whose capacity hint uses total_bytes_len(). That is a different path from quote_array's last - first computation.
quote. No SELECT quote(NULL) and no SELECT quote('') appear in quote.slt. The rewritten quote_impl's new append_null() branch and empty-run fallback are unexercised. Sibling soundex.slt does cover both.
quote. All 29 existing cases are SELECT quote(<scalar>); (single-row arrays). Cross-row StringBuilder reuse across many rows is not directly exercised. Add a SELECT quote(col) FROM (VALUES ...) batch test.
soundex. No LargeUtf8 or Utf8View case exercises the soundex_array::<i64> or soundex_view branches.
soundex. No non-ASCII alphabetic first-character case (e.g. Ñoño, Éclair) confirms the passthrough append_value(s) route. The from_utf8_unchecked SAFETY 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.

@Jefffrey

- `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.

…row-alloc

andygrove · 2026-08-15T19:01:08Z

Could you take another look @comphead?

…row-alloc

andygrove · 2026-09-03T17:03:04Z

@comphead all seven of these went in as f622ed1, which landed the day after your review — my "take another look" ping didn't say what had changed, so here is the mapping:

quote.slt — new # LargeUtf8 and Utf8View inputs and # Multi-row batches, so the output builder is reused across rows sections:

LargeUtf8: quote(arrow_cast('it''s', 'LargeUtf8')) and the empty-string variant, so quote_array::<i64> and the LargeUtf8 arm of return_type are both reached.
Utf8View: quote(arrow_cast('it''s', 'Utf8View')) plus a batch that deliberately mixes values inlined in the views (<= 12 bytes) with one held in a data buffer, since total_bytes_len() counts those two differently.
SELECT quote(NULL) and SELECT quote('') are now present, covering quote_impl's append_null() branch and the empty-run fallback.
Cross-row builder reuse: SELECT quote(c) FROM VALUES ('a'), ('it''s'), (NULL), (''), ('x''''y'), with LargeUtf8 and Utf8View batch variants alongside.

soundex.slt:

LargeUtf8 and Utf8View cases now exercise soundex_array::<i64> and soundex_view.
Non-ASCII alphabetic first characters: Ñoño and Éclair. Worth noting the from_utf8_unchecked you were guarding against is gone — f622ed1 dropped the unsafe block entirely, so there is no longer a SAFETY invariant to protect if the first-char test broadens.
Multi-row batch over VALUES, in both Utf8 and Utf8View.

Sliced and empty arrays can't be built through sqllogictest, so those stayed as Rust unit tests: soundex_sliced_array, soundex_empty_array, and on the quote side quote_sliced_array, quote_sliced_view_array, quote_empty_array.

Also addressed @Jefffrey's note above — the offsets match is now a plain unwrap with the non-empty guarantee recorded in a comment.

Just merged latest main in. Ready for another look.

comphead

Thanks @andygrove

github-actions Bot added the spark Changes to datafusion-spark crate label Jul 25, 2026

andygrove mentioned this pull request Jul 25, 2026

bench: add Criterion benchmarks for Spark bin, soundex, quote, url_encode, url_decode, and next_day #23882

Closed

andygrove force-pushed the opt/spark-string-per-row-alloc branch from 812da2f to 3e80e81 Compare July 25, 2026 14:08

neilconway reviewed Jul 25, 2026

View reviewed changes

Comment thread datafusion/spark/src/function/string/quote.rs Resolved

Comment thread datafusion/spark/src/function/string/quote.rs Resolved

andygrove mentioned this pull request Jul 26, 2026

perf: optimize spark soundex function #23909

Closed

Jefffrey reviewed Jul 28, 2026

View reviewed changes

Comment thread datafusion/spark/src/function/string/quote.rs Resolved

comphead reviewed Aug 12, 2026

View reviewed changes

github-actions Bot added the sqllogictest SQL Logic Tests (.slt) label Aug 13, 2026

Merge remote-tracking branch 'apache/main' into opt/spark-string-per-…

b747c33

…row-alloc

Merge remote-tracking branch 'apache/main' into opt/spark-string-per-…

211db45

…row-alloc

comphead approved these changes Sep 3, 2026

View reviewed changes

neilconway added this pull request to the merge queue Sep 5, 2026

Merged via the queue into apache:main with commit be5f265 Sep 5, 2026
38 checks passed

nuno-faria added the v56.0.0 label Oct 3, 2026

Navigation Menu

Repository navigation

perf: remove per-row String allocation from Spark soundex and quote - #23880

perf: remove per-row String allocation from Spark soundex and quote#23880
neilconway merged 5 commits into
apache:mainfrom
andygrove:opt/spark-string-per-row-alloc

andygrove commented Jul 25, 2026 •

edited

Loading

Uh oh!

codecov-commenter commented Jul 25, 2026 •

edited

Loading

Uh oh!

neilconway left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

comphead left a comment

Uh oh!

andygrove commented Aug 15, 2026

Uh oh!

andygrove commented Sep 3, 2026

Uh oh!

comphead left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

andygrove commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Benchmarks

Are there any user-facing changes?

Uh oh!

codecov-commenter commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

neilconway left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

comphead left a comment

Choose a reason for hiding this comment

Uh oh!

andygrove commented Aug 15, 2026

Uh oh!

andygrove commented Sep 3, 2026

Uh oh!

comphead left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

andygrove commented Jul 25, 2026 •

edited

Loading

codecov-commenter commented Jul 25, 2026 •

edited

Loading