Skip to content

Commit 8f5158a

Browse files
logan-keedelogan-keede
authored
Make Diagnostic easy/convinient to attach by using macro and avoiding map_err (#15796)
* First Step * Final Step? * Homogenisation
1 parent e0fd892 commit 8f5158a

File tree

7 files changed

+125
-102
lines changed

7 files changed

+125
-102
lines changed

‎datafusion/common/src/error.rs‎

Lines changed: 61 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -759,23 +759,33 @@ macro_rules! make_error {
759759
/// Macro wraps `$ERR` to add backtrace feature
760760
#[macro_export]
761761
macro_rules! $NAME_DF_ERR {
762-
($d($d args:expr),*) => {
763-
$crate::DataFusionError::$ERR(
762+
($d($d args:expr),* $d(; diagnostic=$d DIAG:expr)?) => {{
763+
let err =$crate::DataFusionError::$ERR(
764764
::std::format!(
765765
"{}{}",
766766
::std::format!($d($d args),*),
767767
$crate::DataFusionError::get_back_trace(),
768768
).into()
769-
)
769+
);
770+
$d (
771+
let err = err.with_diagnostic($d DIAG);
772+
)?
773+
err
770774
}
771775
}
776+
}
772777

773778
/// Macro wraps Err(`$ERR`) to add backtrace feature
774779
#[macro_export]
775780
macro_rules! $NAME_ERR {
776-
($d($d args:expr),*) => {
777-
Err($crate::[<_ $NAME_DF_ERR>]!($d($d args),*))
778-
}
781+
($d($d args:expr),* $d(; diagnostic = $d DIAG:expr)?) => {{
782+
let err = $crate::[<_ $NAME_DF_ERR>]!($d($d args),*);
783+
$d (
784+
let err = err.with_diagnostic($d DIAG);
785+
)?
786+
Err(err)
787+
788+
}}
779789
}
780790

781791

@@ -816,54 +826,80 @@ make_error!(resources_err, resources_datafusion_err, ResourcesExhausted);
816826
// Exposes a macro to create `DataFusionError::SQL` with optional backtrace
817827
#[macro_export]
818828
macro_rules! sql_datafusion_err {
819-
($ERR:expr) => {
820-
DataFusionError::SQL($ERR, Some(DataFusionError::get_back_trace()))
821-
};
829+
($ERR:expr $(; diagnostic = $DIAG:expr)?) => {{
830+
let err = DataFusionError::SQL($ERR, Some(DataFusionError::get_back_trace()));
831+
$(
832+
let err = err.with_diagnostic($DIAG);
833+
)?
834+
err
835+
}};
822836
}
823837

824838
// Exposes a macro to create `Err(DataFusionError::SQL)` with optional backtrace
825839
#[macro_export]
826840
macro_rules! sql_err {
827-
($ERR:expr) => {
828-
Err(datafusion_common::sql_datafusion_err!($ERR))
829-
};
841+
($ERR:expr $(; diagnostic = $DIAG:expr)?) => {{
842+
let err = datafusion_common::sql_datafusion_err!($ERR);
843+
$(
844+
let err = err.with_diagnostic($DIAG);
845+
)?
846+
Err(err)
847+
}};
830848
}
831849

832850
// Exposes a macro to create `DataFusionError::ArrowError` with optional backtrace
833851
#[macro_export]
834852
macro_rules! arrow_datafusion_err {
835-
($ERR:expr) => {
836-
DataFusionError::ArrowError($ERR, Some(DataFusionError::get_back_trace()))
837-
};
853+
($ERR:expr $(; diagnostic = $DIAG:expr)?) => {{
854+
let err = DataFusionError::ArrowError($ERR, Some(DataFusionError::get_back_trace()));
855+
$(
856+
let err = err.with_diagnostic($DIAG);
857+
)?
858+
err
859+
}};
838860
}
839861

840862
// Exposes a macro to create `Err(DataFusionError::ArrowError)` with optional backtrace
841863
#[macro_export]
842864
macro_rules! arrow_err {
843-
($ERR:expr) => {
844-
Err(datafusion_common::arrow_datafusion_err!($ERR))
845-
};
865+
($ERR:expr $(; diagnostic = $DIAG:expr)?) => {
866+
{
867+
let err = datafusion_common::arrow_datafusion_err!($ERR);
868+
$(
869+
let err = err.with_diagnostic($DIAG);
870+
)?
871+
Err(err)
872+
}};
846873
}
847874

848875
// Exposes a macro to create `DataFusionError::SchemaError` with optional backtrace
849876
#[macro_export]
850877
macro_rules! schema_datafusion_err {
851-
($ERR:expr) => {
852-
$crate::error::DataFusionError::SchemaError(
878+
($ERR:expr $(; diagnostic = $DIAG:expr)?) => {{
879+
let err = $crate::error::DataFusionError::SchemaError(
853880
$ERR,
854881
Box::new(Some($crate::error::DataFusionError::get_back_trace())),
855-
)
856-
};
882+
);
883+
$(
884+
let err = err.with_diagnostic($DIAG);
885+
)?
886+
err
887+
}};
857888
}
858889

859890
// Exposes a macro to create `Err(DataFusionError::SchemaError)` with optional backtrace
860891
#[macro_export]
861892
macro_rules! schema_err {
862-
($ERR:expr) => {
863-
Err($crate::error::DataFusionError::SchemaError(
893+
($ERR:expr $(; diagnostic = $DIAG:expr)?) => {{
894+
let err = $crate::error::DataFusionError::SchemaError(
864895
$ERR,
865896
Box::new(Some($crate::error::DataFusionError::get_back_trace())),
866-
))
897+
);
898+
$(
899+
let err = err.with_diagnostic($DIAG);
900+
)?
901+
Err(err)
902+
}
867903
};
868904
}
869905

‎datafusion/sql/src/expr/function.rs‎

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -457,17 +457,12 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
457457
if let Some(suggested_func_name) =
458458
suggest_valid_function(&name, is_function_window, self.context_provider)
459459
{
460-
plan_err!("Invalid function '{name}'.\nDid you mean '{suggested_func_name}'?")
461-
.map_err(|e| {
462-
let span = Span::try_from_sqlparser_span(sql_parser_span);
463-
let mut diagnostic =
464-
Diagnostic::new_error(format!("Invalid function '{name}'"), span);
465-
diagnostic.add_note(
466-
format!("Possible function '{}'", suggested_func_name),
467-
None,
468-
);
469-
e.with_diagnostic(diagnostic)
470-
})
460+
let span = Span::try_from_sqlparser_span(sql_parser_span);
461+
let mut diagnostic =
462+
Diagnostic::new_error(format!("Invalid function '{name}'"), span);
463+
diagnostic
464+
.add_note(format!("Possible function '{}'", suggested_func_name), None);
465+
plan_err!("Invalid function '{name}'.\nDid you mean '{suggested_func_name}'?"; diagnostic=diagnostic)
471466
} else {
472467
internal_err!("No functions registered with this context.")
473468
}

‎datafusion/sql/src/expr/subquery.rs‎

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -138,15 +138,9 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
138138
if sub_plan.schema().fields().len() > 1 {
139139
let sub_schema = sub_plan.schema();
140140
let field_names = sub_schema.field_names();
141-
142-
plan_err!("{}: {}", error_message, field_names.join(", ")).map_err(|err| {
143-
let diagnostic = self.build_multi_column_diagnostic(
144-
spans,
145-
error_message,
146-
help_message,
147-
);
148-
err.with_diagnostic(diagnostic)
149-
})
141+
let diagnostic =
142+
self.build_multi_column_diagnostic(spans, error_message, help_message);
143+
plan_err!("{}: {}", error_message, field_names.join(", "); diagnostic=diagnostic)
150144
} else {
151145
Ok(())
152146
}

‎datafusion/sql/src/expr/unary_op.rs‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -45,16 +45,18 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
4545
{
4646
Ok(operand)
4747
} else {
48-
plan_err!("Unary operator '+' only supports numeric, interval and timestamp types").map_err(|e| {
49-
let span = operand.spans().and_then(|s| s.first());
50-
let mut diagnostic = Diagnostic::new_error(
51-
format!("+ cannot be used with {data_type}"),
52-
span
53-
);
54-
diagnostic.add_note("+ can only be used with numbers, intervals, and timestamps", None);
55-
diagnostic.add_help(format!("perhaps you need to cast {operand}"), None);
56-
e.with_diagnostic(diagnostic)
57-
})
48+
let span = operand.spans().and_then(|s| s.first());
49+
let mut diagnostic = Diagnostic::new_error(
50+
format!("+ cannot be used with {data_type}"),
51+
span,
52+
);
53+
diagnostic.add_note(
54+
"+ can only be used with numbers, intervals, and timestamps",
55+
None,
56+
);
57+
diagnostic
58+
.add_help(format!("perhaps you need to cast {operand}"), None);
59+
plan_err!("Unary operator '+' only supports numeric, interval and timestamp types"; diagnostic=diagnostic)
5860
}
5961
}
6062
UnaryOperator::Minus => {

‎datafusion/sql/src/parser.rs‎

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -39,9 +39,14 @@ use std::fmt;
3939

4040
// Use `Parser::expected` instead, if possible
4141
macro_rules! parser_err {
42-
($MSG:expr) => {
43-
Err(ParserError::ParserError($MSG.to_string()))
44-
};
42+
($MSG:expr $(; diagnostic = $DIAG:expr)?) => {{
43+
44+
let err = DataFusionError::from(ParserError::ParserError($MSG.to_string()));
45+
$(
46+
let err = err.with_diagnostic($DIAG);
47+
)?
48+
Err(err)
49+
}};
4550
}
4651

4752
fn parse_file_type(s: &str) -> Result<String, DataFusionError> {
@@ -448,20 +453,16 @@ impl<'a> DFParser<'a> {
448453
found: TokenWithSpan,
449454
) -> Result<T, DataFusionError> {
450455
let sql_parser_span = found.span;
451-
parser_err!(format!(
452-
"Expected: {expected}, found: {found}{}",
453-
found.span.start
454-
))
455-
.map_err(|e| {
456-
let e = DataFusionError::from(e);
457-
let span = Span::try_from_sqlparser_span(sql_parser_span);
458-
let diagnostic = Diagnostic::new_error(
459-
format!("Expected: {expected}, found: {found}{}", found.span.start),
460-
span,
461-
);
462-
463-
e.with_diagnostic(diagnostic)
464-
})
456+
let span = Span::try_from_sqlparser_span(sql_parser_span);
457+
let diagnostic = Diagnostic::new_error(
458+
format!("Expected: {expected}, found: {found}{}", found.span.start),
459+
span,
460+
);
461+
parser_err!(
462+
format!("Expected: {expected}, found: {found}{}", found.span.start);
463+
diagnostic=
464+
diagnostic
465+
)
465466
}
466467

467468
/// Parse a new expression

‎datafusion/sql/src/set_expr.rs‎

Lines changed: 16 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -95,26 +95,22 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
9595
if left_plan.schema().fields().len() == right_plan.schema().fields().len() {
9696
return Ok(());
9797
}
98-
99-
plan_err!("{} queries have different number of columns", op).map_err(|err| {
100-
err.with_diagnostic(
101-
Diagnostic::new_error(
102-
format!("{} queries have different number of columns", op),
103-
set_expr_span,
104-
)
105-
.with_note(
106-
format!("this side has {} fields", left_plan.schema().fields().len()),
107-
left_span,
108-
)
109-
.with_note(
110-
format!(
111-
"this side has {} fields",
112-
right_plan.schema().fields().len()
113-
),
114-
right_span,
115-
),
116-
)
117-
})
98+
let diagnostic = Diagnostic::new_error(
99+
format!("{} queries have different number of columns", op),
100+
set_expr_span,
101+
)
102+
.with_note(
103+
format!("this side has {} fields", left_plan.schema().fields().len()),
104+
left_span,
105+
)
106+
.with_note(
107+
format!(
108+
"this side has {} fields",
109+
right_plan.schema().fields().len()
110+
),
111+
right_span,
112+
);
113+
plan_err!("{} queries have different number of columns", op; diagnostic =diagnostic)
118114
}
119115

120116
pub(super) fn set_operation_to_plan(

‎datafusion/sql/src/utils.rs‎

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -158,20 +158,19 @@ fn check_column_satisfies_expr(
158158
purpose: CheckColumnsSatisfyExprsPurpose,
159159
) -> Result<()> {
160160
if !columns.contains(expr) {
161+
let diagnostic = Diagnostic::new_error(
162+
purpose.diagnostic_message(expr),
163+
expr.spans().and_then(|spans| spans.first()),
164+
)
165+
.with_help(format!("Either add '{expr}' to GROUP BY clause, or use an aggregare function like ANY_VALUE({expr})"), None);
166+
161167
return plan_err!(
162168
"{}: 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",
163169
purpose.message_prefix(),
164170
expr,
165-
expr_vec_fmt!(columns)
166-
)
167-
.map_err(|err| {
168-
let diagnostic = Diagnostic::new_error(
169-
purpose.diagnostic_message(expr),
170-
expr.spans().and_then(|spans| spans.first()),
171-
)
172-
.with_help(format!("Either add '{expr}' to GROUP BY clause, or use an aggregare function like ANY_VALUE({expr})"), None);
173-
err.with_diagnostic(diagnostic)
174-
});
171+
expr_vec_fmt!(columns);
172+
diagnostic=diagnostic
173+
);
175174
}
176175
Ok(())
177176
}

0 commit comments

Comments
 (0)