concat_elements_utf8view panics with large buffer on 64bit machines

@comphead

Describe the bug

It is possible to cause a panic in Datafusion on 64-bit machines. Datafusion does not handle the panic caused by the underlying append_value method in the GenericByteViewBuilder. (See affected line)

See the To Reproduce section for the recursive concat query.

Few notes/thoughts:

The panic is reproducible in the latest version of Data Fusion, but it was also there in the previous versions.
The panic originates in arrow-rs as on 64-bit systems, usize can be up to u64::MAX, but everything is assumed to be u32::MAX. The panic is already declared in the doc of the append_value method, but never handled by the consumer (DataFusion). Is there a reason for that?
I understand the panic happens in arrow-rs, but should datafusion handle the panic coming from Arrow? (e.g. in the append_value call of concat_elements_utf8view) to prevent the panic from happening
Should datafusion limit somehow the append_value calls to prevent the panic from happening?

cc @comphead

To Reproduce

Sample repository: https://github.com/samueleresca/datafusion-byte-view-builder-issue

Run on a 64-bit machine.
Include some dummy data (I attached an example):

 ctx.register_parquet("users","./data/users_shorten.parquet", ParquetReadOptions::default()).await?;

Run a recursive string concatenation query (see query in main.rs)
Observe the panic

Expected behavior

Should Data Fusion handle the panic?
(maybe) restrictions on the builder view calls from datafusion
(maybe) arrow-rs handling this more gracefully

Additional context

Truncated panic trace happening on my local machine:

thread 'tokio-runtime-worker' (38043518) panicked at /Users/samuele/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/arrow-array-56.2.0/src/builder/generic_bytes_view_builder.rs:310:46:
called `Result::unwrap()` on an `Err` value: TryFromIntError(())
stack backtrace:
   0: __rustc::rust_begin_unwind
             at /rustc/a454fccb02df9d361f1201b747c01257f58a8b37/library/std/src/panicking.rs:698:5
   1: core::panicking::panic_fmt
             at /rustc/a454fccb02df9d361f1201b747c01257f58a8b37/library/core/src/panicking.rs:75:14
   2: core::result::unwrap_failed
             at /rustc/a454fccb02df9d361f1201b747c01257f58a8b37/library/core/src/result.rs:1855:5
   3: core::result::Result<T,E>::unwrap
             at /Users/samuele/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/result.rs:1226:23
   4: arrow_array::builder::generic_bytes_view_builder::GenericByteViewBuilder<T>::append_value
             at /Users/samuele/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/arrow-array-56.2.0/src/builder/generic_bytes_view_builder.rs:310:46
   5: datafusion_physical_expr::expressions::binary::kernels::concat_elements_utf8view
             at /Users/samuele/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/datafusion-physical-expr-50.0.0/src/expressions/binary/kernels.rs:159:20
   6: datafusion_physical_expr::expressions::binary::concat_elements
             at /Users/samuele/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/datafusion-physical-expr-50.0.0/src/expressions/binary.rs:1078:40
   7: datafusion_physical_expr::expressions::binary::BinaryExpr::evaluate_with_resolved_args
             at /Users/samuele/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/datafusion-physical-expr-50.0.0/src/expressions/binary.rs:848:29
   8: <datafusion_physical_expr::expressions::binary::BinaryExpr as datafusion_physical_expr_common::physical_expr::PhysicalExpr>::evaluate
             at /Users/samuele/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/datafusion-physical-expr-50.0.0/src/expressions/binary.rs:479:14

Should datafusion limit somehow the append_value calls to prevent the panic from happening?

Finding some way to check for string overflow sounds like a good idea to me. Panics aren't good in general

arrow-rs method is supposed to panic in case of overflow

    #[inline]
    pub fn append_value(&mut self, value: impl AsRef<T::Native>) {
        self.value_builder
            .extend_from_slice(value.as_ref().as_ref());
        self.null_buffer_builder.append(true);
        self.offsets_builder.push(self.next_offset());
    }

we probably can come up with safer variant in arrow-rs and use it in DF

I'm taking a look at possible ways to fix this.

the key will be to figure out how to avoid slowing things down -- one idea is to see if you can check for overflow once per batch (rather than once per row) 🤔

Could you please double-check my understanding for the following? Can't be the capacity calculation replaced with left.len() (assuming left.len() and right.len() are the same)? As the with_capacity refer to the length of the views (or strings), not to the length of the bytes?

datafusion/datafusion/physical-expr/src/expressions/binary/kernels.rs

Lines 136 to 146 in 182d5dc

    
           pub fn concat_elements_utf8view( 
        
               left: &StringViewArray, 
        
               right: &StringViewArray, 
        
           ) -> std::result::Result<StringViewArray, ArrowError> { 
        
               let capacity = left 
        
                   .data_buffers() 
        
                   .iter() 
        
                   .zip(right.data_buffers().iter()) 
        
                   .map(|(b1, b2)| b1.len() + b2.len()) 
        
                   .sum(); 
        
               let mut result = StringViewBuilder::with_capacity(capacity);

arrow-rs method is supposed to panic in case of overflow
    #[inline]
    pub fn append_value(&mut self, value: impl AsRef<T::Native>) {
        self.value_builder
            .extend_from_slice(value.as_ref().as_ref());
        self.null_buffer_builder.append(true);
        self.offsets_builder.push(self.next_offset());
    }
we probably can come up with safer variant in arrow-rs and use it in DF

Do you mean something like adding a try_append_value in arrow-rs?

the key will be to figure out how to avoid slowing things down -- one idea is to see if you can check for overflow once per batch (rather than once per row) 🤔

Do you mean checking the overflow at left: StringViewArray combined right: StringViewArray instead of iterating in every single string of the view array?

the key will be to figure out how to avoid slowing things down -- one idea is to see if you can check for overflow once per batch (rather than once per row) 🤔

Do you mean checking the overflow at left: StringViewArray combined right: StringViewArray instead of iterating in every single string of the view array?

Yeah, I was thinking something like this where you could figure out how much you could be extending the the array in one check

let total_lengths = array.iter().map(|v| v.len()).sum();
if i32::MAX - total_len() > total_len {
 return err!("overflow")
}

That being said, adding a try_append_value in arrow-rs seems very reasonable to me as well

Fix `concat_elements_utf8view` capacity initialization. (#18003)

Adding `try_append_value` implementation to `ByteViewBuilder` (#8594)

Using `try_append_value` from arrow-rs 57.0.0 (#18313)

Update fork (#12)

Fix `concat_elements_utf8view` capacity initialization. (apache#18003)

Using `try_append_value` from arrow-rs 57.0.0 (apache#18313)

Fix `concat_elements_utf8view` capacity initialization. (apache#18003)

Navigation Menu

Repository navigation

`concat_elements_utf8view` panics with large buffer on 64bit machines #17857

Describe the bug

To Reproduce

Expected behavior

Additional context

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	pub fn concat_elements_utf8view(
	left: &StringViewArray,
	right: &StringViewArray,
	) -> std::result::Result<StringViewArray, ArrowError> {
	let capacity = left
	.data_buffers()
	.iter()
	.zip(right.data_buffers().iter())
	.map(\|(b1, b2)\| b1.len() + b2.len())
	.sum();
	let mut result = StringViewBuilder::with_capacity(capacity);

concat_elements_utf8view panics with large buffer on 64bit machines #17857

Description

Describe the bug

To Reproduce

Expected behavior

Additional context

Activity

alamb commented on Oct 1, 2025

comphead commented on Oct 1, 2025

samueleresca commented on Oct 2, 2025

alamb commented on Oct 2, 2025

samueleresca commented on Oct 4, 2025

samueleresca commented on Oct 4, 2025

samueleresca commented on Oct 4, 2025

alamb commented on Oct 6, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

`concat_elements_utf8view` panics with large buffer on 64bit machines #17857