fix(arrow-string): saturate the offset casts in substring - #10995
Conversation
substring takes start as i64 and length as 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. LargeUtf8 and LargeBinary narrow to i64 and were not affected, which meant the same call returned different data depending on the offset width of the input. Saturate instead. A start past the end of every value is what the caller asked for and what the 64 bit arms already do. The length cast was wrong on all four arms, not just 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. Inside byte_substring the two additions that can carry a saturated value past the offset type now go through checked_add and clamp to the end of the value. The third, pair[1] + start on the negative branch, cannot overflow, since pair[1] is non-negative and start is at worst i32::MIN.
|
run benchmark substring_kernels |
This comment was marked as duplicate.
This comment was marked as duplicate.
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing fix/substring-saturate-offset-cast (4136775) to b1aa94b (merge-base) diff Run configurationrun benchmark substring_kernelsCPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
i ran it on my machine too, aarch64. different rows moved for me. your +22% row came out +1.8%. instead by char so i ran the same branch again against the same baseline, no code change at all by char (non-ascii, prefix) +53% same code both times. so this bench can swing 50% on my box by itself. the row that runs the new branches is start = 1, length = str_len - 1. run one was my laptop is noisier than your runner, sure. but every by char row goes through if you would rather not have the extra branch in the loop at all, i can move the |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Which issue does this PR close?
Rationale for this change
substring takes start as i64 and length as Option 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_substringreads a negative start as counting from the end ofthe value, so the call did not fail, it quietly did something else. LargeUtf8 and
LargeBinary 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_substringthe two additions that can carry a saturated valuepast the offset type go through
checked_addand 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 i64is -1, and a negative length puts the end of asubstring before its start, which drives the output offsets negative and then
allocates on the result of as_usize. i only found it 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.