More CR notes

spiraldb · Jun 24, 2024 · 751a153 · 751a153
1 parent 4d8ee5f
commit 751a153
Show file tree

Hide file tree

Showing 3 changed files with 19 additions and 17 deletions.
diff --git 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
@@ -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
@@ -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)
     }