Repository: jorgecarleitao/arrow2
Pull request #1255 (2022-09-22 14:46:23)

Added `BinaryArray::into_mut` and double-ended support for its iterator
Copy pasted the implementation of `Utf8ValuesIter`

codecov[bot]: # [Codecov](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1255?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao) Report
Base: **83.12**% // Head: **83.10**% // Decreases project coverage by **`-0.02%`** :warning:
> Coverage data is based on head [(`271b46d`)](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1255?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao) compared to base [(`e972df0`)](https://codecov.io/gh/jorgecarleitao/arrow2/commit/e972df0fbe9aa1481831db53f47771b0ca1954a5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao).
> Patch coverage: 45.83% of modified lines in pull request are covered.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main    #1255      +/-   ##
==========================================
- Coverage   83.12%   83.10%   -0.03%     
==========================================
  Files         359      359              
  Lines       38087    38088       +1     
==========================================
- Hits        31661    31652       -9     
- Misses       6426     6436      +10     
```


| [Impacted Files](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1255?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/iterator.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2FycmF5L2JpbmFyeS9pdGVyYXRvci5ycw==) | `50.00% <45.83%> (-30.96%)` | :arrow_down: |
| [src/bitmap/immutable.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2JpdG1hcC9pbW11dGFibGUucnM=) | `85.80% <0.00%> (-0.62%)` | :arrow_down: |
| [src/io/ipc/read/schema.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2lvL2lwYy9yZWFkL3NjaGVtYS5ycw==) | `95.58% <0.00%> (-0.30%)` | :arrow_down: |
| [src/compute/sort/lex\_sort.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2NvbXB1dGUvc29ydC9sZXhfc29ydC5ycw==) | `75.80% <0.00%> (-0.20%)` | :arrow_down: |
| [src/compute/arithmetics/decimal/div.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2NvbXB1dGUvYXJpdGhtZXRpY3MvZGVjaW1hbC9kaXYucnM=) | `95.27% <0.00%> (-0.13%)` | :arrow_down: |
| [src/compute/arithmetics/decimal/add.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2NvbXB1dGUvYXJpdGhtZXRpY3MvZGVjaW1hbC9hZGQucnM=) | `96.59% <0.00%> (-0.12%)` | :arrow_down: |
| [src/compute/arithmetics/decimal/sub.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2NvbXB1dGUvYXJpdGhtZXRpY3MvZGVjaW1hbC9zdWIucnM=) | `96.73% <0.00%> (-0.11%)` | :arrow_down: |
| [src/array/struct\_/fmt.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2FycmF5L3N0cnVjdF8vZm10LnJz) | `100.00% <0.00%> (ø)` | |
| [src/compute/merge\_sort/mod.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2NvbXB1dGUvbWVyZ2Vfc29ydC9tb2QucnM=) | `95.60% <0.00%> (ø)` | |
| ... and [5 more](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1255/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao) | |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao)

</details>

[:umbrella: View full report at Codecov](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1255?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao).

ozgrakkurt: Hey @jorgecarleitao, I think the tests are failing because of the code that already exists in main branch 

Patch:
diff --git a/Cargo.toml b/Cargo.toml
index bd3674de4d..a5141e2a16 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -50,7 +50,7 @@ regex-syntax = { version = "^0.6", optional = true }
 streaming-iterator = { version = "0.1", optional = true }
 fallible-streaming-iterator = { version = "0.1", optional = true }
 
-json-deserializer = { version = "0.4", optional = true, features = ["preserve_order"] }
+json-deserializer = { version = "0.4.1", optional = true, features = ["preserve_order"] }
 indexmap = { version = "^1.6", optional = true }
 
 # used to print columns in a nice columnar format
diff --git a/src/array/binary/iterator.rs b/src/array/binary/iterator.rs
index 10f0e8a43d..4c80a78042 100644
--- a/src/array/binary/iterator.rs
+++ b/src/array/binary/iterator.rs
@@ -7,12 +7,17 @@ use super::BinaryArray;
 pub struct BinaryValueIter<'a, O: Offset> {
     array: &'a BinaryArray<O>,
     index: usize,
+    end: usize,
 }
 
 impl<'a, O: Offset> BinaryValueIter<'a, O> {
     /// Creates a new [`BinaryValueIter`]
     pub fn new(array: &'a BinaryArray<O>) -> Self {
-        Self { array, index: 0 }
+        Self {
+            array,
+            index: 0,
+            end: array.len(),
+        }
     }
 }
 
@@ -21,19 +26,41 @@ impl<'a, O: Offset> Iterator for BinaryValueIter<'a, O> {
 
     #[inline]
     fn next(&mut self) -> Option<Self::Item> {
-        if self.index >= self.array.len() {
+        if self.index == self.end {
             return None;
-        } else {
-            self.index += 1;
         }
-        Some(unsafe { self.array.value_unchecked(self.index - 1) })
+        let old = self.index;
+        self.index += 1;
+        Some(unsafe { self.array.value_unchecked(old) })
     }
 
+    #[inline]
     fn size_hint(&self) -> (usize, Option<usize>) {
-        (
-            self.array.len() - self.index,
-            Some(self.array.len() - self.index),
-        )
+        (self.end - self.index, Some(self.end - self.index))
+    }
+
+    #[inline]
+    fn nth(&mut self, n: usize) -> Option<Self::Item> {
+        let new_index = self.index + n;
+        if new_index > self.end {
+            self.index = self.end;
+            None
+        } else {
+            self.index = new_index;
+            self.next()
+        }
+    }
+}
+
+impl<'a, O: Offset> DoubleEndedIterator for BinaryValueIter<'a, O> {
+    #[inline]
+    fn next_back(&mut self) -> Option<Self::Item> {
+        if self.index == self.end {
+            None
+        } else {
+            self.end -= 1;
+            Some(unsafe { self.array.value_unchecked(self.end) })
+        }
     }
 }
 
diff --git a/src/array/binary/mod.rs b/src/array/binary/mod.rs
index b1efa2f6f2..39c2a4fcfe 100644
--- a/src/array/binary/mod.rs
+++ b/src/array/binary/mod.rs
@@ -9,6 +9,8 @@ use crate::{
     trusted_len::TrustedLen,
 };
 
+use either::Either;
+
 use super::{
     specification::{try_check_offsets, try_check_offsets_bounds},
     Array, GenericBinaryArray, Offset,
@@ -238,6 +240,88 @@ impl<O: Offset> BinaryArray<O> {
         self.validity = validity;
     }
 
+    /// Try to convert this `BinaryArray` to a `MutableBinaryArray`
+    pub fn into_mut(mut self) -> Either<Self, MutableBinaryArray<O>> {
+        use Either::*;
+        if let Some(bitmap) = self.validity {
+            match bitmap.into_mut() {
+                // Safety: invariants are preserved
+                Left(bitmap) => Left(unsafe {
+                    BinaryArray::new_unchecked(
+                        self.data_type,
+                        self.offsets,
+                        self.values,
+                        Some(bitmap),
+                    )
+                }),
+                Right(mutable_bitmap) => match (
+                    self.values.get_mut().map(std::mem::take),
+                    self.offsets.get_mut().map(std::mem::take),
+                ) {
+                    (None, None) => {
+                        // Safety: invariants are preserved
+                        Left(unsafe {
+                            BinaryArray::new_unchecked(
+                                self.data_type,
+                                self.offsets,
+                                self.values,
+                                Some(mutable_bitmap.into()),
+                            )
+                        })
+                    }
+                    (None, Some(offsets)) => {
+                        // Safety: invariants are preserved
+                        Left(unsafe {
+                            BinaryArray::new_unchecked(
+                                self.data_type,
+                                offsets.into(),
+                                self.values,
+                                Some(mutable_bitmap.into()),
+                            )
+                        })
+                    }
+                    (Some(mutable_values), None) => {
+                        // Safety: invariants are preserved
+                        Left(unsafe {
+                            BinaryArray::new_unchecked(
+                                self.data_type,
+                                self.offsets,
+                                mutable_values.into(),
+                                Some(mutable_bitmap.into()),
+                            )
+                        })
+                    }
+                    (Some(values), Some(offsets)) => Right(unsafe {
+                        MutableBinaryArray::from_data(
+                            self.data_type,
+                            offsets,
+                            values,
+                            Some(mutable_bitmap),
+                        )
+                    }),
+                },
+            }
+        } else {
+            match (
+                self.values.get_mut().map(std::mem::take),
+                self.offsets.get_mut().map(std::mem::take),
+            ) {
+                (None, None) => Left(unsafe {
+                    BinaryArray::new_unchecked(self.data_type, self.offsets, self.values, None)
+                }),
+                (None, Some(offsets)) => Left(unsafe {
+                    BinaryArray::new_unchecked(self.data_type, offsets.into(), self.values, None)
+                }),
+                (Some(values), None) => Left(unsafe {
+                    BinaryArray::new_unchecked(self.data_type, self.offsets, values.into(), None)
+                }),
+                (Some(values), Some(offsets)) => Right(unsafe {
+                    MutableBinaryArray::from_data(self.data_type, offsets, values, None)
+                }),
+            }
+        }
+    }
+
     /// Creates an empty [`BinaryArray`], i.e. whose `.len` is zero.
     pub fn new_empty(data_type: DataType) -> Self {
         Self::new(
diff --git a/src/array/binary/mutable.rs b/src/array/binary/mutable.rs
index 9990a542c2..c1ee4ba8a6 100644
--- a/src/array/binary/mutable.rs
+++ b/src/array/binary/mutable.rs
@@ -345,6 +345,21 @@ impl<O: Offset> MutableBinaryArray<O> {
         unsafe { self.extend_trusted_len_values_unchecked(iterator) }
     }
 
+    /// Extends the [`MutableBinaryArray`] from an iterator of values.
+    /// This differs from `extended_trusted_len` which accepts iterator of optional values.
+    #[inline]
+    pub fn extend_values<I, P>(&mut self, iterator: I)
+    where
+        P: AsRef<[u8]>,
+        I: Iterator<Item = P>,
+    {
+        let additional = extend_from_values_iter(&mut self.offsets, &mut self.values, iterator);
+
+        if let Some(validity) = self.validity.as_mut() {
+            validity.extend_constant(additional, true);
+        }
+    }
+
     /// Extends the [`MutableBinaryArray`] from an `iterator` of values of trusted length.
     /// This differs from `extend_trusted_len_unchecked` which accepts iterator of optional
     /// values.
diff --git a/src/io/parquet/write/binary/basic.rs b/src/io/parquet/write/binary/basic.rs
index f8a8cdc47a..58156c901e 100644
--- a/src/io/parquet/write/binary/basic.rs
+++ b/src/io/parquet/write/binary/basic.rs
@@ -138,13 +138,6 @@ pub(crate) fn encode_delta<O: Offset>(
 
             delta_bitpacked::encode(lengths, buffer);
         } else {
-            println!(
-                "{:?}",
-                offsets
-                    .windows(2)
-                    .map(|w| (w[1] - w[0]).to_usize() as i64)
-                    .collect::<Vec<_>>()
-            );
             let lengths = offsets.windows(2).map(|w| (w[1] - w[0]).to_usize() as i64);
             delta_bitpacked::encode(lengths, buffer);
         }
diff --git a/src/io/parquet/write/nested/mod.rs b/src/io/parquet/write/nested/mod.rs
index 9fd603a524..d9f01e3f48 100644
--- a/src/io/parquet/write/nested/mod.rs
+++ b/src/io/parquet/write/nested/mod.rs
@@ -37,9 +37,6 @@ fn write_rep_levels(buffer: &mut Vec<u8>, nested: &[Nested], version: Version) -
 
     let levels = rep::RepLevelsIter::new(nested);
 
-    let mut buffer1 = vec![];
-    encode_u32(&mut buffer1, rep::RepLevelsIter::new(nested), num_bits).unwrap();
-
     match version {
         Version::V1 => {
             write_levels_v1(buffer, |buffer: &mut Vec<u8>| {
diff --git a/src/lib.rs b/src/lib.rs
index 16bd56ed04..e479cb7d08 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -4,6 +4,8 @@
 #![allow(unused_unsafe)]
 //
 #![allow(clippy::len_without_is_empty)]
+// this landed on 1.60. Let's not force everyone to bump just yet
+#![allow(clippy::unnecessary_lazy_evaluations)]
 // Trait objects must be returned as a &Box<dyn Array> so that they can be cloned
 #![allow(clippy::borrowed_box)]
 // Allow type complexity warning to avoid API break.


Test patch:
diff --git a/tests/it/array/binary/mod.rs b/tests/it/array/binary/mod.rs
index 3e93a35c56..c36b712141 100644
--- a/tests/it/array/binary/mod.rs
+++ b/tests/it/array/binary/mod.rs
@@ -6,6 +6,7 @@ use arrow2::{
 };
 
 mod mutable;
+mod to_mutable;
 
 #[test]
 fn basics() {
@@ -152,3 +153,61 @@ fn debug() {
 
     assert_eq!(format!("{:?}", array), "BinaryArray[[1, 2], [], None]");
 }
+
+#[test]
+fn into_mut_1() {
+    let offsets = Buffer::from(vec![0, 1]);
+    let values = Buffer::from(b"a".to_vec());
+    let a = values.clone(); // cloned values
+    assert_eq!(a, values);
+    let array = BinaryArray::<i32>::from_data(DataType::Binary, offsets, values, None);
+    assert!(array.into_mut().is_left());
+}
+
+#[test]
+fn into_mut_2() {
+    let offsets = Buffer::from(vec![0, 1]);
+    let values = Buffer::from(b"a".to_vec());
+    let a = offsets.clone(); // cloned offsets
+    assert_eq!(a, offsets);
+    let array = BinaryArray::<i32>::from_data(DataType::Binary, offsets, values, None);
+    assert!(array.into_mut().is_left());
+}
+
+#[test]
+fn into_mut_3() {
+    let offsets = Buffer::from(vec![0, 1]);
+    let values = Buffer::from(b"a".to_vec());
+    let validity = Some([true].into());
+    let a = validity.clone(); // cloned validity
+    assert_eq!(a, validity);
+    let array = BinaryArray::<i32>::new(DataType::Binary, offsets, values, validity);
+    assert!(array.into_mut().is_left());
+}
+
+#[test]
+fn into_mut_4() {
+    let offsets = Buffer::from(vec![0, 1]);
+    let values = Buffer::from(b"a".to_vec());
+    let validity = Some([true].into());
+    let array = BinaryArray::<i32>::new(DataType::Binary, offsets, values, validity);
+    assert!(array.into_mut().is_right());
+}
+
+#[test]
+fn rev_iter() {
+    let array = BinaryArray::<i32>::from(&[Some("hello".as_bytes()), Some(" ".as_bytes()), None]);
+
+    assert_eq!(
+        array.into_iter().rev().collect::<Vec<_>>(),
+        vec![None, Some(" ".as_bytes()), Some("hello".as_bytes())]
+    );
+}
+
+#[test]
+fn iter_nth() {
+    let array = BinaryArray::<i32>::from(&[Some("hello"), Some(" "), None]);
+
+    assert_eq!(array.iter().nth(1), Some(Some(" ".as_bytes())));
+    assert_eq!(array.iter().nth(10), None);
+}
diff --git a/tests/it/array/binary/to_mutable.rs b/tests/it/array/binary/to_mutable.rs
new file mode 100644
index 0000000000..e828244abb
--- /dev/null
+++ b/tests/it/array/binary/to_mutable.rs
@@ -0,0 +1,67 @@
+use arrow2::{array::BinaryArray, bitmap::Bitmap, buffer::Buffer, datatypes::DataType};
+
+#[test]
+fn not_shared() {
+    let array = BinaryArray::<i32>::from(&[Some("hello"), Some(" "), None]);
+    assert!(array.into_mut().is_right());
+}
+
+#[test]
+#[allow(clippy::redundant_clone)]
+fn shared_validity() {
+    let validity = Bitmap::from([true]);
+    let array = BinaryArray::<i32>::new(
+        DataType::Binary,
+        vec![0, 1].into(),
+        b"a".to_vec().into(),
+        Some(validity.clone()),
+    );
+    assert!(array.into_mut().is_left())
+}
+
+#[test]
+#[allow(clippy::redundant_clone)]
+fn shared_values() {
+    let values: Buffer<u8> = b"a".to_vec().into();
+    let array = BinaryArray::<i32>::new(
+        DataType::Binary,
+        vec![0, 1].into(),
+        values.clone(),
+        Some(Bitmap::from([true])),
+    );
+    assert!(array.into_mut().is_left())
+}
+
+#[test]
+#[allow(clippy::redundant_clone)]
+fn shared_offsets_values() {
+    let offsets: Buffer<i32> = vec![0, 1].into();
+    let values: Buffer<u8> = b"a".to_vec().into();
+    let array = BinaryArray::<i32>::new(
+        DataType::Binary,
+        offsets.clone(),
+        values.clone(),
+        Some(Bitmap::from([true])),
+    );
+    assert!(array.into_mut().is_left())
+}
+
+#[test]
+#[allow(clippy::redundant_clone)]
+fn shared_offsets() {
+    let offsets: Buffer<i32> = vec![0, 1].into();
+    let array = BinaryArray::<i32>::new(
+        DataType::Binary,
+        offsets.clone(),
+        b"a".to_vec().into(),
+        Some(Bitmap::from([true])),
+    );
+    assert!(array.into_mut().is_left())
+}
+
+#[test]
+#[allow(clippy::redundant_clone)]
+fn shared_all() {
+    let array = BinaryArray::<i32>::from(&[Some("hello"), Some(" "), None]);
+    assert!(array.clone().into_mut().is_left())
+}
diff --git a/tests/it/main.rs b/tests/it/main.rs
index 4c9448ea81..436969f47b 100644
--- a/tests/it/main.rs
+++ b/tests/it/main.rs
@@ -1,3 +1,6 @@
+// this landed on 1.60. Let's not force everyone to bump just yet
+#![allow(clippy::unnecessary_lazy_evaluations)]
+
 mod array;
 mod bitmap;
 mod buffer;