Re: [PR] Replace OffsetBufferBuilder with Vec in byte-array zip [arrow-rs]

["yashkuceriya (via GitHub)" <gi...@apache.org>]

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?
   



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org