[GitHub] [arrow-datafusion] 2010YOUY01 commented on a diff in pull request #6462: Improve `BuiltInFunction` Name Lookup

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

2010YOUY01 commented on code in PR #6462:
URL: https://github.com/apache/arrow-datafusion/pull/6462#discussion_r1207188425


##########
datafusion/expr/src/built_in_function.rs:
##########
@@ -431,29 +452,21 @@ impl BuiltinScalarFunction {
 
 impl fmt::Display for BuiltinScalarFunction {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        for (func_name, func) in NAME_TO_FUNCTION.iter() {
-            if func == self {
-                return write!(f, "{}", func_name);
-            }
+        if let Some(func_name) = FUNCTION_TO_NAME.get(self) {
+            write!(f, "{}", func_name)
+        } else {
+            // Should not be reached
+            panic!("Internal error: {self:?} not in ALL_FUNCTIONS list");

Review Comment:
   If I'm getting this correct, the only problem for now is: when adding a new function, if it's missed in `ALL_FUNCTIONS`, the compiler won't notice, so it's better to have matching arms for all `BuiltinScalarFunction` variants for `Display`.
   I think this implementation works (though a bit lengthy)
   ```rust
   impl fmt::Display for BuiltinScalarFunction {
       fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
           match self { // Ensure no variant is missing when adding new function
               BuiltinScalarFunction::Abs
               | BuiltinScalarFunction::Acos
               // ... add all other variants here ...
               | BuiltinScalarFunction::MakeArray => {
                   if let Some(func_name) = FUNCTION_TO_NAME.get(self) {
                       write!(f, "{}", func_name)
                   } else {
                       // This branch should be unreachable if the ALL_FUNCTIONS is complete
                       panic!("Internal error: {self:?} not in ALL_FUNCTIONS list", self);
                   }
               },
           }
       }
   }
   ```



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