Simplified Utf8Scalar and BinaryScalar. (#660)

jorgecarleitao · Dec 5, 2021 · 021a8e3 · 021a8e3
1 parent a18555c
commit 021a8e3
Show file tree

Hide file tree

Showing 5 changed files with 26 additions and 49 deletions.
diff --git a/src/compute/comparison/mod.rs b/src/compute/comparison/mod.rs
@@ -297,12 +297,12 @@ macro_rules! compare_scalar {
             Utf8 => {
                 let lhs = lhs.as_any().downcast_ref().unwrap();
                 let rhs = rhs.as_any().downcast_ref::<Utf8Scalar<i32>>().unwrap();
-                utf8::$op::<i32>(lhs, rhs.value())
+                utf8::$op::<i32>(lhs, rhs.value().unwrap())
             }
             LargeUtf8 => {
                 let lhs = lhs.as_any().downcast_ref().unwrap();
                 let rhs = rhs.as_any().downcast_ref::<Utf8Scalar<i64>>().unwrap();
-                utf8::$op::<i64>(lhs, rhs.value())
+                utf8::$op::<i64>(lhs, rhs.value().unwrap())
             }
             Decimal(_, _) => {
                 let lhs = lhs.as_any().downcast_ref().unwrap();
@@ -315,12 +315,12 @@ macro_rules! compare_scalar {
             Binary => {
                 let lhs = lhs.as_any().downcast_ref().unwrap();
                 let rhs = rhs.as_any().downcast_ref::<BinaryScalar<i32>>().unwrap();
-                binary::$op::<i32>(lhs, rhs.value())
+                binary::$op::<i32>(lhs, rhs.value().unwrap())
             }
             LargeBinary => {
                 let lhs = lhs.as_any().downcast_ref().unwrap();
                 let rhs = rhs.as_any().downcast_ref::<BinaryScalar<i64>>().unwrap();
-                binary::$op::<i64>(lhs, rhs.value())
+                binary::$op::<i64>(lhs, rhs.value().unwrap())
             }
             _ => todo!("Comparisons of {:?} are not yet supported", data_type),
         }

diff --git a/src/scalar/binary.rs b/src/scalar/binary.rs
@@ -1,43 +1,32 @@
-use crate::{array::*, buffer::Buffer, datatypes::DataType};
+use crate::{array::*, datatypes::DataType};
 
 use super::Scalar;
 
-/// The [`Scalar`] implementation of binary (`Vec<u8>`).
-#[derive(Debug, Clone)]
+/// The [`Scalar`] implementation of binary ([`Option<Vec<u8>>`]).
+#[derive(Debug, Clone, PartialEq)]
 pub struct BinaryScalar<O: Offset> {
-    value: Buffer<u8>,
-    is_valid: bool,
+    value: Option<Vec<u8>>,
     phantom: std::marker::PhantomData<O>,
 }
 
-impl<O: Offset> PartialEq for BinaryScalar<O> {
-    fn eq(&self, other: &Self) -> bool {
-        self.is_valid == other.is_valid && ((!self.is_valid) | (self.value == other.value))
-    }
-}
-
 impl<O: Offset> BinaryScalar<O> {
     /// Returns a new [`BinaryScalar`].
     #[inline]
-    pub fn new<P: AsRef<[u8]>>(v: Option<P>) -> Self {
-        let is_valid = v.is_some();
-        O::from_usize(v.as_ref().map(|x| x.as_ref().len()).unwrap_or_default()).expect("Too large");
-        let value = Buffer::from(v.as_ref().map(|x| x.as_ref()).unwrap_or(&[]));
+    pub fn new<P: Into<Vec<u8>>>(value: Option<P>) -> Self {
         Self {
-            value,
-            is_valid,
+            value: value.map(|x| x.into()),
             phantom: std::marker::PhantomData,
         }
     }
 
     /// Its value
     #[inline]
-    pub fn value(&self) -> &[u8] {
-        self.value.as_slice()
+    pub fn value(&self) -> Option<&[u8]> {
+        self.value.as_ref().map(|x| x.as_ref())
     }
 }
 
-impl<O: Offset, P: AsRef<[u8]>> From<Option<P>> for BinaryScalar<O> {
+impl<O: Offset, P: Into<Vec<u8>>> From<Option<P>> for BinaryScalar<O> {
     #[inline]
     fn from(v: Option<P>) -> Self {
         Self::new(v)
@@ -52,7 +41,7 @@ impl<O: Offset> Scalar for BinaryScalar<O> {
 
     #[inline]
     fn is_valid(&self) -> bool {
-        self.is_valid
+        self.value.is_some()
     }
 
     #[inline]

diff --git a/src/scalar/utf8.rs b/src/scalar/utf8.rs
@@ -1,44 +1,32 @@
-use crate::{array::*, buffer::Buffer, datatypes::DataType};
+use crate::{array::*, datatypes::DataType};
 
 use super::Scalar;
 
-/// The implementation of [`Scalar`] for utf8, semantically equivalent to [`Option<&str>`].
-#[derive(Debug, Clone)]
+/// The implementation of [`Scalar`] for utf8, semantically equivalent to [`Option<String>`].
+#[derive(Debug, Clone, PartialEq)]
 pub struct Utf8Scalar<O: Offset> {
-    value: Buffer<u8>, // safety: valid utf8
-    is_valid: bool,
+    value: Option<String>,
     phantom: std::marker::PhantomData<O>,
 }
 
-impl<O: Offset> PartialEq for Utf8Scalar<O> {
-    fn eq(&self, other: &Self) -> bool {
-        self.is_valid == other.is_valid && ((!self.is_valid) | (self.value == other.value))
-    }
-}
-
 impl<O: Offset> Utf8Scalar<O> {
     /// Returns a new [`Utf8Scalar`]
     #[inline]
-    pub fn new<P: AsRef<str>>(v: Option<P>) -> Self {
-        let is_valid = v.is_some();
-        O::from_usize(v.as_ref().map(|x| x.as_ref().len()).unwrap_or_default()).expect("Too large");
-        let value = Buffer::from(v.as_ref().map(|x| x.as_ref().as_bytes()).unwrap_or(&[]));
+    pub fn new<P: Into<String>>(value: Option<P>) -> Self {
         Self {
-            value,
-            is_valid,
+            value: value.map(|x| x.into()),
             phantom: std::marker::PhantomData,
         }
     }
 
     /// Returns the value irrespectively of the validity.
     #[inline]
-    pub fn value(&self) -> &str {
-        // Safety: invariant of the struct
-        unsafe { std::str::from_utf8_unchecked(self.value.as_slice()) }
+    pub fn value(&self) -> Option<&str> {
+        self.value.as_ref().map(|x| x.as_ref())
     }
 }
 
-impl<O: Offset, P: AsRef<str>> From<Option<P>> for Utf8Scalar<O> {
+impl<O: Offset, P: Into<String>> From<Option<P>> for Utf8Scalar<O> {
     #[inline]
     fn from(v: Option<P>) -> Self {
         Self::new(v)
@@ -53,7 +41,7 @@ impl<O: Offset> Scalar for Utf8Scalar<O> {
 
     #[inline]
     fn is_valid(&self) -> bool {
-        self.is_valid
+        self.value.is_some()
     }
 
     #[inline]

diff --git a/tests/it/scalar/binary.rs b/tests/it/scalar/binary.rs
@@ -20,7 +20,7 @@ fn equal() {
 fn basics() {
     let a = BinaryScalar::<i32>::from(Some("a"));
 
-    assert_eq!(a.value(), b"a");
+    assert_eq!(a.value(), Some(b"a".as_ref()));
     assert_eq!(a.data_type(), &DataType::Binary);
     assert!(a.is_valid());
 

diff --git a/tests/it/scalar/utf8.rs b/tests/it/scalar/utf8.rs
@@ -20,7 +20,7 @@ fn equal() {
 fn basics() {
     let a = Utf8Scalar::<i32>::from(Some("a"));
 
-    assert_eq!(a.value(), "a");
+    assert_eq!(a.value(), Some("a"));
     assert_eq!(a.data_type(), &DataType::Utf8);
     assert!(a.is_valid());