Make Diagnostic easy/convinient to attach by using macro and avoiding map_err

datafusion/common/src/error.rs

@@ -759,23 +759,33 @@ macro_rules! make_error {
             /// Macro wraps `$ERR` to add backtrace feature
             #[macro_export]
             macro_rules! $NAME_DF_ERR {
-                ($d($d args:expr),*) => {
-                    $crate::DataFusionError::$ERR(
+                ($d($d args:expr),* $d(; diagnostic=$d DIAG:expr)?) => {{
+                    let err =$crate::DataFusionError::$ERR(
                         ::std::format!(
                             "{}{}",
                             ::std::format!($d($d args),*),
                             $crate::DataFusionError::get_back_trace(),
                         ).into()
-                    )
+                    );
+                    $d (
+                        let err = err.with_diagnostic($d DIAG);
+                    )?
+                    err
                 }
             }
+        }
 
             /// Macro wraps Err(`$ERR`) to add backtrace feature
             #[macro_export]
             macro_rules! $NAME_ERR {
-                ($d($d args:expr),*) => {
-                    Err($crate::[<_ $NAME_DF_ERR>]!($d($d args),*))
-                }
+                ($d($d args:expr),* $d(; diagnostic = $d DIAG:expr)?) => {{
+                    let err = $crate::[<_ $NAME_DF_ERR>]!($d($d args),*);
+                    $d (
+                        let err = err.with_diagnostic($d DIAG);
+                    )?
+                    Err(err)
+
+                }}
             }
 
 
@@ -816,54 +826,80 @@ make_error!(resources_err, resources_datafusion_err, ResourcesExhausted);
 // Exposes a macro to create `DataFusionError::SQL` with optional backtrace
 #[macro_export]
 macro_rules! sql_datafusion_err {
-    ($ERR:expr) => {
-        DataFusionError::SQL($ERR, Some(DataFusionError::get_back_trace()))
-    };
+    ($ERR:expr $(; diagnostic = $DIAG:expr)?) => {{
+        let err = DataFusionError::SQL($ERR, Some(DataFusionError::get_back_trace()));
+        $(
+            let err = err.with_diagnostic($DIAG);
+        )?
+        err
+    }};
 }
 
 // Exposes a macro to create `Err(DataFusionError::SQL)` with optional backtrace
 #[macro_export]
 macro_rules! sql_err {
-    ($ERR:expr) => {
-        Err(datafusion_common::sql_datafusion_err!($ERR))
-    };
+    ($ERR:expr $(; diagnostic = $DIAG:expr)?) => {{
+        let err = datafusion_common::sql_datafusion_err!($ERR);
+        $(
+            let err = err.with_diagnostic($DIAG);
+        )?
+        Err(err)
+    }};
 }
 
 // Exposes a macro to create `DataFusionError::ArrowError` with optional backtrace
 #[macro_export]
 macro_rules! arrow_datafusion_err {
-    ($ERR:expr) => {
-        DataFusionError::ArrowError($ERR, Some(DataFusionError::get_back_trace()))
-    };
+    ($ERR:expr $(; diagnostic = $DIAG:expr)?) => {{
+        let err = DataFusionError::ArrowError($ERR, Some(DataFusionError::get_back_trace()));
+        $(
+            let err = err.with_diagnostic($DIAG);
+        )?
+        err
+    }};
 }
 
 // Exposes a macro to create `Err(DataFusionError::ArrowError)` with optional backtrace
 #[macro_export]
 macro_rules! arrow_err {
-    ($ERR:expr) => {
-        Err(datafusion_common::arrow_datafusion_err!($ERR))
-    };
+    ($ERR:expr $(; diagnostic = $DIAG:expr)?) => {
+    {
+        let err = datafusion_common::arrow_datafusion_err!($ERR);
+        $(
+            let err = err.with_diagnostic($DIAG);
+        )?
+        Err(err)
+    }};
 }
 
 // Exposes a macro to create `DataFusionError::SchemaError` with optional backtrace
 #[macro_export]
 macro_rules! schema_datafusion_err {
-    ($ERR:expr) => {
-        $crate::error::DataFusionError::SchemaError(
+    ($ERR:expr $(; diagnostic = $DIAG:expr)?) => {{
+        let err = $crate::error::DataFusionError::SchemaError(
             $ERR,
             Box::new(Some($crate::error::DataFusionError::get_back_trace())),
-        )
-    };
+        );
+        $(
+            let err = err.with_diagnostic($DIAG);
+        )?
+        err
+    }};
 }
 
 // Exposes a macro to create `Err(DataFusionError::SchemaError)` with optional backtrace
 #[macro_export]
 macro_rules! schema_err {
-    ($ERR:expr) => {
-        Err($crate::error::DataFusionError::SchemaError(
+    ($ERR:expr $(; diagnostic = $DIAG:expr)?) => {{
+        let err = $crate::error::DataFusionError::SchemaError(
             $ERR,
             Box::new(Some($crate::error::DataFusionError::get_back_trace())),
-        ))
+        );
+        $(
+            let err = err.with_diagnostic($DIAG);
+        )?
+        Err(err)
+    }
     };
 }
 

datafusion/sql/src/expr/function.rs

@@ -408,17 +408,12 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
         if let Some(suggested_func_name) =
             suggest_valid_function(&name, is_function_window, self.context_provider)
         {
-            plan_err!("Invalid function '{name}'.\nDid you mean '{suggested_func_name}'?")
-                .map_err(|e| {
-                    let span = Span::try_from_sqlparser_span(sql_parser_span);
-                    let mut diagnostic =
-                        Diagnostic::new_error(format!("Invalid function '{name}'"), span);
-                    diagnostic.add_note(
-                        format!("Possible function '{}'", suggested_func_name),
-                        None,
-                    );
-                    e.with_diagnostic(diagnostic)
-                })
+            let span = Span::try_from_sqlparser_span(sql_parser_span);
+            let mut diagnostic =
+                Diagnostic::new_error(format!("Invalid function '{name}'"), span);
+            diagnostic
+                .add_note(format!("Possible function '{}'", suggested_func_name), None);
+            plan_err!("Invalid function '{name}'.\nDid you mean '{suggested_func_name}'?"; diagnostic=diagnostic)
         } else {
             internal_err!("No functions registered with this context.")
         }

datafusion/sql/src/expr/subquery.rs

@@ -138,15 +138,9 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
         if sub_plan.schema().fields().len() > 1 {
             let sub_schema = sub_plan.schema();
             let field_names = sub_schema.field_names();
-
-            plan_err!("{}: {}", error_message, field_names.join(", ")).map_err(|err| {
-                let diagnostic = self.build_multi_column_diagnostic(
-                    spans,
-                    error_message,
-                    help_message,
-                );
-                err.with_diagnostic(diagnostic)
-            })
+            let diagnostic =
+                self.build_multi_column_diagnostic(spans, error_message, help_message);
+            plan_err!("{}: {}", error_message, field_names.join(", "); diagnostic=diagnostic)
         } else {
             Ok(())
         }

datafusion/sql/src/expr/unary_op.rs

@@ -45,16 +45,18 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
                 {
                     Ok(operand)
                 } else {
-                    plan_err!("Unary operator '+' only supports numeric, interval and timestamp types").map_err(|e| {
-                        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 numbers, intervals, and timestamps", None);
-                        diagnostic.add_help(format!("perhaps you need to cast {operand}"), None);
-                        e.with_diagnostic(diagnostic)
-                    })
+                    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 numbers, intervals, and timestamps",
+                        None,
+                    );
+                    diagnostic
+                        .add_help(format!("perhaps you need to cast {operand}"), None);
+                    plan_err!("Unary operator '+' only supports numeric, interval and timestamp types"; diagnostic=diagnostic)
                 }
             }
             UnaryOperator::Minus => {

datafusion/sql/src/parser.rs

@@ -39,9 +39,14 @@ use std::fmt;
 
 // Use `Parser::expected` instead, if possible
 macro_rules! parser_err {
-    ($MSG:expr) => {
-        Err(ParserError::ParserError($MSG.to_string()))
-    };
+    ($MSG:expr $(; diagnostic = $DIAG:expr)?) => {{
+
+        let err = DataFusionError::from(ParserError::ParserError($MSG.to_string()));
+        $(
+            let err = err.with_diagnostic($DIAG);
+        )?
+        Err(err)
+    }};
 }
 
 fn parse_file_type(s: &str) -> Result<String, DataFusionError> {
@@ -448,20 +453,16 @@ impl<'a> DFParser<'a> {
         found: TokenWithSpan,
     ) -> Result<T, DataFusionError> {
         let sql_parser_span = found.span;
-        parser_err!(format!(
-            "Expected: {expected}, found: {found}{}",
-            found.span.start
-        ))
-        .map_err(|e| {
-            let e = DataFusionError::from(e);
-            let span = Span::try_from_sqlparser_span(sql_parser_span);
-            let diagnostic = Diagnostic::new_error(
-                format!("Expected: {expected}, found: {found}{}", found.span.start),
-                span,
-            );
-
-            e.with_diagnostic(diagnostic)
-        })
+        let span = Span::try_from_sqlparser_span(sql_parser_span);
+        let diagnostic = Diagnostic::new_error(
+            format!("Expected: {expected}, found: {found}{}", found.span.start),
+            span,
+        );
+        parser_err!(
+            format!("Expected: {expected}, found: {found}{}", found.span.start);
+            diagnostic=
+            diagnostic
+        )
     }
 
     /// Parse a new expression

datafusion/sql/src/set_expr.rs

@@ -95,26 +95,22 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
         if left_plan.schema().fields().len() == right_plan.schema().fields().len() {
             return Ok(());
         }
-
-        plan_err!("{} queries have different number of columns", op).map_err(|err| {
-            err.with_diagnostic(
-                Diagnostic::new_error(
-                    format!("{} queries have different number of columns", op),
-                    set_expr_span,
-                )
-                .with_note(
-                    format!("this side has {} fields", left_plan.schema().fields().len()),
-                    left_span,
-                )
-                .with_note(
-                    format!(
-                        "this side has {} fields",
-                        right_plan.schema().fields().len()
-                    ),
-                    right_span,
-                ),
-            )
-        })
+        let diagnostic = Diagnostic::new_error(
+            format!("{} queries have different number of columns", op),
+            set_expr_span,
+        )
+        .with_note(
+            format!("this side has {} fields", left_plan.schema().fields().len()),
+            left_span,
+        )
+        .with_note(
+            format!(
+                "this side has {} fields",
+                right_plan.schema().fields().len()
+            ),
+            right_span,
+        );
+        plan_err!("{} queries have different number of columns", op; diagnostic =diagnostic)
     }
 
     pub(super) fn set_operation_to_plan(

datafusion/sql/src/utils.rs

@@ -158,20 +158,19 @@ fn check_column_satisfies_expr(
     purpose: CheckColumnsSatisfyExprsPurpose,
 ) -> Result<()> {
     if !columns.contains(expr) {
+        let diagnostic = Diagnostic::new_error(
+            purpose.diagnostic_message(expr),
+            expr.spans().and_then(|spans| spans.first()),
+        )
+        .with_help(format!("Either add '{expr}' to GROUP BY clause, or use an aggregare function like ANY_VALUE({expr})"), None);
+
         return plan_err!(
             "{}: While expanding wildcard, column \"{}\" must appear in the GROUP BY clause or must be part of an aggregate function, currently only \"{}\" appears in the SELECT clause satisfies this requirement",
             purpose.message_prefix(),
             expr,
-            expr_vec_fmt!(columns)
-        )
-        .map_err(|err| {
-            let diagnostic = Diagnostic::new_error(
-                purpose.diagnostic_message(expr),
-                expr.spans().and_then(|spans| spans.first()),
-            )
-            .with_help(format!("Either add '{expr}' to GROUP BY clause, or use an aggregare function like ANY_VALUE({expr})"), None);
-            err.with_diagnostic(diagnostic)
-        });
+            expr_vec_fmt!(columns);
+            diagnostic=diagnostic
+        );
     }
     Ok(())
 }