From a41fb8fe2b626d167d9a3b4c3a560f77b23f0c27 Mon Sep 17 00:00:00 2001 From: onlyjackfrost Date: Mon, 24 Feb 2025 18:22:31 +0800 Subject: [PATCH 1/4] Add detailed error message for invalid function error --- datafusion/sql/src/expr/function.rs | 17 ++++++++++++----- datafusion/sql/tests/cases/diagnostic.rs | 18 +++++++++++++++--- 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/datafusion/sql/src/expr/function.rs b/datafusion/sql/src/expr/function.rs index 1cf3dcb289a6..c994be1bb3fe 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 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.clone(); 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 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 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 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.") } diff --git a/datafusion/sql/tests/cases/diagnostic.rs b/datafusion/sql/tests/cases/diagnostic.rs index 9dae2d0c3e93..fe99f6f30a2f 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(()) +} From d458b995c704cc2006d44ded619a0f1ac6f7465b Mon Sep 17 00:00:00 2001 From: onlyjackfrost Date: Mon, 24 Feb 2025 23:32:14 +0800 Subject: [PATCH 2/4] remove clone --- datafusion/sql/src/expr/function.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datafusion/sql/src/expr/function.rs b/datafusion/sql/src/expr/function.rs index c994be1bb3fe..d25c643dc0a1 100644 --- a/datafusion/sql/src/expr/function.rs +++ b/datafusion/sql/src/expr/function.rs @@ -217,7 +217,7 @@ impl 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.clone(); + 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 From 8441bb8faba136f31dec38150d96bebf0c97a354 Mon Sep 17 00:00:00 2001 From: onlyjackfrost Date: Wed, 26 Feb 2025 00:20:40 +0800 Subject: [PATCH 3/4] better note --- datafusion/sql/src/expr/function.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datafusion/sql/src/expr/function.rs b/datafusion/sql/src/expr/function.rs index d25c643dc0a1..e9bf083f3279 100644 --- a/datafusion/sql/src/expr/function.rs +++ b/datafusion/sql/src/expr/function.rs @@ -359,7 +359,7 @@ impl SqlToRel<'_, S> { let mut diagnostic = Diagnostic::new_error(format!("Invalid function '{name}'"), span); diagnostic.add_note( - format!("possible function {}", suggested_func_name), + format!("Possible function '{}'", suggested_func_name), None, ); e.with_diagnostic(diagnostic) From 12e3220755ef5a12f9655388e65bceedf1f36117 Mon Sep 17 00:00:00 2001 From: onlyjackfrost Date: Wed, 26 Feb 2025 09:36:59 +0800 Subject: [PATCH 4/4] fix test case --- datafusion/sql/tests/cases/diagnostic.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datafusion/sql/tests/cases/diagnostic.rs b/datafusion/sql/tests/cases/diagnostic.rs index fe99f6f30a2f..d70484f718c8 100644 --- a/datafusion/sql/tests/cases/diagnostic.rs +++ b/datafusion/sql/tests/cases/diagnostic.rs @@ -282,7 +282,7 @@ fn test_invalid_function() -> Result<()> { 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.notes[0].message, "Possible function 'concat'"); assert_eq!(diag.span, Some(spans["whole"])); Ok(()) }