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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
