You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "neilconway (via GitHub)" <gi...@apache.org> on 2026/10/01 17:40:48 UTC
[PR] fix: Only require valid UTF-8 within the offset range of string arrays [arrow-rs]
neilconway opened a new pull request, #11323:
URL: https://github.com/apache/arrow-rs/pull/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 also reserves only the bytes the offsets span, not the whole values buffer.
* 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.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: Only require valid UTF-8 within the visible span of string arrays [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey merged PR #11323:
URL: https://github.com/apache/arrow-rs/pull/11323
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: Only require valid UTF-8 within the visible span of string arrays [arrow-rs]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #11323:
URL: https://github.com/apache/arrow-rs/pull/11323#discussion_r4166668665
##########
arrow-data/src/data.rs:
##########
@@ -1707,26 +1720,32 @@ 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 {
Review Comment:
No, we should be okay -- the issue is that `validate_each_offset` validates prior offsets, but here we need to check the "next" / "last" offset. As I mentioned above, I think this whole approach merits refactoring, but I'll do that in a separate PR.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: Only require valid UTF-8 within the visible span of string arrays [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #11323:
URL: https://github.com/apache/arrow-rs/pull/11323#discussion_r4178026453
##########
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();
Review Comment:
Thank you for checking
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: Only require valid UTF-8 within the visible span of string arrays [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #11323:
URL: https://github.com/apache/arrow-rs/pull/11323#discussion_r4169921348
##########
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();
Review Comment:
are we sure there is no way to "unslice" an array (or take the buffer and then put the array back together and skip the validation)?
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: Only require valid UTF-8 within the visible span of string arrays [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #11323:
URL: https://github.com/apache/arrow-rs/pull/11323#discussion_r4162425373
##########
arrow-data/src/data.rs:
##########
@@ -1707,26 +1720,32 @@ 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 {
Review Comment:
would we need to do a similar check for `range.start`?
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: Only require valid UTF-8 within the visible span of string arrays [arrow-rs]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #11323:
URL: https://github.com/apache/arrow-rs/pull/11323#discussion_r4170260152
##########
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();
Review Comment:
That should only be possible with unsafe code (i.e., where the caller can somehow guarantee that bytes outside the visible span are well-defined and valid UTF-8). That was true before this PR (e.g., ArrayData did not validate bytes outside the visible span) and is consistent with the general Arrow philosophy of allowing arbitrary data to appear outside the visible span (similarly to how null slots can contain arbitrary data).
I had Claude look for a way to do that with safe code today and it didn't find a way to do it -- but if such a way does exist, I think it would clearly be buggy code.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: Only require valid UTF-8 within the visible span of string arrays [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #11323:
URL: https://github.com/apache/arrow-rs/pull/11323#issuecomment-5976241583
thanks @neilconway & @alamb
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: Only require valid UTF-8 within the offset range of string arrays [arrow-rs]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #11323:
URL: https://github.com/apache/arrow-rs/pull/11323#discussion_r4158567132
##########
arrow-data/src/data.rs:
##########
@@ -1707,26 +1720,32 @@ 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(());
+ }
Review Comment:
This strikes me as awkward; probably how we do validation in general here could be both refactored and optimized. But I'll defer that to a subsequent PR.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org