Skip to content

Commit

Permalink
Minor: use Option rather than Result for not found suggestion (#12512)
Browse files Browse the repository at this point in the history
  • Loading branch information
alamb committed Sep 17, 2024
1 parent 8555e41 commit aeca7ea
Showing 1 changed file with 20 additions and 18 deletions.
38 changes: 20 additions & 18 deletions datafusion/sql/src/expr/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ use crate::planner::{ContextProvider, PlannerContext, SqlToRel};

use arrow_schema::DataType;
use datafusion_common::{
internal_datafusion_err, not_impl_err, plan_datafusion_err, plan_err, DFSchema,
Dependency, Result,
internal_datafusion_err, internal_err, not_impl_err, plan_datafusion_err, plan_err,
DFSchema, Dependency, Result,
};
use datafusion_expr::expr::WildcardOptions;
use datafusion_expr::planner::PlannerResult;
Expand All @@ -39,11 +39,14 @@ use sqlparser::ast::{
use strum::IntoEnumIterator;

/// Suggest a valid function based on an invalid input function name
///
/// Returns `None` if no valid matches are found. This happens when there are no
/// functions registered with the context.
pub fn suggest_valid_function(
input_function_name: &str,
is_window_func: bool,
ctx: &dyn ContextProvider,
) -> Result<String> {
) -> Option<String> {
let valid_funcs = if is_window_func {
// All aggregate functions and builtin window functions
let mut funcs = Vec::new();
Expand All @@ -67,19 +70,14 @@ pub fn suggest_valid_function(

/// Find the closest matching string to the target string in the candidates list, using edit distance(case insensitive)
/// Input `candidates` must not be empty otherwise an error is returned.
fn find_closest_match(candidates: Vec<String>, target: &str) -> Result<String> {
fn find_closest_match(candidates: Vec<String>, target: &str) -> Option<String> {
let target = target.to_lowercase();
candidates
.into_iter()
.min_by_key(|candidate| {
datafusion_common::utils::datafusion_strsim::levenshtein(
&candidate.to_lowercase(),
&target,
)
})
.ok_or_else(|| {
internal_datafusion_err!("No functions registered with this context.")
})
candidates.into_iter().min_by_key(|candidate| {
datafusion_common::utils::datafusion_strsim::levenshtein(
&candidate.to_lowercase(),
&target,
)
})
}

/// Arguments to for a function call extracted from the SQL AST
Expand Down Expand Up @@ -355,9 +353,13 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
}

// Could not find the relevant function, so return an error
let suggested_func_name =
suggest_valid_function(&name, is_function_window, self.context_provider)?;
plan_err!("Invalid function '{name}'.\nDid you mean '{suggested_func_name}'?")
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}'?")
} else {
internal_err!("No functions registered with this context.")
}
}

pub(super) fn sql_fn_name_to_expr(
Expand Down

0 comments on commit aeca7ea

Please sign in to comment.