You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "neilconway (via GitHub)" <gi...@apache.org> on 2026/04/22 16:42:04 UTC
[PR] perf: Add new string builders that support bulk NULLs, use in `lower` and `upper` [datafusion]
neilconway opened a new pull request, #21789:
URL: https://github.com/apache/datafusion/pull/21789
## Which issue does this PR close?
- Part of #21684
## Rationale for this change
Introduce three new string array builders with bulk null tracking:
- `StringArrayBuilder` (Utf8)
- `LargeStringArrayBuilder` (LargeUtf8)
- `StringViewArrayBuilder` (Utf8View)
Each builder has the following API:
- append_value(&str) -- add a non-NULL value (row)
- append_placeholder() -- add a NULL row placeholder
- finish(Option<NullBuffer>) -- finish the build, specify NULLs
These are the counterpart of Arrow's `GenericStringBuilder` /
`StringViewBuilder` but it skips per-row NULL buffer maintenance, which
lets callers compute the NULL buffer in bulk when possible.
This PR also switches `case_conversion` to use the new APIs, which is
used to implement `lower`, `upper`, and the Spark equivalents. This
improves `lower` / `upper` performance by 3-15% on microbenchmarks. More
UDFs (~10) will be converted to use this API in future PRs.
## What changes are included in this PR?
* Add new builders
* Add unit tests
* Adopt builders in `case_conversion`
## Are these changes tested?
Yes.
## Are there any user-facing changes?
No.
--
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: Add bulk NULL-aware string builders, use in `lower` and `upper` [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on PR #21789:
URL: https://github.com/apache/datafusion/pull/21789#issuecomment-4315682768
cc @mbutrovich , in case you have a free moment to assist in the next step of the quest to optimize NULL handling 😊
--
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: Add bulk NULL-aware string builders, use in `lower` and `upper` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #21789:
URL: https://github.com/apache/datafusion/pull/21789#issuecomment-4329638932
> > Should we be porting these changes upstream (to arrow-rs) I wonder 🤔
>
> Perhaps -- you'd have a better perspective on whether the arrow-rs folks would be interested. Probably to get the performance improvement you'd need to add another string builder API to Arrow, rather than extending the existing builders...
Yeah, I don't have a great story there.... I don't have the bandwidth to pursue it, but it has been on my mind ever since we made the parallel builders
--
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: Add bulk NULL-aware string builders, use in `lower` and `upper` [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #21789:
URL: https://github.com/apache/datafusion/pull/21789#discussion_r3131981689
##########
datafusion/functions/src/strings.rs:
##########
@@ -414,6 +430,223 @@ impl ConcatLargeStringBuilder {
}
}
+// ----------------------------------------------------------------------------
+// Bulk-nulls builders
+//
+// These builders are similar to Arrow's `GenericStringBuilder` and
+// `StringViewBuilder`, except that callers must pass the NULL bitmap to
+// `finish()`, rather than maintaining it iteratively (per-row). For callers
+// that can compute the NULL bitmap in bulk (which is true of many
+// string-related UDFs), this can be significantly more efficient.
+//
+// For a row known to be null, call `append_placeholder` to advance the row
+// count without touching the value buffer; the placeholder slot will be
+// masked by the caller-supplied null buffer.
+// ----------------------------------------------------------------------------
+
+/// Builder for a [`GenericStringArray<O>`] that defers null tracking to
+/// `finish`. Instantiate with `O = i32` for [`StringArray`] (Utf8) or
+/// `O = i64` for [`LargeStringArray`] (LargeUtf8).
+pub(crate) struct GenericStringArrayBuilder<O: OffsetSizeTrait> {
+ offsets_buffer: MutableBuffer,
+ value_buffer: MutableBuffer,
+ _phantom: PhantomData<O>,
+}
+
+impl<O: OffsetSizeTrait> GenericStringArrayBuilder<O> {
+ pub fn with_capacity(item_capacity: usize, data_capacity: usize) -> Self {
+ let capacity = item_capacity
+ .checked_add(1)
+ .map(|i| i.saturating_mul(size_of::<O>()))
+ .expect("capacity integer overflow");
+
+ let mut offsets_buffer = MutableBuffer::with_capacity(capacity);
+ offsets_buffer.push(O::usize_as(0));
+ Self {
+ offsets_buffer,
+ value_buffer: MutableBuffer::with_capacity(data_capacity),
+ _phantom: PhantomData,
+ }
+ }
+
+ /// Append `value` as the next row.
+ ///
+ /// # Panics
+ ///
+ /// Panics if the cumulative byte length exceeds `O::MAX`.
+ pub fn append_value(&mut self, value: &str) {
+ self.value_buffer.extend_from_slice(value.as_bytes());
+ let next_offset =
+ O::from_usize(self.value_buffer.len()).expect("byte array offset overflow");
+ self.offsets_buffer.push(next_offset);
+ }
+
+ /// Append an empty placeholder row. The corresponding slot is meaningful
+ /// only if the caller masks it via the null buffer passed to `finish`.
+ pub fn append_placeholder(&mut self) {
+ let next_offset =
+ O::from_usize(self.value_buffer.len()).expect("byte array offset overflow");
+ self.offsets_buffer.push(next_offset);
+ }
+
+ /// Finalize into a [`GenericStringArray<O>`] using the caller-supplied
+ /// null buffer.
+ ///
+ /// # Errors
+ ///
+ /// Returns an error when `null_buffer.len()` does not match the number of
+ /// appended rows.
+ pub fn finish(
+ self,
+ null_buffer: Option<NullBuffer>,
+ ) -> Result<GenericStringArray<O>> {
+ let row_count = self.offsets_buffer.len() / size_of::<O>() - 1;
+ if let Some(ref n) = null_buffer
+ && n.len() != row_count
+ {
+ return internal_err!(
+ "Null buffer length ({}) must match row count ({row_count})",
+ n.len()
+ );
+ }
+ let array_data = ArrayDataBuilder::new(GenericStringArray::<O>::DATA_TYPE)
+ .len(row_count)
+ .add_buffer(self.offsets_buffer.into())
+ .add_buffer(self.value_buffer.into())
+ .nulls(null_buffer);
+ // SAFETY: every appended value came from a `&str`, so the value
+ // buffer is valid UTF-8 and offsets are monotonically non-decreasing.
+ let array_data = unsafe { array_data.build_unchecked() };
+ Ok(GenericStringArray::<O>::from(array_data))
+ }
+}
+
+/// Starting size for the long-string data block; matches Arrow's
+/// `GenericByteViewBuilder` default.
+const STARTING_BLOCK_SIZE: u32 = 8 * 1024;
+/// Maximum size each long-string data block grows to; matches Arrow's
+/// `GenericByteViewBuilder` default.
+const MAX_BLOCK_SIZE: u32 = 2 * 1024 * 1024;
+
+/// Builder for a [`StringViewArray`] that defers null tracking to `finish`.
+///
+/// Modeled on Arrow's [`arrow::array::builder::StringViewBuilder`] but
+/// without per-row [`arrow::array::builder::NullBufferBuilder`] maintenance.
+/// Long strings (> 12 bytes) are appended into an in-progress data block;
+/// short strings are inlined into the view itself. When the in-progress block
+/// fills up it is flushed into `completed` and a new block — double the size
+/// of the last, capped at [`MAX_BLOCK_SIZE`] — is started.
+pub(crate) struct StringViewArrayBuilder {
+ views: Vec<u128>,
+ in_progress: Vec<u8>,
+ completed: Vec<Buffer>,
+ /// Current block-size target; doubles each time a block is flushed, up to
+ /// [`MAX_BLOCK_SIZE`].
+ block_size: u32,
+}
+
+impl StringViewArrayBuilder {
+ pub fn with_capacity(item_capacity: usize) -> Self {
+ Self {
+ views: Vec::with_capacity(item_capacity),
+ in_progress: Vec::new(),
+ completed: Vec::new(),
+ block_size: STARTING_BLOCK_SIZE,
+ }
+ }
+
+ /// Doubles the block-size target (capped at [`MAX_BLOCK_SIZE`]) and
+ /// returns the new size. The first call returns `2 * STARTING_BLOCK_SIZE`.
+ fn next_block_size(&mut self) -> u32 {
+ if self.block_size < MAX_BLOCK_SIZE {
+ self.block_size = self.block_size.saturating_mul(2);
+ }
+ self.block_size
+ }
+
+ /// Append `value` as the next row.
+ ///
+ /// # Panics
+ ///
+ /// Panics if the value length, the in-progress buffer offset, or the
+ /// number of completed buffers exceeds `i32::MAX`. The ByteView spec
+ /// uses signed 32-bit integers for these fields; exceeding `i32::MAX`
+ /// would produce an array that does not round-trip through Arrow IPC
+ /// (see <https://github.com/apache/arrow-rs/issues/6172>).
+ #[inline]
+ pub fn append_value(&mut self, value: &str) {
+ let v = value.as_bytes();
+ let length: u32 =
+ i32::try_from(v.len()).expect("value length exceeds i32::MAX") as u32;
+ if length <= 12 {
+ self.views.push(make_view(v, 0, 0));
+ return;
+ }
+
+ let required_cap = self.in_progress.len() + length as usize;
+ if self.in_progress.capacity() < required_cap {
+ self.flush_in_progress();
+ let to_reserve = (length as usize).max(self.next_block_size() as usize);
+ self.in_progress.reserve(to_reserve);
+ }
+
+ let buffer_index: u32 = i32::try_from(self.completed.len())
+ .expect("buffer count exceeds i32::MAX")
+ as u32;
+ let offset: u32 = i32::try_from(self.in_progress.len())
+ .expect("offset exceeds i32::MAX") as u32;
+ self.in_progress.extend_from_slice(v);
+ self.views.push(make_view(v, buffer_index, offset));
+ }
+
+ /// Append an empty placeholder row. The corresponding slot is meaningful
+ /// only if the caller masks it via the null buffer passed to `finish`.
+ #[inline]
+ pub fn append_placeholder(&mut self) {
Review Comment:
Yeah, I was considering whether to do that. It wouldn't be the first Arrow/DF API that has some footguns :) But on reflection I think the performance cost of tracking the placeholder count is pretty tiny and probably worth doing; added.
--
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: Add bulk NULL-aware string builders, use in `lower` and `upper` [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on PR #21789:
URL: https://github.com/apache/datafusion/pull/21789#issuecomment-4315776923
Benchmarks:
```
lower_all_values_are_ascii: 1024 5.4µs -> 5.3µs -1.9%
lower_all_values_are_ascii: 4096 21.4µs -> 21.1µs -1.4%
lower_all_values_are_ascii: 8192 41.7µs -> 41.3µs -1.0%
ascii_sv 4096 len10 nulls0 mixed=f 160.2µs -> 151.4µs -5.5%
ascii_sv 4096 len10 nulls0 mixed=t 179.7µs -> 181.1µs +0.8%
ascii_sv 4096 len10 nulls0.1 mixed=f 164.6µs -> 141.7µs -13.9%
ascii_sv 4096 len10 nulls0.1 mixed=t 181.7µs -> 172.8µs -4.9%
ascii_sv 4096 len64 nulls0 mixed=f 149.0µs -> 154.1µs +3.4%
ascii_sv 4096 len64 nulls0 mixed=t 187.8µs -> 194.8µs +3.7%
ascii_sv 4096 len64 nulls0.1 mixed=f 147.6µs -> 145.4µs -1.5%
ascii_sv 4096 len64 nulls0.1 mixed=t 189.2µs -> 178.2µs -5.8%
ascii_sv 4096 len128 nulls0 mixed=f 188.7µs -> 191.2µs +1.3%
ascii_sv 4096 len128 nulls0 mixed=t 204.6µs -> 202.6µs -1.0%
ascii_sv 4096 len128 nulls0.1 mixed=f 190.2µs -> 172.8µs -9.1%
ascii_sv 4096 len128 nulls0.1 mixed=t 201.7µs -> 195.0µs -3.3%
ascii_sv 8192 len10 nulls0 mixed=f 319.4µs -> 303.8µs -4.9%
ascii_sv 8192 len10 nulls0 mixed=t 366.3µs -> 363.6µs -0.7%
ascii_sv 8192 len10 nulls0.1 mixed=f 334.8µs -> 284.6µs -15.0%
ascii_sv 8192 len10 nulls0.1 mixed=t 362.1µs -> 339.0µs -6.4%
ascii_sv 8192 len64 nulls0 mixed=f 296.6µs -> 309.1µs +4.2%
ascii_sv 8192 len64 nulls0 mixed=t 384.6µs -> 384.0µs -0.2%
ascii_sv 8192 len64 nulls0.1 mixed=f 299.7µs -> 290.6µs -3.0%
ascii_sv 8192 len64 nulls0.1 mixed=t 378.9µs -> 355.4µs -6.2%
ascii_sv 8192 len128 nulls0 mixed=f 394.9µs -> 415.3µs +5.2%
ascii_sv 8192 len128 nulls0 mixed=t 408.4µs -> 407.4µs -0.2%
ascii_sv 8192 len128 nulls0.1 mixed=f 419.0µs -> 350.9µs -16.3%
ascii_sv 8192 len128 nulls0.1 mixed=t 409.6µs -> 388.3µs -5.2%
nonascii_sv 4096 len10 nulls0 mixed=f 372.8µs -> 377.0µs +1.1%
nonascii_sv 4096 len10 nulls0 mixed=t 375.2µs -> 372.6µs -0.7%
nonascii_sv 4096 len10 nulls0.1 mixed=f 345.5µs -> 357.9µs +3.6%
nonascii_sv 4096 len10 nulls0.1 mixed=t 346.0µs -> 340.7µs -1.5%
nonascii_sv 4096 len64 nulls0 mixed=f 358.9µs -> 370.5µs +3.2%
nonascii_sv 4096 len64 nulls0 mixed=t 382.0µs -> 386.4µs +1.2%
nonascii_sv 4096 len64 nulls0.1 mixed=f 353.7µs -> 349.7µs -1.1%
nonascii_sv 4096 len64 nulls0.1 mixed=t 363.7µs -> 340.3µs -6.4%
nonascii_sv 4096 len128 nulls0 mixed=f 371.6µs -> 393.8µs +6.0%
nonascii_sv 4096 len128 nulls0 mixed=t 376.2µs -> 387.4µs +3.0%
nonascii_sv 4096 len128 nulls0.1 mixed=f 349.6µs -> 349.0µs -0.2%
nonascii_sv 4096 len128 nulls0.1 mixed=t 363.4µs -> 341.1µs -6.1%
nonascii_sv 8192 len10 nulls0 mixed=f 738.5µs -> 771.2µs +4.4%
nonascii_sv 8192 len10 nulls0 mixed=t 759.2µs -> 760.1µs +0.1%
nonascii_sv 8192 len10 nulls0.1 mixed=f 705.8µs -> 700.4µs -0.8%
nonascii_sv 8192 len10 nulls0.1 mixed=t 710.9µs -> 695.0µs -2.2%
nonascii_sv 8192 len64 nulls0 mixed=f 781.1µs -> 759.5µs -2.8%
nonascii_sv 8192 len64 nulls0 mixed=t 746.3µs -> 791.6µs +6.1%
nonascii_sv 8192 len64 nulls0.1 mixed=f 726.4µs -> 696.9µs -4.1%
nonascii_sv 8192 len64 nulls0.1 mixed=t 733.7µs -> 690.0µs -6.0%
nonascii_sv 8192 len128 nulls0 mixed=f 746.5µs -> 769.6µs +3.1%
nonascii_sv 8192 len128 nulls0 mixed=t 742.5µs -> 764.1µs +2.9%
nonascii_sv 8192 len128 nulls0.1 mixed=f 745.2µs -> 717.4µs -3.7%
nonascii_sv 8192 len128 nulls0.1 mixed=t 707.5µs -> 699.9µs -1.1%
lower_the_first_value_is_nonascii: 1024 45.6µs -> 42.6µs -6.6%
lower_the_first_value_is_nonascii: 4096 183.6µs -> 172.7µs -5.9%
lower_the_first_value_is_nonascii: 8192 372.1µs -> 356.5µs -4.2%
lower_the_middle_value_is_nonascii: 1024 45.8µs -> 43.2µs -5.7%
lower_the_middle_value_is_nonascii: 4096 187.0µs -> 175.7µs -6.0%
lower_the_middle_value_is_nonascii: 8192 373.2µs -> 360.0µs -3.5%
```
--
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: Add bulk NULL-aware string builders, use in `lower` and `upper` [datafusion]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich merged PR #21789:
URL: https://github.com/apache/datafusion/pull/21789
--
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: Add bulk NULL-aware string builders, use in `lower` and `upper` [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #21789:
URL: https://github.com/apache/datafusion/pull/21789#discussion_r3131975433
##########
datafusion/functions/src/string/common.rs:
##########
@@ -399,18 +410,28 @@ where
// Values contain non-ASCII.
let item_len = string_array.len();
- let capacity = string_array.value_data().len() + PRE_ALLOC_BYTES;
- let mut builder = GenericStringBuilder::<O>::with_capacity(item_len, capacity);
+ let capacity = value_data.len() + PRE_ALLOC_BYTES;
+ let nulls = string_array.nulls().cloned();
Review Comment:
Thanks, I added some comments to clarify this.
--
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: Add bulk NULL-aware string builders, use in `lower` and `upper` [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on PR #21789:
URL: https://github.com/apache/datafusion/pull/21789#issuecomment-4304556093
Thanks for the review @EeshanBembi !
> Nit: consider exposing an `append_null()` alias alongside `append_placeholder()`.
I don't agree with adding an alias; I'd prefer to pick one name or the other. `append_null` is what `StringBuilder` in Arrow does, but the semantics are a little different there -- in that case, `append_null` is *all* you need to do to append a null value. In this case, calling `append_placeholder` isn't sufficient to add a NULL row; you also need to pass a NULL bitmap with the corresponding bit set. I think using a name other than `append_null` makes this a bit more clear.
--
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: Add bulk NULL-aware string builders, use in `lower` and `upper` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #21789:
URL: https://github.com/apache/datafusion/pull/21789#issuecomment-4327440683
Should we be porting these changes upstream (to arrow-rs) I wonder 🤔
--
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: Add bulk NULL-aware string builders, use in `lower` and `upper` [datafusion]
Posted by "EeshanBembi (via GitHub)" <gi...@apache.org>.
EeshanBembi commented on code in PR #21789:
URL: https://github.com/apache/datafusion/pull/21789#discussion_r3130157145
##########
datafusion/functions/src/strings.rs:
##########
@@ -414,6 +430,223 @@ impl ConcatLargeStringBuilder {
}
}
+// ----------------------------------------------------------------------------
+// Bulk-nulls builders
+//
+// These builders are similar to Arrow's `GenericStringBuilder` and
+// `StringViewBuilder`, except that callers must pass the NULL bitmap to
+// `finish()`, rather than maintaining it iteratively (per-row). For callers
+// that can compute the NULL bitmap in bulk (which is true of many
+// string-related UDFs), this can be significantly more efficient.
+//
+// For a row known to be null, call `append_placeholder` to advance the row
+// count without touching the value buffer; the placeholder slot will be
+// masked by the caller-supplied null buffer.
+// ----------------------------------------------------------------------------
+
+/// Builder for a [`GenericStringArray<O>`] that defers null tracking to
+/// `finish`. Instantiate with `O = i32` for [`StringArray`] (Utf8) or
+/// `O = i64` for [`LargeStringArray`] (LargeUtf8).
+pub(crate) struct GenericStringArrayBuilder<O: OffsetSizeTrait> {
+ offsets_buffer: MutableBuffer,
+ value_buffer: MutableBuffer,
+ _phantom: PhantomData<O>,
+}
+
+impl<O: OffsetSizeTrait> GenericStringArrayBuilder<O> {
+ pub fn with_capacity(item_capacity: usize, data_capacity: usize) -> Self {
+ let capacity = item_capacity
+ .checked_add(1)
+ .map(|i| i.saturating_mul(size_of::<O>()))
+ .expect("capacity integer overflow");
+
+ let mut offsets_buffer = MutableBuffer::with_capacity(capacity);
+ offsets_buffer.push(O::usize_as(0));
+ Self {
+ offsets_buffer,
+ value_buffer: MutableBuffer::with_capacity(data_capacity),
+ _phantom: PhantomData,
+ }
+ }
+
+ /// Append `value` as the next row.
+ ///
+ /// # Panics
+ ///
+ /// Panics if the cumulative byte length exceeds `O::MAX`.
+ pub fn append_value(&mut self, value: &str) {
+ self.value_buffer.extend_from_slice(value.as_bytes());
+ let next_offset =
+ O::from_usize(self.value_buffer.len()).expect("byte array offset overflow");
+ self.offsets_buffer.push(next_offset);
+ }
+
+ /// Append an empty placeholder row. The corresponding slot is meaningful
+ /// only if the caller masks it via the null buffer passed to `finish`.
+ pub fn append_placeholder(&mut self) {
+ let next_offset =
+ O::from_usize(self.value_buffer.len()).expect("byte array offset overflow");
+ self.offsets_buffer.push(next_offset);
+ }
+
+ /// Finalize into a [`GenericStringArray<O>`] using the caller-supplied
+ /// null buffer.
+ ///
+ /// # Errors
+ ///
+ /// Returns an error when `null_buffer.len()` does not match the number of
+ /// appended rows.
+ pub fn finish(
+ self,
+ null_buffer: Option<NullBuffer>,
+ ) -> Result<GenericStringArray<O>> {
+ let row_count = self.offsets_buffer.len() / size_of::<O>() - 1;
+ if let Some(ref n) = null_buffer
+ && n.len() != row_count
+ {
+ return internal_err!(
+ "Null buffer length ({}) must match row count ({row_count})",
+ n.len()
+ );
+ }
+ let array_data = ArrayDataBuilder::new(GenericStringArray::<O>::DATA_TYPE)
+ .len(row_count)
+ .add_buffer(self.offsets_buffer.into())
+ .add_buffer(self.value_buffer.into())
+ .nulls(null_buffer);
+ // SAFETY: every appended value came from a `&str`, so the value
+ // buffer is valid UTF-8 and offsets are monotonically non-decreasing.
+ let array_data = unsafe { array_data.build_unchecked() };
+ Ok(GenericStringArray::<O>::from(array_data))
+ }
+}
+
+/// Starting size for the long-string data block; matches Arrow's
+/// `GenericByteViewBuilder` default.
+const STARTING_BLOCK_SIZE: u32 = 8 * 1024;
+/// Maximum size each long-string data block grows to; matches Arrow's
+/// `GenericByteViewBuilder` default.
+const MAX_BLOCK_SIZE: u32 = 2 * 1024 * 1024;
+
+/// Builder for a [`StringViewArray`] that defers null tracking to `finish`.
+///
+/// Modeled on Arrow's [`arrow::array::builder::StringViewBuilder`] but
+/// without per-row [`arrow::array::builder::NullBufferBuilder`] maintenance.
+/// Long strings (> 12 bytes) are appended into an in-progress data block;
+/// short strings are inlined into the view itself. When the in-progress block
+/// fills up it is flushed into `completed` and a new block — double the size
+/// of the last, capped at [`MAX_BLOCK_SIZE`] — is started.
+pub(crate) struct StringViewArrayBuilder {
+ views: Vec<u128>,
+ in_progress: Vec<u8>,
+ completed: Vec<Buffer>,
+ /// Current block-size target; doubles each time a block is flushed, up to
+ /// [`MAX_BLOCK_SIZE`].
+ block_size: u32,
+}
+
+impl StringViewArrayBuilder {
+ pub fn with_capacity(item_capacity: usize) -> Self {
+ Self {
+ views: Vec::with_capacity(item_capacity),
+ in_progress: Vec::new(),
+ completed: Vec::new(),
+ block_size: STARTING_BLOCK_SIZE,
+ }
+ }
+
+ /// Doubles the block-size target (capped at [`MAX_BLOCK_SIZE`]) and
+ /// returns the new size. The first call returns `2 * STARTING_BLOCK_SIZE`.
+ fn next_block_size(&mut self) -> u32 {
+ if self.block_size < MAX_BLOCK_SIZE {
+ self.block_size = self.block_size.saturating_mul(2);
+ }
+ self.block_size
+ }
+
+ /// Append `value` as the next row.
+ ///
+ /// # Panics
+ ///
+ /// Panics if the value length, the in-progress buffer offset, or the
+ /// number of completed buffers exceeds `i32::MAX`. The ByteView spec
+ /// uses signed 32-bit integers for these fields; exceeding `i32::MAX`
+ /// would produce an array that does not round-trip through Arrow IPC
+ /// (see <https://github.com/apache/arrow-rs/issues/6172>).
+ #[inline]
+ pub fn append_value(&mut self, value: &str) {
+ let v = value.as_bytes();
+ let length: u32 =
+ i32::try_from(v.len()).expect("value length exceeds i32::MAX") as u32;
+ if length <= 12 {
+ self.views.push(make_view(v, 0, 0));
+ return;
+ }
+
+ let required_cap = self.in_progress.len() + length as usize;
+ if self.in_progress.capacity() < required_cap {
+ self.flush_in_progress();
+ let to_reserve = (length as usize).max(self.next_block_size() as usize);
+ self.in_progress.reserve(to_reserve);
+ }
+
+ let buffer_index: u32 = i32::try_from(self.completed.len())
+ .expect("buffer count exceeds i32::MAX")
+ as u32;
+ let offset: u32 = i32::try_from(self.in_progress.len())
+ .expect("offset exceeds i32::MAX") as u32;
+ self.in_progress.extend_from_slice(v);
+ self.views.push(make_view(v, buffer_index, offset));
+ }
+
+ /// Append an empty placeholder row. The corresponding slot is meaningful
+ /// only if the caller masks it via the null buffer passed to `finish`.
+ #[inline]
+ pub fn append_placeholder(&mut self) {
Review Comment:
`append_placeholder()` pushes a zero-length view (`0u128`) that is byte-for-byte identical to a real empty-string view. If a caller ever calls `finish(None)` after having called `append_placeholder()`, those rows will materialise as `""` instead of `null` — silently wrong.
The current `case_conversion` usage always provides the input null buffer, so there's no bug today. But please either:
1. Add a `debug_assert!` in `finish()` that `null_buffer.is_some()` when any placeholder was appended (you can track `self.placeholder_count > 0`), **or**
2. Document in the `append_placeholder` doc-comment that callers **must** pass a valid null buffer to `finish()`.
Option 1 is safer as more UDFs adopt this builder.
##########
datafusion/functions/src/string/common.rs:
##########
@@ -399,18 +410,28 @@ where
// Values contain non-ASCII.
let item_len = string_array.len();
- let capacity = string_array.value_data().len() + PRE_ALLOC_BYTES;
- let mut builder = GenericStringBuilder::<O>::with_capacity(item_len, capacity);
+ let capacity = value_data.len() + PRE_ALLOC_BYTES;
+ let nulls = string_array.nulls().cloned();
Review Comment:
The code passes the *input* null buffer directly to the *output* builder:
```rust
let nulls = string_array.nulls().cloned();
// ... write output values for non-null inputs ...
builder.finish(nulls)?
```
This is correct for case conversion (null-in ↔ null-out), but the assumption isn't stated. Since more UDFs are planned to adopt this builder, a brief comment here will prevent future callers from reusing the pattern in non-null-preserving operations:
```rust
// Case conversion is null-preserving: input null ↔ output null,
// so we can pass the input null buffer directly to the output builder.
let nulls = string_array.nulls().cloned();
```
--
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: Add bulk NULL-aware string builders, use in `lower` and `upper` [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on PR #21789:
URL: https://github.com/apache/datafusion/pull/21789#issuecomment-4327500571
> Should we be porting these changes upstream (to arrow-rs) I wonder 🤔
Perhaps -- you'd have a better perspective on whether the arrow-rs folks would be interested. Probably to get the performance improvement you'd need to add another string builder API to Arrow, rather than extending the existing builders...
--
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: Add bulk NULL-aware string builders, use in `lower` and `upper` [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on PR #21789:
URL: https://github.com/apache/datafusion/pull/21789#issuecomment-4316542885
@mbutrovich Thanks, good idea! Done.
--
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