You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2026/07/25 13:52:56 UTC
[PR] perf: remove per-row String allocation from Spark soundex and quote [datafusion]
andygrove opened a new pull request, #23880:
URL: https://github.com/apache/datafusion/pull/23880
## 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.
A new benchmark, `datafusion/spark/benches/soundex_quote.rs`, 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: remove per-row String allocation from Spark soundex and quote [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #23880:
URL: https://github.com/apache/datafusion/pull/23880#discussion_r3662030627
##########
datafusion/spark/src/function/string/quote.rs:
##########
@@ -88,34 +89,62 @@ fn spark_quote_inner(arg: &[ArrayRef]) -> Result<ArrayRef> {
fn quote_array<T: OffsetSizeTrait>(array: &ArrayRef) -> Result<ArrayRef> {
let str_array = as_generic_string_array::<T>(array)?;
- let result = str_array
- .iter()
- .map(|s| s.map(compute_quote))
- .collect::<StringArray>();
- Ok(Arc::new(result))
+ // Slicing an array keeps the whole value buffer and narrows only the
+ // offsets, so measure the data through the offsets rather than through
+ // `value_data()`.
+ let offsets = str_array.value_offsets();
+ let data_len = match (offsets.first(), offsets.last()) {
+ (Some(first), Some(last)) => last.as_usize() - first.as_usize(),
+ _ => 0,
+ };
Review Comment:
offset buffers are guaranteed to be non-empty so we can unwrap instead of matching here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: remove per-row String allocation from Spark soundex and quote [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #23880:
URL: https://github.com/apache/datafusion/pull/23880#discussion_r3926807684
##########
datafusion/spark/src/function/string/quote.rs:
##########
@@ -88,34 +89,62 @@ fn spark_quote_inner(arg: &[ArrayRef]) -> Result<ArrayRef> {
fn quote_array<T: OffsetSizeTrait>(array: &ArrayRef) -> Result<ArrayRef> {
let str_array = as_generic_string_array::<T>(array)?;
- let result = str_array
- .iter()
- .map(|s| s.map(compute_quote))
- .collect::<StringArray>();
- Ok(Arc::new(result))
+ // Slicing an array keeps the whole value buffer and narrows only the
+ // offsets, so measure the data through the offsets rather than through
+ // `value_data()`.
+ let offsets = str_array.value_offsets();
+ let data_len = match (offsets.first(), offsets.last()) {
+ (Some(first), Some(last)) => last.as_usize() - first.as_usize(),
+ _ => 0,
+ };
Review Comment:
Done in f622ed1c0 — `quote_array` now takes the length directly:
```rust
let offsets = str_array.value_offsets();
let data_len = offsets.last().unwrap().as_usize() - offsets[0].as_usize();
```
with a comment recording why the unwrap is sound (an offset buffer holds one more entry than the array has rows, so it is never empty).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: remove per-row String allocation from Spark soundex and quote [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #23880:
URL: https://github.com/apache/datafusion/pull/23880#discussion_r3660602220
##########
datafusion/spark/src/function/string/quote.rs:
##########
@@ -88,34 +89,55 @@ fn spark_quote_inner(arg: &[ArrayRef]) -> Result<ArrayRef> {
fn quote_array<T: OffsetSizeTrait>(array: &ArrayRef) -> Result<ArrayRef> {
let str_array = as_generic_string_array::<T>(array)?;
- let result = str_array
- .iter()
- .map(|s| s.map(compute_quote))
- .collect::<StringArray>();
- Ok(Arc::new(result))
+ Ok(quote_impl(str_array.iter(), str_array.value_data().len()))
Review Comment:
Confirmed — `GenericByteArray::slice` clones the whole `value_data` buffer and narrows only the offsets, so this over-allocates by however much of the buffer the slice excludes. Now taking the length from the sliced offsets (`last - first`) in 4a7577fe7.
##########
datafusion/spark/src/function/string/quote.rs:
##########
@@ -88,34 +89,55 @@ fn spark_quote_inner(arg: &[ArrayRef]) -> Result<ArrayRef> {
fn quote_array<T: OffsetSizeTrait>(array: &ArrayRef) -> Result<ArrayRef> {
let str_array = as_generic_string_array::<T>(array)?;
- let result = str_array
- .iter()
- .map(|s| s.map(compute_quote))
- .collect::<StringArray>();
- Ok(Arc::new(result))
+ Ok(quote_impl(str_array.iter(), str_array.value_data().len()))
}
fn quote_view(str_view: &ArrayRef) -> Result<ArrayRef> {
let str_array = as_string_view_array(str_view)?;
- let result = str_array
- .iter()
- .map(|opt_str| opt_str.map(compute_quote))
- .collect::<StringArray>();
- Ok(Arc::new(result) as ArrayRef)
+ Ok(quote_impl(
+ str_array.iter(),
+ str_array.get_buffer_memory_size(),
Review Comment:
Right, and it is wrong in the other direction too: strings of 12 bytes or fewer live inline in the views, so the data buffers can be near-empty while the array holds plenty of data. Switched to `total_bytes_len()`, which walks the sliced views and counts inlined values, in 4a7577fe7.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: remove per-row String allocation from Spark soundex and quote [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #23880:
URL: https://github.com/apache/datafusion/pull/23880#issuecomment-5303760379
Could you take another look @comphead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: remove per-row String allocation from Spark soundex and quote [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #23880:
URL: https://github.com/apache/datafusion/pull/23880#discussion_r3650733629
##########
datafusion/spark/src/function/string/quote.rs:
##########
@@ -88,34 +89,55 @@ fn spark_quote_inner(arg: &[ArrayRef]) -> Result<ArrayRef> {
fn quote_array<T: OffsetSizeTrait>(array: &ArrayRef) -> Result<ArrayRef> {
let str_array = as_generic_string_array::<T>(array)?;
- let result = str_array
- .iter()
- .map(|s| s.map(compute_quote))
- .collect::<StringArray>();
- Ok(Arc::new(result))
+ Ok(quote_impl(str_array.iter(), str_array.value_data().len()))
Review Comment:
`str_array.value_data().len()` will over-allocate for sliced arrays.
##########
datafusion/spark/src/function/string/quote.rs:
##########
@@ -88,34 +89,55 @@ fn spark_quote_inner(arg: &[ArrayRef]) -> Result<ArrayRef> {
fn quote_array<T: OffsetSizeTrait>(array: &ArrayRef) -> Result<ArrayRef> {
let str_array = as_generic_string_array::<T>(array)?;
- let result = str_array
- .iter()
- .map(|s| s.map(compute_quote))
- .collect::<StringArray>();
- Ok(Arc::new(result))
+ Ok(quote_impl(str_array.iter(), str_array.value_data().len()))
}
fn quote_view(str_view: &ArrayRef) -> Result<ArrayRef> {
let str_array = as_string_view_array(str_view)?;
- let result = str_array
- .iter()
- .map(|opt_str| opt_str.map(compute_quote))
- .collect::<StringArray>();
- Ok(Arc::new(result) as ArrayRef)
+ Ok(quote_impl(
+ str_array.iter(),
+ str_array.get_buffer_memory_size(),
Review Comment:
Looks like `get_buffer_memory_size` sums the buffer capacities, not their actual valid contents, so this will also over-allocate for sliced arrays.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: remove per-row String allocation from Spark soundex and quote [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #23880:
URL: https://github.com/apache/datafusion/pull/23880#issuecomment-5529229269
@comphead all seven of these went in as f622ed1c0, 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 — f622ed1c0 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: remove per-row String allocation from Spark soundex and quote [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #23880:
URL: https://github.com/apache/datafusion/pull/23880#issuecomment-5078874872
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/23880?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `91.66667%` with `4 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 80.65%. Comparing base ([`f1ab86d`](https://app.codecov.io/gh/apache/datafusion/commit/f1ab86dad406a189e43ac19d24965b3fbf9dbba9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`3e80e81`](https://app.codecov.io/gh/apache/datafusion/commit/3e80e814c49af02137bf6c032b1145b35646fdc9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/23880?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/spark/src/function/string/soundex.rs](https://app.codecov.io/gh/apache/datafusion/pull/23880?src=pr&el=tree&filepath=datafusion%2Fspark%2Fsrc%2Ffunction%2Fstring%2Fsoundex.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9zcGFyay9zcmMvZnVuY3Rpb24vc3RyaW5nL3NvdW5kZXgucnM=) | 85.71% | [2 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/23880?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/spark/src/function/string/quote.rs](https://app.codecov.io/gh/apache/datafusion/pull/23880?src=pr&el=tree&filepath=datafusion%2Fspark%2Fsrc%2Ffunction%2Fstring%2Fquote.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9zcGFyay9zcmMvZnVuY3Rpb24vc3RyaW5nL3F1b3RlLnJz) | 96.29% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/23880?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #23880 +/- ##
==========================================
- Coverage 80.65% 80.65% -0.01%
==========================================
Files 1091 1091
Lines 371031 371040 +9
Branches 371031 371040 +9
==========================================
+ Hits 299256 299257 +1
- Misses 53935 53936 +1
- Partials 17840 17847 +7
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/23880?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: remove per-row String allocation from Spark soundex and quote [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway merged PR #23880:
URL: https://github.com/apache/datafusion/pull/23880
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org