Skip to content

chore: Attach Diagnostic to "function x does not exist" error #14849

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Feb 26, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions datafusion/sql/src/expr/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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)?;
Expand All @@ -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
Expand Down Expand Up @@ -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.")
}
Expand Down
18 changes: 15 additions & 3 deletions datafusion/sql/tests/cases/diagnostic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -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) {
Expand Down Expand Up @@ -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(())
}