wip

Signed-off-by: Joe Isaacs <[email protected]>

Author: joseph-isaacs

vortex-array/src/expr/analysis/annotation_union_set.rs

@@ -35,12 +35,6 @@ pub type Annotations<'a, A> = HashMap<&'a Expression, HashSet<A>>;
 ///
 /// Returns a map of each expression to all annotations that any of its descendent (child)
 /// expressions are annotated with.
-///
-/// This "skip" behavior makes this function different from [`label_tree`], which always
-/// visits all nodes. Use this when you want to find the "shallowest" matches in a tree.
-///
-/// Note: This cannot use [`label_tree`] because the early termination (skip) requires
-/// conditional traversal based on the node's direct annotations.
 pub fn descendent_annotation_union_set<A: AnnotationFn>(
     expr: &Expression,
     annotate: A,

vortex-array/src/expr/analysis/labeling.rs

@@ -84,15 +84,12 @@ where
     type NodeTy = Expression;
 
     fn visit_down(&mut self, _node: &'a Self::NodeTy) -> VortexResult<TraversalOrder> {
-        // Continue traversing down
         Ok(TraversalOrder::Continue)
     }
 
     fn visit_up(&mut self, node: &'a Expression) -> VortexResult<TraversalOrder> {
-        // Compute self-label for this node
         let self_label = (self.label_edge)(node);
 
-        // Fold all child labels into the self label
         let final_label = node.children().iter().fold(self_label, |acc, child| {
             let child_label = self
                 .labels

vortex-array/src/expr/analysis/null_sensitive.rs

@@ -6,44 +6,16 @@ use vortex_utils::aliases::hash_map::HashMap;
 use super::labeling::label_tree;
 use crate::expr::Expression;
 
-/// Tracks whether an expression is null-sensitive.
-///
-/// An expression is null-sensitive if it or any of its children is an `is_null` operation.
-#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
-pub enum NullSensitive {
-    /// The expression or one of its children contains an `is_null` operation.
-    Yes,
-    /// The expression and all of its children do not contain an `is_null` operation.
-    No,
-}
-
-impl NullSensitive {
-    /// Combine two null sensitivity labels.
-    ///
-    /// Returns `Yes` if either label is `Yes`, otherwise `No`.
-    pub fn combine(self, other: Self) -> Self {
-        match (self, other) {
-            (NullSensitive::Yes, _) | (_, NullSensitive::Yes) => NullSensitive::Yes,
-            (NullSensitive::No, NullSensitive::No) => NullSensitive::No,
-        }
-    }
-}
-
 pub type NullSensitiveLabels<'a> = HashMap<&'a Expression, bool>;
 
 /// Label each expression in the tree with whether it is null-sensitive.
 ///
-/// An expression is null-sensitive if it or any of its descendants contain an `is_null` operation.
-///
-/// This function demonstrates the use of the general [`label_tree`] framework with:
-/// - **Label Edge**: Check if the node itself is null-sensitive using [`Expression::is_null_sensitive`]
-/// - **Merge Child**: Fold children labels with OR operation
+/// See [`VTable::is_null_sensitive`] for a definition of null sensitivity.
+/// This function operates on a tree of expressions, not just a single expression.
 pub fn label_null_sensitive(expr: &Expression) -> NullSensitiveLabels<'_> {
     label_tree(
         expr,
-        // Label edge: check if this node itself is null-sensitive
         |expr| expr.is_null_sensitive(),
-        // Merge child: fold children with OR (true if self OR any child is true)
         |acc, &child| acc | child,
     )
 }
@@ -90,24 +62,4 @@ mod tests {
         assert_eq!(labels.get(&right), Some(&true));
         assert_eq!(labels.get(&expr), Some(&true));
     }
-
-    #[test]
-    fn test_combine() {
-        assert_eq!(
-            NullSensitive::Yes.combine(NullSensitive::Yes),
-            NullSensitive::Yes
-        );
-        assert_eq!(
-            NullSensitive::Yes.combine(NullSensitive::No),
-            NullSensitive::Yes
-        );
-        assert_eq!(
-            NullSensitive::No.combine(NullSensitive::Yes),
-            NullSensitive::Yes
-        );
-        assert_eq!(
-            NullSensitive::No.combine(NullSensitive::No),
-            NullSensitive::No
-        );
-    }
 }

vortex-array/src/expr/expression.rs

@@ -233,21 +233,7 @@ impl Expression {
     }
 
     /// Returns whether this expression itself is null-sensitive.
-    ///
-    /// An expression is null-sensitive if it directly operates on null values,
-    /// such as `is_null`. Most expressions are not null-sensitive.
-    ///
-    /// The property we are interested in is if the expression (e) distributes over
-    /// mask.
-    /// Define a `mask(a, m)` expression that applies the boolean array `m` to the validity of the
-    /// array `a`.
-    /// An unary expression `e` to be null-sensitive iff forall arrays `a` and masks `m`.
-    /// `e(mask(a, m)) == mask(e(a), m)`.
-    /// This can be extended to an n-ary expression.
-    ///
-    /// This method only checks the expression itself, not its children. To check
-    /// if an expression or any of its descendants are null-sensitive, use the
-    /// [`crate::expr::analysis::label_null_sensitive`] function.
+    /// See [`VTable::is_null_sensitive`].
     pub fn is_null_sensitive(&self) -> bool {
         self.vtable.as_dyn().is_null_sensitive(self.data.as_ref())
     }

vortex-array/src/expr/vtable.rs

@@ -114,12 +114,21 @@ pub trait VTable: 'static + Sized + Send + Sync {
         None
     }
 
-    /// Returns whether this expression itself is null-sensitive. Default to *true*.
+    /// Returns whether this expression itself is null-sensitive. Conservatively default to *true*.
     ///
     /// An expression is null-sensitive if it directly operates on null values,
-    /// such as `is_null`. We must conservatively assume that all expression are null-sensitive.
+    /// such as `is_null`. Most expressions are not null-sensitive.
     ///
-    /// This must only account for the expression itself being null-sensitive, not its children.
+    /// The property we are interested in is if the expression (e) distributes over
+    /// mask.
+    /// Define a `mask(a, m)` expression that applies the boolean array `m` to the validity of the
+    /// array `a`.
+    /// An unary expression `e` to be null-sensitive iff forall arrays `a` and masks `m`.
+    /// `e(mask(a, m)) == mask(e(a), m)`.
+    /// This can be extended to an n-ary expression.
+    ///
+    /// This method only checks the expression itself, not its children. To check
+    /// if an expression or any of its descendants are null-sensitive.
     fn is_null_sensitive(&self, _instance: &Self::Instance) -> bool {
         true
     }