Remove duplicate function name in its aliases list (#10661)

* remove duplicate name in function name aliases for array-function * add test for registering default functions * rename test case * add tests for agg and core function and refactor the test * remove unused mut * add comments for public function * cargo fmt * fix clippy * fix missing list_element * fix clippy * remove duplicate aliase name for median * add test for function list and remove the test for SessionState * remove the debug message * revert the change for medain and remove case insensitive test --------- Co-authored-by: Andrew Lamb <[email protected]>
apache · May 28, 2024 · 98cd19e · 98cd19e
1 parent 3dc1773
commit 98cd19e
Show file tree

Hide file tree

Showing 24 changed files with 143 additions and 80 deletions.
diff --git a/datafusion/functions-aggregate/src/first_last.rs b/datafusion/functions-aggregate/src/first_last.rs
@@ -72,7 +72,7 @@ impl Default for FirstValue {
 impl FirstValue {
     pub fn new() -> Self {
         Self {
-            aliases: vec![String::from("FIRST_VALUE"), String::from("first_value")],
+            aliases: vec![String::from("first_value")],
             signature: Signature::one_of(
                 vec![
                     // TODO: we can introduce more strict signature that only numeric of array types are allowed
@@ -372,7 +372,7 @@ impl Default for LastValue {
 impl LastValue {
     pub fn new() -> Self {
         Self {
-            aliases: vec![String::from("LAST_VALUE"), String::from("last_value")],
+            aliases: vec![String::from("last_value")],
             signature: Signature::one_of(
                 vec![
                     // TODO: we can introduce more strict signature that only numeric of array types are allowed

diff --git a/datafusion/functions-aggregate/src/lib.rs b/datafusion/functions-aggregate/src/lib.rs
@@ -72,15 +72,20 @@ pub mod expr_fn {
     pub use super::median::median;
 }
 
-/// Registers all enabled packages with a [`FunctionRegistry`]
-pub fn register_all(registry: &mut dyn FunctionRegistry) -> Result<()> {
-    let functions: Vec<Arc<AggregateUDF>> = vec![
+/// Returns all default aggregate functions
+pub fn all_default_aggregate_functions() -> Vec<Arc<AggregateUDF>> {
+    vec![
         first_last::first_value_udaf(),
         first_last::last_value_udaf(),
         covariance::covar_samp_udaf(),
         covariance::covar_pop_udaf(),
         median::median_udaf(),
-    ];
+    ]
+}
+
+/// Registers all enabled packages with a [`FunctionRegistry`]
+pub fn register_all(registry: &mut dyn FunctionRegistry) -> Result<()> {
+    let functions: Vec<Arc<AggregateUDF>> = all_default_aggregate_functions();
 
     functions.into_iter().try_for_each(|udf| {
         let existing_udaf = registry.register_udaf(udf)?;
@@ -92,3 +97,30 @@ pub fn register_all(registry: &mut dyn FunctionRegistry) -> Result<()> {
 
     Ok(())
 }
+
+#[cfg(test)]
+mod tests {
+    use crate::all_default_aggregate_functions;
+    use datafusion_common::Result;
+    use std::collections::HashSet;
+
+    #[test]
+    fn test_no_duplicate_name() -> Result<()> {
+        let mut names = HashSet::new();
+        for func in all_default_aggregate_functions() {
+            assert!(
+                names.insert(func.name().to_string()),
+                "duplicate function name: {}",
+                func.name()
+            );
+            for alias in func.aliases() {
+                assert!(
+                    names.insert(alias.to_string()),
+                    "duplicate function name: {}",
+                    alias
+                );
+            }
+        }
+        Ok(())
+    }
+}
diff --git a/datafusion/functions-array/src/array_has.rs b/datafusion/functions-array/src/array_has.rs
@@ -68,7 +68,6 @@ impl ArrayHas {
         Self {
             signature: Signature::array_and_element(Volatility::Immutable),
             aliases: vec![
-                String::from("array_has"),
                 String::from("list_has"),
                 String::from("array_contains"),
                 String::from("list_contains"),
@@ -140,7 +139,7 @@ impl ArrayHasAll {
     pub fn new() -> Self {
         Self {
             signature: Signature::any(2, Volatility::Immutable),
-            aliases: vec![String::from("array_has_all"), String::from("list_has_all")],
+            aliases: vec![String::from("list_has_all")],
         }
     }
 }
@@ -203,7 +202,7 @@ impl ArrayHasAny {
     pub fn new() -> Self {
         Self {
             signature: Signature::any(2, Volatility::Immutable),
-            aliases: vec![String::from("array_has_any"), String::from("list_has_any")],
+            aliases: vec![String::from("list_has_any")],
         }
     }
 }

diff --git a/datafusion/functions-array/src/cardinality.rs b/datafusion/functions-array/src/cardinality.rs
@@ -40,7 +40,7 @@ impl Cardinality {
     pub fn new() -> Self {
         Self {
             signature: Signature::array(Volatility::Immutable),
-            aliases: vec![String::from("cardinality")],
+            aliases: vec![],
         }
     }
 }

diff --git a/datafusion/functions-array/src/concat.rs b/datafusion/functions-array/src/concat.rs
@@ -59,7 +59,6 @@ impl ArrayAppend {
         Self {
             signature: Signature::array_and_element(Volatility::Immutable),
             aliases: vec![
-                String::from("array_append"),
                 String::from("list_append"),
                 String::from("array_push_back"),
                 String::from("list_push_back"),
@@ -119,7 +118,6 @@ impl ArrayPrepend {
         Self {
             signature: Signature::element_and_array(Volatility::Immutable),
             aliases: vec![
-                String::from("array_prepend"),
                 String::from("list_prepend"),
                 String::from("array_push_front"),
                 String::from("list_push_front"),
@@ -178,7 +176,6 @@ impl ArrayConcat {
         Self {
             signature: Signature::variadic_any(Volatility::Immutable),
             aliases: vec![
-                String::from("array_concat"),
                 String::from("array_cat"),
                 String::from("list_concat"),
                 String::from("list_cat"),

diff --git a/datafusion/functions-array/src/dimension.rs b/datafusion/functions-array/src/dimension.rs
@@ -50,7 +50,7 @@ impl ArrayDims {
     pub fn new() -> Self {
         Self {
             signature: Signature::array(Volatility::Immutable),
-            aliases: vec!["array_dims".to_string(), "list_dims".to_string()],
+            aliases: vec!["list_dims".to_string()],
         }
     }
 }
@@ -104,7 +104,7 @@ impl ArrayNdims {
     pub fn new() -> Self {
         Self {
             signature: Signature::array(Volatility::Immutable),
-            aliases: vec![String::from("array_ndims"), String::from("list_ndims")],
+            aliases: vec![String::from("list_ndims")],
         }
     }
 }

diff --git a/datafusion/functions-array/src/empty.rs b/datafusion/functions-array/src/empty.rs
@@ -44,11 +44,7 @@ impl ArrayEmpty {
     pub fn new() -> Self {
         Self {
             signature: Signature::array(Volatility::Immutable),
-            aliases: vec![
-                "empty".to_string(),
-                "array_empty".to_string(),
-                "list_empty".to_string(),
-            ],
+            aliases: vec!["array_empty".to_string(), "list_empty".to_string()],
         }
     }
 }

diff --git a/datafusion/functions-array/src/except.rs b/datafusion/functions-array/src/except.rs
@@ -47,7 +47,7 @@ impl ArrayExcept {
     pub fn new() -> Self {
         Self {
             signature: Signature::any(2, Volatility::Immutable),
-            aliases: vec!["array_except".to_string(), "list_except".to_string()],
+            aliases: vec!["list_except".to_string()],
         }
     }
 }

diff --git a/datafusion/functions-array/src/extract.rs b/datafusion/functions-array/src/extract.rs
@@ -80,7 +80,6 @@ impl ArrayElement {
         Self {
             signature: Signature::array_and_index(Volatility::Immutable),
             aliases: vec![
-                String::from("array_element"),
                 String::from("array_extract"),
                 String::from("list_element"),
                 String::from("list_extract"),
@@ -241,7 +240,7 @@ impl ArraySlice {
     pub fn new() -> Self {
         Self {
             signature: Signature::variadic_any(Volatility::Immutable),
-            aliases: vec![String::from("array_slice"), String::from("list_slice")],
+            aliases: vec![String::from("list_slice")],
         }
     }
 }
@@ -513,10 +512,7 @@ impl ArrayPopFront {
     pub fn new() -> Self {
         Self {
             signature: Signature::array(Volatility::Immutable),
-            aliases: vec![
-                String::from("array_pop_front"),
-                String::from("list_pop_front"),
-            ],
+            aliases: vec![String::from("list_pop_front")],
         }
     }
 }
@@ -591,10 +587,7 @@ impl ArrayPopBack {
     pub fn new() -> Self {
         Self {
             signature: Signature::array(Volatility::Immutable),
-            aliases: vec![
-                String::from("array_pop_back"),
-                String::from("list_pop_back"),
-            ],
+            aliases: vec![String::from("list_pop_back")],
         }
     }
 }

diff --git a/datafusion/functions-array/src/flatten.rs b/datafusion/functions-array/src/flatten.rs
@@ -47,7 +47,7 @@ impl Flatten {
     pub fn new() -> Self {
         Self {
             signature: Signature::array(Volatility::Immutable),
-            aliases: vec![String::from("flatten")],
+            aliases: vec![],
         }
     }
 }

diff --git a/datafusion/functions-array/src/length.rs b/datafusion/functions-array/src/length.rs
@@ -48,7 +48,7 @@ impl ArrayLength {
     pub fn new() -> Self {
         Self {
             signature: Signature::variadic_any(Volatility::Immutable),
-            aliases: vec![String::from("array_length"), String::from("list_length")],
+            aliases: vec![String::from("list_length")],
         }
     }
 }

diff --git a/datafusion/functions-array/src/lib.rs b/datafusion/functions-array/src/lib.rs
@@ -99,9 +99,9 @@ pub mod expr_fn {
     pub use super::string::string_to_array;
 }
 
-/// Registers all enabled packages with a [`FunctionRegistry`]
-pub fn register_all(registry: &mut dyn FunctionRegistry) -> Result<()> {
-    let functions: Vec<Arc<ScalarUDF>> = vec![
+/// Return all default array functions
+pub fn all_default_array_functions() -> Vec<Arc<ScalarUDF>> {
+    vec![
         string::array_to_string_udf(),
         string::string_to_array_udf(),
         range::range_udf(),
@@ -139,7 +139,12 @@ pub fn register_all(registry: &mut dyn FunctionRegistry) -> Result<()> {
         replace::array_replace_n_udf(),
         replace::array_replace_all_udf(),
         replace::array_replace_udf(),
-    ];
+    ]
+}
+
+/// Registers all enabled packages with a [`FunctionRegistry`]
+pub fn register_all(registry: &mut dyn FunctionRegistry) -> Result<()> {
+    let functions: Vec<Arc<ScalarUDF>> = all_default_array_functions();
     functions.into_iter().try_for_each(|udf| {
         let existing_udf = registry.register_udf(udf)?;
         if let Some(existing_udf) = existing_udf {
@@ -151,3 +156,30 @@ pub fn register_all(registry: &mut dyn FunctionRegistry) -> Result<()> {
 
     Ok(())
 }
+
+#[cfg(test)]
+mod tests {
+    use crate::all_default_array_functions;
+    use datafusion_common::Result;
+    use std::collections::HashSet;
+
+    #[test]
+    fn test_no_duplicate_name() -> Result<()> {
+        let mut names = HashSet::new();
+        for func in all_default_array_functions() {
+            assert!(
+                names.insert(func.name().to_string()),
+                "duplicate function name: {}",
+                func.name()
+            );
+            for alias in func.aliases() {
+                assert!(
+                    names.insert(alias.to_string()),
+                    "duplicate function name: {}",
+                    alias
+                );
+            }
+        }
+        Ok(())
+    }
+}
diff --git a/datafusion/functions-array/src/position.rs b/datafusion/functions-array/src/position.rs
@@ -55,7 +55,6 @@ impl ArrayPosition {
                 Volatility::Immutable,
             ),
             aliases: vec![
-                String::from("array_position"),
                 String::from("list_position"),
                 String::from("array_indexof"),
                 String::from("list_indexof"),
@@ -183,10 +182,7 @@ impl ArrayPositions {
     pub fn new() -> Self {
         Self {
             signature: Signature::array_and_element(Volatility::Immutable),
-            aliases: vec![
-                String::from("array_positions"),
-                String::from("list_positions"),
-            ],
+            aliases: vec![String::from("list_positions")],
         }
     }
 }

diff --git a/datafusion/functions-array/src/range.rs b/datafusion/functions-array/src/range.rs
@@ -58,7 +58,7 @@ impl Range {
                 ],
                 Volatility::Immutable,
             ),
-            aliases: vec![String::from("range")],
+            aliases: vec![],
         }
     }
 }
@@ -129,7 +129,7 @@ impl GenSeries {
                 ],
                 Volatility::Immutable,
             ),
-            aliases: vec![String::from("generate_series")],
+            aliases: vec![],
         }
     }
 }

diff --git a/datafusion/functions-array/src/remove.rs b/datafusion/functions-array/src/remove.rs
@@ -49,7 +49,7 @@ impl ArrayRemove {
     pub fn new() -> Self {
         Self {
             signature: Signature::array_and_element(Volatility::Immutable),
-            aliases: vec!["array_remove".to_string(), "list_remove".to_string()],
+            aliases: vec!["list_remove".to_string()],
         }
     }
 }
@@ -98,7 +98,7 @@ impl ArrayRemoveN {
     pub fn new() -> Self {
         Self {
             signature: Signature::any(3, Volatility::Immutable),
-            aliases: vec!["array_remove_n".to_string(), "list_remove_n".to_string()],
+            aliases: vec!["list_remove_n".to_string()],
         }
     }
 }
@@ -147,10 +147,7 @@ impl ArrayRemoveAll {
     pub fn new() -> Self {
         Self {
             signature: Signature::array_and_element(Volatility::Immutable),
-            aliases: vec![
-                "array_remove_all".to_string(),
-                "list_remove_all".to_string(),
-            ],
+            aliases: vec!["list_remove_all".to_string()],
         }
     }
 }

diff --git a/datafusion/functions-array/src/repeat.rs b/datafusion/functions-array/src/repeat.rs
@@ -50,7 +50,7 @@ impl ArrayRepeat {
     pub fn new() -> Self {
         Self {
             signature: Signature::variadic_any(Volatility::Immutable),
-            aliases: vec![String::from("array_repeat"), String::from("list_repeat")],
+            aliases: vec![String::from("list_repeat")],
         }
     }
 }

diff --git a/datafusion/functions-array/src/replace.rs b/datafusion/functions-array/src/replace.rs
@@ -65,7 +65,7 @@ impl ArrayReplace {
     pub fn new() -> Self {
         Self {
             signature: Signature::any(3, Volatility::Immutable),
-            aliases: vec![String::from("array_replace"), String::from("list_replace")],
+            aliases: vec![String::from("list_replace")],
         }
     }
 }
@@ -106,10 +106,7 @@ impl ArrayReplaceN {
     pub fn new() -> Self {
         Self {
             signature: Signature::any(4, Volatility::Immutable),
-            aliases: vec![
-                String::from("array_replace_n"),
-                String::from("list_replace_n"),
-            ],
+            aliases: vec![String::from("list_replace_n")],
         }
     }
 }
@@ -150,10 +147,7 @@ impl ArrayReplaceAll {
     pub fn new() -> Self {
         Self {
             signature: Signature::any(3, Volatility::Immutable),
-            aliases: vec![
-                String::from("array_replace_all"),
-                String::from("list_replace_all"),
-            ],
+            aliases: vec![String::from("list_replace_all")],
         }
     }
 }