Repository: apache/datafusion (language: rust)

Issue:
Attach `Diagnostic` to "function x does not exist" error
### Is your feature request related to a problem or challenge?

For a query like:

```sql
SELECT idontexist(1)'
```

The only message that the end user of an application built atop of DataFusion sees is:

```
Error during planning: Invalid function 'idontexist'.
Did you mean 'datepart'?
```

We want to provide a richer message that _references and highlights locations in the original SQL query_, and contextualises and helps the user understand the error. In the end, it would be possible to display errors in a fashion akin to what was enabled by #13664 for some errors:

![](https://github.com/user-attachments/assets/32efeb4f-aad9-41c9-a1e3-600a00d525c3)

See #14429 for more information.

### Describe the solution you'd like

Attach a well crafted `Diagnostic` to the `DataFusionError`, building on top of the foundations laid in #13664. See #14429 for more information.

### Describe alternatives you've considered

_No response_

### Additional context

_No response_


Discussion / hints:
take
I think this is a good first issue as the need is clear and the tests in https://github.com/apache/datafusion/blob/85fbde2661bdb462fc498dc18f055c44f229604c/datafusion/sql/tests/cases/diagnostic.rs are well structured for extension.

Thanks @onlyjackfrost  



Hey @onlyjackfrost how is it going with this ticket :) Can I help with anything?
Hi @eliaperantoni  sorry for the late reply.
I'm working on this and aim to raise a PR over the weekend. 
I'll let you know if I have any questions. Thanks for being so friendly!  :)
Hi @eliaperantoni, just updating my progress.
I've implemented the diagnostic like this. I'm not sure my implementation is in the right direction. 
@eliaperantoni  thanks for implementing the `try_from_sqlparser_span` in the first place, I was struggling with how to get the span from the function.
```
.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)
})
```

I'm trying to add test case in the `/sql/cases/diagnostic.rs` file... will raise PR later.
@onlyjackfrost yes that's perfect!

> I'm trying to add test case in the /sql/cases/diagnostic.rs file... will raise PR later.

Sweet! Looking forward to it. Great job :)

Fix patch:
diff --git a/datafusion/sql/src/expr/function.rs b/datafusion/sql/src/expr/function.rs
index 1cf3dcb289a6..e9bf083f3279 100644
--- a/datafusion/sql/src/expr/function.rs
+++ b/datafusion/sql/src/expr/function.rs
@@ -20,7 +20,7 @@ use crate::planner::{ContextProvider, PlannerContext, SqlToRel};
 use arrow::datatypes::DataType;
 use datafusion_common::{
     internal_datafusion_err, internal_err, not_impl_err, plan_datafusion_err, plan_err,
-    DFSchema, Dependency, Result,
+    DFSchema, Dependency, Diagnostic, Result, Span,
 };
 use datafusion_expr::expr::{ScalarFunction, Unnest};
 use datafusion_expr::planner::PlannerResult;
@@ -217,7 +217,7 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
         // it shouldn't have ordering requirement as function argument
         // required ordering should be defined in OVER clause.
         let is_function_window = over.is_some();
-
+        let sql_parser_span = name.0[0].span;
         let name = if name.0.len() > 1 {
             // DF doesn't handle compound identifiers
             // (e.g. "foo.bar") for function names yet
@@ -236,7 +236,6 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
                 }
             }
         }
-
         // User-defined function (UDF) should have precedence
         if let Some(fm) = self.context_provider.get_function_meta(&name) {
             let args = self.function_args_to_expr(args, schema, planner_context)?;
@@ -259,7 +258,6 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
                 "Aggregate ORDER BY is not implemented for window functions"
             );
         }
-
         // Then, window function
         if let Some(WindowType::WindowSpec(window)) = over {
             let partition_by = window
@@ -351,12 +349,21 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
                 )));
             }
         }
-
         // Could not find the relevant function, so return an error
         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)
+                })
         } else {
             internal_err!("No functions registered with this context.")
         }


Test patch:
diff --git a/datafusion/sql/tests/cases/diagnostic.rs b/datafusion/sql/tests/cases/diagnostic.rs
index 9dae2d0c3e93..d70484f718c8 100644
--- a/datafusion/sql/tests/cases/diagnostic.rs
+++ b/datafusion/sql/tests/cases/diagnostic.rs
@@ -15,7 +15,8 @@
 // specific language governing permissions and limitations
 // under the License.
 
-use std::collections::HashMap;
+use datafusion_functions::string;
+use std::{collections::HashMap, sync::Arc};
 
 use datafusion_common::{Diagnostic, Location, Result, Span};
 use datafusion_sql::planner::{ParserOptions, SqlToRel};
@@ -35,8 +36,8 @@ fn do_query(sql: &'static str) -> Diagnostic {
         collect_spans: true,
         ..ParserOptions::default()
     };
-
-    let state = MockSessionState::default();
+    let state = MockSessionState::default()
+        .with_scalar_function(Arc::new(string::concat().as_ref().clone()));
     let context = MockContextProvider { state };
     let sql_to_rel = SqlToRel::new_with_options(&context, options);
     match sql_to_rel.sql_statement_to_plan(statement) {
@@ -274,3 +275,14 @@ fn test_ambiguous_column_suggestion() -> Result<()> {
 
     Ok(())
 }
+
+#[test]
+fn test_invalid_function() -> Result<()> {
+    let query = "SELECT /*whole*/concat_not_exist/*whole*/()";
+    let spans = get_spans(query);
+    let diag = do_query(query);
+    assert_eq!(diag.message, "Invalid function 'concat_not_exist'");
+    assert_eq!(diag.notes[0].message, "Possible function 'concat'");
+    assert_eq!(diag.span, Some(spans["whole"]));
+    Ok(())
+}