Remove redundant upper case aliases for median, first_value and last_value
#10696
Conversation
| @@ -72,7 +72,7 @@ impl Default for FirstValue { | |||
| impl FirstValue { | |||
| pub fn new() -> Self { | |||
| Self { | |||
| aliases: vec![String::from("first_value")], | |||
| aliases: vec![], | |||
There was a problem hiding this comment.
A minor comment is we can probably remove the aliases field entirely and just return an empty slice
fn aliases(&self) -> &[String] {
&[]
}
| @@ -109,13 +109,13 @@ mod tests { | |||
| let mut names = HashSet::new(); | |||
| for func in all_default_aggregate_functions() { | |||
| assert!( | |||
| names.insert(func.name().to_string()), | |||
| names.insert(func.name().to_string().to_lowercase()), | |||
There was a problem hiding this comment.
Can we not convert to lowercase here? If someone changes the name to uppercase again, the function in the function map (lowercase) is not the same as the function name (uppercase), and it is easy to introduce a bug.
There was a problem hiding this comment.
Thanks @jayzhan211
I think to_lowercase is used to ensure that the function name and its alias won't be duplicates when case-insensitive (e.g., unquoted identifier). If someone changes the function name to uppercase, the test will pass. However, they may find that they can't query this function in lowercase. They might try to add a lowercase alias or change the function name back to lowercase. I think this is the purpose of this test.
There was a problem hiding this comment.
Oh, it is the test, then it is fine
|
Thanks again @alamb @jayzhan211 |
…last_value` (apache#10696) * change udaf name to lowercase * enhance test for duplicate function name * fix test * fix tests * remove redundant alias field
Which issue does this PR close?
Closes #10695 .
Rationale for this change
What changes are included in this PR?
Fix
median,first_valueandlast_valueAre these changes tested?
Yes
Are there any user-facing changes?
No