Skip to content

Commit 3bee970

Browse files
authored
Migrate case conversion and substr_index to fallible string view builder APIs (#23074)
## Which issue does this PR close? * Part of #22688 ## Rationale for this change This PR continues the migration to fallible string builder APIs for string UDFs that previously relied on infallible `append_value` and `append_placeholder` calls. The non-ASCII case conversion paths in `string/common.rs` and the generic implementations in `unicode/substrindex.rs` could panic when string view metadata exceeds ByteView's `i32::MAX` limits. Converting these call sites to use fallible builder APIs allows overflow conditions to be propagated as `DataFusionError`s instead of causing panics. This work is part of the broader effort in #22688 to eliminate panic-based overflow handling in string builders. ## What changes are included in this PR? * Migrated non-ASCII case conversion paths in `string/common.rs` to use: * `try_append_value` * `try_append_placeholder` * Migrated `substr_index_general` and `map_strings` in `unicode/substrindex.rs` to use: * `try_append_value` * `try_append_placeholder` * Added fallible APIs to `StringViewArrayBuilder`: * `try_append_value` * `try_append_placeholder` * `try_ensure_long_capacity` * Added overflow error generation for StringView-specific limits: * `string_view_overflow_error` * Refactored StringView view construction to validate: * value length * buffer offsets * completed buffer count * Preserved existing infallible APIs (`append_value`, `append_placeholder`, `ensure_long_capacity`) as wrappers around the fallible implementations, maintaining existing behavior for callers that still use them. ## Are these changes tested? Yes. Added the following unit tests: * `string_view_builder_try_append_success_path` * `test_substr_index_all_nulls` These tests verify: * Successful operation of the new fallible StringView builder APIs. * Correct null propagation behavior in `substr_index_general`. Existing case conversion and `substr_index` tests continue to validate the normal execution paths. No dedicated overflow-triggering tests were added in this PR. ## Are there any user-facing changes? No user-facing functionality changes are expected. The primary behavioral change is that certain internal string-builder overflow conditions can now be returned as `DataFusionError`s rather than triggering panics, improving robustness in extreme cases. ## LLM-generated code disclosure This PR includes LLM-generated code and comments. All LLM-generated content has been manually reviewed.
1 parent 284ae30 commit 3bee970

3 files changed

Lines changed: 136 additions & 43 deletions

File tree

datafusion/functions/src/string/common.rs

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -370,18 +370,18 @@ fn case_conversion(
370370
if let Some(ref n) = nulls {
371371
for i in 0..item_len {
372372
if n.is_null(i) {
373-
builder.append_placeholder();
373+
builder.try_append_placeholder()?;
374374
} else {
375375
// SAFETY: `n.is_null(i)` was false in the branch above.
376376
let s = unsafe { string_array.value_unchecked(i) };
377-
builder.append_value(&unicode_case(s, lower));
377+
builder.try_append_value(&unicode_case(s, lower))?;
378378
}
379379
}
380380
} else {
381381
for i in 0..item_len {
382382
// SAFETY: no null buffer means every index is valid.
383383
let s = unsafe { string_array.value_unchecked(i) };
384-
builder.append_value(&unicode_case(s, lower));
384+
builder.try_append_value(&unicode_case(s, lower))?;
385385
}
386386
}
387387

@@ -431,18 +431,18 @@ fn case_conversion_array<O: OffsetSizeTrait>(
431431
if let Some(ref n) = nulls {
432432
for i in 0..item_len {
433433
if n.is_null(i) {
434-
builder.append_placeholder();
434+
builder.try_append_placeholder()?;
435435
} else {
436436
// SAFETY: `n.is_null(i)` was false in the branch above.
437437
let s = unsafe { string_array.value_unchecked(i) };
438-
builder.append_value(&unicode_case(s, lower));
438+
builder.try_append_value(&unicode_case(s, lower))?;
439439
}
440440
}
441441
} else {
442442
for i in 0..item_len {
443443
// SAFETY: no null buffer means every index is valid.
444444
let s = unsafe { string_array.value_unchecked(i) };
445-
builder.append_value(&unicode_case(s, lower));
445+
builder.try_append_value(&unicode_case(s, lower))?;
446446
}
447447
}
448448
Ok(Arc::new(builder.finish(nulls)?))

datafusion/functions/src/strings.rs

Lines changed: 105 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -339,6 +339,10 @@ fn offset_overflow_error<O: OffsetSizeTrait>() -> DataFusionError {
339339
)
340340
}
341341

342+
fn string_view_overflow_error(field: &str) -> DataFusionError {
343+
exec_datafusion_err!("byte array offset overflow: {field} exceeds i32::MAX")
344+
}
345+
342346
fn try_offset<O: OffsetSizeTrait>(len: usize) -> Result<O> {
343347
if len > O::MAX_OFFSET {
344348
return Err(offset_overflow_error::<O>());
@@ -684,57 +688,90 @@ impl StringViewArrayBuilder {
684688
self.block_size
685689
}
686690

687-
/// See [`BulkNullStringArrayBuilder::append_value`].
691+
/// Fallible variant of [`Self::append_value`].
688692
///
689-
/// # Panics
693+
/// # Errors
690694
///
691-
/// Panics if the value length, the in-progress buffer offset, or the
692-
/// number of completed buffers exceeds `i32::MAX`. The ByteView spec
693-
/// uses signed 32-bit integers for these fields; exceeding `i32::MAX`
694-
/// would produce an array that does not round-trip through Arrow IPC
695-
/// (see <https://github.com/apache/arrow-rs/issues/6172>).
695+
/// Returns an error if the value length, in-progress buffer offset, or
696+
/// number of completed buffers exceeds `i32::MAX`. The ByteView spec uses
697+
/// signed 32-bit integers for these fields; exceeding `i32::MAX` would
698+
/// produce an array that does not round-trip through Arrow IPC (see
699+
/// <https://github.com/apache/arrow-rs/issues/6172>).
696700
#[inline]
697-
pub fn append_value(&mut self, value: &str) {
701+
pub fn try_append_value(&mut self, value: &str) -> Result<()> {
698702
let v = value.as_bytes();
699-
let length: u32 =
700-
i32::try_from(v.len()).expect("value length exceeds i32::MAX") as u32;
703+
let length: u32 = i32::try_from(v.len())
704+
.map_err(|_| string_view_overflow_error("value length"))?
705+
as u32;
701706
if length <= 12 {
702707
self.views.push(make_view(v, 0, 0));
703-
return;
708+
return Ok(());
704709
}
705710

706-
let required_cap = self.in_progress.len() + length as usize;
707-
if self.in_progress.capacity() < required_cap {
708-
self.flush_in_progress();
709-
let to_reserve = (length as usize).max(self.next_block_size() as usize);
710-
#[expect(
711-
clippy::disallowed_methods,
712-
reason = "StringView's block size bounds growth, so reserve cannot overflow capacity arithmetically. This hot loop intentionally avoids the extra `try_reserve` checks. It remains subject to allocator failure/OOM, which must be managed externally."
713-
)]
714-
self.in_progress.reserve(to_reserve);
715-
}
711+
self.try_ensure_long_capacity(length)?;
716712

717713
let offset: u32 = i32::try_from(self.in_progress.len())
718-
.expect("offset exceeds i32::MAX") as u32;
714+
.map_err(|_| string_view_overflow_error("offset"))?
715+
as u32;
716+
let buffer_index: u32 = i32::try_from(self.completed.len())
717+
.map_err(|_| string_view_overflow_error("buffer count"))?
718+
as u32;
719719
self.in_progress.extend_from_slice(v);
720-
self.views.push(self.make_long_view(length, offset, v));
720+
self.views.push(Self::make_long_view_checked(
721+
length,
722+
buffer_index,
723+
offset,
724+
v,
725+
));
726+
Ok(())
727+
}
728+
729+
/// See [`BulkNullStringArrayBuilder::append_value`].
730+
///
731+
/// Note: new call sites that need recoverable overflow handling should
732+
/// prefer [`Self::try_append_value`].
733+
///
734+
/// # Panics
735+
///
736+
/// Panics under the same conditions that [`Self::try_append_value`] returns
737+
/// an error.
738+
#[inline]
739+
pub fn append_value(&mut self, value: &str) {
740+
self.try_append_value(value)
741+
.expect("byte array offset overflow");
742+
}
743+
744+
/// Fallible variant of [`Self::append_placeholder`].
745+
///
746+
/// # Errors
747+
///
748+
/// This currently cannot fail; it returns `Result` for API symmetry with
749+
/// other fallible append methods.
750+
#[inline]
751+
pub fn try_append_placeholder(&mut self) -> Result<()> {
752+
self.append_placeholder();
753+
Ok(())
721754
}
722755

723756
/// See [`BulkNullStringArrayBuilder::append_placeholder`].
757+
///
758+
/// Note: new call sites that need recoverable overflow handling should
759+
/// prefer [`Self::try_append_placeholder`].
724760
#[inline]
725761
pub fn append_placeholder(&mut self) {
726762
// Zero-length inline view — `length` field is 0, no buffer ref.
727763
self.views.push(0);
728764
self.placeholder_count += 1;
729765
}
730766

731-
/// Ensure the in-progress block has room for `length` more bytes,
732-
/// flushing the current block and starting a new (doubled) one if not.
733-
/// Caller must invoke this only when no bytes of the current row are
734-
/// yet in `in_progress` — flushing mid-row would orphan partial data.
767+
/// Fallible variant of [`Self::ensure_long_capacity`].
735768
#[inline]
736-
fn ensure_long_capacity(&mut self, length: u32) {
737-
let required_cap = self.in_progress.len() + length as usize;
769+
fn try_ensure_long_capacity(&mut self, length: u32) -> Result<()> {
770+
let required_cap = self
771+
.in_progress
772+
.len()
773+
.checked_add(length as usize)
774+
.ok_or_else(|| string_view_overflow_error("string view block size"))?;
738775
if self.in_progress.capacity() < required_cap {
739776
self.flush_in_progress();
740777
let to_reserve = (length as usize).max(self.next_block_size() as usize);
@@ -744,6 +781,17 @@ impl StringViewArrayBuilder {
744781
)]
745782
self.in_progress.reserve(to_reserve);
746783
}
784+
Ok(())
785+
}
786+
787+
/// Ensure the in-progress block has room for `length` more bytes,
788+
/// flushing the current block and starting a new (doubled) one if not.
789+
/// Caller must invoke this only when no bytes of the current row are
790+
/// yet in `in_progress` — flushing mid-row would orphan partial data.
791+
#[inline]
792+
fn ensure_long_capacity(&mut self, length: u32) {
793+
self.try_ensure_long_capacity(length)
794+
.expect("byte array offset overflow");
747795
}
748796

749797
/// Encode a long-form view referencing `length` bytes already written
@@ -754,10 +802,12 @@ impl StringViewArrayBuilder {
754802
/// function is `[inline(never)]` and has to handle short strings, so
755803
/// building the view here ourselves is faster.
756804
#[inline]
757-
fn make_long_view(&self, length: u32, offset: u32, prefix_bytes: &[u8]) -> u128 {
758-
let buffer_index: u32 = i32::try_from(self.completed.len())
759-
.expect("buffer count exceeds i32::MAX")
760-
as u32;
805+
fn make_long_view_checked(
806+
length: u32,
807+
buffer_index: u32,
808+
offset: u32,
809+
prefix_bytes: &[u8],
810+
) -> u128 {
761811
ByteView {
762812
length,
763813
// length > 12, so prefix_bytes has at least 4 bytes.
@@ -768,6 +818,14 @@ impl StringViewArrayBuilder {
768818
.into()
769819
}
770820

821+
#[inline]
822+
fn make_long_view(&self, length: u32, offset: u32, prefix_bytes: &[u8]) -> u128 {
823+
let buffer_index: u32 = i32::try_from(self.completed.len())
824+
.expect("buffer count exceeds i32::MAX")
825+
as u32;
826+
Self::make_long_view_checked(length, buffer_index, offset, prefix_bytes)
827+
}
828+
771829
/// See [`BulkNullStringArrayBuilder::append_byte_map`].
772830
///
773831
/// # Safety
@@ -1601,6 +1659,20 @@ mod tests {
16011659
);
16021660
}
16031661

1662+
#[test]
1663+
fn string_view_builder_try_append_success_path() {
1664+
let mut builder = StringViewArrayBuilder::with_capacity(3);
1665+
builder.try_append_value("abc").unwrap();
1666+
builder.try_append_placeholder().unwrap();
1667+
builder.try_append_value("a long string value").unwrap();
1668+
1669+
let nulls = Some(NullBuffer::from(vec![true, false, true]));
1670+
let array = builder.finish(nulls).unwrap();
1671+
assert_eq!(array.value(0), "abc");
1672+
assert!(array.is_null(1));
1673+
assert_eq!(array.value(2), "a long string value");
1674+
}
1675+
16041676
#[test]
16051677
fn generic_string_builder_try_offset_overflow() {
16061678
let err = try_offset::<i32>(i32::MAX as usize + 1)

datafusion/functions/src/unicode/substrindex.rs

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -281,15 +281,15 @@ where
281281

282282
for i in 0..num_rows {
283283
if nulls.as_ref().is_some_and(|n| n.is_null(i)) {
284-
builder.append_placeholder();
284+
builder.try_append_placeholder()?;
285285
continue;
286286
}
287287
// SAFETY: `i < num_rows` and the union of input nulls is valid at i,
288288
// so each input is also valid at i.
289289
let string = unsafe { string_array.value_unchecked(i) };
290290
let delimiter = unsafe { delimiter_array.value_unchecked(i) };
291291
let n = unsafe { count_array.value_unchecked(i) };
292-
builder.append_value(substr_index_slice(string, delimiter, n));
292+
builder.try_append_value(substr_index_slice(string, delimiter, n))?;
293293
}
294294

295295
Ok(Arc::new(builder.finish(nulls)?) as ArrayRef)
@@ -487,13 +487,13 @@ where
487487
let nulls = string_array.nulls().cloned();
488488
for i in 0..string_array.len() {
489489
if nulls.as_ref().is_some_and(|n| n.is_null(i)) {
490-
builder.append_placeholder();
490+
builder.try_append_placeholder()?;
491491
continue;
492492
}
493493
// SAFETY: `i < string_array.len()` and `nulls` is valid at i, so the
494494
// input is also valid at i.
495495
let s = unsafe { string_array.value_unchecked(i) };
496-
builder.append_value(f(s));
496+
builder.try_append_value(f(s))?;
497497
}
498498
Ok(Arc::new(builder.finish(nulls)?) as ArrayRef)
499499
}
@@ -797,6 +797,27 @@ mod tests {
797797
Ok(())
798798
}
799799

800+
#[test]
801+
fn test_substr_index_all_nulls() -> Result<()> {
802+
use super::substr_index_general;
803+
use crate::strings::GenericStringArrayBuilder;
804+
805+
let strings = StringArray::from(vec![None::<&str>, None]);
806+
let delimiters = StringArray::from(vec![None::<&str>, Some(".")]);
807+
let counts = Int64Array::from(vec![None, None]);
808+
809+
let result = substr_index_general(
810+
&strings,
811+
&delimiters,
812+
&counts,
813+
GenericStringArrayBuilder::<i32>::with_capacity(strings.len(), 0),
814+
)?;
815+
let result = result.as_string::<i32>();
816+
assert_eq!(result, &StringArray::from(vec![None::<&str>, None]));
817+
818+
Ok(())
819+
}
820+
800821
#[test]
801822
fn test_substr_index_utf8view_array_sliced() -> Result<()> {
802823
use super::substr_index_view;

0 commit comments

Comments
 (0)