Converting LogicalValidity into Validity requires passing Nullability (…

…#1834) Without nullability we can't accurately reconstruct Validity as LogicalValidity losses nonnullable state
spiraldb · Jan 7, 2025 · 0e61bb5 · 0e61bb5
1 parent 116de09
commit 0e61bb5
Show file tree

Hide file tree

Showing 4 changed files with 49 additions and 24 deletions.
diff --git a/encodings/alp/src/alp_rd/array.rs b/encodings/alp/src/alp_rd/array.rs
@@ -207,7 +207,8 @@ impl IntoCanonical for ALPRDArray {
                     right_parts.into_buffer_mut::<u32>(),
                     self.left_parts_patches(),
                 )?,
-                self.logical_validity().into_validity(),
+                self.logical_validity()
+                    .into_validity(self.dtype().nullability()),
             )
         } else {
             PrimitiveArray::new(
@@ -218,7 +219,8 @@ impl IntoCanonical for ALPRDArray {
                     right_parts.into_buffer_mut::<u64>(),
                     self.left_parts_patches(),
                 )?,
-                self.logical_validity().into_validity(),
+                self.logical_validity()
+                    .into_validity(self.dtype().nullability()),
             )
         };
 

diff --git a/encodings/datetime-parts/src/array.rs b/encodings/datetime-parts/src/array.rs
@@ -103,11 +103,9 @@ impl DateTimePartsArray {
     }
 
     pub fn validity(&self) -> Validity {
-        if self.dtype().is_nullable() {
-            self.days().logical_validity().into_validity()
-        } else {
-            Validity::NonNullable
-        }
+        self.days()
+            .logical_validity()
+            .into_validity(self.dtype().nullability())
     }
 }
 

diff --git a/vortex-array/src/array/chunked/canonical.rs b/vortex-array/src/array/chunked/canonical.rs
@@ -18,11 +18,9 @@ use crate::{
 
 impl IntoCanonical for ChunkedArray {
     fn into_canonical(self) -> VortexResult<Canonical> {
-        let validity = if self.dtype().is_nullable() {
-            self.logical_validity().into_validity()
-        } else {
-            Validity::NonNullable
-        };
+        let validity = self
+            .logical_validity()
+            .into_validity(self.dtype().nullability());
         try_canonicalize_chunks(self.chunks().collect(), validity, self.dtype())
     }
 }

diff --git a/vortex-array/src/validity.rs b/vortex-array/src/validity.rs
@@ -301,6 +301,12 @@ impl Validity {
             _ => {}
         };
 
+        let own_nullability = if self == Validity::NonNullable {
+            Nullability::NonNullable
+        } else {
+            Nullability::Nullable
+        };
+
         let source = match self {
             Validity::NonNullable => BoolArray::from(BooleanBuffer::new_set(len)),
             Validity::AllValid => BoolArray::from(BooleanBuffer::new_set(len)),
@@ -317,7 +323,7 @@ impl Validity {
 
         let patches = Patches::new(len, indices.clone(), patch_values.into_array());
 
-        Validity::try_from(source.patch(patches)?.into_array())
+        Self::from_array(source.patch(patches)?.into_array(), own_nullability)
     }
 
     /// Convert into a nullable variant
@@ -327,6 +333,14 @@ impl Validity {
             _ => self,
         }
     }
+
+    /// Create Validity from boolean array with given nullability of the array.
+    ///
+    /// Note: You want to pass the nullability of parent array and not the nullability of the validity array itself
+    ///     as that is always nonnullable
+    pub fn from_array(value: ArrayData, nullability: Nullability) -> VortexResult<Self> {
+        LogicalValidity::try_from(value).map(|a| a.into_validity(nullability))
+    }
 }
 
 impl PartialEq for Validity {
@@ -371,14 +385,6 @@ impl From<NullBuffer> for Validity {
     }
 }
 
-impl TryFrom<ArrayData> for Validity {
-    type Error = VortexError;
-
-    fn try_from(value: ArrayData) -> Result<Self, Self::Error> {
-        LogicalValidity::try_from(value).map(|a| a.into_validity())
-    }
-}
-
 impl FromIterator<LogicalValidity> for Validity {
     fn from_iter<T: IntoIterator<Item = LogicalValidity>>(iter: T) -> Self {
         let validities: Vec<LogicalValidity> = iter.into_iter().collect();
@@ -489,9 +495,16 @@ impl LogicalValidity {
         }
     }
 
-    pub fn into_validity(self) -> Validity {
+    pub fn into_validity(self, nullability: Nullability) -> Validity {
+        assert!(
+            nullability == Nullability::Nullable || matches!(self, Self::AllValid(_)),
+            "NonNullable validity must be AllValid",
+        );
         match self {
-            Self::AllValid(_) => Validity::AllValid,
+            Self::AllValid(_) => match nullability {
+                Nullability::NonNullable => Validity::NonNullable,
+                Nullability::Nullable => Validity::AllValid,
+            },
             Self::AllInvalid(_) => Validity::AllInvalid,
             Self::Array(a) => Validity::Array(a),
         }
@@ -533,9 +546,10 @@ impl IntoArrayData for LogicalValidity {
 mod tests {
     use rstest::rstest;
     use vortex_buffer::{buffer, Buffer};
+    use vortex_dtype::Nullability;
 
     use crate::array::{BoolArray, PrimitiveArray};
-    use crate::validity::Validity;
+    use crate::validity::{LogicalValidity, Validity};
     use crate::IntoArrayData;
 
     #[rstest]
@@ -574,4 +588,17 @@ mod tests {
             .patch(2, &buffer![4].into_array(), Validity::AllInvalid)
             .unwrap();
     }
+
+    #[test]
+    #[should_panic]
+    fn into_validity_nullable() {
+        LogicalValidity::AllInvalid(10).into_validity(Nullability::NonNullable);
+    }
+
+    #[test]
+    #[should_panic]
+    fn into_validity_nullable_array() {
+        LogicalValidity::Array(BoolArray::from_iter(vec![true, false]).into_array())
+            .into_validity(Nullability::NonNullable);
+    }
 }