[GitHub] [arrow] alamb commented on a change in pull request #8005: ARROW-9778: [Rust] [DataFusion] Implement Expr.nullable() and make consistent between logical and physical plans

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

alamb commented on a change in pull request #8005:
URL: https://github.com/apache/arrow/pull/8005#discussion_r473118534



##########
File path: rust/datafusion/src/logicalplan.rs
##########
@@ -419,13 +410,50 @@ impl Expr {
         }
     }
 
+    /// Determine if this expression can produce null values
+    pub fn nullable(&self, input_schema: &Schema) -> Result<bool> {
+        match self {
+            Expr::Alias(expr, _) => expr.nullable(input_schema),
+            Expr::Column(name) => Ok(input_schema.field_with_name(name)?.is_nullable()),
+            Expr::Literal(value) => match value {
+                ScalarValue::Null => Ok(true),
+                _ => Ok(false),
+            },
+            Expr::Cast { expr, .. } => expr.nullable(input_schema),
+            Expr::ScalarFunction { .. } => Ok(true),
+            Expr::AggregateFunction { .. } => Ok(true),
+            Expr::Not(_) => Ok(false),

Review comment:
       ```suggestion
               Expr::Not(expr) => expr.nullable(input_schema),
   ```
   At least in SQL, `!` can be null, if the input is

##########
File path: rust/datafusion/src/logicalplan.rs
##########
@@ -419,13 +410,50 @@ impl Expr {
         }
     }
 
+    /// Determine if this expression can produce null values
+    pub fn nullable(&self, input_schema: &Schema) -> Result<bool> {
+        match self {
+            Expr::Alias(expr, _) => expr.nullable(input_schema),
+            Expr::Column(name) => Ok(input_schema.field_with_name(name)?.is_nullable()),
+            Expr::Literal(value) => match value {
+                ScalarValue::Null => Ok(true),
+                _ => Ok(false),
+            },
+            Expr::Cast { expr, .. } => expr.nullable(input_schema),
+            Expr::ScalarFunction { .. } => Ok(true),
+            Expr::AggregateFunction { .. } => Ok(true),

Review comment:
       I think the rules here are subtle:
   If the aggregate is MIN/MAX/SUM then the nullability is the nullability of the input
   If the aggregate is COUNT then the nullability is false (COUNT never can produce NULL)
   
   However, `true` is the safe default, so I think this is ok

##########
File path: rust/datafusion/src/logicalplan.rs
##########
@@ -419,13 +410,50 @@ impl Expr {
         }
     }
 
+    /// Determine if this expression can produce null values

Review comment:
       ```suggestion
       /// return true if this expression might produce null values
   ```




----------------------------------------------------------------
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.

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