[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #6448: Improve Display for BuiltinScalarFunction

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

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


##########
datafusion/expr/src/built_in_function.rs:
##########
@@ -202,6 +203,120 @@ pub enum BuiltinScalarFunction {
     ArrowTypeof,
 }
 
+lazy_static! {
+    /// Mapping between SQL function names to `BuiltinScalarFunction` types.
+    /// Note that multiple SQL function names can represent the same `BuiltinScalarFunction`. These are treated as aliases.
+    /// In case of such aliases, the first SQL function name in the vector is used when displaying the function.
+    static ref NAME_TO_FUNCTION: Vec<(&'static str, BuiltinScalarFunction)> = vec![

Review Comment:
   This is a really nice solution and writeup -- thank you @2010YOUY01 
   
   I am slightly concerned about having to do a linear search for each function name, but I don't think it is a big deal and I will propose a follow on PR to improve the situation



##########
datafusion/expr/src/built_in_function.rs:
##########
@@ -316,124 +431,46 @@ impl BuiltinScalarFunction {
 
 impl fmt::Display for BuiltinScalarFunction {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        // lowercase of the debug.
+        for (func_name, func) in NAME_TO_FUNCTION.iter() {
+            if func == self {
+                return write!(f, "{}", func_name);
+            }
+        }
+
+        // Should not be reached

Review Comment:
   🤔  Maybe we should put an assert here? I'll do so as a follow on PR



-- 
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