substring silently wraps start and length to i32 on Utf8 and Binary, so the same call returns different data than on LargeUtf8
- Dominant language
- Rust
- Stars
- 3.6k
- Forks
- 1.3k
- Avg merge
- 2d 18h
- Merged PRs (30d)
- 169
Description
## Describe the bug
`substring` takes `start: i64` and `length: Option`, and narrows both to `i32` with an unchecked cast on the `Utf8` and `Binary` arms (`arrow-string/src/substring.rs:87` and `:104`):
```rust
DataType::Utf8 => byte_substring(
array.as_string::(),
start as i32,
length.map(|e| e as i32),
),
```
a `start` at or above 2^31 wraps to a negative number, and `byte_substring` reads a negative start as counting from the end of the value rather than the front. so the call does not fail, it silently performs a different operation.
the `LargeUtf8` and `LargeBinary` arms narrow to `i64` instead and are unaffected, which makes the same call return different data depending only on the offset width of the input.
## To Reproduce
```rust
use arrow_array::{LargeStringArray, StringArray};
use arrow_string::substring::substring;
let utf8 = StringArray::from(vec![Some("hello"), Some("world")]);
let large = LargeStringArray::from(vec![Some("hello"), Some("world")]);
// skipping 2^31 characters of a 5 character string should give empty strings
let start: i64 = 1 << 31;
substring(&utf8, start, None) // -> ["hello", "world"]
substring(&large, start, None) // -> ["", ""]
```
```
start=2147483648 Utf8 -> [Some("hello"), Some("world")]
start=2147483648 LargeUtf8 -> [Some(""), Some("")]
start=3 Utf8 -> [Some("lo"), Some("ld")] (control)
```
the 64 bit path is right. the 32 bit path wraps to `i32::MIN`, takes the `Ordering::Less` arm at `substring.rs:361`, clamps to `pair[0]`, and returns the whole value.
`length` wraps the same way but surfaces differently. `substring(&utf8, 0, Some(1 << 31))` returns
```
Compute error: The offset 18446744071562067968 is at an invalid utf-8 boundary.
```
which is an error rather than bad data, but 18446744071562067968 is just -2147483648 read back as a `u64`, so the message points at nothing real.
## Expected behavior
the two offset widths should not disagree. either the out of range value is rejected, or the 32 bit arm saturates so it matches what the 64 bit arm already does.
i have not sent a patch yet because the choice matters and i would rather ask. rejecting with an `InvalidArgumentError` is the smaller change and matches how the width overflow in `concat_elements_fixed_size_binary` was handled in #10981, but it leaves `Utf8` erroring where `LargeUtf8` returns empty, so the two still differ. saturating makes them agree, but `pair[0] + start` inside `byte_substring` would then need checked arithmetic since `i32::MAX` plus an offset overflows on its own.
happy to send either. i found this looking for the same shape as #10972 after that one merged.
Contributor guide
Research direction
Read arrow-string/src/substring.rs at the Utf8, Binary, LargeUtf8, LargeBinary, and byte_substring paths, then compare the overflow handling discussed in #10981. Decide and document one consistent out-of-range behavior for both offset widths, including start and length, and add coverage showing the affected calls no longer disagree or report misleading values.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- data
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100