Replace `ArrayData` with direct Array construction by liamzwbao · Pull Request #9338 · apache/arrow-rs

liamzwbao · 2026-02-03T02:08:10Z

Which issue does this PR close?

Part of [Epic] Replace ArrayData with direct Array construction, when possible #9298.

Rationale for this change

Reduce unsafe ArrayData::new_unchecked usage by switching to concrete array constructors

What changes are included in this PR?

Use concrete array constructors in arrow-array and arrow-string (BooleanArray, regexp, substring).
Preserve FixedSizeBinaryArray length when size == 0 and clarify the expectations of the function in substring.

Are these changes tested?

Covered by existing tests

Are there any user-facing changes?

No

alamb

Looks nice to be @liamzwbao -- I think we should maintain the sliced() call here but otherwise 👍

alamb · 2026-02-03T21:59:01Z

arrow-string/src/regexp.rs

 where
    &'a S: StringArrayType<'a>,
 {
-    let null_bit_buffer = array.nulls().map(|x| x.inner().sliced());


The sliced call seems to have been lost 🤔

But all the tests pass

I am not sure how important it is to "materalize" the buffer like this , but I recommend we keep it the same

So something like

let nulls = array.nulls().sliced().filter(|n| n.null_count() > 0);

I think sliced was needed because when using ArrayData we must pass in a Buffer for nulls; if we don't do sliced then the potential offset which is stored at the BooleanBuffer level gets lost in translation. If we are moving away from ArrayData here we can simply pass the nulls as is as the slice becomes unnecessary.

We don't have .sliced() unfortunately, but we could rebuild the NullBuffer

My point is we should be able to pass the nullbuffer as is, without needing to explicitly slice it, I believe?

If we are moving away from ArrayData here we can simply pass the nulls as is as the slice becomes unnecessary

This makes sense, will wait for further discussion before making changes

I think it would be safest to leave the sliced() call in (avoid potential regressions) and make a follow on PR to try and remove it separately

sliced should not be needed now, but its also safe to leave it in. I doubt we could show any performance impact in a benchmark since regex matching would be significantly slower than bit slicing.

arrow-string/src/substring.rs

alamb · 2026-02-03T22:03:29Z

arrow-string/src/substring.rs

+        array
+            .nulls()
+            .cloned()
+            .or_else(|| Some(NullBuffer::new_valid(num_of_elements)))


why does it need a Null buffer if the input didn't have a null buffer ?

That's mainly because FixedSizeBinaryArray::new treats size == 0 specially: if both values and nulls are None, it infers len = 0.

arrow-rs/arrow-array/src/array/fixed_size_binary_array.rs

Lines 97 to 105 in 7291147

let len = if s == 0 {

if !values.is_empty() {

return Err(ArrowError::InvalidArgumentError(

"Buffer cannot have non-zero length if the item size is zero".to_owned(),

));

}

// If the item size is zero, try to determine the length from the null buffer

nulls.as_ref().map(|n| n.len()).unwrap_or(0)

In substring::tests::without_nulls_fixed_size_binary, some cases produce size‑0 substrings, and without this change the result collapses to an empty array and fail the tests. Providing a nulls buffer here simply preserves the expected length to produce correct results (e.g., ["", "", ""]).

Jefffrey

I think we can simplify further by removing need of preserving sliced() which seems to be a relic of using ArrayData API

Jefffrey · 2026-02-04T01:44:27Z

arrow-string/src/regexp.rs

 where
    &'a S: StringArrayType<'a>,
 {
-    let null_bit_buffer = array.nulls().map(|x| x.inner().sliced());


I think sliced was needed because when using ArrayData we must pass in a Buffer for nulls; if we don't do sliced then the potential offset which is stored at the BooleanBuffer level gets lost in translation. If we are moving away from ArrayData here we can simply pass the nulls as is as the slice becomes unnecessary.

Jefffrey · 2026-02-04T01:47:09Z

arrow-array/src/array/boolean_array.rs

+            0,
+            data_len,
+        )))
+        .filter(|n| n.null_count() > 0);


I do wonder if there is a way to filter in advance, before the into() 🤔

Might be a minor optimization but worth thinking about perhaps?

Good point! We can track number of non_nulls to see if we need to create a NullBuffer

Jefffrey · 2026-02-04T02:24:15Z

arrow-array/src/array/boolean_array.rs

        let data = val_builder.as_slice_mut();

        let null_slice = null_builder.as_slice_mut();
+        let mut non_nulls = 0usize;


I was more thinking along the lines of if we can leverage information from null_builder (or perhaps explore using NullBufferBuilder), rather than have a separate variable to track this

null_builder is just a MutableBuffer, so it doesn’t carry validity metadata we can get directly.

In terms of NullBufferBuilder, it’s not obvious it would outperform this approach. We could evaluate it with a benchmark in a separate PR.

This is very performance critical code. I'll run some benchmarks to make sure this doesn't mess something up by accident

Also it seems like this code previously didn't bother to check the null counts, so we could always leave the existing behavior as is 🤔

NullBuffer::new already counts the set bits, counting the nulls separately here could allow the usage of NullBuffer::new_unchecked, but I'm not sure that will really improve performance. I agree that the performance optimizations should be left to a separate PR.

Just checked that ArrayData::new_unchecked also filters out null buffers with all nulls, so the current code in the PR does not change any observable behavior.

Given the discussion, I think it's better to limit the scope of this PR to simply replace the unsafe calls. I will revert the last commit and we could open a separate PR to benchmark the performance

alamb

Makes sense to me. Thank you @liamzwbao and @Jefffrey

I am just worried about the potential performance implications. I'll fire off some benchmarks to see

alamb · 2026-02-04T22:02:38Z

arrow-string/src/regexp.rs

 where
    &'a S: StringArrayType<'a>,
 {
-    let null_bit_buffer = array.nulls().map(|x| x.inner().sliced());


I think it would be safest to leave the sliced() call in (avoid potential regressions) and make a follow on PR to try and remove it separately

alamb · 2026-02-04T22:06:51Z

arrow-string/src/regexp.rs

+        .map(|b| NullBuffer::new(BooleanBuffer::new(b, 0, array.len())))
+        .filter(|n| n.null_count() > 0);


This pattern of creating a BooleanBuffer from with a length and then filtering for nulls count is quite common (and becoming more so)

I wonder if we should (as a follow on PR) make a function to do it, maybe like

let Option<NullBuffer> = NullBuffer::try_from_unsliced(b, array.len())

Then this code would look like

let nulls = array .nulls() .map(|n| n.inner().sliced()) .map(|b| NullBuffer::try_from_unsliced(b, array.len()

🤔

alamb · 2026-02-04T22:08:58Z

arrow-array/src/array/boolean_array.rs

        let data = val_builder.as_slice_mut();

        let null_slice = null_builder.as_slice_mut();
+        let mut non_nulls = 0usize;


This is very performance critical code. I'll run some benchmarks to make sure this doesn't mess something up by accident

alamb · 2026-02-04T22:09:40Z

arrow-array/src/array/boolean_array.rs

        let data = val_builder.as_slice_mut();

        let null_slice = null_builder.as_slice_mut();
+        let mut non_nulls = 0usize;


Also it seems like this code previously didn't bother to check the null counts, so we could always leave the existing behavior as is 🤔

alamb · 2026-02-04T22:12:35Z

run benchmark array_from

alamb-ghbot · 2026-02-04T23:45:18Z

🤖 ./gh_compare_arrow.sh gh_compare_arrow.sh Running
Linux aal-dev 6.14.0-1018-gcp #19~24.04.1-Ubuntu SMP Wed Sep 24 23:23:09 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing issue-9298-replace-arraydata (6047a3c) to dc99a52 diff
BENCH_NAME=array_from
BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench array_from
BENCH_FILTER=
BENCH_BRANCH_NAME=issue-9298-replace-arraydata
Results will be posted here when complete

alamb-ghbot · 2026-02-04T23:53:25Z

🤖: Benchmark completed

Details

group                                  issue-9298-replace-arraydata           main
-----                                  ----------------------------           ----
BooleanArray::from_iter                1.00     18.8±0.14µs        ? ?/sec    1.19     22.3±0.32µs        ? ?/sec
BooleanArray::from_trusted_len_iter    1.00     17.7±0.45µs        ? ?/sec    1.28     22.6±0.52µs        ? ?/sec
Int64Array::from_iter                  1.01     52.6±1.61µs        ? ?/sec    1.00     52.4±0.11µs        ? ?/sec
Int64Array::from_trusted_len_iter      1.00     18.9±0.08µs        ? ?/sec    1.00     18.9±0.13µs        ? ?/sec
array_from_vec 128                     1.01     67.2±0.30ns        ? ?/sec    1.00     66.5±0.35ns        ? ?/sec
array_from_vec 256                     1.00     75.1±0.65ns        ? ?/sec    1.00     75.1±0.80ns        ? ?/sec
array_from_vec 512                     1.00    125.1±0.48ns        ? ?/sec    1.02    127.2±1.21ns        ? ?/sec
array_string_from_vec 128              1.00  1092.5±12.34ns        ? ?/sec    1.10  1198.6±115.69ns        ? ?/sec
array_string_from_vec 256              1.00  1886.0±16.63ns        ? ?/sec    1.02  1926.5±18.22ns        ? ?/sec
array_string_from_vec 512              1.04      3.4±0.03µs        ? ?/sec    1.00      3.2±0.03µs        ? ?/sec
decimal128_array_from_vec 32768        1.00     54.3±3.91µs        ? ?/sec    1.14     62.2±0.70µs        ? ?/sec
decimal256_array_from_vec 32768        1.00      2.7±0.05µs        ? ?/sec    1.01      2.7±0.01µs        ? ?/sec
decimal32_array_from_vec 32768         1.06     44.0±0.59µs        ? ?/sec    1.00     41.5±0.66µs        ? ?/sec
decimal64_array_from_vec 32768         1.19     53.0±0.34µs        ? ?/sec    1.00     44.5±1.14µs        ? ?/sec
struct_array_from_vec 1024             1.00      9.4±0.18µs        ? ?/sec    1.02      9.6±1.07µs        ? ?/sec
struct_array_from_vec 128              1.00   1889.4±9.53ns        ? ?/sec    1.02  1929.1±59.68ns        ? ?/sec
struct_array_from_vec 256              1.00      3.0±0.04µs        ? ?/sec    1.02      3.0±0.03µs        ? ?/sec
struct_array_from_vec 512              1.00      5.1±0.21µs        ? ?/sec    1.01      5.1±0.08µs        ? ?/sec

This reverts commit 6047a3c.

Replace ArrayData with direct Array construction

2f5999e

github-actions bot added the arrow Changes to the arrow crate label Feb 3, 2026

liamzwbao marked this pull request as ready for review February 3, 2026 02:35

alamb added the performance label Feb 3, 2026

alamb reviewed Feb 3, 2026

View reviewed changes

Preserve sliced call

758e68c

Jefffrey reviewed Feb 4, 2026

View reviewed changes

Filter before building NullBuffer

6047a3c

Jefffrey reviewed Feb 4, 2026

View reviewed changes

alamb reviewed Feb 4, 2026

View reviewed changes

This comment was marked as outdated.

Sign in to view

liamzwbao added 2 commits February 5, 2026 19:46

Revert "Filter before building NullBuffer"

f166328

This reverts commit 6047a3c.

Merge branch 'main' into issue-9298-replace-arraydata

07c13dd

Jefffrey approved these changes Feb 6, 2026

View reviewed changes

	let len = if s == 0 {
	if !values.is_empty() {
	return Err(ArrowError::InvalidArgumentError(
	"Buffer cannot have non-zero length if the item size is zero".to_owned(),
	));
	}

	// If the item size is zero, try to determine the length from the null buffer
	nulls.as_ref().map(\|n\| n.len()).unwrap_or(0)

		.map(\|b\| NullBuffer::new(BooleanBuffer::new(b, 0, array.len())))
		.filter(\|n\| n.null_count() > 0);

Conversation

liamzwbao commented Feb 3, 2026 • 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!

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jefffrey 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Feb 4, 2026

Uh oh!

This comment was marked as outdated.

alamb-ghbot commented Feb 4, 2026

Uh oh!

alamb-ghbot commented Feb 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

liamzwbao commented Feb 3, 2026 •

edited

Loading