yashkuceriya commented on code in PR #10913:
URL: https://github.com/apache/arrow-rs/pull/10913#discussion_r4011648941
##########
arrow-select/src/zip.rs:
##########
@@ -615,9 +617,13 @@ impl<T: ByteArrayType> BytesScalarImpl<T> {
.try_repeat_slice_n_times(falsy_val, false_repeat_count)
.map_err(|e| ArrowError::MemoryError(e.to_string()))?;
- for _ in 0..false_repeat_count {
- offset_buffer_builder.push_length(falsy_len)
- }
+ let start_offset = current_offset;
+ let added =
falsy_len.checked_mul(false_repeat_count).expect("overflow");
Review Comment:
Fixed in 1ec832648. The mixed-mask path now checks the total output size and
offset range before allocating. The `expect("overflow")` calls introduced in
this PR are gone; the run offsets are bounded by the validated total.
Added a regression test for an output exceeding `i32::MAX`, with the large
scalar on either side. It returns `OffsetOverflowError` without allocating the
2 GiB output. All 418 unit tests, 17 doc tests, Clippy, and formatting pass
locally.
The PR description now includes measurements from the existing zip
benchmark. The revised code remains faster than the upstream baseline in the
eight tested cases, but some long-value cases were slower than the previous
revision locally. Could you rerun the project benchmark on this commit to
confirm that tradeoff? GitHub also reports `action_required` for the new CI
runs; could you approve those runs?
--
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]
