From 751a1536839e755e6cf781050d4183f86629f5d5 Mon Sep 17 00:00:00 2001
From: Adam Gutglick <adamgsal@gmail.com>
Date: Mon, 24 Jun 2024 21:37:55 +0300
Subject: [PATCH] More CR notes

---
 encodings/byte_bool/src/compute/mod.rs |  2 +-
 encodings/byte_bool/src/lib.rs         | 33 +++++++++++++-------------
 encodings/byte_bool/src/stats.rs       |  1 +
 3 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/encodings/byte_bool/src/compute/mod.rs b/encodings/byte_bool/src/compute/mod.rs
index d1fa3c8256..eb0e273f87 100644
--- a/encodings/byte_bool/src/compute/mod.rs
+++ b/encodings/byte_bool/src/compute/mod.rs
@@ -154,7 +154,7 @@ impl CompareFn for ByteBoolArray {
             validity.push(l & r);
         }
 
-        ByteBoolArray::try_with_validity(Vec::from_iter(result_buf.iter()), validity)
+        ByteBoolArray::try_from_vec(Vec::from_iter(result_buf.iter()), validity)
             .map(ByteBoolArray::into_array)
     }
 }
diff --git a/encodings/byte_bool/src/lib.rs b/encodings/byte_bool/src/lib.rs
index 2ac8b54321..7cb9508d6d 100644
--- a/encodings/byte_bool/src/lib.rs
+++ b/encodings/byte_bool/src/lib.rs
@@ -28,10 +28,20 @@ impl ByteBoolArray {
             .to_validity(self.array().child(0, &Validity::DTYPE))
     }
 
-    pub fn try_with_validity<V: Into<Validity>>(
-        data: Vec<bool>,
-        validity: V,
-    ) -> VortexResult<Self> {
+    pub fn try_new(buffer: Buffer, validity: Validity) -> VortexResult<Self> {
+        let length = buffer.len();
+
+        Self::try_from_parts(
+            DType::Bool(validity.nullability()),
+            ByteBoolMetadata {
+                validity: validity.to_metadata(length)?,
+            },
+            validity.into_array().into_iter().collect::<Vec<_>>().into(),
+            StatsSet::new(),
+        )
+    }
+
+    pub fn try_from_vec<V: Into<Validity>>(data: Vec<bool>, validity: V) -> VortexResult<Self> {
         let validity = validity.into();
         let mut vec = ManuallyDrop::new(data);
         vec.shrink_to_fit();
@@ -43,17 +53,8 @@ impl ByteBoolArray {
         let bytes = unsafe { Vec::from_raw_parts(ptr, length, capacity) };
 
         let buffer = Buffer::from(bytes);
-        let typed = TypedArray::try_from_parts(
-            DType::Bool(validity.nullability()),
-            ByteBoolMetadata {
-                validity: validity.to_metadata(length)?,
-            },
-            Some(buffer),
-            validity.into_array().into_iter().collect::<Vec<_>>().into(),
-            StatsSet::new(),
-        )?;
 
-        Ok(typed.into())
+        Self::try_new(buffer, validity)
     }
 
     pub fn buffer(&self) -> &Buffer {
@@ -68,7 +69,7 @@ impl ByteBoolArray {
 
 impl From<Vec<bool>> for ByteBoolArray {
     fn from(value: Vec<bool>) -> Self {
-        Self::try_with_validity(value, Validity::AllValid).unwrap()
+        Self::try_from_vec(value, Validity::AllValid).unwrap()
     }
 }
 
@@ -79,7 +80,7 @@ impl From<Vec<Option<bool>>> for ByteBoolArray {
         // This doesn't reallocate, and the compiler even vectorizes it
         let data = value.into_iter().map(|b| b.unwrap_or_default()).collect();
 
-        Self::try_with_validity(data, validity).unwrap()
+        Self::try_from_vec(data, validity).unwrap()
     }
 }
 
diff --git a/encodings/byte_bool/src/stats.rs b/encodings/byte_bool/src/stats.rs
index 67cb4ab30f..e7124dc0ad 100644
--- a/encodings/byte_bool/src/stats.rs
+++ b/encodings/byte_bool/src/stats.rs
@@ -12,6 +12,7 @@ impl ArrayStatisticsCompute for ByteBoolArray {
             return Ok(StatsSet::new());
         }
 
+        // TODO(adamgs): This is slightly wasteful and could be optimized in the future
         let bools = self.as_array_ref().clone().into_canonical()?.into_bool()?;
         bools.compute_statistics(stat)
     }