[GitHub] [arrow-datafusion] alamb commented on a change in pull request #1401: Fix bugs with nullability during rewrites: Combine `simplify` and `Simplifier`

[GitBox <gi...@apache.org>]

alamb commented on a change in pull request #1401:
URL: https://github.com/apache/arrow-datafusion/pull/1401#discussion_r780489370



##########
File path: datafusion/src/optimizer/simplify_expressions.rs
##########
@@ -554,212 +416,250 @@ impl<'a> Simplifier<'a> {
         false
     }
 
-    fn boolean_folding_for_or(
-        const_bool: &Option<bool>,
-        bool_expr: Box<Expr>,
-        left_right_order: bool,
-    ) -> Expr {
-        // See if we can fold 'const_bool OR bool_expr' to a constant boolean
-        match const_bool {
-            // TRUE or expr (including NULL) = TRUE
-            Some(true) => Expr::Literal(ScalarValue::Boolean(Some(true))),
-            // FALSE or expr (including NULL) = expr
-            Some(false) => *bool_expr,
-            None => match *bool_expr {
-                // NULL or TRUE = TRUE
-                Expr::Literal(ScalarValue::Boolean(Some(true))) => {
-                    Expr::Literal(ScalarValue::Boolean(Some(true)))
-                }
-                // NULL or FALSE = NULL
-                Expr::Literal(ScalarValue::Boolean(Some(false))) => {
-                    Expr::Literal(ScalarValue::Boolean(None))
-                }
-                // NULL or NULL = NULL
-                Expr::Literal(ScalarValue::Boolean(None)) => {
-                    Expr::Literal(ScalarValue::Boolean(None))
-                }
-                // NULL or expr can be either NULL or TRUE
-                // So let us not rewrite it
-                _ => {
-                    let mut left =
-                        Box::new(Expr::Literal(ScalarValue::Boolean(*const_bool)));
-                    let mut right = bool_expr;
-                    if !left_right_order {
-                        std::mem::swap(&mut left, &mut right);
-                    }
-
-                    Expr::BinaryExpr {
-                        left,
-                        op: Operator::Or,
-                        right,
-                    }
-                }
-            },
+    /// Returns true if expr is nullable
+    fn nullable(&self, expr: &Expr) -> Result<bool> {
+        for schema in &self.schemas {

Review comment:
       Here is what I got to:
   
   ```rust
       /// Returns true if expr is nullable
       fn nullable(&self, expr: &Expr) -> Result<bool> {
           self.schemas.iter()
               .find_map(|schema| {
                   // expr may be from another input, so ignore errors
                   // by converting to None to keep trying
                   expr.nullable(schema.as_ref()).ok()
               })
               .ok_or_else(|| {
                   // This means we weren't able to compute `Expr::nullable` with
                   // *any* input schemas, signalling a problem
                   DataFusionError::Internal(format!(
                       "Could not find find columns in '{}' during simplify",
                       expr
                   ))
               })
       }
   ```
   
   which isn't quite a one linter, but is a more functional style. Updated in 8642e56bb




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org