Cintu07 opened a new pull request, #10995: URL: https://github.com/apache/arrow-rs/pull/10995
# Which issue does this PR close? - Closes #10983. # Rationale for this change `substring` takes `start: i64` and `length: Option<u64>` and cast both straight into the offset type. on the `Utf8` and `Binary` arms a start at or above 2^31 wrapped negative, and `byte_substring` reads a negative start as counting from the end of the value, so the call did not fail, it quietly did something else. the `LargeUtf8` and `LargeBinary` arms narrow to i64 and were not affected, so the same call returned different data depending only on the offset width of the input. saturating rather than rejecting, per the discussion on the issue. a start past the end of every value is what the caller asked for, and it is already what the 64 bit arms do. # What changes are included in this PR? - `start` and `length` saturate into the offset type at the dispatch instead of being cast - inside `byte_substring`, the two additions that can carry a saturated value past the offset type go through `checked_add` and clamp to the end of the value one thing i did not expect. the `length` cast was wrong on all four arms, not only the 32 bit ones. `u64::MAX as i64` is -1, and a negative length puts the end of a substring before its start, which drives the output offsets negative and then allocates on the result of `as_usize`. i only found that because the test compares the narrow and wide arms against each other, and the wide one panicked. so `LargeUtf8` and `LargeBinary` are fixed here too. the third addition, `pair[1] + start` on the negative branch, is left alone. `pair[1]` is non-negative and `start` is at worst `i32::MIN`, so it cannot overflow. # Are these changes tested? yes. `out_of_range_start_and_length_match_the_64_bit_arms` runs four out of range starts against three lengths, on `Utf8` against `LargeUtf8` and on `Binary` against `LargeBinary`, and asserts the pairs agree. it also pins the answer itself, since agreeing on the wrong result would still pass: skipping 2^31 characters of a five character string gives empty strings, and a start that already fits is untouched. on current main that test fails twice. `Utf8` returns `["hello", "world"]` where `LargeUtf8` returns `["", ""]`, and `Some(u64::MAX)` panics inside `MutableBuffer`. `arrow-string` is 189 passed, and fmt and clippy with `-D warnings` are clean. # Are there any user-facing changes? yes, for input that was previously wrong. a `start` or `length` outside the offset type now saturates, so `Utf8` and `Binary` return what `LargeUtf8` and `LargeBinary` already returned. values that fit are unaffected. -- 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]
