You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2026/08/27 17:36:44 UTC
[PR] perf(arrow-array): compile `PrimitiveArray` → `ArrayData` conversion once, not per type [arrow-rs]
alamb opened a new pull request, #10893:
URL: https://github.com/apache/arrow-rs/pull/10893
# Which issue does this PR close?
- Part of the binary-size / LLVM IR reduction effort started in #10889.
# Rationale for this change
`From<PrimitiveArray<T>> for ArrayData` (reached via `Array::to_data` / `into_data`) is monomorphized for all ~32 primitive types in every crate that calls it — ~156 lines of LLVM IR per copy, ~5,000 lines per consumer crate (arrow-cast, arrow-ord, arrow-select, arrow-row, parquet, ...). After extracting the untyped `Buffer`, the conversion is identical for every type, so almost all of that is duplicated code.
# What changes are included in this PR?
Move the `ArrayDataBuilder` construction into a private non-generic `primitive_to_array_data(data_type, len, values, nulls)` helper, compiled once in `arrow-array`. The generic `From` impl just destructures the array and calls it. Still statically dispatched — no behavior or performance change.
Measured with `cargo llvm-lines --release -p arrow-row --lib` (cargo-llvm-lines 0.4.41, aarch64-apple-darwin); other consumer crates have near-identical numbers:
| | before | after |
|---|---:|---:|
| `From<PrimitiveArray<T>> for ArrayData` IR in arrow-row | 4,992 lines / 32 copies | 1,952 lines / 32 thin shims **(−61%)** |
The same pattern applies to the byte/list array `From` impls (far fewer instantiations each); those can be follow-ups if this approach is agreeable.
# Are these changes tested?
Covered by existing tests (`to_data`/`into_data` round-trips are exercised throughout the test suite).
# Are there any user-facing changes?
No. The helper is private and the produced `ArrayData` is identical.
--
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
Re: [PR] perf(arrow-array): compile `PrimitiveArray` → `ArrayData` conversion once, not per type [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10893:
URL: https://github.com/apache/arrow-rs/pull/10893#discussion_r3875058489
##########
arrow-array/src/array/primitive_array.rs:
##########
@@ -1196,15 +1196,31 @@ impl<T: ArrowPrimitiveType> PrimitiveArray<T> {
impl<T: ArrowPrimitiveType> From<PrimitiveArray<T>> for ArrayData {
fn from(array: PrimitiveArray<T>) -> Self {
- let builder = ArrayDataBuilder::new(array.data_type)
- .len(array.values.len())
- .nulls(array.nulls)
- .buffers(vec![array.values.into_inner()]);
-
- unsafe { builder.build_unchecked() }
+ let len = array.values.len();
+ primitive_to_array_data(array.data_type, len, array.values.into_inner(), array.nulls)
Review Comment:
the point is to move the code into a single function as most of the logic doesn't vary by type
##########
arrow-array/src/array/primitive_array.rs:
##########
@@ -1196,15 +1196,31 @@ impl<T: ArrowPrimitiveType> PrimitiveArray<T> {
impl<T: ArrowPrimitiveType> From<PrimitiveArray<T>> for ArrayData {
fn from(array: PrimitiveArray<T>) -> Self {
- let builder = ArrayDataBuilder::new(array.data_type)
- .len(array.values.len())
- .nulls(array.nulls)
- .buffers(vec![array.values.into_inner()]);
-
- unsafe { builder.build_unchecked() }
+ let len = array.values.len();
+ primitive_to_array_data(array.data_type, len, array.values.into_inner(), array.nulls)
}
}
+/// Converts the parts of a valid `PrimitiveArray` into [`ArrayData`]
+///
+/// Deliberately not generic over [`ArrowPrimitiveType`]: after extracting the
+/// untyped [`Buffer`] the conversion is identical for all primitive types, so
+/// keeping it non-generic means it is compiled once rather than once per type
+fn primitive_to_array_data(
+ data_type: DataType,
+ len: usize,
+ values: Buffer,
+ nulls: Option<NullBuffer>,
+) -> ArrayData {
+ let builder = ArrayDataBuilder::new(data_type)
+ .len(len)
+ .nulls(nulls)
+ .buffers(vec![values]);
+
+ // SAFETY: the parts came from a valid PrimitiveArray
Review Comment:
bonus we now get a safetly comment too
--
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
Re: [PR] perf(arrow-array): Reduce binary size 0.5%: do `PrimitiveArray` → `ArrayData` conversion once, not per type [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10893:
URL: https://github.com/apache/arrow-rs/pull/10893#discussion_r3880197560
##########
arrow-array/src/array/primitive_array.rs:
##########
@@ -1196,15 +1196,31 @@ impl<T: ArrowPrimitiveType> PrimitiveArray<T> {
impl<T: ArrowPrimitiveType> From<PrimitiveArray<T>> for ArrayData {
fn from(array: PrimitiveArray<T>) -> Self {
- let builder = ArrayDataBuilder::new(array.data_type)
- .len(array.values.len())
- .nulls(array.nulls)
- .buffers(vec![array.values.into_inner()]);
-
- unsafe { builder.build_unchecked() }
+ let len = array.values.len();
+ primitive_to_array_data(array.data_type, len, array.values.into_inner(), array.nulls)
Review Comment:
done
--
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
Re: [PR] perf(arrow-array): Reduce binary size 0.5%: do `PrimitiveArray` → `ArrayData` conversion once, not per type [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10893:
URL: https://github.com/apache/arrow-rs/pull/10893#discussion_r3880196002
##########
arrow-array/src/array/primitive_array.rs:
##########
@@ -1196,15 +1196,31 @@ impl<T: ArrowPrimitiveType> PrimitiveArray<T> {
impl<T: ArrowPrimitiveType> From<PrimitiveArray<T>> for ArrayData {
fn from(array: PrimitiveArray<T>) -> Self {
- let builder = ArrayDataBuilder::new(array.data_type)
- .len(array.values.len())
- .nulls(array.nulls)
- .buffers(vec![array.values.into_inner()]);
-
- unsafe { builder.build_unchecked() }
+ let len = array.values.len();
+ primitive_to_array_data(array.data_type, len, array.values.into_inner(), array.nulls)
}
}
+/// Converts the parts of a valid `PrimitiveArray` into [`ArrayData`]
+///
+/// Deliberately not generic over [`ArrowPrimitiveType`]: after extracting the
+/// untyped [`Buffer`] the conversion is identical for all primitive types, so
+/// keeping it non-generic means it is compiled once rather than once per type
+fn primitive_to_array_data(
Review Comment:
this is an excellent idea -- done
--
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
Re: [PR] perf(arrow-array): Reduce binary size 0.5%: do `PrimitiveArray` → `ArrayData` conversion once, not per type [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10893:
URL: https://github.com/apache/arrow-rs/pull/10893#issuecomment-5462656991
Thank you everyone
--
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
Re: [PR] perf(arrow-array): Reduce binary size 0.5%: do `PrimitiveArray` → `ArrayData` conversion once, not per type [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #10893:
URL: https://github.com/apache/arrow-rs/pull/10893#discussion_r3877296751
##########
arrow-array/src/array/primitive_array.rs:
##########
@@ -1196,15 +1196,31 @@ impl<T: ArrowPrimitiveType> PrimitiveArray<T> {
impl<T: ArrowPrimitiveType> From<PrimitiveArray<T>> for ArrayData {
fn from(array: PrimitiveArray<T>) -> Self {
- let builder = ArrayDataBuilder::new(array.data_type)
- .len(array.values.len())
- .nulls(array.nulls)
- .buffers(vec![array.values.into_inner()]);
-
- unsafe { builder.build_unchecked() }
+ let len = array.values.len();
+ primitive_to_array_data(array.data_type, len, array.values.into_inner(), array.nulls)
}
}
+/// Converts the parts of a valid `PrimitiveArray` into [`ArrayData`]
+///
+/// Deliberately not generic over [`ArrowPrimitiveType`]: after extracting the
+/// untyped [`Buffer`] the conversion is identical for all primitive types, so
+/// keeping it non-generic means it is compiled once rather than once per type
+fn primitive_to_array_data(
Review Comment:
should we just make this an inner function to the from, similar to how stdlib does it here?
https://doc.rust-lang.org/nightly/src/std/fs.rs.html#346-355
--
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
Re: [PR] perf(arrow-array): Reduce binary size 0.5%: do `PrimitiveArray` → `ArrayData` conversion once, not per type [arrow-rs]
Posted by "jhorstmann (via GitHub)" <gi...@apache.org>.
jhorstmann commented on code in PR #10893:
URL: https://github.com/apache/arrow-rs/pull/10893#discussion_r3875889938
##########
arrow-array/src/array/primitive_array.rs:
##########
@@ -1196,15 +1196,31 @@ impl<T: ArrowPrimitiveType> PrimitiveArray<T> {
impl<T: ArrowPrimitiveType> From<PrimitiveArray<T>> for ArrayData {
fn from(array: PrimitiveArray<T>) -> Self {
- let builder = ArrayDataBuilder::new(array.data_type)
- .len(array.values.len())
- .nulls(array.nulls)
- .buffers(vec![array.values.into_inner()]);
-
- unsafe { builder.build_unchecked() }
+ let len = array.values.len();
+ primitive_to_array_data(array.data_type, len, array.values.into_inner(), array.nulls)
Review Comment:
Nice improvement.
I think `primitive_to_array_data` needs to be `unsafe`, and the safety comment moved to the call here, since the new function does no validation on the parameters and calls `build_unchecked`.
--
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
Re: [PR] perf(arrow-array): Reduce binary size 0.5%: do `PrimitiveArray` → `ArrayData` conversion once, not per type [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #10893:
URL: https://github.com/apache/arrow-rs/pull/10893
--
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
Re: [PR] perf(arrow-array): Reduce binary size 0.5%: do `PrimitiveArray` → `ArrayData` conversion once, not per type [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10893:
URL: https://github.com/apache/arrow-rs/pull/10893#issuecomment-5451969875
I re-ran the binary size tests with the updated change (inner-function version, 6102547236) and the results are essentially the same as before:
**Binary size** (`cargo bench --bench cast_kernels --no-run`):
| before | after | delta |
|---:|---:|---:|
| 13,232,256 bytes | 13,188,848 bytes | **−43,408 bytes (−0.33%)** |
**LLVM IR** (`cargo llvm-lines --release -p arrow-row --lib`, `From<PrimitiveArray<T>> for ArrayData`):
| before | after | delta |
|---|---:|---:|
| 4,992 lines / 32 copies (156 lines each) | 1,952 lines / 32 thin shims (61 lines each) | **−61%** |
So moving the helper to a non-generic inner function (and marking it `unsafe`) preserves the monomorphization savings.
--
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
Re: [PR] perf(arrow-array): compile `PrimitiveArray` → `ArrayData` conversion once, not per type [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10893:
URL: https://github.com/apache/arrow-rs/pull/10893#issuecomment-5444042363
This is a pretty small change and I think it does save a meaningful number of bytes in aggregate (across all crates that use arrow)
--
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