chore: Add detailed error for sum::coerce_type #14710
Conversation
dentiny
changed the title
|
Fyi @eliaperantoni -- can you give this PR a review? |
There was a problem hiding this comment.
Unfortunately I don't quite think this is the correct approach. You're adding Diagnostic in one function implementation provided by datafusion::functions_aggregate. This won't work with custom UDFs that are registered in the SessionState by third party applications using DataFusion.
I would prefer if we enriched with diagnostics the errors that DataFusion already returns in a generic way for all functions, even those that are not provided in datafusion::function*, leveraging DataFusion's type system.
| let diagnostic = Diagnostic::new_error( | ||
| format!( | ||
| "Coercion only supports integer and floating point values" | ||
| ), | ||
| sum.spans().first(), | ||
| ); |
There was a problem hiding this comment.
I don't think "Coercion only supports integer and floating point values" is a message that an end user of an application built on DataFusion can understand. Perhaps something like 'sum' only works on numeric values?
| #[test] | ||
| fn test_sum_coerce_type() -> Result<()> { | ||
| let mut sum = Sum::new(); | ||
| let span = Span::new(Location::new(1, 1), Location::new(2, 1)); | ||
| let span_for_test = span; | ||
| sum.spans_mut().add_span(span); | ||
| let datatype_vec = vec![DataType::Date64]; | ||
| let res = sum.coerce_types(&datatype_vec); | ||
| let err = res.expect_err("Expected an error, but got success"); |
There was a problem hiding this comment.
Can you use the same setup as other tests in this file? e.g. get_spans, run_query, etc.
|
Thank you @dentiny ! Looks like this PR has some good comments from @eliaperantoni and needs a fix for |
Which issue does this PR close?
Diagnosticto "invalid function argument types" error #14431What changes are included in this PR?
This PR integrates
DiagnosticintoSum::coerce_type.Reading through the code, there're quite a few function worth updating for "invalid function argument type" category, this PR only updates one particular function.