eliaperantoni commented on code in PR #15490:
URL: https://github.com/apache/datafusion/pull/15490#discussion_r2022232702
##########
datafusion/common/src/utils/mod.rs:
##########
@@ -979,16 +981,19 @@ pub fn take_function_args<const N: usize, T>(
) -> Result<[T; N]> {
let args = args.into_iter().collect::<Vec<_>>();
args.try_into().map_err(|v: Vec<T>| {
- _exec_datafusion_err!(
+ let msg = format!(
"{} function requires {} {}, got {}",
function_name,
N,
if N == 1 { "argument" } else { "arguments" },
v.len()
- )
+ );
+ let diag = Diagnostic::new_error(msg.clone(), None);
+ DataFusionError::Execution(msg).with_diagnostic(diag)
Review Comment:
Have you checked that `take_function_args` is also called for all function
calls?
In https://github.com/apache/datafusion/issues/14432#issuecomment-2658302749
it was discovered that sometimes `function_length_check` is called instead.
##########
datafusion/sql/tests/cases/diagnostic.rs:
##########
@@ -392,3 +392,21 @@ fn test_unary_op_plus_with_non_column() -> Result<()> {
assert_eq!(diag.span, None);
Ok(())
}
+
+
+#[test]
+fn test_wrong_number_of_arguments_sum() -> Result<()> {
+ let query = "SELECT /*a*/sum(1, 2)/*a*/";
+ let spans = get_spans(query);
+ let diag = do_query(query);
+
+ assert_eq!(
+ diag.message,
+ "sum function requires 1 argument, got 2"
+ );
+
+ // Verify that the span actually targets "sum(1, 2)"
+ assert_eq!(diag.span, Some(spans["a"]));
+
+ Ok(())
+}
Review Comment:
This test case is triggering the error not because there's anything wrong
with the number of arguments, but because `sum` is not registered. And the
parser incorrectly attaches a `sum function requires 1 argument, got 2`
`Diagnostic` to a `Invalid function 'sum'` error.
##########
datafusion/sql/src/expr/function.rs:
##########
@@ -409,20 +410,28 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
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)
- })
- } else {
- internal_err!("No functions registered with this context.")
- }
+ .map_err(|_e| {
+ let msg = format!("{} function requires 1 argument, got 2",
name);
Review Comment:
This is an hardcoded error message in the parser, that only works for the
specific test case you added
##########
datafusion/sql/src/expr/function.rs:
##########
@@ -409,20 +410,28 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
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)
- })
- } else {
- internal_err!("No functions registered with this context.")
- }
+ .map_err(|_e| {
+ let msg = format!("{} function requires 1 argument, got 2",
name);
Review Comment:
The `DataFusionError` is inconsistent with its `Diagnostic`. You are
attaching a `Diagnostic` about a wrong number of arguments when the error was
that the function could not be found.
##########
datafusion/sql/src/expr/function.rs:
##########
@@ -409,20 +410,28 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
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)
- })
- } else {
- internal_err!("No functions registered with this context.")
- }
+ .map_err(|_e| {
+ let msg = format!("{} function requires 1 argument, got 2",
name);
+
+ let mut span = Span::try_from_sqlparser_span(sql_parser_span);
+ if let Some(s) = &mut span {
+ // Temporary patch: manually extend the span end to column
22
+ s.end.column = 22;
+ }
+
+
+ let mut diagnostic = Diagnostic::new_error(msg.clone(), span);
+ diagnostic.add_note(
+ format!("Possible function '{}'", suggested_func_name),
+ None,
+ );
+
+ DataFusionError::Execution(msg).with_diagnostic(diagnostic)
Review Comment:
You're discarding the original `DataFusionError`. A `map_err` for
`Diagnostic` purposes should preserve it and call `with_diagnostic` on it.
##########
datafusion/sql/tests/cases/diagnostic.rs:
##########
@@ -392,3 +392,21 @@ fn test_unary_op_plus_with_non_column() -> Result<()> {
assert_eq!(diag.span, None);
Ok(())
}
+
+
+#[test]
+fn test_wrong_number_of_arguments_sum() -> Result<()> {
+ let query = "SELECT /*a*/sum(1, 2)/*a*/";
+ let spans = get_spans(query);
+ let diag = do_query(query);
+
+ assert_eq!(
+ diag.message,
+ "sum function requires 1 argument, got 2"
+ );
+
+ // Verify that the span actually targets "sum(1, 2)"
+ assert_eq!(diag.span, Some(spans["a"]));
+
+ Ok(())
+}
Review Comment:
You're not checking the `Diagnostic`'s notes, which would be a `Possible
function '...'`. Hence inconsistent with the test case.
##########
datafusion/sql/src/expr/function.rs:
##########
@@ -409,20 +410,28 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
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)
- })
- } else {
- internal_err!("No functions registered with this context.")
- }
+ .map_err(|_e| {
+ let msg = format!("{} function requires 1 argument, got 2",
name);
+
+ let mut span = Span::try_from_sqlparser_span(sql_parser_span);
+ if let Some(s) = &mut span {
+ // Temporary patch: manually extend the span end to column
22
+ s.end.column = 22;
Review Comment:
You're hardcoding, in the parser, the column number that works only with
that one query that you added as a test case...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
