reduce search complexity for BuiltinScalarFunction#6479
Conversation
github-actions
bot
added
logical-expr
2010YOUY01
left a comment
2010YOUY01
left a comment
There was a problem hiding this comment.
This implementation makes it very clear what code to change when adding new functions. Suggested some minor changes, but overall looks good to me, thank you!
| static ref FUNCTION_TO_NAME: HashMap<BuiltinScalarFunction, &'static str> = { | ||
| let mut map: HashMap<BuiltinScalarFunction, &'static str> = HashMap::new(); | ||
| BuiltinScalarFunction::iter().for_each(|func| { | ||
| map.insert(func, aliases(&func).iter().next().unwrap_or(&"NO_ALIAS")); |
There was a problem hiding this comment.
.iter().next() == .first()
| @@ -429,31 +340,130 @@ impl BuiltinScalarFunction { | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
/// First alias in the array is used to display function names
| if func == self { | ||
| return write!(f, "{}", func_name); | ||
| } | ||
| fn aliases(func: &BuiltinScalarFunction) -> &'static [&'static str] { |
There was a problem hiding this comment.
Sorry I commented on the wrong line previously @comphead , should be here.
Adding a comment for aliases():
/// First alias in the array is used to display function names
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @comphead and @2010YOUY01
| BuiltinScalarFunction::CurrentDate => &["current_date"], | ||
| BuiltinScalarFunction::CurrentTime => &["current_time"], | ||
| BuiltinScalarFunction::DateBin => &["date_bin"], | ||
| BuiltinScalarFunction::DateTrunc => &["date_trunc", "datetrunc"], |
There was a problem hiding this comment.
I like how this formulation makes it easier to understand what functions have aliases and which do not 👍
|
I think this PR looks good to go and all outstanding comments have been addressed, so merging it in 🚀 |
3 participants
Which issue does this PR close?
Related #6448.
Closes #6462
Rationale for this change
Reduce search complexity when mapping BuiltinScalarFunction and its alias. Currently it is linear search and the place becoming a hotspot. The change is to make the search O(1) using static maps
What changes are included in this PR?
Are these changes tested?
Yes
Are there any user-facing changes?
No