Repository: jorgecarleitao/arrow2
Pull request #1027 (2022-05-30 20:49:27)

Added more tests (increase coverage)
Also implemented `TrustedLen` for `iter.cloned()` when `iter` is trusted len.

codecov[bot]: # [Codecov](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1027?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao) Report
> Merging [#1027](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1027?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao) (406a7b8) into [main](https://codecov.io/gh/jorgecarleitao/arrow2/commit/266fbbe9e307b94a6006695b1c8654510e191804?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao) (266fbbe) will **increase** coverage by `0.41%`.
> The diff coverage is `88.57%`.

```diff
@@            Coverage Diff             @@
##             main    #1027      +/-   ##
==========================================
+ Coverage   71.62%   72.04%   +0.41%     
==========================================
  Files         359      359              
  Lines       19907    19926      +19     
==========================================
+ Hits        14259    14355      +96     
+ Misses       5648     5571      -77     
```


| [Impacted Files](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1027?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao) | Coverage Δ | |
|---|---|---|
| [src/array/binary/mutable.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1027/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2FycmF5L2JpbmFyeS9tdXRhYmxlLnJz) | `85.45% <ø> (+1.40%)` | :arrow_up: |
| [src/ffi/array.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1027/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2ZmaS9hcnJheS5ycw==) | `86.62% <ø> (+2.54%)` | :arrow_up: |
| [src/bitmap/mutable.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1027/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2JpdG1hcC9tdXRhYmxlLnJz) | `92.30% <87.87%> (+2.05%)` | :arrow_up: |
| [src/array/fixed\_size\_binary/mod.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1027/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2FycmF5L2ZpeGVkX3NpemVfYmluYXJ5L21vZC5ycw==) | `90.29% <100.00%> (+1.94%)` | :arrow_up: |
| [src/ffi/schema.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1027/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2ZmaS9zY2hlbWEucnM=) | `75.16% <100.00%> (+6.87%)` | :arrow_up: |
| [src/array/list/mod.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1027/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2FycmF5L2xpc3QvbW9kLnJz) | `70.79% <0.00%> (+0.88%)` | :arrow_up: |
| [src/array/mod.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1027/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2FycmF5L21vZC5ycw==) | `68.08% <0.00%> (+1.06%)` | :arrow_up: |
| [src/bitmap/utils/slice\_iterator.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1027/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2JpdG1hcC91dGlscy9zbGljZV9pdGVyYXRvci5ycw==) | `87.93% <0.00%> (+1.72%)` | :arrow_up: |
| ... and [10 more](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1027/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/1027?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/1027?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao). Last update [266fbbe...406a7b8](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1027?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/binary/mutable.rs b/src/array/binary/mutable.rs
index df12e6d9d6..78de609976 100644
--- a/src/array/binary/mutable.rs
+++ b/src/array/binary/mutable.rs
@@ -163,6 +163,7 @@ impl<O: Offset> MutableBinaryArray<O> {
         let a: BinaryArray<O> = self.into();
         Arc::new(a)
     }
+
     /// Shrinks the capacity of the [`MutableBinaryArray`] to fit its current length.
     pub fn shrink_to_fit(&mut self) {
         self.values.shrink_to_fit();
diff --git a/src/array/fixed_size_binary/mod.rs b/src/array/fixed_size_binary/mod.rs
index 4209998ec1..8ed5c5cec3 100644
--- a/src/array/fixed_size_binary/mod.rs
+++ b/src/array/fixed_size_binary/mod.rs
@@ -125,7 +125,7 @@ impl FixedSizeBinaryArray {
         let values = self
             .values
             .clone()
-            .slice_unchecked(offset * self.size as usize, length * self.size as usize);
+            .slice_unchecked(offset * self.size, length * self.size);
         Self {
             data_type: self.data_type.clone(),
             size: self.size,
diff --git a/src/ffi/array.rs b/src/ffi/array.rs
index 76df63ceb5..5ebdf5d7fc 100644
--- a/src/ffi/array.rs
+++ b/src/ffi/array.rs
@@ -243,6 +243,13 @@ fn buffer_offset(array: &ArrowArray, data_type: &DataType, i: usize) -> usize {
     use PhysicalType::*;
     match (data_type.to_physical_type(), i) {
         (LargeUtf8, 2) | (LargeBinary, 2) | (Utf8, 2) | (Binary, 2) => 0,
+        (FixedSizeBinary, 1) => {
+            if let DataType::FixedSizeBinary(size) = data_type.to_logical_type() {
+                (array.offset as usize) * *size
+            } else {
+                unreachable!()
+            }
+        }
         _ => array.offset as usize,
     }
 }
diff --git a/src/ffi/schema.rs b/src/ffi/schema.rs
index 3ab3b9edcf..e06ab4184c 100644
--- a/src/ffi/schema.rs
+++ b/src/ffi/schema.rs
@@ -306,6 +306,7 @@ unsafe fn to_data_type(schema: &ArrowSchema) -> Result<DataType> {
                     let size = size_raw
                         .parse::<usize>()
                         .map_err(|_| Error::OutOfSpec("size is not a valid integer".to_string()))?;
+                    println!("schema: {}", size);
                     DataType::FixedSizeBinary(size)
                 }
                 ["+w", size_raw] => {
diff --git a/src/trusted_len.rs b/src/trusted_len.rs
index 98ff458e56..a1c38bd51c 100644
--- a/src/trusted_len.rs
+++ b/src/trusted_len.rs
@@ -21,6 +21,12 @@ where
     T: Copy,
 {
 }
+unsafe impl<'a, I, T: 'a> TrustedLen for std::iter::Cloned<I>
+where
+    I: TrustedLen<Item = &'a T>,
+    T: Clone,
+{
+}
 
 unsafe impl<I> TrustedLen for std::iter::Enumerate<I> where I: TrustedLen {}
 


Test patch:
diff --git a/tests/it/array/binary/mod.rs b/tests/it/array/binary/mod.rs
index fb1bb9dcbe..3e93a35c56 100644
--- a/tests/it/array/binary/mod.rs
+++ b/tests/it/array/binary/mod.rs
@@ -66,6 +66,16 @@ fn from_trusted_len_iter() {
     assert_eq!(a.len(), 2);
 }
 
+#[test]
+fn try_from_trusted_len_iter() {
+    let iter = std::iter::repeat(b"hello".as_ref())
+        .take(2)
+        .map(Some)
+        .map(arrow2::error::Result::Ok);
+    let a = BinaryArray::<i32>::try_from_trusted_len_iter(iter).unwrap();
+    assert_eq!(a.len(), 2);
+}
+
 #[test]
 fn from_iter() {
     let iter = std::iter::repeat(b"hello").take(2).map(Some);
diff --git a/tests/it/array/binary/mutable.rs b/tests/it/array/binary/mutable.rs
index d16ba14ad7..e363b47bcf 100644
--- a/tests/it/array/binary/mutable.rs
+++ b/tests/it/array/binary/mutable.rs
@@ -35,30 +35,26 @@ fn from_iter() {
 
 #[test]
 fn from_trusted_len_iter() {
-    let iter = (0..3u8).map(|x| vec![x; x as usize]);
-    let a: MutableBinaryArray<i32> = iter.clone().map(Some).collect();
+    let data = vec![vec![0; 0], vec![1; 1], vec![2; 2]];
+    let a: MutableBinaryArray<i32> = data.iter().cloned().map(Some).collect();
     assert_eq!(a.values().deref(), &[1u8, 2, 2]);
     assert_eq!(a.offsets().deref(), &[0, 0, 1, 3]);
     assert_eq!(a.validity(), None);
 
-    let a = unsafe {
-        MutableBinaryArray::<i32>::from_trusted_len_iter_unchecked(iter.clone().map(Some))
-    };
+    let a = MutableBinaryArray::<i32>::from_trusted_len_iter(data.iter().cloned().map(Some));
     assert_eq!(a.values().deref(), &[1u8, 2, 2]);
     assert_eq!(a.offsets().deref(), &[0, 0, 1, 3]);
     assert_eq!(a.validity(), None);
 
-    let a = unsafe {
-        MutableBinaryArray::<i32>::try_from_trusted_len_iter_unchecked::<Error, _, _>(
-            iter.clone().map(Some).map(Ok),
-        )
-    }
+    let a = MutableBinaryArray::<i32>::try_from_trusted_len_iter::<Error, _, _>(
+        data.iter().cloned().map(Some).map(Ok),
+    )
     .unwrap();
     assert_eq!(a.values().deref(), &[1u8, 2, 2]);
     assert_eq!(a.offsets().deref(), &[0, 0, 1, 3]);
     assert_eq!(a.validity(), None);
 
-    let a = unsafe { MutableBinaryArray::<i32>::from_trusted_len_values_iter_unchecked(iter) };
+    let a = MutableBinaryArray::<i32>::from_trusted_len_values_iter(data.iter().cloned());
     assert_eq!(a.values().deref(), &[1u8, 2, 2]);
     assert_eq!(a.offsets().deref(), &[0, 0, 1, 3]);
     assert_eq!(a.validity(), None);
diff --git a/tests/it/array/boolean/mod.rs b/tests/it/array/boolean/mod.rs
index 9b9ee57bd2..2dd47d480d 100644
--- a/tests/it/array/boolean/mod.rs
+++ b/tests/it/array/boolean/mod.rs
@@ -39,6 +39,15 @@ fn basics() {
     assert!(!array.value(1));
 }
 
+#[test]
+fn try_new_invalid() {
+    assert!(BooleanArray::try_new(DataType::Int32, [true].into(), None).is_err());
+    assert!(
+        BooleanArray::try_new(DataType::Boolean, [true].into(), Some([false, true].into()))
+            .is_err()
+    );
+}
+
 #[test]
 fn with_validity() {
     let bitmap = Bitmap::from([true, false, true]);
@@ -58,14 +67,25 @@ fn into_mut_valid() {
     let bitmap = Bitmap::from([true, false, true]);
     let a = BooleanArray::from_data(DataType::Boolean, bitmap, None);
     let _ = a.into_mut().right().unwrap();
+
+    let bitmap = Bitmap::from([true, false, true]);
+    let validity = Bitmap::from([true, false, true]);
+    let a = BooleanArray::from_data(DataType::Boolean, bitmap, Some(validity));
+    let _ = a.into_mut().right().unwrap();
 }
 
 #[test]
 fn into_mut_invalid() {
     let bitmap = Bitmap::from([true, false, true]);
-    let _other = bitmap.clone(); // values are shared
+    let _other = bitmap.clone(); // values is shared
     let a = BooleanArray::from_data(DataType::Boolean, bitmap, None);
     let _ = a.into_mut().left().unwrap();
+
+    let bitmap = Bitmap::from([true, false, true]);
+    let validity = Bitmap::from([true, false, true]);
+    let _other = validity.clone(); // validity is shared
+    let a = BooleanArray::from_data(DataType::Boolean, bitmap, Some(validity));
+    let _ = a.into_mut().left().unwrap();
 }
 
 #[test]
@@ -78,7 +98,30 @@ fn empty() {
 #[test]
 fn from_trusted_len_iter() {
     let iter = std::iter::repeat(true).take(2).map(Some);
-    let a = BooleanArray::from_trusted_len_iter(iter);
+    let a = BooleanArray::from_trusted_len_iter(iter.clone());
+    assert_eq!(a.len(), 2);
+    let a = unsafe { BooleanArray::from_trusted_len_iter_unchecked(iter) };
+    assert_eq!(a.len(), 2);
+}
+
+#[test]
+fn try_from_trusted_len_iter() {
+    let iter = std::iter::repeat(true)
+        .take(2)
+        .map(Some)
+        .map(arrow2::error::Result::Ok);
+    let a = BooleanArray::try_from_trusted_len_iter(iter.clone()).unwrap();
+    assert_eq!(a.len(), 2);
+    let a = unsafe { BooleanArray::try_from_trusted_len_iter_unchecked(iter).unwrap() };
+    assert_eq!(a.len(), 2);
+}
+
+#[test]
+fn from_trusted_len_values_iter() {
+    let iter = std::iter::repeat(true).take(2);
+    let a = BooleanArray::from_trusted_len_values_iter(iter.clone());
+    assert_eq!(a.len(), 2);
+    let a = unsafe { BooleanArray::from_trusted_len_values_iter_unchecked(iter) };
     assert_eq!(a.len(), 2);
 }
 
diff --git a/tests/it/array/boolean/mutable.rs b/tests/it/array/boolean/mutable.rs
index 1a7d926b64..173472106a 100644
--- a/tests/it/array/boolean/mutable.rs
+++ b/tests/it/array/boolean/mutable.rs
@@ -142,3 +142,23 @@ fn extend_trusted_len_values() {
     );
     assert_eq!(a.values(), &MutableBitmap::from([false, true, false]));
 }
+
+#[test]
+fn into_iter() {
+    let ve = MutableBitmap::from([true, false])
+        .into_iter()
+        .collect::<Vec<_>>();
+    assert_eq!(ve, vec![true, false]);
+    let ve = MutableBitmap::from([true, false])
+        .iter()
+        .collect::<Vec<_>>();
+    assert_eq!(ve, vec![true, false]);
+}
+
+#[test]
+fn shrink_to_fit() {
+    let mut a = MutableBitmap::with_capacity(100);
+    a.push(true);
+    a.shrink_to_fit();
+    assert_eq!(a.capacity(), 8);
+}
diff --git a/tests/it/bitmap/mutable.rs b/tests/it/bitmap/mutable.rs
index d8bb85c067..f228c323b7 100644
--- a/tests/it/bitmap/mutable.rs
+++ b/tests/it/bitmap/mutable.rs
@@ -7,6 +7,32 @@ fn from_slice() {
     assert_eq!(a.iter().collect::<Vec<_>>(), slice);
 }
 
+#[test]
+fn from_len_zeroed() {
+    let a = MutableBitmap::from_len_zeroed(10);
+    assert_eq!(a.len(), 10);
+    assert_eq!(a.null_count(), 10);
+}
+
+#[test]
+fn from_len_set() {
+    let a = MutableBitmap::from_len_set(10);
+    assert_eq!(a.len(), 10);
+    assert_eq!(a.null_count(), 0);
+}
+
+#[test]
+fn try_new_invalid() {
+    assert!(MutableBitmap::try_new(vec![], 2).is_err());
+}
+
+#[test]
+fn clear() {
+    let mut a = MutableBitmap::from_len_zeroed(10);
+    a.clear();
+    assert_eq!(a.len(), 0);
+}
+
 #[test]
 fn trusted_len() {
     let data = vec![true; 65];
diff --git a/tests/it/ffi/data.rs b/tests/it/ffi/data.rs
index 63a4489a0d..39c9400114 100644
--- a/tests/it/ffi/data.rs
+++ b/tests/it/ffi/data.rs
@@ -81,6 +81,18 @@ fn u32() -> Result<()> {
     test_round_trip(data)
 }
 
+#[test]
+fn decimal() -> Result<()> {
+    let data = Int128Array::from_slice(&[1, 0, 2, 0]);
+    test_round_trip(data)
+}
+
+#[test]
+fn decimal_nullable() -> Result<()> {
+    let data = Int128Array::from(&[Some(1), None, Some(2), None]);
+    test_round_trip(data)
+}
+
 #[test]
 fn timestamp_tz() -> Result<()> {
     let data = Int64Array::from(&vec![Some(2), None, None]).to(DataType::Timestamp(
@@ -128,6 +140,26 @@ fn large_binary() -> Result<()> {
     test_round_trip(data)
 }
 
+#[test]
+fn fixed_size_binary() -> Result<()> {
+    let data = FixedSizeBinaryArray::new(
+        DataType::FixedSizeBinary(2),
+        vec![1, 2, 3, 4, 5, 6].into(),
+        None,
+    );
+    test_round_trip(data)
+}
+
+#[test]
+fn fixed_size_binary_nullable() -> Result<()> {
+    let data = FixedSizeBinaryArray::new(
+        DataType::FixedSizeBinary(2),
+        vec![1, 2, 3, 4, 5, 6].into(),
+        Some([true, true, false].into()),
+    );
+    test_round_trip(data)
+}
+
 #[test]
 fn list() -> Result<()> {
     let data = vec![
@@ -144,6 +176,22 @@ fn list() -> Result<()> {
     test_round_trip(array)
 }
 
+#[test]
+fn large_list() -> Result<()> {
+    let data = vec![
+        Some(vec![Some(1i32), Some(2), Some(3)]),
+        None,
+        Some(vec![Some(4), None, Some(6)]),
+    ];
+
+    let mut array = MutableListArray::<i64, MutablePrimitiveArray<i32>>::new();
+    array.try_extend(data)?;
+
+    let array: ListArray<i64> = array.into();
+
+    test_round_trip(array)
+}
+
 #[test]
 fn fixed_list() -> Result<()> {
     let data = vec![
@@ -231,7 +279,11 @@ fn schema() -> Result<()> {
 fn extension() -> Result<()> {
     let field = Field::new(
         "a",
-        DataType::Extension("a".to_string(), Box::new(DataType::Int32), None),
+        DataType::Extension(
+            "a".to_string(),
+            Box::new(DataType::Int32),
+            Some("bla".to_string()),
+        ),
         true,
     );
     test_round_trip_schema(field)