alamb commented on code in PR #10895: URL: https://github.com/apache/arrow-rs/pull/10895#discussion_r3875286951
########## arrow-array/src/array/primitive_array.rs: ########## @@ -581,6 +581,18 @@ pub use crate::types::ArrowPrimitiveType; /// assert!(array.is_null(1)); /// ``` /// +/// # Performance: Choosing Between `from` and [`PrimitiveBuilder`] +/// +/// When all values are known upfront, constructing a `PrimitiveArray` directly via +/// [`PrimitiveArray::from`] or [`PrimitiveArray::new`] is significantly faster than +/// using [`PrimitiveBuilder`]: +/// +/// - **`PrimitiveArray::from(vec![...])`** — zero-copy from `Vec`; no per-element +/// bookkeeping. Prefer this whenever values are already collected. Review Comment: it isn't just when the values are collected -- I think it is when you have all the values already in a Vec or can get them into a Vec. We should also mention that Rust has a very highly optimized Vec implementation, and Arrow has zero copy conversion from Vec to Array -- which is why Vec is preferred ########## arrow-array/src/array/primitive_array.rs: ########## @@ -581,6 +581,18 @@ pub use crate::types::ArrowPrimitiveType; /// assert!(array.is_null(1)); /// ``` /// +/// # Performance: Choosing Between `from` and [`PrimitiveBuilder`] +/// +/// When all values are known upfront, constructing a `PrimitiveArray` directly via +/// [`PrimitiveArray::from`] or [`PrimitiveArray::new`] is significantly faster than +/// using [`PrimitiveBuilder`]: +/// +/// - **`PrimitiveArray::from(vec![...])`** — zero-copy from `Vec`; no per-element +/// bookkeeping. Prefer this whenever values are already collected. +/// - **[`PrimitiveBuilder`]** — allocates incrementally and tracks nullability +/// per-element. Use this only when values must be appended one-at-a-time inside a Review Comment: I think it would be good to emphasize a bit more that PrimitiveBuilder may be faster for arrays that may have null values in them. ########## arrow-array/src/builder/primitive_builder.rs: ########## @@ -96,6 +96,37 @@ pub type Decimal128Builder = PrimitiveBuilder<Decimal128Type>; pub type Decimal256Builder = PrimitiveBuilder<Decimal256Type>; /// Builder for [`PrimitiveArray`] +/// +/// # Performance +/// +/// When all values are known upfront, prefer constructing a [`PrimitiveArray`] directly Review Comment: agian, as above I think it would be good to lead here with the "Vec is super fast and there is a zero copy conversion to PrimtiiveArray from Vec" In fact you could even mention that a PrimitiveBuilder simply uses Vec and a NullBufferBuilder internally Reasons when builder might be better: 1. You have nulls but don't know where they are -- so handling both the Vec and NullBuffer is less ergonomic -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
