[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #2144: fix `not(null)` with constant `null`

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

alamb commented on code in PR #2144:
URL: https://github.com/apache/arrow-datafusion/pull/2144#discussion_r842028466


##########
datafusion/physical-expr/src/expressions/not.rs:
##########
@@ -86,10 +86,29 @@ impl PhysicalExpr for NotExpr {
                 )))
             }
             ColumnarValue::Scalar(scalar) => {
-                let bool_value: bool = scalar.try_into()?;
-                Ok(ColumnarValue::Scalar(ScalarValue::Boolean(Some(
-                    !bool_value,
-                ))))
+                match scalar {
+                    ScalarValue::Utf8(utf8_val) => {
+                        // NOT op supports `NOT(NULL)` usage that returns `NULL` directly
+                        if utf8_val.is_none() {
+                            Ok(ColumnarValue::Scalar(ScalarValue::Utf8(None)))

Review Comment:
   I think this may be problematic as the output type of `not()` is supposed to be `Boolean`. Does returning
   
   ```suggestion
                               Ok(ColumnarValue::Scalar(ScalarValue::Boolean(None)))
   ```
   
   work?
   
   



##########
datafusion/physical-expr/src/expressions/not.rs:
##########
@@ -104,14 +123,14 @@ pub fn not(
     arg: Arc<dyn PhysicalExpr>,
     input_schema: &Schema,
 ) -> Result<Arc<dyn PhysicalExpr>> {
-    let data_type = arg.data_type(input_schema)?;
-    if data_type != DataType::Boolean {
-        Err(DataFusionError::Internal(format!(
-            "NOT '{:?}' can't be evaluated because the expression's type is {:?}, not boolean",
+    match arg.data_type(input_schema)? {
+        // NULL is present as literal `ScalarValue::Utf8(None)` in logical expresion,
+        // to support `NOT(NULL)`, Utf8 arg is passed to `NotExpr` for further evaluate.
+        DataType::Boolean | DataType::Utf8 => Ok(Arc::new(NotExpr::new(arg))),

Review Comment:
   What about detecting if the input is a `ScalarValue` and if that `ScalarValue::is_null()` returns true (rather than checking for utf8`?



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