apache / apache/arrow-rs

substring silently wraps start and length to i32 on Utf8 and Binary, so the same call returns different data than on LargeUtf8

Open
#10,983 2 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.