Repository: jorgecarleitao/arrow2
Pull request #1085 (2022-06-19 08:32:33)

Removed un-necessary allocation in `assign_ops`
This PR removes unnecessary allocation on `assign_ops`.

When converting `PrimitiveArray -> MutablePrimitiveArray`, we need to check that both the values and validities are unique refs. However, when operating over values and validities independently, we do not need a mutable ref to both at the same time.

A common case where this was hitting was when `rhs` had a validity and `lhs` did not. In this case, we first cloned the `rhs`'s validity for `lhs`, but them tried to get a mut ref to both the validity and the values (to only use the values). This would naturally fail, causing us to have to re-use the `lhs`.

This PR adds `PrimitiveArray::get_mut_values` and removes (un-released) functions from `PrimitiveArray`, since the function `get_mut_values` is more generic.

codecov[bot]: # [Codecov](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1085?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao) Report
> Merging [#1085](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1085?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao) (75c7bfb) into [main](https://codecov.io/gh/jorgecarleitao/arrow2/commit/16d01efcd211ff29280f214d9476f2f3ba2d5c97?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao) (16d01ef) will **decrease** coverage by `0.06%`.
> The diff coverage is `61.90%`.

```diff
@@            Coverage Diff             @@
##             main    #1085      +/-   ##
==========================================
- Coverage   81.14%   81.07%   -0.07%     
==========================================
  Files         367      367              
  Lines       35316    35281      -35     
==========================================
- Hits        28657    28605      -52     
- Misses       6659     6676      +17     
```


| [Impacted Files](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1085?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao) | Coverage Δ | |
|---|---|---|
| [src/array/boolean/mod.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1085/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2FycmF5L2Jvb2xlYW4vbW9kLnJz) | `80.86% <ø> (-4.01%)` | :arrow_down: |
| [src/array/primitive/mutable.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1085/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2FycmF5L3ByaW1pdGl2ZS9tdXRhYmxlLnJz) | `95.57% <ø> (+0.63%)` | :arrow_up: |
| [src/compute/arity\_assign.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1085/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2NvbXB1dGUvYXJpdHlfYXNzaWduLnJz) | `64.00% <46.42%> (-10.08%)` | :arrow_down: |
| [src/buffer/immutable.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1085/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2J1ZmZlci9pbW11dGFibGUucnM=) | `83.95% <87.50%> (-4.94%)` | :arrow_down: |
| [src/array/primitive/mod.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1085/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2FycmF5L3ByaW1pdGl2ZS9tb2QucnM=) | `79.53% <100.00%> (-2.13%)` | :arrow_down: |
| [src/bitmap/assign\_ops.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1085/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2JpdG1hcC9hc3NpZ25fb3BzLnJz) | `83.06% <0.00%> (-3.23%)` | :arrow_down: |
| ... and [6 more](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1085/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1085?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1085?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao). Last update [16d01ef...75c7bfb](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1085?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao).


Patch:
diff --git a/benches/assign_ops.rs b/benches/assign_ops.rs
index dc4c9b182f..7b169ec42d 100644
--- a/benches/assign_ops.rs
+++ b/benches/assign_ops.rs
@@ -1,6 +1,6 @@
 use criterion::{criterion_group, criterion_main, Criterion};
 
-use arrow2::compute::arity_assign::binary;
+use arrow2::compute::arity_assign::{binary, unary};
 use arrow2::{
     compute::arithmetics::basic::{mul, mul_scalar},
     util::bench_util::*,
@@ -13,8 +13,7 @@ fn add_benchmark(c: &mut Criterion) {
         let mut arr_a = create_primitive_array::<f32>(size, 0.2);
         c.bench_function(&format!("apply_mul 2^{}", log2_size), |b| {
             b.iter(|| {
-                criterion::black_box(&mut arr_a)
-                    .apply_values_mut(|x| x.iter_mut().for_each(|x| *x *= 1.01));
+                unary(criterion::black_box(&mut arr_a), |x| x * 1.01);
                 assert!(!arr_a.value(10).is_nan());
             })
         });
@@ -30,7 +29,7 @@ fn add_benchmark(c: &mut Criterion) {
         let mut arr_a = create_primitive_array::<f32>(size, 0.2);
         let mut arr_b = create_primitive_array_with_seed::<f32>(size, 0.2, 10);
         // convert to be close to 1.01
-        arr_b.apply_values_mut(|x| x.iter_mut().for_each(|x| *x = 1.01 + *x / 20.0));
+        unary(&mut arr_b, |x| 1.01 + x / 20.0);
 
         c.bench_function(&format!("apply_mul null 2^{}", log2_size), |b| {
             b.iter(|| {
diff --git a/examples/cow.rs b/examples/cow.rs
index 3255d365f4..5548f453a1 100644
--- a/examples/cow.rs
+++ b/examples/cow.rs
@@ -1,19 +1,21 @@
 // This example demos how to operate on arrays in-place.
 use arrow2::array::{Array, PrimitiveArray};
+use arrow2::compute::arity_assign;
 
 fn main() {
-    // say we have have received an array
+    // say we have have received an `Array`
     let mut array: Box<dyn Array> = PrimitiveArray::from_vec(vec![1i32, 2]).boxed();
 
     // we can apply a transformation to its values without allocating a new array as follows:
+
     // 1. downcast it to the correct type (known via `array.data_type().to_physical_type()`)
     let array = array
         .as_any_mut()
         .downcast_mut::<PrimitiveArray<i32>>()
         .unwrap();
 
-    // 2. call `apply_values` with the function to apply over the values
-    array.apply_values_mut(|x| x.iter_mut().for_each(|x| *x *= 10));
+    // 2. call `unary` with the function to apply to each value
+    arity_assign::unary(array, |x| x * 10);
 
     // confirm that it gives the right result :)
     assert_eq!(array, &PrimitiveArray::from_vec(vec![10i32, 20]));
diff --git a/src/array/boolean/mod.rs b/src/array/boolean/mod.rs
index 85ad172be0..9901386cc4 100644
--- a/src/array/boolean/mod.rs
+++ b/src/array/boolean/mod.rs
@@ -236,25 +236,6 @@ impl BooleanArray {
         self.values = values.into();
     }
 
-    /// Applies a function `f` to the validity of this array, cloning it
-    /// iff it is being shared.
-    ///
-    /// This is an API to leverage clone-on-write
-    /// # Implementation
-    /// This function is `O(f)` if the data is not being shared, and `O(N) + O(f)`
-    /// if it is being shared (since it results in a `O(N)` memcopy).
-    /// # Panics
-    /// This function panics if the function modifies the length of the [`MutableBitmap`].
-    pub fn apply_validity_mut<F: Fn(&mut MutableBitmap)>(&mut self, f: F) {
-        if let Some(validity) = self.validity.as_mut() {
-            let owned_validity = std::mem::take(validity);
-            let mut mut_bitmap = owned_validity.make_mut();
-            f(&mut mut_bitmap);
-            assert_eq!(mut_bitmap.len(), self.values.len());
-            *validity = mut_bitmap.into();
-        }
-    }
-
     /// Try to convert this [`BooleanArray`] to a [`MutableBooleanArray`]
     pub fn into_mut(self) -> Either<Self, MutableBooleanArray> {
         use Either::*;
diff --git a/src/array/primitive/mod.rs b/src/array/primitive/mod.rs
index 925b9a26fc..f358b12dba 100644
--- a/src/array/primitive/mod.rs
+++ b/src/array/primitive/mod.rs
@@ -1,7 +1,7 @@
 use crate::{
     bitmap::{
         utils::{zip_validity, ZipValidity},
-        Bitmap, MutableBitmap,
+        Bitmap,
     },
     buffer::Buffer,
     datatypes::*,
@@ -283,59 +283,24 @@ impl<T: NativeType> PrimitiveArray<T> {
         self.values = values;
     }
 
-    /// Applies a function `f` to the values of this array, cloning the values
-    /// iff they are being shared with others
-    ///
-    /// This is an API to use clone-on-write
-    /// # Implementation
-    /// This function is `O(f)` if the data is not being shared, and `O(N) + O(f)`
-    /// if it is being shared (since it results in a `O(N)` memcopy).
-    /// # Panics
-    /// This function panics, if `f` modifies the length of `&mut [T]`
-    pub fn apply_values_mut<F: Fn(&mut [T])>(&mut self, f: F) {
-        let values = std::mem::take(&mut self.values);
-        let mut values = values.make_mut();
-        let len = values.len();
-        f(&mut values);
-        assert_eq!(values.len(), len, "values length must remain the same");
-        self.values = values.into();
-    }
-
-    /// Applies a function `f` to the validity of this array, cloning it
-    /// iff it is being shared.
-    ///
-    /// This is an API to leverage clone-on-write
-    /// # Implementation
-    /// This function is `O(f)` if the data is not being shared, and `O(N) + O(f)`
-    /// if it is being shared (since it results in a `O(N)` memcopy).
-    /// # Panics
-    /// This function panics if the function modifies the length of the [`MutableBitmap`].
-    pub fn apply_validity_mut<F: Fn(&mut MutableBitmap)>(&mut self, f: F) {
-        if let Some(validity) = self.validity.as_mut() {
-            let owned_validity = std::mem::take(validity);
-            let mut mut_bitmap = owned_validity.make_mut();
-            f(&mut mut_bitmap);
-            assert_eq!(mut_bitmap.len(), self.values.len());
-            *validity = mut_bitmap.into();
-        }
-    }
-
     /// Applies a function `f` to the validity of this array, the caller can decide to make
     /// it mutable or not.
     ///
     /// This is an API to leverage clone-on-write
-    /// # Implementation
-    /// This function is `O(f)` if the data is not being shared, and `O(N) + O(f)`
-    /// if it is being shared (since it results in a `O(N)` memcopy).
     /// # Panics
-    /// This function panics if the function modifies the length of the [`MutableBitmap`].
-    pub fn apply_validity<F: Fn(&mut Bitmap)>(&mut self, f: F) {
-        if let Some(validity) = self.validity.as_mut() {
-            f(validity);
+    /// This function panics if the function `f` modifies the length of the [`Bitmap`].
+    pub fn apply_validity<F: Fn(Bitmap) -> Bitmap>(&mut self, f: F) {
+        if let Some(validity) = std::mem::take(&mut self.validity) {
             assert_eq!(validity.len(), self.values.len());
+            self.validity = Some(f(validity))
         }
     }
 
+    /// Returns an option of a mutable reference to the values of this [`PrimitiveArray`].
+    pub fn get_mut_values(&mut self) -> Option<&mut [T]> {
+        self.values.get_mut().map(|x| x.as_mut())
+    }
+
     /// Try to convert this [`PrimitiveArray`] to a [`MutablePrimitiveArray`] via copy-on-write semantics.
     ///
     /// A [`PrimitiveArray`] is backed by a [`Buffer`] and [`Bitmap`] which are essentially `Arc<Vec<_>>`.
diff --git a/src/array/primitive/mutable.rs b/src/array/primitive/mutable.rs
index ae983c10af..573fa8953a 100644
--- a/src/array/primitive/mutable.rs
+++ b/src/array/primitive/mutable.rs
@@ -90,27 +90,9 @@ impl<T: NativeType> MutablePrimitiveArray<T> {
     /// This function is `O(f)` if the data is not being shared, and `O(N) + O(f)`
     /// if it is being shared (since it results in a `O(N)` memcopy).
     /// # Panics
-    /// This function panics, if `f` modifies the length of `&mut [T]`
+    /// This function panics iff `f` panics
     pub fn apply_values<F: Fn(&mut [T])>(&mut self, f: F) {
-        let len = self.values.len();
         f(&mut self.values);
-        assert_eq!(len, self.values.len(), "values length must remain the same")
-    }
-
-    /// Applies a function `f` to the validity of this array, cloning it
-    /// iff it is being shared.
-    ///
-    /// This is an API to leverage clone-on-write
-    /// # Implementation
-    /// This function is `O(f)` if the data is not being shared, and `O(N) + O(f)`
-    /// if it is being shared (since it results in a `O(N)` memcopy).
-    /// # Panics
-    /// This function panics if the function modifies the length of the [`MutableBitmap`].
-    pub fn apply_validity<F: Fn(&mut MutableBitmap)>(&mut self, f: F) {
-        if let Some(validity) = &mut self.validity {
-            f(validity);
-            assert_eq!(validity.len(), self.values.len());
-        }
     }
 }
 
diff --git a/src/buffer/immutable.rs b/src/buffer/immutable.rs
index 614089a905..c4e73ce33d 100644
--- a/src/buffer/immutable.rs
+++ b/src/buffer/immutable.rs
@@ -137,6 +137,20 @@ impl<T: NativeType> Buffer<T> {
         self.offset
     }
 
+    /// Gets a mutable reference to its underlying [`Vec`], if it not being shared.
+    ///
+    /// This operation returns a [`Vec`] iff this [`Buffer`]:
+    /// * is not an offsetted slice of another [`Buffer`]
+    /// * has not been cloned (i.e. [`Arc`]`::get_mut` yields [`Some`])
+    /// * has not been imported from the c data interface (FFI)
+    pub fn get_mut(&mut self) -> Option<&mut Vec<T>> {
+        if self.offset != 0 {
+            None
+        } else {
+            Arc::get_mut(&mut self.data).and_then(|b| b.get_vec())
+        }
+    }
+
     /// Converts this [`Buffer`] to either a [`Buffer`] or a [`Vec`], returning itself if the conversion
     /// is not possible
     ///
@@ -145,16 +159,10 @@ impl<T: NativeType> Buffer<T> {
     /// * has not been cloned (i.e. [`Arc`]`::get_mut` yields [`Some`])
     /// * has not been imported from the c data interface (FFI)
     pub fn into_mut(mut self) -> Either<Self, Vec<T>> {
-        if self.offset != 0 {
-            Either::Left(self)
+        if let Some(vec) = self.get_mut() {
+            Either::Right(std::mem::take(vec))
         } else {
-            match Arc::get_mut(&mut self.data).and_then(|b| b.get_vec()) {
-                Some(v) => {
-                    let data = std::mem::take(v);
-                    Either::Right(data)
-                }
-                None => Either::Left(self),
-            }
+            Either::Left(self)
         }
     }
 
diff --git a/src/compute/arithmetics/mod.rs b/src/compute/arithmetics/mod.rs
index 2b6bb7e6c0..07391675ed 100644
--- a/src/compute/arithmetics/mod.rs
+++ b/src/compute/arithmetics/mod.rs
@@ -21,17 +21,17 @@ use crate::{
     bitmap::Bitmap,
     datatypes::{DataType, IntervalUnit, TimeUnit},
     scalar::{PrimitiveScalar, Scalar},
+    types::NativeType,
 };
 
-// Macro to evaluate match branch in arithmetic function.
-macro_rules! primitive {
-    ($lhs:expr, $rhs:expr, $op:tt, $type:ty) => {{
-        let lhs = $lhs.as_any().downcast_ref().unwrap();
-        let rhs = $rhs.as_any().downcast_ref().unwrap();
-
-        let result = basic::$op::<$type>(lhs, rhs);
-        Box::new(result) as Box<dyn Array>
-    }};
+fn binary_dyn<T: NativeType, F: Fn(&PrimitiveArray<T>, &PrimitiveArray<T>) -> PrimitiveArray<T>>(
+    lhs: &dyn Array,
+    rhs: &dyn Array,
+    op: F,
+) -> Box<dyn Array> {
+    let lhs = lhs.as_any().downcast_ref().unwrap();
+    let rhs = rhs.as_any().downcast_ref().unwrap();
+    op(lhs, rhs).boxed()
 }
 
 // Macro to create a `match` statement with dynamic dispatch to functions based on
@@ -42,18 +42,18 @@ macro_rules! arith {
         let rhs = $rhs;
         use DataType::*;
         match (lhs.data_type(), rhs.data_type()) {
-            (Int8, Int8) => primitive!(lhs, rhs, $op, i8),
-            (Int16, Int16) => primitive!(lhs, rhs, $op, i16),
-            (Int32, Int32) => primitive!(lhs, rhs, $op, i32),
+            (Int8, Int8) => binary_dyn::<i8, _>(lhs, rhs, basic::$op),
+            (Int16, Int16) => binary_dyn::<i16, _>(lhs, rhs, basic::$op),
+            (Int32, Int32) => binary_dyn::<i32, _>(lhs, rhs, basic::$op),
             (Int64, Int64) | (Duration(_), Duration(_)) => {
-                primitive!(lhs, rhs, $op, i64)
+                binary_dyn::<i64, _>(lhs, rhs, basic::$op)
             }
-            (UInt8, UInt8) => primitive!(lhs, rhs, $op, u8),
-            (UInt16, UInt16) => primitive!(lhs, rhs, $op, u16),
-            (UInt32, UInt32) => primitive!(lhs, rhs, $op, u32),
-            (UInt64, UInt64) => primitive!(lhs, rhs, $op, u64),
-            (Float32, Float32) => primitive!(lhs, rhs, $op, f32),
-            (Float64, Float64) => primitive!(lhs, rhs, $op, f64),
+            (UInt8, UInt8) => binary_dyn::<u8, _>(lhs, rhs, basic::$op),
+            (UInt16, UInt16) => binary_dyn::<u16, _>(lhs, rhs, basic::$op),
+            (UInt32, UInt32) => binary_dyn::<u32, _>(lhs, rhs, basic::$op),
+            (UInt64, UInt64) => binary_dyn::<u64, _>(lhs, rhs, basic::$op),
+            (Float32, Float32) => binary_dyn::<f32, _>(lhs, rhs, basic::$op),
+            (Float64, Float64) => binary_dyn::<f64, _>(lhs, rhs, basic::$op),
             $ (
             (Decimal(_, _), Decimal(_, _)) => {
                 let lhs = lhs.as_any().downcast_ref().unwrap();
@@ -101,30 +101,27 @@ macro_rules! arith {
     }};
 }
 
-// Macro to evaluate match branch in arithmetic function.
-macro_rules! primitive_scalar {
-    ($lhs:expr, $rhs:expr, $op:tt, $type:ty) => {{
-        let lhs = $lhs
-            .as_any()
-            .downcast_ref::<PrimitiveArray<$type>>()
-            .unwrap();
-        let rhs = $rhs
-            .as_any()
-            .downcast_ref::<PrimitiveScalar<$type>>()
-            .unwrap();
-
-        let rhs = if let Some(rhs) = rhs.value() {
-            rhs
-        } else {
-            return Box::new(PrimitiveArray::<$type>::new_null(
-                lhs.data_type().clone(),
-                lhs.len(),
-            )) as Box<dyn Array>;
-        };
-
-        let result = basic::$op::<$type>(lhs, &rhs);
-        Box::new(result) as Box<dyn Array>
-    }};
+fn binary_scalar<T: NativeType, F: Fn(&PrimitiveArray<T>, &T) -> PrimitiveArray<T>>(
+    lhs: &PrimitiveArray<T>,
+    rhs: &PrimitiveScalar<T>,
+    op: F,
+) -> PrimitiveArray<T> {
+    let rhs = if let Some(rhs) = rhs.value() {
+        rhs
+    } else {
+        return PrimitiveArray::<T>::new_null(lhs.data_type().clone(), lhs.len());
+    };
+    op(lhs, &rhs)
+}
+
+fn binary_scalar_dyn<T: NativeType, F: Fn(&PrimitiveArray<T>, &T) -> PrimitiveArray<T>>(
+    lhs: &dyn Array,
+    rhs: &dyn Scalar,
+    op: F,
+) -> Box<dyn Array> {
+    let lhs = lhs.as_any().downcast_ref().unwrap();
+    let rhs = rhs.as_any().downcast_ref().unwrap();
+    binary_scalar(lhs, rhs, op).boxed()
 }
 
 // Macro to create a `match` statement with dynamic dispatch to functions based on
@@ -135,23 +132,23 @@ macro_rules! arith_scalar {
         let rhs = $rhs;
         use DataType::*;
         match (lhs.data_type(), rhs.data_type()) {
-            (Int8, Int8) => primitive_scalar!(lhs, rhs, $op, i8),
-            (Int16, Int16) => primitive_scalar!(lhs, rhs, $op, i16),
-            (Int32, Int32) => primitive_scalar!(lhs, rhs, $op, i32),
+            (Int8, Int8) => binary_scalar_dyn::<i8, _>(lhs, rhs, basic::$op),
+            (Int16, Int16) => binary_scalar_dyn::<i16, _>(lhs, rhs, basic::$op),
+            (Int32, Int32) => binary_scalar_dyn::<i32, _>(lhs, rhs, basic::$op),
             (Int64, Int64) | (Duration(_), Duration(_)) => {
-                primitive_scalar!(lhs, rhs, $op, i64)
+                binary_scalar_dyn::<i64, _>(lhs, rhs, basic::$op)
             }
-            (UInt8, UInt8) => primitive_scalar!(lhs, rhs, $op, u8),
-            (UInt16, UInt16) => primitive_scalar!(lhs, rhs, $op, u16),
-            (UInt32, UInt32) => primitive_scalar!(lhs, rhs, $op, u32),
-            (UInt64, UInt64) => primitive_scalar!(lhs, rhs, $op, u64),
-            (Float32, Float32) => primitive_scalar!(lhs, rhs, $op, f32),
-            (Float64, Float64) => primitive_scalar!(lhs, rhs, $op, f64),
+            (UInt8, UInt8) => binary_scalar_dyn::<u8, _>(lhs, rhs, basic::$op),
+            (UInt16, UInt16) => binary_scalar_dyn::<u16, _>(lhs, rhs, basic::$op),
+            (UInt32, UInt32) => binary_scalar_dyn::<u32, _>(lhs, rhs, basic::$op),
+            (UInt64, UInt64) => binary_scalar_dyn::<u64, _>(lhs, rhs, basic::$op),
+            (Float32, Float32) => binary_scalar_dyn::<f32, _>(lhs, rhs, basic::$op),
+            (Float64, Float64) => binary_scalar_dyn::<f64, _>(lhs, rhs, basic::$op),
             $ (
             (Decimal(_, _), Decimal(_, _)) => {
                 let lhs = lhs.as_any().downcast_ref().unwrap();
                 let rhs = rhs.as_any().downcast_ref().unwrap();
-                Box::new(decimal::$op_decimal(lhs, rhs)) as Box<dyn Array>
+                decimal::$op_decimal(lhs, rhs).boxed()
             }
             )?
             $ (
@@ -160,7 +157,7 @@ macro_rules! arith_scalar {
             | (Date32, Duration(_)) => {
                 let lhs = lhs.as_any().downcast_ref().unwrap();
                 let rhs = rhs.as_any().downcast_ref().unwrap();
-                Box::new(time::$op_duration::<i32>(lhs, rhs)) as Box<dyn Array>
+                time::$op_duration::<i32>(lhs, rhs).boxed()
             }
             (Time64(TimeUnit::Microsecond), Duration(_))
             | (Time64(TimeUnit::Nanosecond), Duration(_))
@@ -168,21 +165,21 @@ macro_rules! arith_scalar {
             | (Timestamp(_, _), Duration(_)) => {
                 let lhs = lhs.as_any().downcast_ref().unwrap();
                 let rhs = rhs.as_any().downcast_ref().unwrap();
-                Box::new(time::$op_duration::<i64>(lhs, rhs)) as Box<dyn Array>
+                time::$op_duration::<i64>(lhs, rhs).boxed()
             }
             )?
             $ (
             (Timestamp(_, _), Interval(IntervalUnit::MonthDayNano)) => {
                 let lhs = lhs.as_any().downcast_ref().unwrap();
                 let rhs = rhs.as_any().downcast_ref().unwrap();
-                time::$op_interval(lhs, rhs).map(|x| Box::new(x) as Box<dyn Array>).unwrap()
+                time::$op_interval(lhs, rhs).unwrap().boxed()
             }
             )?
             $ (
             (Timestamp(_, None), Timestamp(_, None)) => {
                 let lhs = lhs.as_any().downcast_ref().unwrap();
                 let rhs = rhs.as_any().downcast_ref().unwrap();
-                time::$op_timestamp(lhs, rhs).map(|x| Box::new(x) as Box<dyn Array>).unwrap()
+                time::$op_timestamp(lhs, rhs).unwrap().boxed()
             }
             )?
             _ => todo!(
@@ -197,7 +194,7 @@ macro_rules! arith_scalar {
 /// Adds two [`Array`]s.
 /// # Panic
 /// This function panics iff
-/// * the opertion is not supported for the logical types (use [`can_add`] to check)
+/// * the operation is not supported for the logical types (use [`can_add`] to check)
 /// * the arrays have a different length
 /// * one of the arrays is a timestamp with timezone and the timezone is not valid.
 pub fn add(lhs: &dyn Array, rhs: &dyn Array) -> Box<dyn Array> {
diff --git a/src/compute/arity_assign.rs b/src/compute/arity_assign.rs
index ff504c9690..79d8f579b3 100644
--- a/src/compute/arity_assign.rs
+++ b/src/compute/arity_assign.rs
@@ -4,10 +4,12 @@ use super::utils::check_same_len;
 use crate::{array::PrimitiveArray, types::NativeType};
 use either::Either;
 
-/// Applies an unary function to a [`PrimitiveArray`] in-place via cow semantics.
+/// Applies an unary function to a [`PrimitiveArray`], optionally in-place.
 ///
 /// # Implementation
-/// This is the fastest method to apply a binary operation and it is often vectorized (SIMD).
+/// This function tries to apply the function directly to the values of the array.
+/// If that region is shared, this function creates a new region and writes to it.
+///
 /// # Panics
 /// This function panics iff
 /// * the arrays have a different length.
@@ -18,14 +20,22 @@ where
     I: NativeType,
     F: Fn(I) -> I,
 {
-    array.apply_values_mut(|values| values.iter_mut().for_each(|v| *v = op(*v)));
+    if let Some(values) = array.get_mut_values() {
+        // mutate in place
+        values.iter_mut().for_each(|l| *l = op(*l));
+    } else {
+        // alloc and write to new region
+        let values = array.values().iter().map(|l| op(*l)).collect::<Vec<_>>();
+        array.set_values(values.into());
+    }
 }
 
-/// Applies a binary operations to two [`PrimitiveArray`], applying the operation
-/// in-place to the `lhs` via cow semantics.
+/// Applies a binary function to two [`PrimitiveArray`]s, optionally in-place, returning
+/// a new [`PrimitiveArray`].
 ///
 /// # Implementation
-/// This is the fastest way to perform a binary operation and it is often vectorized (SIMD).
+/// This function tries to apply the function directly to the values of the array.
+/// If that region is shared, this function creates a new region and writes to it.
 /// # Panics
 /// This function panics iff
 /// * the arrays have a different length.
@@ -48,21 +58,16 @@ where
     // bitmap has an offset.
     if let Some(rhs) = rhs.validity() {
         if lhs.validity().is_none() {
-            *lhs = lhs.with_validity(Some(rhs.clone()))
+            lhs.set_validity(Some(rhs.clone()));
         } else {
             lhs.apply_validity(|bitmap| {
-                // we need to take ownership for the `into_mut` call, but leave the `&mut` lhs intact
-                // so that we can later assign the result to out `&mut bitmap`
-                let owned_lhs = std::mem::take(bitmap);
-
-                *bitmap = match owned_lhs.into_mut() {
-                    // we take alloc and write to new buffer
+                match bitmap.into_mut() {
                     Either::Left(immutable) => {
-                        // we allocate a new bitmap
+                        // alloc new region
                         &immutable & rhs
                     }
-                    // we can mutate in place, happy days.
                     Either::Right(mut mutable) => {
+                        // mutate in place
                         let mut mutable_ref = &mut mutable;
                         mutable_ref &= rhs;
                         mutable.into()
@@ -70,32 +75,22 @@ where
                 }
             });
         }
-    }
+    };
 
-    // we need to take ownership for the `into_mut` call, but leave the `&mut` lhs intact
-    // so that we can later assign the result to out `&mut lhs`
-    let owned_lhs = std::mem::take(lhs);
-
-    *lhs = match owned_lhs.into_mut() {
-        // we take alloc and write to new buffer
-        Either::Left(mut immutable) => {
-            let values = immutable
-                .values()
-                .iter()
-                .zip(rhs.values().iter())
-                .map(|(l, r)| op(*l, *r))
-                .collect::<Vec<_>>();
-            immutable.set_values(values.into());
-            immutable
-        }
-        // we can mutate in place
-        Either::Right(mut mutable) => {
-            mutable.apply_values(|x| {
-                x.iter_mut()
-                    .zip(rhs.values().iter())
-                    .for_each(|(l, r)| *l = op(*l, *r))
-            });
-            mutable.into()
-        }
+    if let Some(values) = lhs.get_mut_values() {
+        // mutate values in place
+        values
+            .iter_mut()
+            .zip(rhs.values().iter())
+            .for_each(|(l, r)| *l = op(*l, *r));
+    } else {
+        // alloc new region
+        let values = lhs
+            .values()
+            .iter()
+            .zip(rhs.values().iter())
+            .map(|(l, r)| op(*l, *r))
+            .collect::<Vec<_>>();
+        lhs.set_values(values.into());
     }
 }
diff --git a/src/io/ipc/read/array/fixed_size_binary.rs b/src/io/ipc/read/array/fixed_size_binary.rs
index 1c7bc4aa8d..04029cd7c8 100644
--- a/src/io/ipc/read/array/fixed_size_binary.rs
+++ b/src/io/ipc/read/array/fixed_size_binary.rs
@@ -38,7 +38,7 @@ pub fn read_fixed_size_binary<R: Read + Seek>(
         .try_into()
         .map_err(|_| Error::from(OutOfSpecKind::NegativeFooterLength))?;
 
-    let length = length.saturating_mul(FixedSizeBinaryArray::get_size(&data_type));
+    let length = length.saturating_mul(FixedSizeBinaryArray::maybe_get_size(&data_type)?);
     let values = read_buffer(
         buffers,
         length,


Test patch:
diff --git a/tests/it/array/boolean/mod.rs b/tests/it/array/boolean/mod.rs
index ffd5867bbb..2dd47d480d 100644
--- a/tests/it/array/boolean/mod.rs
+++ b/tests/it/array/boolean/mod.rs
@@ -131,27 +131,3 @@ fn from_iter() {
     let a: BooleanArray = iter.collect();
     assert_eq!(a.len(), 2);
 }
-
-#[test]
-fn apply_values() {
-    let mut a = BooleanArray::from([Some(true), Some(false), None]);
-    a.apply_values_mut(|x| {
-        let mut a = std::mem::take(x);
-        a = !a;
-        *x = a;
-    });
-    let expected = BooleanArray::from([Some(false), Some(true), None]);
-    assert_eq!(a, expected);
-}
-
-#[test]
-fn apply_validity() {
-    let mut a = BooleanArray::from([Some(true), Some(false), None]);
-    a.apply_validity_mut(|x| {
-        let mut a = std::mem::take(x);
-        a = !a;
-        *x = a;
-    });
-    let expected = BooleanArray::from([None, None, Some(false)]);
-    assert_eq!(a, expected);
-}
diff --git a/tests/it/array/primitive/mod.rs b/tests/it/array/primitive/mod.rs
index bb0402f400..058d503f47 100644
--- a/tests/it/array/primitive/mod.rs
+++ b/tests/it/array/primitive/mod.rs
@@ -124,25 +124,3 @@ fn into_mut_3() {
     let array = PrimitiveArray::new(DataType::Int32, values, validity);
     assert!(array.into_mut().is_right());
 }
-
-#[test]
-fn apply_values() {
-    let mut a = PrimitiveArray::from([Some(1), Some(2), None]);
-    a.apply_values_mut(|x| {
-        x[0] = 10;
-    });
-    let expected = PrimitiveArray::from([Some(10), Some(2), None]);
-    assert_eq!(a, expected);
-}
-
-#[test]
-fn apply_validity() {
-    let mut a = PrimitiveArray::from([Some(1), Some(2), None]);
-    a.apply_validity_mut(|x| {
-        let mut a = std::mem::take(x);
-        a = !a;
-        *x = a;
-    });
-    let expected = PrimitiveArray::from([None, None, Some(0)]);
-    assert_eq!(a, expected);
-}