This is an automated email from the ASF dual-hosted git repository.
comphead pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/datafusion.git
The following commit(s) were added to refs/heads/main by this push:
new 8f5158a071 Make `Diagnostic` easy/convinient to attach by using macro
and avoiding `map_err` (#15796)
8f5158a071 is described below
commit 8f5158a071a49142e2ef2a83eacd0e30ca76eee1
Author: logan-keede <68557630+logan-ke...@users.noreply.github.com>
AuthorDate: Thu Apr 24 20:13:49 2025 +0530
Make `Diagnostic` easy/convinient to attach by using macro and avoiding
`map_err` (#15796)
* First Step
* Final Step?
* Homogenisation
---
datafusion/common/src/error.rs | 86 ++++++++++++++++++++++++++-----------
datafusion/sql/src/expr/function.rs | 17 +++-----
datafusion/sql/src/expr/subquery.rs | 12 ++----
datafusion/sql/src/expr/unary_op.rs | 22 +++++-----
datafusion/sql/src/parser.rs | 35 +++++++--------
datafusion/sql/src/set_expr.rs | 36 +++++++---------
datafusion/sql/src/utils.rs | 19 ++++----
7 files changed, 125 insertions(+), 102 deletions(-)
diff --git a/datafusion/common/src/error.rs b/datafusion/common/src/error.rs
index c50ec64759..e1f3d13315 100644
--- a/datafusion/common/src/error.rs
+++ b/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)
+ }
};
}
diff --git a/datafusion/sql/src/expr/function.rs
b/datafusion/sql/src/expr/function.rs
index c0cb5b38ff..535300427a 100644
--- a/datafusion/sql/src/expr/function.rs
+++ b/datafusion/sql/src/expr/function.rs
@@ -457,17 +457,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.")
}
diff --git a/datafusion/sql/src/expr/subquery.rs
b/datafusion/sql/src/expr/subquery.rs
index 225c5d74c2..602d39233d 100644
--- a/datafusion/sql/src/expr/subquery.rs
+++ b/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(())
}
diff --git a/datafusion/sql/src/expr/unary_op.rs
b/datafusion/sql/src/expr/unary_op.rs
index 626b79d6c3..e0c94543f6 100644
--- a/datafusion/sql/src/expr/unary_op.rs
+++ b/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 => {
diff --git a/datafusion/sql/src/parser.rs b/datafusion/sql/src/parser.rs
index 27c897f7ad..f3e94bfc5e 100644
--- a/datafusion/sql/src/parser.rs
+++ b/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
diff --git a/datafusion/sql/src/set_expr.rs b/datafusion/sql/src/set_expr.rs
index 272d6f874b..78c63d7db7 100644
--- a/datafusion/sql/src/set_expr.rs
+++ b/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(
diff --git a/datafusion/sql/src/utils.rs b/datafusion/sql/src/utils.rs
index bc2a94cd44..8be45f3d8c 100644
--- a/datafusion/sql/src/utils.rs
+++ b/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(())
}
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@datafusion.apache.org
For additional commands, e-mail: commits-h...@datafusion.apache.org
