[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #6520: Add function name suggestion.

["alamb (via GitHub)" <gi...@apache.org>]

alamb commented on code in PR #6520:
URL: https://github.com/apache/arrow-datafusion/pull/6520#discussion_r1214679733


##########
datafusion/sql/src/expr/function.rs:
##########
@@ -84,73 +86,88 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
             } else {
                 WindowFrame::new(!order_by.is_empty())
             };
-            let fun = self.find_window_func(&name)?;
-            let expr = match fun {
-                WindowFunction::AggregateFunction(aggregate_fun) => {
-                    let (aggregate_fun, args) = self.aggregate_fn_to_expr(
-                        aggregate_fun,
-                        function.args,
-                        schema,
-                        planner_context,
-                    )?;
-
-                    Expr::WindowFunction(expr::WindowFunction::new(
-                        WindowFunction::AggregateFunction(aggregate_fun),
-                        args,
+            if let Ok(fun) = self.find_window_func(&name) {

Review Comment:
   The whitespace blind diff also shows this nicely: https://github.com/apache/arrow-datafusion/pull/6520/files?w=1



##########
datafusion/expr/src/window_function.rs:
##########
@@ -28,6 +28,7 @@ use arrow::datatypes::DataType;
 use datafusion_common::{DataFusionError, Result};
 use std::sync::Arc;
 use std::{fmt, str::FromStr};
+use strum_macros::EnumIter;

Review Comment:
   TIL https://docs.rs/strum_macros/0.24.3/strum_macros/derive.EnumIter.html -- very cool



##########
datafusion/expr/Cargo.toml:
##########
@@ -40,6 +40,7 @@ arrow = { workspace = true }
 datafusion-common = { path = "../common", version = "25.0.0" }
 lazy_static = { version = "^1.4.0" }
 sqlparser = "0.34"
+strsim = "0.10.0"

Review Comment:
   I reviewed this crate and it is widely used in the ecosystem (used by clap): https://crates.io/crates/strsim but hasn't been updated for 3 years
   
   However, it does appear to be a new dependency for datafusion,
   
   An alternate if we are worried about this new dependency is we could inline the definition of `levenshtein` into datafusion as it is not large
   
   https://github.com/dguo/strsim-rs/blob/65eac453cbd10ba4e13273002c843e95c81ae93f/src/lib.rs#L192-L238
   
   



-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org