chore: attach Diagnostic to unary operator type errors

datafusion/core/tests/sql/sql_api.rs

@@ -209,17 +209,15 @@ async fn ddl_can_not_be_planned_by_session_state() {
 }
 
 #[tokio::test]
-async fn invalid_wrapped_negation_fails_during_optimization() {
+async fn invalid_wrapped_negation_fails_during_planning() {
     let ctx = SessionContext::new();
     let err = ctx
         .sql("SELECT * FROM (SELECT 1) WHERE ((-'a') IS NULL)")
         .await
-        .unwrap()
-        .into_optimized_plan()
         .unwrap_err();
 
     assert_contains!(
         err.strip_backtrace(),
-        "Negation only supports numeric, interval and timestamp types"
+        "Unary operator '-' only supports signed numeric, interval and timestamp types"
     );
 }

datafusion/expr/src/expr.rs

@@ -2286,6 +2286,7 @@ impl Expr {
     pub fn spans(&self) -> Option<&Spans> {
         match self {
             Expr::Column(col) => Some(&col.spans),
+            Expr::Not(inner) | Expr::Negative(inner) => inner.spans(),
             _ => None,
         }
     }

datafusion/sql/src/expr/unary_op.rs

@@ -15,11 +15,14 @@
 // specific language governing permissions and limitations
 // under the License.
 
+use arrow::datatypes::DataType;
+
 use crate::planner::{ContextProvider, PlannerContext, SqlToRel};
 use datafusion_common::{DFSchema, Diagnostic, Result, not_impl_err, plan_err};
 use datafusion_expr::{
-    Expr, ExprSchemable,
-    type_coercion::{is_interval, is_timestamp},
+    Expr, ExprSchemable, Operator,
+    binary::BinaryTypeCoercer,
+    type_coercion::{is_interval, is_signed_numeric, is_timestamp},
 };
 use sqlparser::ast::{Expr as SQLExpr, UnaryOperator, Value, ValueWithSpan};
 
@@ -32,9 +35,37 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
         planner_context: &mut PlannerContext,
     ) -> Result<Expr> {
         match op {
-            UnaryOperator::Not => Ok(Expr::Not(Box::new(
-                self.sql_expr_to_logical_expr(expr, schema, planner_context)?,
-            ))),
+            UnaryOperator::Not => {
+                let operand =
+                    self.sql_expr_to_logical_expr(expr, schema, planner_context)?;
+                let field = operand.to_field(schema)?.1;
+                let data_type = field.data_type();
+                let bool_coercible = BinaryTypeCoercer::new(
+                    data_type,
+                    &Operator::IsDistinctFrom,
+                    &DataType::Boolean,
+                )
+                .get_input_types()
+                .is_ok();
+                if bool_coercible {
+                    Ok(Expr::Not(Box::new(operand)))
+                } else {
+                    let span = operand.spans().and_then(|s| s.first());
+                    let mut diagnostic = Diagnostic::new_error(
+                        format!("NOT cannot be used with {data_type}"),
+                        span,
+                    );
+                    diagnostic
+                        .add_note("NOT can only be used with boolean expressions", None);
+                    diagnostic
+                        .add_help(format!("perhaps you need to cast {operand}"), None);
+                    plan_err!(
+                        "Unary operator 'NOT' requires a boolean expression, \
+                         got {data_type}";
+                        diagnostic = diagnostic
+                    )
+                }
+            }
             UnaryOperator::Plus => {
                 let operand =
                     self.sql_expr_to_logical_expr(expr, schema, planner_context)?;
@@ -72,11 +103,38 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
                         self.sql_interval_to_expr(true, interval)
                     }
                     // Not a literal, apply negative operator on expression
-                    _ => Ok(Expr::Negative(Box::new(self.sql_expr_to_logical_expr(
-                        expr,
-                        schema,
-                        planner_context,
-                    )?))),
+                    _ => {
+                        let operand =
+                            self.sql_expr_to_logical_expr(expr, schema, planner_context)?;
+                        let field = operand.to_field(schema)?.1;
+                        let data_type = field.data_type();
+                        if data_type.is_null()
+                            || is_signed_numeric(data_type)
+                            || is_interval(data_type)
+                            || is_timestamp(data_type)
+                        {
+                            Ok(Expr::Negative(Box::new(operand)))
+                        } else {
+                            let span = operand.spans().and_then(|s| s.first());
+                            let mut diagnostic = Diagnostic::new_error(
+                                format!("- cannot be used with {data_type}"),
+                                span,
+                            );
+                            diagnostic.add_note(
+                                "- can only be used with signed numeric types, intervals, and timestamps",
+                                None,
+                            );
+                            diagnostic.add_help(
+                                format!("perhaps you need to cast {operand}"),
+                                None,
+                            );
+                            plan_err!(
+                                "Unary operator '-' only supports signed numeric, \
+                                 interval and timestamp types";
+                                diagnostic = diagnostic
+                            )
+                        }
+                    }
                 }
             }
             _ => not_impl_err!("Unsupported SQL unary operator {op:?}"),

datafusion/sql/tests/cases/diagnostic.rs

@@ -369,6 +369,56 @@ fn test_unary_op_plus_with_non_column() -> Result<()> {
     Ok(())
 }
 
+#[test]
+fn test_unary_op_minus_with_column() -> Result<()> {
+    let query = "SELECT -/*whole*/first_name/*whole*/ FROM person";
+    let spans = get_spans(query);
+    let diag = do_query(query);
+    assert_snapshot!(diag.message, @"- cannot be used with Utf8");
+    assert_eq!(diag.span, Some(spans["whole"]));
+    assert_snapshot!(diag.notes[0].message, @"- can only be used with signed numeric types, intervals, and timestamps");
+    assert_snapshot!(diag.helps[0].message, @"perhaps you need to cast person.first_name");
+    Ok(())
+}
+
+#[test]
+fn test_unary_op_minus_with_non_column() -> Result<()> {
+    let query = "SELECT -'a'";
+    let diag = do_query(query);
+    assert_eq!(diag.message, "- cannot be used with Utf8");
+    assert_snapshot!(diag.notes[0].message, @"- can only be used with signed numeric types, intervals, and timestamps");
+    assert_eq!(diag.notes[0].span, None);
+    assert_snapshot!(diag.helps[0].message, @r#"perhaps you need to cast Utf8("a")"#);
+    assert_eq!(diag.helps[0].span, None);
+    assert_eq!(diag.span, None);
+    Ok(())
+}
+
+#[test]
+fn test_unary_op_not_with_column() -> Result<()> {
+    let query = "SELECT NOT /*whole*/first_name/*whole*/ FROM person";
+    let spans = get_spans(query);
+    let diag = do_query(query);
+    assert_snapshot!(diag.message, @"NOT cannot be used with Utf8");
+    assert_eq!(diag.span, Some(spans["whole"]));
+    assert_snapshot!(diag.notes[0].message, @"NOT can only be used with boolean expressions");
+    assert_snapshot!(diag.helps[0].message, @"perhaps you need to cast person.first_name");
+    Ok(())
+}
+
+#[test]
+fn test_unary_op_not_with_non_column() -> Result<()> {
+    let query = "SELECT NOT 'a'";
+    let diag = do_query(query);
+    assert_eq!(diag.message, "NOT cannot be used with Utf8");
+    assert_snapshot!(diag.notes[0].message, @"NOT can only be used with boolean expressions");
+    assert_eq!(diag.notes[0].span, None);
+    assert_snapshot!(diag.helps[0].message, @r#"perhaps you need to cast Utf8("a")"#);
+    assert_eq!(diag.helps[0].span, None);
+    assert_eq!(diag.span, None);
+    Ok(())
+}
+
 #[test]
 fn test_syntax_error() -> Result<()> {
     // create a table with a column of type varchar

datafusion/sql/tests/sql_integration.rs

@@ -867,19 +867,27 @@ fn select_filter_cannot_use_alias() {
 
 #[test]
 fn select_neg_filter() {
+    // NOT requires a boolean expression; applying it to a Utf8 column is an error
     let sql = "SELECT id, first_name, last_name \
                    FROM person WHERE NOT state";
-    let plan = logical_plan(sql).unwrap();
-    assert_snapshot!(
-        plan,
-        @r"
-    Projection: person.id, person.first_name, person.last_name
-      Filter: NOT person.state
-        TableScan: person
-    "
+    let err = logical_plan(sql).unwrap_err();
+    assert!(
+        err.to_string()
+            .contains("Unary operator 'NOT' requires a boolean expression"),
+        "unexpected error: {err}"
     );
 }
 
+#[test]
+fn select_not_bool_filter() {
+    let sql = "SELECT order_id FROM orders WHERE NOT delivered";
+    let plan = logical_plan(sql).unwrap();
+    let expected = "Projection: orders.order_id\
+        \n  Filter: NOT orders.delivered\
+        \n    TableScan: orders";
+    assert_eq!(expected, format!("{plan}"));
+}
+
 #[test]
 fn select_compound_filter() {
     let sql = "SELECT id, first_name, last_name \

datafusion/sqllogictest/test_files/scalar.slt

@@ -1809,15 +1809,15 @@ SELECT not(true), not(false)
 ----
 false true
 
-query error type_coercion\ncaused by\nError during planning: Cannot infer common argument type for comparison operation Int64 IS DISTINCT FROM Boolean
+query error Error during planning: Unary operator 'NOT' requires a boolean expression, got Int64
 SELECT not(1), not(0)
 
 query ?B
 SELECT null, not(null)
 ----
 NULL NULL
 
-query error type_coercion\ncaused by\nError during planning: Cannot infer common argument type for comparison operation Utf8 IS DISTINCT FROM Boolean
+query error Error during planning: Unary operator 'NOT' requires a boolean expression, got Utf8
 SELECT NOT('hi')
 
 # test_negative_expressions()
@@ -1827,7 +1827,7 @@ SELECT null, -null
 ----
 NULL NULL
 
-query error type_coercion\ncaused by\nError during planning: Negation only supports numeric, interval and timestamp types
+query error Error during planning: Unary operator '-' only supports signed numeric, interval and timestamp types
 SELECT -'100'
 
 query error DataFusion error: Error during planning: Unary operator '\+' only supports numeric, interval and timestamp types