-
Notifications
You must be signed in to change notification settings - Fork 752
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: ignore case when matching function name #16912
base: main
Are you sure you want to change the base?
fix: ignore case when matching function name #16912
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Does it make sense to add ignore case search in is_builtin_function(func_name) as well? |
@@ -721,9 +721,7 @@ impl<'a> TypeChecker<'a> { | |||
} => { | |||
let func_name = normalize_identifier(name, self.name_resolution_ctx).to_string(); | |||
let func_name = func_name.as_str(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about handling case here?
let func_name = func_name.as_str(); | |
let func_name = func_name.to_lowercase(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion.
Since sugar and built-in functions are matched ignoring case, that's why I created a sep function and moved the logic inside there, and we could do the same in is_builtin_function, otherwise each caller has to convert to lower case before calling the functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I see. Then I'll suggest replacing &str
in all_sugar_functions
and builtin_functions
to https://crates.io/crates/unicase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've used the Ascii type from the unicase crate, a case-insensitive wrapper for ASCII strings. Is this assumption correct? If not, I can switch to the UniCase type, which provides a case-insensitive wrapper for general strings.
Let's add a test in |
f64459f
to
5ad40f7
Compare
c08754d
to
d4bd725
Compare
Added a test |
I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/
Summary
Update logic to search by the lowercase of func_name. Uses unicode crate to facilitate searching function name ignoring case
fixes: #16737
Tests
Type of change
This change is