Repository: jorgecarleitao/arrow2
Pull request #303 (2021-08-20 06:30:22)

Improved MutableFixedSizeBinaryArray
* Added equality
* More tests
* `&mut [u8]` of its values
* `from_data`

and more tests.

codecov[bot]: # [Codecov](https://codecov.io/gh/jorgecarleitao/arrow2/pull/303?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao) Report
> Merging [#303](https://codecov.io/gh/jorgecarleitao/arrow2/pull/303?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao) (4238570) into [main](https://codecov.io/gh/jorgecarleitao/arrow2/commit/bbe607ccbf3d7079c1fac6ad53fc6e5d4368e165?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao) (bbe607c) will **increase** coverage by `0.03%`.
> The diff coverage is `93.24%`.

[![Impacted file tree graph](https://codecov.io/gh/jorgecarleitao/arrow2/pull/303/graphs/tree.svg?width=650&height=150&src=pr&token=AgyTF60R3D&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao)](https://codecov.io/gh/jorgecarleitao/arrow2/pull/303?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao)

```diff
@@            Coverage Diff             @@
##             main     #303      +/-   ##
==========================================
+ Coverage   80.52%   80.55%   +0.03%     
==========================================
  Files         323      323              
  Lines       21135    21193      +58     
==========================================
+ Hits        17019    17073      +54     
- Misses       4116     4120       +4     
```


| [Impacted Files](https://codecov.io/gh/jorgecarleitao/arrow2/pull/303?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao) | Coverage Δ | |
|---|---|---|
| [src/array/fixed\_size\_binary/iterator.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/303/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2FycmF5L2ZpeGVkX3NpemVfYmluYXJ5L2l0ZXJhdG9yLnJz) | `75.00% <82.35%> (+10.71%)` | :arrow_up: |
| [src/array/fixed\_size\_binary/mutable.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/303/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2FycmF5L2ZpeGVkX3NpemVfYmluYXJ5L211dGFibGUucnM=) | `75.00% <92.85%> (+10.08%)` | :arrow_up: |
| [src/array/fixed\_size\_binary/mod.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/303/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2FycmF5L2ZpeGVkX3NpemVfYmluYXJ5L21vZC5ycw==) | `45.76% <100.00%> (-1.52%)` | :arrow_down: |
| [src/array/primitive/mutable.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/303/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2FycmF5L3ByaW1pdGl2ZS9tdXRhYmxlLnJz) | `87.09% <100.00%> (ø)` | |
| [tests/it/array/fixed\_size\_binary/mutable.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/303/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-dGVzdHMvaXQvYXJyYXkvZml4ZWRfc2l6ZV9iaW5hcnkvbXV0YWJsZS5ycw==) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/jorgecarleitao/arrow2/pull/303?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/303?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao). Last update [bbe607c...4238570](https://codecov.io/gh/jorgecarleitao/arrow2/pull/303?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/src/array/fixed_size_binary/iterator.rs b/src/array/fixed_size_binary/iterator.rs
index b24117fea7..9eb77b43fd 100644
--- a/src/array/fixed_size_binary/iterator.rs
+++ b/src/array/fixed_size_binary/iterator.rs
@@ -1,46 +1,54 @@
-use crate::array::Array;
 use crate::bitmap::utils::{zip_validity, ZipValidity};
 
-use super::FixedSizeBinaryArray;
+use super::super::MutableArray;
+use super::{FixedSizeBinaryArray, FixedSizeBinaryValues, MutableFixedSizeBinaryArray};
 
 /// # Safety
 /// This iterator is `TrustedLen`
-pub struct FixedSizeBinaryValuesIter<'a> {
-    array: &'a FixedSizeBinaryArray,
+pub struct FixedSizeBinaryValuesIter<'a, T: FixedSizeBinaryValues> {
+    array: &'a T,
+    len: usize,
     index: usize,
 }
 
-impl<'a> FixedSizeBinaryValuesIter<'a> {
+impl<'a, T: FixedSizeBinaryValues> FixedSizeBinaryValuesIter<'a, T> {
     #[inline]
-    pub fn new(array: &'a FixedSizeBinaryArray) -> Self {
-        Self { array, index: 0 }
+    pub fn new(array: &'a T) -> Self {
+        Self {
+            array,
+            len: array.values().len() / array.size(),
+            index: 0,
+        }
     }
 }
 
-impl<'a> Iterator for FixedSizeBinaryValuesIter<'a> {
+impl<'a, T: FixedSizeBinaryValues> Iterator for FixedSizeBinaryValuesIter<'a, T> {
     type Item = &'a [u8];
 
     #[inline]
     fn next(&mut self) -> Option<Self::Item> {
-        if self.index >= self.array.len() {
+        if self.index >= self.len {
             return None;
-        } else {
-            self.index += 1;
         }
-        Some(unsafe { self.array.value_unchecked(self.index - 1) })
+        let index = self.index;
+        let r = Some(unsafe {
+            std::slice::from_raw_parts(
+                self.array.values().as_ptr().add(index * self.array.size()),
+                self.array.size(),
+            )
+        });
+        self.index += 1;
+        r
     }
 
     fn size_hint(&self) -> (usize, Option<usize>) {
-        (
-            self.array.len() - self.index,
-            Some(self.array.len() - self.index),
-        )
+        (self.len - self.index, Some(self.len - self.index))
     }
 }
 
 impl<'a> IntoIterator for &'a FixedSizeBinaryArray {
     type Item = Option<&'a [u8]>;
-    type IntoIter = ZipValidity<'a, &'a [u8], FixedSizeBinaryValuesIter<'a>>;
+    type IntoIter = ZipValidity<'a, &'a [u8], FixedSizeBinaryValuesIter<'a, FixedSizeBinaryArray>>;
 
     fn into_iter(self) -> Self::IntoIter {
         self.iter()
@@ -49,10 +57,34 @@ impl<'a> IntoIterator for &'a FixedSizeBinaryArray {
 
 impl<'a> FixedSizeBinaryArray {
     /// constructs a new iterator
-    pub fn iter(&'a self) -> ZipValidity<'a, &'a [u8], FixedSizeBinaryValuesIter<'a>> {
+    pub fn iter(
+        &'a self,
+    ) -> ZipValidity<'a, &'a [u8], FixedSizeBinaryValuesIter<'a, FixedSizeBinaryArray>> {
         zip_validity(
             FixedSizeBinaryValuesIter::new(self),
             self.validity.as_ref().map(|x| x.iter()),
         )
     }
 }
+
+impl<'a> IntoIterator for &'a MutableFixedSizeBinaryArray {
+    type Item = Option<&'a [u8]>;
+    type IntoIter =
+        ZipValidity<'a, &'a [u8], FixedSizeBinaryValuesIter<'a, MutableFixedSizeBinaryArray>>;
+
+    fn into_iter(self) -> Self::IntoIter {
+        self.iter()
+    }
+}
+
+impl<'a> MutableFixedSizeBinaryArray {
+    /// constructs a new iterator
+    pub fn iter(
+        &'a self,
+    ) -> ZipValidity<'a, &'a [u8], FixedSizeBinaryValuesIter<'a, MutableFixedSizeBinaryArray>> {
+        zip_validity(
+            FixedSizeBinaryValuesIter::new(self),
+            self.validity().as_ref().map(|x| x.iter()),
+        )
+    }
+}
diff --git a/src/array/fixed_size_binary/mod.rs b/src/array/fixed_size_binary/mod.rs
index ee4405cfd2..714dccc8cc 100644
--- a/src/array/fixed_size_binary/mod.rs
+++ b/src/array/fixed_size_binary/mod.rs
@@ -161,3 +161,20 @@ impl FixedSizeBinaryArray {
             .into()
     }
 }
+
+pub trait FixedSizeBinaryValues {
+    fn values(&self) -> &[u8];
+    fn size(&self) -> usize;
+}
+
+impl FixedSizeBinaryValues for FixedSizeBinaryArray {
+    #[inline]
+    fn values(&self) -> &[u8] {
+        &self.values
+    }
+
+    #[inline]
+    fn size(&self) -> usize {
+        self.size as usize
+    }
+}
diff --git a/src/array/fixed_size_binary/mutable.rs b/src/array/fixed_size_binary/mutable.rs
index 395ecfca3c..4f06eae347 100644
--- a/src/array/fixed_size_binary/mutable.rs
+++ b/src/array/fixed_size_binary/mutable.rs
@@ -8,7 +8,7 @@ use crate::{
     error::{ArrowError, Result},
 };
 
-use super::FixedSizeBinaryArray;
+use super::{FixedSizeBinaryArray, FixedSizeBinaryValues};
 
 /// Mutable version of [`FixedSizeBinaryArray`].
 #[derive(Debug)]
@@ -30,19 +30,44 @@ impl From<MutableFixedSizeBinaryArray> for FixedSizeBinaryArray {
 }
 
 impl MutableFixedSizeBinaryArray {
+    pub fn from_data(
+        size: usize,
+        values: MutableBuffer<u8>,
+        validity: Option<MutableBitmap>,
+    ) -> Self {
+        assert_eq!(
+            values.len() % size,
+            0,
+            "The len of values must be a multiple of size"
+        );
+        if let Some(validity) = &validity {
+            assert_eq!(
+                validity.len(),
+                values.len() / size,
+                "The len of the validity must be equal to values / size"
+            );
+        }
+        Self {
+            data_type: DataType::FixedSizeBinary(size as i32),
+            size,
+            values,
+            validity,
+        }
+    }
+
     pub fn new(size: usize) -> Self {
         Self::with_capacity(size, 0)
     }
 
     pub fn with_capacity(size: usize, capacity: usize) -> Self {
-        Self {
-            data_type: DataType::FixedSizeBinary(size as i32),
+        Self::from_data(
             size,
-            values: MutableBuffer::<u8>::with_capacity(capacity * size),
-            validity: None,
-        }
+            MutableBuffer::<u8>::with_capacity(capacity * size),
+            None,
+        )
     }
 
+    #[inline]
     pub fn try_push<P: AsRef<[u8]>>(&mut self, value: Option<P>) -> Result<()> {
         match value {
             Some(bytes) => {
@@ -89,11 +114,37 @@ impl MutableFixedSizeBinaryArray {
     }
 
     fn init_validity(&mut self) {
-        self.validity = Some(MutableBitmap::from_trusted_len_iter(
-            std::iter::repeat(true)
-                .take(self.len() - 1)
-                .chain(std::iter::once(false)),
-        ))
+        let mut validity = MutableBitmap::new();
+        validity.extend_constant(self.len(), true);
+        validity.set(self.len() - 1, false);
+        self.validity = Some(validity)
+    }
+
+    /// Returns the element at index `i` as `&[u8]`
+    #[inline]
+    pub fn value(&self, i: usize) -> &[u8] {
+        &self.values[i * self.size..(i + 1) * self.size]
+    }
+
+    /// Returns the element at index `i` as `&[u8]`
+    /// # Safety
+    /// Assumes that the `i < self.len`.
+    #[inline]
+    pub unsafe fn value_unchecked(&self, i: usize) -> &[u8] {
+        std::slice::from_raw_parts(self.values.as_ptr().add(i * self.size), self.size)
+    }
+}
+
+/// Accessors
+impl MutableFixedSizeBinaryArray {
+    /// Returns its values.
+    pub fn values(&self) -> &MutableBuffer<u8> {
+        &self.values
+    }
+
+    /// Returns a mutable slice of values.
+    pub fn values_mut_slice(&mut self) -> &mut [u8] {
+        self.values.as_mut_slice()
     }
 }
 
@@ -130,3 +181,21 @@ impl MutableArray for MutableFixedSizeBinaryArray {
         self.values.extend_constant(self.size, 0);
     }
 }
+
+impl FixedSizeBinaryValues for MutableFixedSizeBinaryArray {
+    #[inline]
+    fn values(&self) -> &[u8] {
+        &self.values
+    }
+
+    #[inline]
+    fn size(&self) -> usize {
+        self.size
+    }
+}
+
+impl PartialEq for MutableFixedSizeBinaryArray {
+    fn eq(&self, other: &Self) -> bool {
+        self.iter().eq(other.iter())
+    }
+}
diff --git a/src/array/primitive/mutable.rs b/src/array/primitive/mutable.rs
index a4f771ebe3..9e126ee485 100644
--- a/src/array/primitive/mutable.rs
+++ b/src/array/primitive/mutable.rs
@@ -173,11 +173,10 @@ impl<T: NativeType> MutablePrimitiveArray<T> {
     }
 
     fn init_validity(&mut self) {
-        self.validity = Some(MutableBitmap::from_trusted_len_iter(
-            std::iter::repeat(true)
-                .take(self.len() - 1)
-                .chain(std::iter::once(false)),
-        ))
+        let mut validity = MutableBitmap::new();
+        validity.extend_constant(self.len(), true);
+        validity.set(self.len() - 1, false);
+        self.validity = Some(validity)
     }
 
     /// Changes the arrays' [`DataType`], returning a new [`MutablePrimitiveArray`].
diff --git a/src/io/json/read/deserialize.rs b/src/io/json/read/deserialize.rs
index 18c7499861..03fb257a27 100644
--- a/src/io/json/read/deserialize.rs
+++ b/src/io/json/read/deserialize.rs
@@ -25,10 +25,7 @@ use serde_json::Value;
 
 use crate::types::NaturalDataType;
 use crate::{
-    array::{
-        Array, BooleanArray, DictionaryArray, DictionaryKey, ListArray, NullArray, Offset,
-        PrimitiveArray, StructArray, Utf8Array,
-    },
+    array::*,
     bitmap::MutableBitmap,
     buffer::MutableBuffer,
     datatypes::{DataType, IntervalUnit},
@@ -90,6 +87,14 @@ fn read_primitive<T: NativeType + NaturalDataType + NumCast>(
     PrimitiveArray::from_trusted_len_iter(iter).to(data_type)
 }
 
+fn read_binary<O: Offset>(rows: &[&Value]) -> BinaryArray<O> {
+    let iter = rows.iter().map(|row| match row {
+        Value::String(v) => Some(v.as_bytes()),
+        _ => None,
+    });
+    BinaryArray::from_trusted_len_iter(iter)
+}
+
 fn read_boolean(rows: &[&Value]) -> BooleanArray {
     let iter = rows.iter().map(|row| match row {
         Value::Bool(v) => Some(v),
@@ -236,9 +241,8 @@ pub fn read(rows: &[&Value], data_type: DataType) -> Arc<dyn Array> {
         DataType::LargeUtf8 => Arc::new(read_utf8::<i64>(rows)),
         DataType::List(_) => Arc::new(read_list::<i32>(rows, data_type)),
         DataType::LargeList(_) => Arc::new(read_list::<i64>(rows, data_type)),
-        // unsupported
-        //DataType::Binary => Box::new(BinaryArray::<i32>::new_empty()),
-        //DataType::LargeBinary => Box::new(BinaryArray::<i64>::new_empty()),
+        DataType::Binary => Arc::new(read_binary::<i32>(rows)),
+        DataType::LargeBinary => Arc::new(read_binary::<i64>(rows)),
         DataType::Struct(_) => Arc::new(read_struct(rows, data_type)),
         DataType::Dictionary(key_type, _) => match key_type.as_ref() {
             DataType::Int8 => Arc::new(read_dictionary::<i8>(rows, data_type)),


Test patch:
diff --git a/tests/it/array/fixed_size_binary/mutable.rs b/tests/it/array/fixed_size_binary/mutable.rs
index 064fcaddda..d8c27ccee5 100644
--- a/tests/it/array/fixed_size_binary/mutable.rs
+++ b/tests/it/array/fixed_size_binary/mutable.rs
@@ -1,8 +1,44 @@
 use arrow2::array::*;
-use arrow2::bitmap::Bitmap;
+use arrow2::bitmap::{Bitmap, MutableBitmap};
+use arrow2::buffer::MutableBuffer;
+use arrow2::datatypes::DataType;
 
 #[test]
 fn basic() {
+    let a = MutableFixedSizeBinaryArray::from_data(2, MutableBuffer::from([1, 2, 3, 4]), None);
+    assert_eq!(a.len(), 2);
+    assert_eq!(a.data_type(), &DataType::FixedSizeBinary(2));
+    assert_eq!(a.values(), &MutableBuffer::from([1, 2, 3, 4]));
+    assert_eq!(a.validity(), &None);
+    assert_eq!(a.value(1), &[3, 4]);
+    assert_eq!(unsafe { a.value_unchecked(1) }, &[3, 4]);
+}
+
+#[allow(clippy::eq_op)]
+#[test]
+fn equal() {
+    let a = MutableFixedSizeBinaryArray::from_data(2, MutableBuffer::from([1, 2, 3, 4]), None);
+    assert_eq!(a, a);
+    let b = MutableFixedSizeBinaryArray::from_data(2, MutableBuffer::from([1, 2]), None);
+    assert_eq!(b, b);
+    assert!(a != b);
+    let a = MutableFixedSizeBinaryArray::from_data(
+        2,
+        MutableBuffer::from([1, 2, 3, 4]),
+        Some(MutableBitmap::from([true, false])),
+    );
+    let b = MutableFixedSizeBinaryArray::from_data(
+        2,
+        MutableBuffer::from([1, 2, 3, 4]),
+        Some(MutableBitmap::from([false, true])),
+    );
+    assert_eq!(a, a);
+    assert_eq!(b, b);
+    assert!(a != b);
+}
+
+#[test]
+fn try_from_iter() {
     let array = MutableFixedSizeBinaryArray::try_from_iter(
         vec![Some(b"ab"), Some(b"bc"), None, Some(b"fh")],
         2,
diff --git a/tests/it/io/json/mod.rs b/tests/it/io/json/mod.rs
index 37810e9ec3..b8bc9a5f7b 100644
--- a/tests/it/io/json/mod.rs
+++ b/tests/it/io/json/mod.rs
@@ -167,8 +167,8 @@ fn case_basics() -> (String, Schema, Vec<Box<dyn Array>>) {
 }
 
 fn case_basics_schema() -> (String, Schema, Vec<Box<dyn Array>>) {
-    let data = r#"{"a":1, "b":2.0, "c":false, "d":"4"}
-    {"a":10, "b":-3.5, "c":true, "d":null}
+    let data = r#"{"a":1, "b":2.0, "c":false, "d":"4", "e":"4"}
+    {"a":10, "b":-3.5, "c":true, "d":null, "e":"text"}
     {"a":100000000, "b":0.6, "d":"text"}"#
         .to_string();
     let schema = Schema::new(vec![
@@ -176,11 +176,17 @@ fn case_basics_schema() -> (String, Schema, Vec<Box<dyn Array>>) {
         Field::new("b", DataType::Float32, true),
         Field::new("c", DataType::Boolean, true),
         // note how "d" is not here
+        Field::new("e", DataType::Binary, true),
     ]);
     let columns = vec![
         Box::new(UInt32Array::from_slice(&[1, 10, 100000000])) as Box<dyn Array>,
         Box::new(Float32Array::from_slice(&[2.0, -3.5, 0.6])),
         Box::new(BooleanArray::from(&[Some(false), Some(true), None])),
+        Box::new(BinaryArray::<i32>::from(&[
+            Some(b"4".as_ref()),
+            Some(b"text".as_ref()),
+            None,
+        ])),
     ];
     (data, schema, columns)
 }