Address stats pruning review comments

Signed-off-by: "Nicholas Gates" <nick@nickgates.com>

Author: gatesn

docs/developer-guide/internals/stats-pruning.md

@@ -4,12 +4,17 @@ Vortex uses statistics to prove when a filter cannot match a row group, zone, or
 file. The proof expression returns `true` when the input can be skipped. It
 returns `false` or `null` when pruning is not proven.
 
+Both `false` and `null` are non-pruning outcomes, but they mean different
+things. `false` means the available stats disproved the skip proof. `null` means
+the proof was unknown, usually because a required stat was missing or inexact.
+
 The pruning pipeline has two phases:
 
 1. `Expression::falsify(scope, session)` asks the session's
    `StatsRewriteRule`s to rewrite a filter into an abstract proof expression.
    Rules describe semantics in terms of `vortex.stat(input, aggregate_fn)`
-   placeholders, so the rewrite does not depend on a particular layout.
+   placeholders. These placeholders name the statistic needed by the proof, but
+   not where that statistic is stored.
 2. `bind_stats` lowers those abstract stat placeholders with a `StatBinder`.
    The binder maps stats to the representation used by the caller, such as
    zone-map table fields, file-level stat literals, or typed null literals for

vortex-array/src/scalar_fn/fns/binary/mod.rs

@@ -26,6 +26,7 @@ use crate::scalar_fn::ChildName;
 use crate::scalar_fn::ExecutionArgs;
 use crate::scalar_fn::ScalarFnId;
 use crate::scalar_fn::ScalarFnVTable;
+use crate::scalar_fn::SimplifyCtx;
 use crate::scalar_fn::fns::literal::Literal;
 use crate::scalar_fn::fns::operators::CompareOperator;
 use crate::scalar_fn::fns::operators::Operator;
@@ -43,40 +44,6 @@ use crate::scalar::Scalar;
 #[derive(Clone)]
 pub struct Binary;
 
-fn simplify_and(lhs: &Expression, rhs: &Expression) -> Option<Expression> {
-    match (bool_literal(lhs), bool_literal(rhs)) {
-        (Some(Some(false)), _) | (_, Some(Some(false))) => Some(lit(false)),
-        (Some(Some(true)), _) => Some(rhs.clone()),
-        (_, Some(Some(true))) => Some(lhs.clone()),
-        (Some(None), Some(None)) => Some(lhs.clone()),
-        _ => None,
-    }
-}
-
-fn simplify_or(lhs: &Expression, rhs: &Expression) -> Option<Expression> {
-    match (bool_literal(lhs), bool_literal(rhs)) {
-        (Some(Some(true)), _) | (_, Some(Some(true))) => Some(lit(true)),
-        (Some(Some(false)), _) => Some(rhs.clone()),
-        (_, Some(Some(false))) => Some(lhs.clone()),
-        (Some(None), Some(None)) => Some(lhs.clone()),
-        _ => None,
-    }
-}
-
-fn bool_literal(expr: &Expression) -> Option<Option<bool>> {
-    expr.as_opt::<Literal>()?
-        .as_bool_opt()
-        .map(|value| value.value())
-}
-
-fn is_null_literal(expr: &Expression) -> bool {
-    expr.as_opt::<Literal>().is_some_and(Scalar::is_null)
-}
-
-fn null_bool() -> Expression {
-    lit(Scalar::null(DType::Bool(Nullability::Nullable)))
-}
-
 impl ScalarFnVTable for Binary {
     type Options = Operator;
 
@@ -201,17 +168,67 @@ impl ScalarFnVTable for Binary {
         let lhs = expr.child(0);
         let rhs = expr.child(1);
 
-        if operator.is_comparison() && (is_null_literal(lhs) || is_null_literal(rhs)) {
-            return Ok(Some(null_bool()));
-        }
-
+        let bool_literal = |expr: &Expression| {
+            expr.as_opt::<Literal>()?
+                .as_bool_opt()
+                .map(|value| value.value())
+        };
+
+        // AND/OR use Kleene three-valued logic. `None` below is a boolean null.
+        //
+        // AND:
+        // - false AND x => false
+        // - true  AND x => x
+        // - null  AND null => null
+        //
+        // OR:
+        // - true  OR x => true
+        // - false OR x => x
+        // - null  OR null => null
+        //
+        // Other null cases either fall out of the identity/annihilator rules
+        // above (`null AND true`, `null OR false`) or cannot be simplified under
+        // Kleene semantics (`null AND x`, `null OR x` for non-literal `x`).
         Ok(match operator {
-            Operator::And => simplify_and(lhs, rhs),
-            Operator::Or => simplify_or(lhs, rhs),
+            Operator::And => match (bool_literal(lhs), bool_literal(rhs)) {
+                (Some(Some(false)), _) | (_, Some(Some(false))) => Some(lit(false)),
+                (Some(Some(true)), _) => Some(rhs.clone()),
+                (_, Some(Some(true))) => Some(lhs.clone()),
+                (Some(None), Some(None)) => Some(lhs.clone()),
+                _ => None,
+            },
+            Operator::Or => match (bool_literal(lhs), bool_literal(rhs)) {
+                (Some(Some(true)), _) | (_, Some(Some(true))) => Some(lit(true)),
+                (Some(Some(false)), _) => Some(rhs.clone()),
+                (_, Some(Some(false))) => Some(lhs.clone()),
+                (Some(None), Some(None)) => Some(lhs.clone()),
+                _ => None,
+            },
             _ => None,
         })
     }
 
+    fn simplify(
+        &self,
+        operator: &Operator,
+        expr: &Expression,
+        ctx: &dyn SimplifyCtx,
+    ) -> VortexResult<Option<Expression>> {
+        let is_literal_null =
+            |expr: &Expression| expr.as_opt::<Literal>().is_some_and(Scalar::is_null);
+
+        if operator.is_comparison()
+            && (is_literal_null(expr.child(0)) || is_literal_null(expr.child(1)))
+        {
+            // Validate the comparison before reducing it. This preserves type
+            // errors for expressions like `int_col = null_utf8`.
+            ctx.return_dtype(expr)?;
+            return Ok(Some(lit(Scalar::null(DType::Bool(Nullability::Nullable)))));
+        }
+
+        Ok(None)
+    }
+
     fn validity(
         &self,
         operator: &Operator,
@@ -257,6 +274,7 @@ impl ScalarFnVTable for Binary {
 #[cfg(test)]
 mod tests {
     use vortex_error::VortexExpect;
+    use vortex_error::VortexResult;
 
     use super::*;
     use crate::LEGACY_SESSION;
@@ -265,6 +283,7 @@ mod tests {
     use crate::builtins::ArrayBuiltins;
     use crate::dtype::DType;
     use crate::dtype::Nullability;
+    use crate::dtype::PType;
     use crate::expr::Expression;
     use crate::expr::and_collect;
     use crate::expr::col;
@@ -389,6 +408,36 @@ mod tests {
         );
     }
 
+    #[test]
+    fn comparison_with_typed_null_simplifies_after_type_check() -> VortexResult<()> {
+        let dtype = test_harness::struct_dtype();
+
+        let expr = eq(
+            col("col1"),
+            lit(Scalar::null(DType::Primitive(
+                PType::U16,
+                Nullability::Nullable,
+            ))),
+        );
+
+        assert_eq!(
+            expr.optimize_recursive(&dtype)?,
+            lit(Scalar::null(DType::Bool(Nullability::Nullable)))
+        );
+        Ok(())
+    }
+
+    #[test]
+    fn comparison_with_incompatible_null_still_type_checks() {
+        let dtype = test_harness::struct_dtype();
+        let expr = eq(
+            col("col1"),
+            lit(Scalar::null(DType::Utf8(Nullability::Nullable))),
+        );
+
+        assert!(expr.optimize_recursive(&dtype).is_err());
+    }
+
     #[test]
     fn test_display_print() {
         let expr = gt(lit(1), lit(2));

vortex-array/src/stats/bind.rs

@@ -4,9 +4,14 @@
 //! Bind abstract `vortex.stat` expressions to a concrete stats representation.
 //!
 //! Stats rewrite rules describe pruning in terms of `vortex.stat(input, aggregate_fn)` placeholders
-//! so the rewrite is independent of where statistics are stored. Binding is the later pass that
-//! replaces those placeholders with the representation used by a caller: zone-map field references,
-//! file-level stat literals, or typed nulls for missing stats.
+//! so the rewrite is independent of where statistics are stored. These stat placeholders are
+//! abstract because they name the statistic needed for a proof, but not how that statistic is
+//! represented by a specific layout or reader.
+//!
+//! Binding is the later pass that replaces each abstract placeholder with the representation used
+//! by a caller: zone-map field references, file-level stat literals, or typed nulls for missing
+//! stats. This lets all callers share the same falsification rules while keeping layout-specific
+//! stat storage behind [`StatBinder`].
 
 use vortex_error::VortexResult;
 
@@ -21,6 +26,11 @@ use crate::scalar::Scalar;
 use crate::scalar_fn::fns::stat::StatFn;
 
 /// A target that can bind abstract statistics to concrete expressions.
+///
+/// Implementations define how a pruning proof should read stats from a specific backing
+/// representation. For example, a zone-map binder can translate a `max(col)` placeholder into a
+/// field reference in the per-zone stats table, while a file-stats binder can translate the same
+/// placeholder into a literal value from the file footer.
 pub trait StatBinder {
     /// The dtype scope used to type-check expressions before stats are bound.
     fn scope(&self) -> &DType;