Cintu07 opened a new issue, #10983:
URL: https://github.com/apache/arrow-rs/issues/10983
## Describe the bug
`substring` takes `start: i64` and `length: Option<u64>`, 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::<i32>(),
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.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
