This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-11323-a95308bd83b1fb7f3f7f4159fc7f5eef7fa69a08 in repository https://gitbox.apache.org/repos/asf/arrow-rs.git
commit cac30c2d586bde9b3c13a3f1166ddba4fbf87cd1 Author: Neil Conway <[email protected]> AuthorDate: Sun Oct 4 03:41:18 2026 +0000 fix: Only require valid UTF-8 within the visible span of string arrays (#11323) # Which issue does this PR close? - N/A # Rationale for this change Arrow does not require bytes outside the visible span of the values buffer to be valid UTF-8. This rule was not consistently applied: * `ArrayData` checked for UTF-8 validity over the visible span, but * `StringArray::try_new` and `try_from_binary` checked for UTF-8 validity of the entire values buffer, which is both inefficient for sliced arrays and also rejects otherwise valid inputs * `substring` assumed that the entire values buffer contained valid UTF-8 bytes, which was sound for inputs from the `StringArray` path but not for the `ArrayData` path * Binary to Utf8 casts (in `safe: true`) reserved enough space for the whole values buffer, not the visible span # What changes are included in this PR? * Both validators now check only the bytes between the first and last offset * `substring` reads only those bytes as a `&str`. * The `safe: true` fallback of the Binary to Utf8 cast only reserves space for the visible span, not the whole values buffer. * Improve some docs * Add unit tests # Are these changes tested? Yes; existing tests pass, new tests added. # Are there any user-facing changes? No. # AI usage Developed with Claude Code (Opus 5.5), reviewed with Codex (Astra 6). I revised and understand the resulting code. --- arrow-array/src/array/byte_array.rs | 7 ++++++- arrow-array/src/array/string_array.rs | 23 +++++++++++++++++++++++ arrow-array/src/types.rs | 25 ++++++++++++++----------- arrow-cast/src/cast/mod.rs | 28 ++++++++++++++++++++++++++++ arrow-cast/src/cast/string.rs | 8 +++++--- arrow-data/src/data.rs | 33 ++++++++++++++++++++++++++------- arrow-string/src/substring.rs | 20 ++++++++++++++++---- 7 files changed, 118 insertions(+), 26 deletions(-) diff --git a/arrow-array/src/array/byte_array.rs b/arrow-array/src/array/byte_array.rs index cd0b9f6358..049d1f47cf 100644 --- a/arrow-array/src/array/byte_array.rs +++ b/arrow-array/src/array/byte_array.rs @@ -282,13 +282,18 @@ impl<T: ByteArrayType> GenericByteArray<T> { /// Returns the values of this array /// /// Unlike [`Self::value_data`] this returns the [`Buffer`] - /// allowing for zero-copy cloning + /// allowing for zero-copy cloning. Like [`Self::value_data`], it can + /// include bytes that are not part of any value. #[inline] pub fn values(&self) -> &Buffer { &self.value_data } /// Returns the raw value data + /// + /// This can include bytes before the first offset or after the last one, + /// for example when the array is a slice of a larger array. Those bytes are + /// not part of any value, and in a string array they need not be valid UTF-8. pub fn value_data(&self) -> &[u8] { self.value_data.as_slice() } diff --git a/arrow-array/src/array/string_array.rs b/arrow-array/src/array/string_array.rs index c98b2a1b2a..39427181d2 100644 --- a/arrow-array/src/array/string_array.rs +++ b/arrow-array/src/array/string_array.rs @@ -504,6 +504,29 @@ mod tests { let _ = StringArray::from(list); } + #[test] + fn test_string_array_invalid_bytes_outside_offsets() { + // Only the bytes that the offsets span need to be valid UTF-8, as in a + // slice of a larger array + let values = Buffer::from_slice_ref(b"\xFFa\xC3\xA9\xFF"); + let offsets = OffsetBuffer::new(vec![1, 2, 4].into()); + let string = StringArray::try_new(offsets, values.clone(), None).unwrap(); + assert_eq!(string, StringArray::from(vec!["a", "é"])); + + let offsets = OffsetBuffer::new(vec![1, 3, 4].into()); + let err = StringArray::try_new(offsets, values, None).unwrap_err(); + assert_eq!( + err.to_string(), + "Invalid argument error: Split UTF-8 codepoint at offset 3" + ); + + // An empty value whose offset is inside a character, in a values buffer + // that is valid UTF-8 as a whole. ArrayData validation must agree. + let offsets = OffsetBuffer::new(vec![1, 1].into()); + let string = StringArray::try_new(offsets, Buffer::from_slice_ref("é"), None).unwrap(); + string.to_data().validate_full().unwrap(); + } + #[test] fn test_empty_offsets() { let string = StringArray::from( diff --git a/arrow-array/src/types.rs b/arrow-array/src/types.rs index ba0d88ae64..c7f2d51989 100644 --- a/arrow-array/src/types.rs +++ b/arrow-array/src/types.rs @@ -1686,23 +1686,26 @@ impl<O: OffsetSizeTrait> ByteArrayType for GenericStringType<O> { }; fn validate(offsets: &OffsetBuffer<Self::Offset>, values: &Buffer) -> Result<(), ArrowError> { - // Verify that the slice as a whole is valid UTF-8 - let validated = std::str::from_utf8(values).map_err(|e| { + let start = offsets.first().as_usize(); + let end = offsets.last().as_usize(); + if end > values.len() { + return Err(ArrowError::InvalidArgumentError(format!( + "Offset of {end} exceeds length of values {}", + values.len() + ))); + } + + // Verify that the bytes the offsets span are valid UTF-8 + let validated = std::str::from_utf8(&values[start..end]).map_err(|e| { ArrowError::InvalidArgumentError(format!("Encountered non UTF-8 data: {e}")) })?; - // Verify each offset is at a valid character boundary in this UTF-8 array + // Verify each offset is at a valid character boundary for offset in offsets.iter() { let o = offset.as_usize(); - if !validated.is_char_boundary(o) { - if o < validated.len() { - return Err(ArrowError::InvalidArgumentError(format!( - "Split UTF-8 codepoint at offset {o}" - ))); - } + if !validated.is_char_boundary(o - start) { return Err(ArrowError::InvalidArgumentError(format!( - "Offset of {o} exceeds length of values {}", - validated.len() + "Split UTF-8 codepoint at offset {o}" ))); } } diff --git a/arrow-cast/src/cast/mod.rs b/arrow-cast/src/cast/mod.rs index cd858859ea..3d7fb88c79 100644 --- a/arrow-cast/src/cast/mod.rs +++ b/arrow-cast/src/cast/mod.rs @@ -12465,6 +12465,34 @@ mod tests { assert_eq!(a.value(1), "\x00 Foo"); } + #[test] + fn test_cast_sliced_binary_with_hidden_invalid_utf8() { + // The invalid first value is outside the slice, so it must not fail the cast + let values: Vec<&[u8]> = vec![b"\xFF", b"a", b"b", b"c", b"d", b"e", b"f"]; + let arrays: [ArrayRef; 2] = [ + Arc::new(BinaryArray::from(values.clone()).slice(1, 6)), + Arc::new(LargeBinaryArray::from(values).slice(1, 6)), + ]; + let options = CastOptions { + safe: false, + format_options: FormatOptions::default(), + }; + let expected = StringArray::from(vec!["a", "b", "c", "d", "e", "f"]); + for array in &arrays { + for to_type in [DataType::Utf8, DataType::LargeUtf8, DataType::Utf8View] { + let result = cast_with_options(array, &to_type, &options).unwrap(); + assert_eq!(result.as_ref(), cast(&expected, &to_type).unwrap().as_ref()); + } + + // A dictionary with fewer keys than half its values takes a separate + // path to Utf8View + let keys = Int32Array::from(vec![5, 0]); + let dict = DictionaryArray::try_new(keys, Arc::clone(array)).unwrap(); + let result = cast_with_options(&dict, &DataType::Utf8View, &options).unwrap(); + assert_eq!(result.as_ref(), &StringViewArray::from(vec!["f", "a"])); + } + } + #[test] fn test_cast_utf8_to_timestamptz() { let valid = StringArray::from(vec!["2023-01-01"]); diff --git a/arrow-cast/src/cast/string.rs b/arrow-cast/src/cast/string.rs index 8d6255f151..c193eacc6e 100644 --- a/arrow-cast/src/cast/string.rs +++ b/arrow-cast/src/cast/string.rs @@ -369,9 +369,11 @@ pub(crate) fn cast_binary_to_string<O: OffsetSizeTrait>( Ok(a) => Ok(Arc::new(a)), Err(e) => match cast_options.safe { true => { - // Fallback to slow method to convert invalid sequences to nulls - let mut builder = - GenericStringBuilder::<O>::with_capacity(array.len(), array.value_data().len()); + // Fallback to slow method to convert invalid sequences to nulls. + // Reserve only the bytes the offsets span, not the whole values buffer. + let offsets = array.offsets(); + let data_len = (offsets.last() - offsets.first()).as_usize(); + let mut builder = GenericStringBuilder::<O>::with_capacity(array.len(), data_len); extend_valid_utf8(&mut builder, array.iter()); Ok(Arc::new(builder.finish())) diff --git a/arrow-data/src/data.rs b/arrow-data/src/data.rs index 7404808a16..c2f32ef9d0 100644 --- a/arrow-data/src/data.rs +++ b/arrow-data/src/data.rs @@ -272,6 +272,19 @@ fn checked_len_plus_offset( }) } +/// Returns the first and last of `offsets`, and the bytes of `values` between +/// them as a `str`. Returns `None` if the offsets are not valid indexes into +/// `values` or those bytes are not valid UTF-8. +fn utf8_span<'a, T: ArrowNativeType>( + offsets: &[T], + values: &'a [u8], +) -> Option<(usize, usize, &'a str)> { + let first = offsets.first()?.to_usize()?; + let last = offsets.last()?.to_usize()?; + let values_str = std::str::from_utf8(values.get(first..last)?).ok()?; + Some((first, last, values_str)) +} + impl ArrayData { /// Create a new ArrayData instance; /// @@ -1696,18 +1709,24 @@ impl ArrayData { } /// Ensures that all strings formed by the offsets in `buffers[0]` - /// into `buffers[1]` are valid utf8 sequences + /// into `buffers[1]` are valid utf8 sequences. Bytes of `buffers[1]` + /// outside the range of the offsets are not checked. fn validate_utf8<T>(&self) -> Result<(), ArrowError> where T: ArrowNativeType + TryInto<usize> + num_traits::Num + std::fmt::Display, { let values_buffer = &self.buffer_at(1)?.as_slice(); - if let Ok(values_str) = std::str::from_utf8(values_buffer) { - // Validate Offsets are correct + let offsets = self.typed_offsets::<T>()?; + if let Some((first, last, values_str)) = utf8_span(offsets, values_buffer) { self.validate_each_offset::<T, _>(values_buffer.len(), |string_index, range| { - if !values_str.is_char_boundary(range.start) - || !values_str.is_char_boundary(range.end) - { + // `values_str` ends at the last offset. Checking each pair of offsets doesn't + // show that `range.end <= last`: a later offset can still be smaller, which + // `validate_each_offset` reports when it gets there. + if range.end > last { + return Ok(()); + } + let (start, end) = (range.start - first, range.end - first); + if !values_str.is_char_boundary(start) || !values_str.is_char_boundary(end) { return Err(ArrowError::InvalidArgumentError(format!( "incomplete utf-8 byte sequence from index {string_index}" ))); @@ -1715,7 +1734,7 @@ impl ArrayData { Ok(()) }) } else { - // find specific offset that failed utf8 validation + // Find specific offset that failed utf8 validation self.validate_each_offset::<T, _>(values_buffer.len(), |string_index, range| { std::str::from_utf8(&values_buffer[range.clone()]).map_err(|e| { ArrowError::InvalidArgumentError(format!( diff --git a/arrow-string/src/substring.rs b/arrow-string/src/substring.rs index 82171b5a1d..dfc10e099c 100644 --- a/arrow-string/src/substring.rs +++ b/arrow-string/src/substring.rs @@ -350,14 +350,17 @@ where // When array is [Large]StringArray, we will check whether `offset` is at a valid char boundary. let check_char_boundary = { - |offset: T::Offset| { + let first = array.offsets().first().as_usize(); + let last = array.offsets().last().as_usize(); + move |offset: T::Offset| { if !matches!(T::DATA_TYPE, DataType::Utf8 | DataType::LargeUtf8) { return Ok(offset); } - // Safety: a StringArray must contain valid UTF8 data - let data_str = unsafe { std::str::from_utf8_unchecked(data) }; + // Safety: in a StringArray, the bytes between the first and last offsets + // are valid UTF-8. The bytes outside that range need not be. + let data_str = unsafe { std::str::from_utf8_unchecked(&data[first..last]) }; let offset_usize = offset.as_usize(); - if data_str.is_char_boundary(offset_usize) { + if data_str.is_char_boundary(offset_usize - first) { Ok(offset) } else { Err(ArrowError::ComputeError(format!( @@ -1129,6 +1132,15 @@ mod tests { assert!(err.contains("invalid utf-8 boundary")); } + #[test] + fn check_invalid_bytes_outside_offsets() { + // The values buffer holds invalid UTF-8 before and after the visible values + let values = Buffer::from_slice_ref(b"\xFFab\x80"); + let array = StringArray::new(OffsetBuffer::new(vec![1, 3].into()), values, None); + let result = substring(&array, 0, Some(5)).unwrap(); + assert_eq!(result.as_string::<i32>(), &StringArray::from(vec!["ab"])); + } + #[test] fn non_utf8_bytes() { // non-utf8 bytes
