Skip to content

Commit 33be555

Browse files
Ian LaiIan Lai
Ian Lai
authored and
Ian Lai
committed
feat: improve diagnostic messages for function argument validation
1 parent cd7d998 commit 33be555

File tree

2 files changed

+46
-75
lines changed

2 files changed

+46
-75
lines changed

‎datafusion/expr/src/expr_schema.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -439,6 +439,11 @@ impl ExprSchemable for Expr {
439439
"{} {}",
440440
match err {
441441
DataFusionError::Plan(msg) => msg,
442+
DataFusionError::Diagnostic(_, boxed_err) =>
443+
match *boxed_err {
444+
DataFusionError::Plan(msg) => msg,
445+
_ => boxed_err.to_string(),
446+
},
442447
err => err.to_string(),
443448
},
444449
utils::generate_signature_error_msg(

‎datafusion/expr/src/type_coercion/functions.rs

Lines changed: 41 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -50,16 +50,12 @@ pub fn data_types_with_scalar_udf(
5050
let signature = func.signature();
5151
let type_signature = &signature.type_signature;
5252

53-
if current_types.is_empty() && type_signature != &TypeSignature::UserDefined {
54-
if type_signature.supports_zero_argument() {
55-
return Ok(vec![]);
56-
} else if type_signature.used_to_support_zero_arguments() {
57-
// Special error to help during upgrade: https://github.com/apache/datafusion/issues/13763
58-
return plan_err!("'{}' does not support zero arguments. Use TypeSignature::Nullary for zero arguments", func.name());
59-
} else {
60-
return plan_err!("'{}' does not support zero arguments", func.name());
61-
}
62-
}
53+
check_function_length_with_diag(
54+
func.name(),
55+
type_signature,
56+
current_types,
57+
None,
58+
)?;
6359

6460
let valid_types =
6561
get_valid_types_with_scalar_udf(type_signature, current_types, func)?;
@@ -88,16 +84,12 @@ pub fn data_types_with_aggregate_udf(
8884
let signature = func.signature();
8985
let type_signature = &signature.type_signature;
9086

91-
if current_types.is_empty() && type_signature != &TypeSignature::UserDefined {
92-
if type_signature.supports_zero_argument() {
93-
return Ok(vec![]);
94-
} else if type_signature.used_to_support_zero_arguments() {
95-
// Special error to help during upgrade: https://github.com/apache/datafusion/issues/13763
96-
return plan_err!("'{}' does not support zero arguments. Use TypeSignature::Nullary for zero arguments", func.name());
97-
} else {
98-
return plan_err!("'{}' does not support zero arguments", func.name());
99-
}
100-
}
87+
check_function_length_with_diag(
88+
func.name(),
89+
type_signature,
90+
current_types,
91+
None,
92+
)?;
10193

10294
let valid_types =
10395
get_valid_types_with_aggregate_udf(type_signature, current_types, func)?;
@@ -125,16 +117,12 @@ pub fn data_types_with_window_udf(
125117
let signature = func.signature();
126118
let type_signature = &signature.type_signature;
127119

128-
if current_types.is_empty() && type_signature != &TypeSignature::UserDefined {
129-
if type_signature.supports_zero_argument() {
130-
return Ok(vec![]);
131-
} else if type_signature.used_to_support_zero_arguments() {
132-
// Special error to help during upgrade: https://github.com/apache/datafusion/issues/13763
133-
return plan_err!("'{}' does not support zero arguments. Use TypeSignature::Nullary for zero arguments", func.name());
134-
} else {
135-
return plan_err!("'{}' does not support zero arguments", func.name());
136-
}
137-
}
120+
check_function_length_with_diag(
121+
func.name(),
122+
type_signature,
123+
current_types,
124+
None,
125+
)?;
138126

139127
let valid_types =
140128
get_valid_types_with_window_udf(type_signature, current_types, func)?;
@@ -328,7 +316,9 @@ pub fn check_function_length_with_diag(
328316
TypeSignature::String(num) |
329317
TypeSignature::Comparable(num) |
330318
TypeSignature::Any(num) => {
319+
if current_types.len() != *num {
331320
return create_error(&format!("expects {} arguments", num), current_types.len());
321+
}
332322
},
333323
TypeSignature::Exact(types) => {
334324
if current_types.len() != types.len() {
@@ -376,19 +366,13 @@ pub fn check_function_length_with_diag(
376366
}
377367
}
378368

379-
// If we're here, none of the signatures matched
369+
// none of the signatures matched
380370
if !all_errors.is_empty() {
381-
let error_messages = all_errors
382-
.iter()
383-
.map(|e| e.to_string())
384-
.collect::<Vec<_>>()
385-
.join("; ");
386371

387372
let base_error = plan_datafusion_err!(
388-
"Function '{}' has no matching signature for {} arguments. Errors: {}",
373+
"Function '{}' has no matching signature for {} arguments.",
389374
function_name,
390-
current_types.len(),
391-
error_messages
375+
current_types.len()
392376
);
393377

394378
let mut diagnostic = Diagnostic::new_error(
@@ -652,27 +636,12 @@ fn get_valid_types(
652636
}
653637
}
654638

655-
fn function_length_check(
656-
function_name: &str,
657-
length: usize,
658-
expected_length: usize,
659-
) -> Result<()> {
660-
if length != expected_length {
661-
return plan_err!(
662-
"Function '{function_name}' expects {expected_length} arguments but received {length}"
663-
);
664-
}
665-
Ok(())
666-
}
667-
668639
let valid_types = match signature {
669640
TypeSignature::Variadic(valid_types) => valid_types
670641
.iter()
671642
.map(|valid_type| current_types.iter().map(|_| valid_type.clone()).collect())
672643
.collect(),
673644
TypeSignature::String(number) => {
674-
function_length_check(function_name, current_types.len(), *number)?;
675-
676645
let mut new_types = Vec::with_capacity(current_types.len());
677646
for data_type in current_types.iter() {
678647
let logical_data_type: NativeType = data_type.into();
@@ -731,8 +700,6 @@ fn get_valid_types(
731700
vec![vec![base_type_or_default_type(&coerced_type); *number]]
732701
}
733702
TypeSignature::Numeric(number) => {
734-
function_length_check(function_name, current_types.len(), *number)?;
735-
736703
// Find common numeric type among given types except string
737704
let mut valid_type = current_types.first().unwrap().to_owned();
738705
for t in current_types.iter().skip(1) {
@@ -771,7 +738,6 @@ fn get_valid_types(
771738
vec![vec![valid_type; *number]]
772739
}
773740
TypeSignature::Comparable(num) => {
774-
function_length_check(function_name, current_types.len(), *num)?;
775741
let mut target_type = current_types[0].to_owned();
776742
for data_type in current_types.iter().skip(1) {
777743
if let Some(dt) = comparison_coercion_numeric(&target_type, data_type) {
@@ -867,27 +833,27 @@ fn get_valid_types(
867833
}
868834
},
869835
TypeSignature::Nullary => {
870-
if !current_types.is_empty() {
871-
return plan_err!(
872-
"The function '{function_name}' expected zero argument but received {}",
873-
current_types.len()
874-
);
875-
}
836+
// if !current_types.is_empty() {
837+
// return plan_err!(
838+
// "The function '{function_name}' expected zero argument but received {}",
839+
// current_types.len()
840+
// );
841+
// }
876842
vec![vec![]]
877843
}
878844
TypeSignature::Any(number) => {
879-
if current_types.is_empty() {
880-
return plan_err!(
881-
"The function '{function_name}' expected at least one argument but received 0"
882-
);
883-
}
884-
885-
if current_types.len() != *number {
886-
return plan_err!(
887-
"The function '{function_name}' expected {number} arguments but received {}",
888-
current_types.len()
889-
);
890-
}
845+
// if current_types.is_empty() {
846+
// return plan_err!(
847+
// "The function '{function_name}' expected at least one argument but received 0"
848+
// );
849+
// }
850+
851+
// if current_types.len() != *number {
852+
// return plan_err!(
853+
// "The function '{function_name}' expected {number} arguments but received {}",
854+
// current_types.len()
855+
// );
856+
// }
891857
vec![(0..*number).map(|i| current_types[i].clone()).collect()]
892858
}
893859
TypeSignature::OneOf(types) => types

0 commit comments

Comments
 (0)