builder: Error when concatenating binary arrays would exceed offset size #8252

mapleFU · 2025-08-29T19:15:34Z

Which issue does this PR close?

Closes compute::concat_batches might produces String exceeds i32 offsets #8247 .

Rationale for this change

When concat array, the final value might:

unwrap when adding
unlucky, some even not unwrap, leaving a negative offset at ending of offsets, causing coredump

What changes are included in this PR?

Prevent from offset here

Are these changes tested?

To add ( I don't know memory size would be too large for this?)

Are there any user-facing changes?

Breaking changes:

append_array now return Result<()>
OffsetSize trait change to have CheckedAdd

mapleFU · 2025-08-29T19:16:04Z

arrow-array/src/array/list_array.rs

 /// [`LargeStringArray`]: crate::array::LargeStringArray
-pub trait OffsetSizeTrait: ArrowNativeType + std::ops::AddAssign + Integer {
+pub trait OffsetSizeTrait:
+    ArrowNativeType + std::ops::AddAssign + Integer + num::CheckedAdd


I'm not so familiar with this so I don't know whether a num::CheckedAdd okay

I think it is ok in general, but this might be a "breaking API change" potentially if any downstream crates have implemented the OffsetSizeTrait

However, that seems unlikely so maybe it is ok

mapleFU · 2025-08-29T19:16:18Z

arrow-array/src/builder/generic_bytes_builder.rs

            //  and reserve the necessary capacity, it's still slower)
            let mut intermediate = Vec::with_capacity(offsets.len() - 1);

+            if shift.checked_add(&offsets[offsets.len() - 1]).is_none() {


This just check once using the last-offset

mapleFU · 2025-08-29T19:21:45Z

arrow-array/src/builder/generic_bytes_builder.rs

    /// (this means that underlying null values are copied as is).
    #[inline]
-    pub fn append_array(&mut self, array: &GenericByteArray<T>) {
+    pub fn append_array(&mut self, array: &GenericByteArray<T>) -> Result<(), ArrowError> {


Note:

// Shifting all the offsets let shift: T::Offset = self.next_offset() - offsets[0];

The self.next_offset() would read the length of binary builder, if:

There're k batches, and the batch before kth batch exceeds the offsets, when merge next array, self.next_offset() would raise error, causing a expect called in self.next_offset()

If final batch exceeds, it would leaving overflow offsets in intermediate

I believe this is also a public API change (and thus would have to wait for the next major release (scheduled for october)

https://docs.rs/arrow/latest/arrow/array/struct.GenericByteBuilder.html#method.append_array

It's ok to me, I'm using this patch in my own branch now

…pleFU/arrow-rs into enhance-append-handling-in-byte-array

mapleFU · 2025-09-03T07:29:07Z

@alamb would you mind take a look?

alamb · 2025-09-05T21:42:36Z

🤖 ./gh_compare_arrow.sh Benchmark Script Running
Linux aal-dev 6.14.0-1014-gcp #15~24.04.1-Ubuntu SMP Fri Jul 25 23:26:08 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing enhance-append-handling-in-byte-array (12a9ba8) to 3dcd23f diff
BENCH_NAME=concatenate_kernel
BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental --bench concatenate_kernel
BENCH_FILTER=
BENCH_BRANCH_NAME=enhance-append-handling-in-byte-array
Results will be posted here when complete

alamb

Thanks @mapleFU -- the code here looks great to me. I kicked off the benchmarks to make sure, but I don't expect any change.

I think the only potential concern is that these are API changes so we will have to be a bit diligent about when we merge them in

alamb · 2025-09-05T21:35:51Z

arrow-array/src/array/list_array.rs

 /// [`LargeStringArray`]: crate::array::LargeStringArray
-pub trait OffsetSizeTrait: ArrowNativeType + std::ops::AddAssign + Integer {
+pub trait OffsetSizeTrait:
+    ArrowNativeType + std::ops::AddAssign + Integer + num::CheckedAdd


I think it is ok in general, but this might be a "breaking API change" potentially if any downstream crates have implemented the OffsetSizeTrait

However, that seems unlikely so maybe it is ok

alamb · 2025-09-05T21:37:24Z

arrow-array/src/builder/generic_bytes_builder.rs

    /// (this means that underlying null values are copied as is).
    #[inline]
-    pub fn append_array(&mut self, array: &GenericByteArray<T>) {
+    pub fn append_array(&mut self, array: &GenericByteArray<T>) -> Result<(), ArrowError> {


I believe this is also a public API change (and thus would have to wait for the next major release (scheduled for october)

https://docs.rs/arrow/latest/arrow/array/struct.GenericByteBuilder.html#method.append_array

alamb · 2025-09-05T21:45:01Z

cc @rluvaton who I think contributed to this code recently. If you have a moment to review I would appreciate it

alamb · 2025-09-05T21:52:57Z

🤖: Benchmark completed

Details

group                                                          enhance-append-handling-in-byte-array    main
-----                                                          -------------------------------------    ----
concat 1024 arrays boolean 4                                   1.00     27.9±0.09µs        ? ?/sec      1.03     28.6±0.16µs        ? ?/sec
concat 1024 arrays i32 4                                       1.00     13.5±0.16µs        ? ?/sec      1.12     15.1±0.16µs        ? ?/sec
concat 1024 arrays str 4                                       1.00     54.3±0.75µs        ? ?/sec      1.01     54.9±0.33µs        ? ?/sec
concat boolean 1024                                            1.04    428.2±0.73ns        ? ?/sec      1.00    412.8±0.84ns        ? ?/sec
concat boolean 8192 over 100 arrays                            1.15     51.0±0.08µs        ? ?/sec      1.00     44.4±0.12µs        ? ?/sec
concat boolean nulls 1024                                      1.05    756.8±1.96ns        ? ?/sec      1.00    723.6±1.39ns        ? ?/sec
concat boolean nulls 8192 over 100 arrays                      1.14    109.5±0.22µs        ? ?/sec      1.00     96.1±0.23µs        ? ?/sec
concat fixed size lists                                        1.00   735.8±48.98µs        ? ?/sec      1.11   815.7±47.85µs        ? ?/sec
concat i32 1024                                                1.00    390.3±4.03ns        ? ?/sec      1.01    394.5±1.05ns        ? ?/sec
concat i32 8192 over 100 arrays                                1.04    210.8±8.55µs        ? ?/sec      1.00    202.7±4.38µs        ? ?/sec
concat i32 nulls 1024                                          1.01    708.7±1.80ns        ? ?/sec      1.00    700.5±1.68ns        ? ?/sec
concat i32 nulls 8192 over 100 arrays                          1.00    266.7±5.53µs        ? ?/sec      1.05    279.8±9.08µs        ? ?/sec
concat str 1024                                                1.00     14.3±1.12µs        ? ?/sec      1.12     16.0±0.88µs        ? ?/sec
concat str 8192 over 100 arrays                                1.01    106.8±0.91ms        ? ?/sec      1.00    106.2±0.83ms        ? ?/sec
concat str nulls 1024                                          1.00      7.1±0.74µs        ? ?/sec      1.03      7.3±0.68µs        ? ?/sec
concat str nulls 8192 over 100 arrays                          1.00     54.3±0.51ms        ? ?/sec      1.00     54.2±0.49ms        ? ?/sec
concat str_dict 1024                                           1.00      2.8±0.02µs        ? ?/sec      1.06      3.0±0.02µs        ? ?/sec
concat str_dict_sparse 1024                                    1.00      6.8±0.04µs        ? ?/sec      1.02      7.0±0.02µs        ? ?/sec
concat struct with int32 and dicts size=1024 count=2           1.00      6.9±0.12µs        ? ?/sec      1.06      7.3±0.03µs        ? ?/sec
concat utf8_view  max_str_len=128 null_density=0               1.01     78.0±1.07µs        ? ?/sec      1.00     77.6±0.71µs        ? ?/sec
concat utf8_view  max_str_len=128 null_density=0.2             1.01     84.4±0.70µs        ? ?/sec      1.00     83.4±0.94µs        ? ?/sec
concat utf8_view  max_str_len=20 null_density=0                1.00     77.6±0.49µs        ? ?/sec      1.14     88.6±0.41µs        ? ?/sec
concat utf8_view  max_str_len=20 null_density=0.2              1.00     84.5±1.11µs        ? ?/sec      1.12     94.4±0.41µs        ? ?/sec
concat utf8_view all_inline max_str_len=12 null_density=0      1.00     39.8±3.86µs        ? ?/sec      1.17     46.5±2.88µs        ? ?/sec
concat utf8_view all_inline max_str_len=12 null_density=0.2    1.00     49.5±4.31µs        ? ?/sec      1.05     51.9±2.30µs        ? ?/sec

alamb · 2025-09-06T09:40:36Z

🤖 ./gh_compare_arrow.sh Benchmark Script Running
Linux aal-dev 6.14.0-1014-gcp #15~24.04.1-Ubuntu SMP Fri Jul 25 23:26:08 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing enhance-append-handling-in-byte-array (12a9ba8) to 3dcd23f diff
BENCH_NAME=concatenate_kernel
BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental --bench concatenate_kernel
BENCH_FILTER=
BENCH_BRANCH_NAME=enhance-append-handling-in-byte-array
Results will be posted here when complete

alamb · 2025-09-06T09:50:38Z

🤖: Benchmark completed

Details

group                                                          enhance-append-handling-in-byte-array    main
-----                                                          -------------------------------------    ----
concat 1024 arrays boolean 4                                   1.00     27.9±0.18µs        ? ?/sec      1.02     28.6±0.06µs        ? ?/sec
concat 1024 arrays i32 4                                       1.00     13.5±0.10µs        ? ?/sec      1.14     15.4±0.03µs        ? ?/sec
concat 1024 arrays str 4                                       1.00     54.7±0.60µs        ? ?/sec      1.01     55.1±0.62µs        ? ?/sec
concat boolean 1024                                            1.10    436.4±3.03ns        ? ?/sec      1.00    395.4±0.63ns        ? ?/sec
concat boolean 8192 over 100 arrays                            1.15     51.0±0.43µs        ? ?/sec      1.00     44.4±0.09µs        ? ?/sec
concat boolean nulls 1024                                      1.08    770.4±4.87ns        ? ?/sec      1.00    711.7±0.75ns        ? ?/sec
concat boolean nulls 8192 over 100 arrays                      1.14    109.9±0.94µs        ? ?/sec      1.00     96.1±0.14µs        ? ?/sec
concat fixed size lists                                        1.00   728.4±18.06µs        ? ?/sec      1.04   759.9±48.69µs        ? ?/sec
concat i32 1024                                                1.00    391.1±0.74ns        ? ?/sec      1.05    411.4±1.36ns        ? ?/sec
concat i32 8192 over 100 arrays                                1.11    228.1±3.29µs        ? ?/sec      1.00    204.8±6.84µs        ? ?/sec
concat i32 nulls 1024                                          1.00    708.9±9.63ns        ? ?/sec      1.03   733.5±13.59ns        ? ?/sec
concat i32 nulls 8192 over 100 arrays                          1.08    297.1±2.57µs        ? ?/sec      1.00    275.2±8.93µs        ? ?/sec
concat str 1024                                                1.00     14.5±1.14µs        ? ?/sec      1.02     14.8±1.02µs        ? ?/sec
concat str 8192 over 100 arrays                                1.00    104.2±0.99ms        ? ?/sec      1.00    104.5±0.58ms        ? ?/sec
concat str nulls 1024                                          1.00      7.0±1.08µs        ? ?/sec      1.04      7.3±0.61µs        ? ?/sec
concat str nulls 8192 over 100 arrays                          1.00     52.6±0.60ms        ? ?/sec      1.00     52.8±0.74ms        ? ?/sec
concat str_dict 1024                                           1.00      2.8±0.01µs        ? ?/sec      1.04      2.9±0.01µs        ? ?/sec
concat str_dict_sparse 1024                                    1.00      6.8±0.03µs        ? ?/sec      1.02      7.0±0.01µs        ? ?/sec
concat struct with int32 and dicts size=1024 count=2           1.00      6.7±0.17µs        ? ?/sec      1.04      6.9±0.18µs        ? ?/sec
concat utf8_view  max_str_len=128 null_density=0               1.00     77.2±0.25µs        ? ?/sec      1.00     77.4±0.29µs        ? ?/sec
concat utf8_view  max_str_len=128 null_density=0.2             1.01     84.0±0.42µs        ? ?/sec      1.00     83.2±0.37µs        ? ?/sec
concat utf8_view  max_str_len=20 null_density=0                1.00     77.2±0.57µs        ? ?/sec      1.15     88.9±0.28µs        ? ?/sec
concat utf8_view  max_str_len=20 null_density=0.2              1.00     83.4±0.53µs        ? ?/sec      1.14     94.7±0.48µs        ? ?/sec
concat utf8_view all_inline max_str_len=12 null_density=0      1.00     46.1±2.76µs        ? ?/sec      1.01     46.6±2.65µs        ? ?/sec
concat utf8_view all_inline max_str_len=12 null_density=0.2    1.00     51.0±2.31µs        ? ?/sec      1.04     53.2±2.55µs        ? ?/sec

rluvaton · 2025-09-06T20:16:23Z

cc @rluvaton who I think contributed to this code recently. If you have a moment to review I would appreciate it

looking

rluvaton

LGTM, thank you

rluvaton · 2025-09-06T20:24:10Z

this contain breaking changes, could you please update the pr description to reflect that (there are 2 breaking changes, append_array now return different types and offset size trait change.

also the pr name should be byte array rather than binary array, but this is just nitpick

mapleFU · 2025-09-07T06:17:27Z

done

…ng-in-byte-array

alamb · 2025-09-25T20:54:52Z

Thank you for your patience @mapleFU

Trying to fix binary array offset exceeds when building

e80b4ac

github-actions bot added the arrow Changes to the arrow crate label Aug 29, 2025

mapleFU commented Aug 29, 2025

View reviewed changes

mapleFU requested a review from alamb August 29, 2025 19:16

mapleFU commented Aug 29, 2025

View reviewed changes

mapleFU added 4 commits August 30, 2025 03:32

add test

9e6dc12

Merge branch 'main' into enhance-append-handling-in-byte-array

5193388

fix ci

7631518

Merge branch 'enhance-append-handling-in-byte-array' of github.com:ma…

12a9ba8

…pleFU/arrow-rs into enhance-append-handling-in-byte-array

alamb reviewed Sep 5, 2025

View reviewed changes

alamb added api-change Changes to the arrow API next-major-release the PR has API changes and it waiting on the next major version labels Sep 5, 2025

alamb changed the title ~~builder: Trying to fix binary array offset exceeds when building~~ builder: Error when concatenating binary arrays would exceed offset size Sep 5, 2025

alamb approved these changes Sep 5, 2025

View reviewed changes

rluvaton approved these changes Sep 6, 2025

View reviewed changes

mbrobbel added this to the 57.0.0 milestone Sep 15, 2025

alamb approved these changes Sep 25, 2025

View reviewed changes

Merge remote-tracking branch 'apache/main' into enhance-append-handli…

7457591

…ng-in-byte-array

alamb merged commit 28c7c52 into apache:main Sep 25, 2025
26 checks passed

builder: Error when concatenating binary arrays would exceed offset size #8252

builder: Error when concatenating binary arrays would exceed offset size #8252

Uh oh!

Conversation

mapleFU commented Aug 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mapleFU commented Sep 3, 2025

Uh oh!

alamb commented Sep 5, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Sep 5, 2025

Uh oh!

alamb commented Sep 5, 2025

Uh oh!

alamb commented Sep 6, 2025

Uh oh!

alamb commented Sep 6, 2025

Uh oh!

rluvaton commented Sep 6, 2025

Uh oh!

rluvaton left a comment

Choose a reason for hiding this comment

Uh oh!

rluvaton commented Sep 6, 2025

Uh oh!

mapleFU commented Sep 7, 2025

Uh oh!

Uh oh!

alamb commented Sep 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mapleFU commented Aug 29, 2025 •

edited

Loading