UTF8 type check to ScalarValue.is_null

datafusion/core/src/physical_plan/planner.rs

@@ -1005,10 +1005,12 @@ pub fn create_physical_expr(
             input_schema,
             data_type.clone(),
         ),
-        Expr::Not(expr) => expressions::not(
-            create_physical_expr(expr, input_dfschema, input_schema, execution_props)?,
+        Expr::Not(expr) => expressions::not(create_physical_expr(
+            expr,
+            input_dfschema,
             input_schema,
-        ),
+            execution_props,
+        )?),
         Expr::Negative(expr) => expressions::negative(
             create_physical_expr(expr, input_dfschema, input_schema, execution_props)?,
             input_schema,
@@ -1087,7 +1089,7 @@ pub fn create_physical_expr(
             );
 
             if *negated {
-                expressions::not(binary_expr?, input_schema)
+                expressions::not(binary_expr?)
             } else {
                 binary_expr
             }
@@ -1526,7 +1528,7 @@ mod tests {
             &schema,
             &make_session_state(),
         )?;
-        let expected = expressions::not(expressions::col("a", &schema)?, &schema)?;
+        let expected = expressions::not(expressions::col("a", &schema)?)?;
 
         assert_eq!(format!("{:?}", expr), format!("{:?}", expected));
 

datafusion/core/tests/sql/expr.rs

@@ -280,6 +280,45 @@ async fn query_scalar_minus_array() -> Result<()> {
     Ok(())
 }
 
+#[tokio::test]
+async fn test_not_expressions() -> Result<()> {
+    let ctx = SessionContext::new();
+
+    let sql = "SELECT not(true), not(false)";
+    let actual = execute_to_batches(&ctx, sql).await;
+    let expected = vec![
+        "+-------------------+--------------------+",
+        "| NOT Boolean(true) | NOT Boolean(false) |",
+        "+-------------------+--------------------+",
+        "| false             | true               |",
+        "+-------------------+--------------------+",
+    ];
+    assert_batches_eq!(expected, &actual);
+
+    let sql = "SELECT null, not(null)";
+    let actual = execute_to_batches(&ctx, sql).await;
+    let expected = vec![
+        "+------------+----------------+",
+        "| Utf8(NULL) | NOT Utf8(NULL) |",
+        "+------------+----------------+",
+        "|            |                |",
+        "+------------+----------------+",
+    ];
+    assert_batches_eq!(expected, &actual);
+
+    let sql = "SELECT NOT('hi')";
+    let result = plan_and_collect(&ctx, sql).await;
+    match result {
+        Ok(_) => panic!("expected error"),
+        Err(e) => {
+            assert_contains!(e.to_string(),
+                             "NOT 'Literal { value: Utf8(\"hi\") }' can't be evaluated because the expression's type is Utf8, not boolean or NULL"
+            );
+        }
+    }
+    Ok(())
+}
+
 #[tokio::test]
 async fn test_boolean_expressions() -> Result<()> {
     test_expression!("true", "true");
@@ -289,14 +328,6 @@ async fn test_boolean_expressions() -> Result<()> {
     Ok(())
 }
 
-#[tokio::test]
-async fn test_not_expressions() -> Result<()> {
-    test_expression!("not(true)", "false");
-    test_expression!("not(false)", "true");
-    test_expression!("not(NULL)", "NULL");
-    Ok(())
-}
-
 #[tokio::test]
 #[cfg_attr(not(feature = "crypto_expressions"), ignore)]
 async fn test_crypto_expressions() -> Result<()> {

datafusion/physical-expr/src/expressions/not.rs

@@ -69,8 +69,8 @@ impl PhysicalExpr for NotExpr {
     }
 
     fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
-        let arg = self.arg.evaluate(batch)?;
-        match arg {
+        let evaluate_arg = self.arg.evaluate(batch)?;
+        match evaluate_arg {
             ColumnarValue::Array(array) => {
                 let array =
                     array
@@ -86,52 +86,28 @@ impl PhysicalExpr for NotExpr {
                 )))
             }
             ColumnarValue::Scalar(scalar) => {
-                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)))
-                        } else {
-                            Err(DataFusionError::Internal(format!(
-                                "NOT op can not evaluate utf8 value: {}",
-                                utf8_val.unwrap()
-                            )))
-                        }
-                    }
-                    ScalarValue::Boolean(_) => {
-                        let bool_value: bool = scalar.try_into()?;
-                        Ok(ColumnarValue::Scalar(ScalarValue::Boolean(Some(
-                            !bool_value,
-                        ))))
-                    }
-                    unexpected_val => Err(DataFusionError::Internal(format!(
-                        "NOT op can not evaluate scalar value type {:?}",
-                        unexpected_val
-                    ))),
+                if scalar.is_null() {
+                    return Ok(ColumnarValue::Scalar(ScalarValue::Boolean(None)));
                 }
+                let value_type = scalar.get_datatype();
+                if value_type != DataType::Boolean {
+                    return Err(DataFusionError::Internal(format!(
+                        "NOT '{:?}' can't be evaluated because the expression's type is {:?}, not boolean or NULL",
+                        self.arg, value_type,
+                    )));
+                }
+                let bool_value: bool = scalar.try_into()?;
+                Ok(ColumnarValue::Scalar(ScalarValue::Boolean(Some(
+                    !bool_value,
+                ))))
             }
         }
     }
 }
 
 /// Creates a unary expression NOT
-///
-/// # Errors
-///
-/// This function errors when the argument's type is not boolean
-pub fn not(
-    arg: Arc<dyn PhysicalExpr>,
-    input_schema: &Schema,
-) -> Result<Arc<dyn PhysicalExpr>> {
-    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))),
-        data_type => Err(DataFusionError::Internal(format!(
-            "NOT '{:?}' can't be evaluated because the expression's type is {:?}, not boolean or NULL",
-            arg, data_type,
-        ))),
-    }
+pub fn not(arg: Arc<dyn PhysicalExpr>) -> Result<Arc<dyn PhysicalExpr>> {
+    Ok(Arc::new(NotExpr::new(arg)))
 }
 
 #[cfg(test)]
@@ -145,7 +121,7 @@ mod tests {
     fn neg_op() -> Result<()> {
         let schema = Schema::new(vec![Field::new("a", DataType::Boolean, true)]);
 
-        let expr = not(col("a", &schema)?, &schema)?;
+        let expr = not(col("a", &schema)?)?;
         assert_eq!(expr.data_type(&schema)?, DataType::Boolean);
         assert!(expr.nullable(&schema)?);
 
@@ -164,12 +140,4 @@ mod tests {
 
         Ok(())
     }
-
-    /// verify that expression errors when the input expression is not a boolean.
-    #[test]
-    fn neg_op_not_null() {
-        let schema = Schema::new(vec![Field::new("a", DataType::UInt32, true)]);
-        let expr = not(col("a", &schema).unwrap(), &schema);
-        assert!(expr.is_err());
-    }
 }