Minor: Remove uncessary name field in ScalarFunctionDefintion (apache…

…#8365)
appletreeisyellow · Nov 30, 2023 · 513fd05 · 513fd05
1 parent d45cf00
commit 513fd05
Show file tree

Hide file tree

Showing 14 changed files with 36 additions and 60 deletions.
diff --git a/datafusion/core/src/datasource/listing/helpers.rs b/datafusion/core/src/datasource/listing/helpers.rs
@@ -92,7 +92,7 @@ pub fn expr_applicable_for_cols(col_names: &[String], expr: &Expr) -> bool {
 
             Expr::ScalarFunction(scalar_function) => {
                 match &scalar_function.func_def {
-                    ScalarFunctionDefinition::BuiltIn { fun, .. } => {
+                    ScalarFunctionDefinition::BuiltIn(fun) => {
                         match fun.volatility() {
                             Volatility::Immutable => Ok(VisitRecursion::Continue),
                             // TODO: Stable functions could be `applicable`, but that would require access to the context

diff --git a/datafusion/expr/src/built_in_function.rs b/datafusion/expr/src/built_in_function.rs
@@ -348,6 +348,12 @@ impl BuiltinScalarFunction {
         self.signature().type_signature.supports_zero_argument()
     }
 
+    /// Returns the name of this function
+    pub fn name(&self) -> &str {
+        // .unwrap is safe here because compiler makes sure the map will have matches for each BuiltinScalarFunction
+        function_to_name().get(self).unwrap()
+    }
+
     /// Returns the [Volatility] of the builtin function.
     pub fn volatility(&self) -> Volatility {
         match self {
@@ -1627,8 +1633,7 @@ impl BuiltinScalarFunction {
 
 impl fmt::Display for BuiltinScalarFunction {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        // .unwrap is safe here because compiler makes sure the map will have matches for each BuiltinScalarFunction
-        write!(f, "{}", function_to_name().get(self).unwrap())
+        write!(f, "{}", self.name())
     }
 }
 

diff --git a/datafusion/expr/src/expr.rs b/datafusion/expr/src/expr.rs
@@ -17,7 +17,6 @@
 
 //! Expr module contains core type definition for `Expr`.
 
-use crate::built_in_function;
 use crate::expr_fn::binary_expr;
 use crate::logical_plan::Subquery;
 use crate::udaf;
@@ -26,6 +25,7 @@ use crate::window_frame;
 use crate::window_function;
 use crate::Operator;
 use crate::{aggregate_function, ExprSchemable};
+use crate::{built_in_function, BuiltinScalarFunction};
 use arrow::datatypes::DataType;
 use datafusion_common::tree_node::{Transformed, TreeNode};
 use datafusion_common::{internal_err, DFSchema, OwnedTableReference};
@@ -340,10 +340,7 @@ pub enum ScalarFunctionDefinition {
     /// Resolved to a `BuiltinScalarFunction`
     /// There is plan to migrate `BuiltinScalarFunction` to UDF-based implementation (issue#8045)
     /// This variant is planned to be removed in long term
-    BuiltIn {
-        fun: built_in_function::BuiltinScalarFunction,
-        name: Arc<str>,
-    },
+    BuiltIn(BuiltinScalarFunction),
     /// Resolved to a user defined function
     UDF(Arc<crate::ScalarUDF>),
     /// A scalar function constructed with name. This variant can not be executed directly
@@ -371,7 +368,7 @@ impl ScalarFunctionDefinition {
     /// Function's name for display
     pub fn name(&self) -> &str {
         match self {
-            ScalarFunctionDefinition::BuiltIn { name, .. } => name.as_ref(),
+            ScalarFunctionDefinition::BuiltIn(fun) => fun.name(),
             ScalarFunctionDefinition::UDF(udf) => udf.name(),
             ScalarFunctionDefinition::Name(func_name) => func_name.as_ref(),
         }
@@ -382,10 +379,7 @@ impl ScalarFunction {
     /// Create a new ScalarFunction expression
     pub fn new(fun: built_in_function::BuiltinScalarFunction, args: Vec<Expr>) -> Self {
         Self {
-            func_def: ScalarFunctionDefinition::BuiltIn {
-                fun,
-                name: Arc::from(fun.to_string()),
-            },
+            func_def: ScalarFunctionDefinition::BuiltIn(fun),
             args,
         }
     }

diff --git a/datafusion/expr/src/expr_fn.rs b/datafusion/expr/src/expr_fn.rs
@@ -1032,7 +1032,7 @@ mod test {
     macro_rules! test_unary_scalar_expr {
         ($ENUM:ident, $FUNC:ident) => {{
             if let Expr::ScalarFunction(ScalarFunction {
-                func_def: ScalarFunctionDefinition::BuiltIn { fun, .. },
+                func_def: ScalarFunctionDefinition::BuiltIn(fun),
                 args,
             }) = $FUNC(col("tableA.a"))
             {
@@ -1053,7 +1053,7 @@ mod test {
                 col(stringify!($arg.to_string()))
             ),*
         );
-        if let Expr::ScalarFunction(ScalarFunction { func_def: ScalarFunctionDefinition::BuiltIn{fun, ..}, args }) = result {
+        if let Expr::ScalarFunction(ScalarFunction { func_def: ScalarFunctionDefinition::BuiltIn(fun), args }) = result {
             let name = built_in_function::BuiltinScalarFunction::$ENUM;
             assert_eq!(name, fun);
             assert_eq!(expected.len(), args.len());
@@ -1073,7 +1073,7 @@ mod test {
                 ),*
             ]
         );
-        if let Expr::ScalarFunction(ScalarFunction { func_def: ScalarFunctionDefinition::BuiltIn{fun, ..}, args }) = result {
+        if let Expr::ScalarFunction(ScalarFunction { func_def: ScalarFunctionDefinition::BuiltIn(fun), args }) = result {
             let name = built_in_function::BuiltinScalarFunction::$ENUM;
             assert_eq!(name, fun);
             assert_eq!(expected.len(), args.len());
@@ -1214,7 +1214,7 @@ mod test {
     #[test]
     fn uuid_function_definitions() {
         if let Expr::ScalarFunction(ScalarFunction {
-            func_def: ScalarFunctionDefinition::BuiltIn { fun, .. },
+            func_def: ScalarFunctionDefinition::BuiltIn(fun),
             args,
         }) = uuid()
         {
@@ -1229,7 +1229,7 @@ mod test {
     #[test]
     fn digest_function_definitions() {
         if let Expr::ScalarFunction(ScalarFunction {
-            func_def: ScalarFunctionDefinition::BuiltIn { fun, .. },
+            func_def: ScalarFunctionDefinition::BuiltIn(fun),
             args,
         }) = digest(col("tableA.a"), lit("md5"))
         {
@@ -1244,7 +1244,7 @@ mod test {
     #[test]
     fn encode_function_definitions() {
         if let Expr::ScalarFunction(ScalarFunction {
-            func_def: ScalarFunctionDefinition::BuiltIn { fun, .. },
+            func_def: ScalarFunctionDefinition::BuiltIn(fun),
             args,
         }) = encode(col("tableA.a"), lit("base64"))
         {
@@ -1259,7 +1259,7 @@ mod test {
     #[test]
     fn decode_function_definitions() {
         if let Expr::ScalarFunction(ScalarFunction {
-            func_def: ScalarFunctionDefinition::BuiltIn { fun, .. },
+            func_def: ScalarFunctionDefinition::BuiltIn(fun),
             args,
         }) = decode(col("tableA.a"), lit("hex"))
         {

diff --git a/datafusion/expr/src/expr_schema.rs b/datafusion/expr/src/expr_schema.rs
@@ -84,7 +84,7 @@ impl ExprSchemable for Expr {
             | Expr::TryCast(TryCast { data_type, .. }) => Ok(data_type.clone()),
             Expr::ScalarFunction(ScalarFunction { func_def, args }) => {
                 match func_def {
-                    ScalarFunctionDefinition::BuiltIn { fun, .. } => {
+                    ScalarFunctionDefinition::BuiltIn(fun) => {
                         let arg_data_types = args
                             .iter()
                             .map(|e| e.get_type(schema))

diff --git a/datafusion/expr/src/tree_node/expr.rs b/datafusion/expr/src/tree_node/expr.rs
@@ -277,7 +277,7 @@ impl TreeNode for Expr {
                 nulls_first,
             )),
             Expr::ScalarFunction(ScalarFunction { func_def, args }) => match func_def {
-                ScalarFunctionDefinition::BuiltIn { fun, .. } => Expr::ScalarFunction(
+                ScalarFunctionDefinition::BuiltIn(fun) => Expr::ScalarFunction(
                     ScalarFunction::new(fun, transform_vec(args, &mut transform)?),
                 ),
                 ScalarFunctionDefinition::UDF(fun) => Expr::ScalarFunction(

diff --git a/datafusion/optimizer/src/analyzer/type_coercion.rs b/datafusion/optimizer/src/analyzer/type_coercion.rs
@@ -320,7 +320,7 @@ impl TreeNodeRewriter for TypeCoercionRewriter {
                 Ok(Expr::Case(case))
             }
             Expr::ScalarFunction(ScalarFunction { func_def, args }) => match func_def {
-                ScalarFunctionDefinition::BuiltIn { fun, .. } => {
+                ScalarFunctionDefinition::BuiltIn(fun) => {
                     let new_args = coerce_arguments_for_signature(
                         args.as_slice(),
                         &self.schema,

diff --git a/datafusion/optimizer/src/optimize_projections.rs b/datafusion/optimizer/src/optimize_projections.rs
@@ -510,9 +510,7 @@ fn rewrite_expr(expr: &Expr, input: &Projection) -> Result<Option<Expr>> {
             )))
         }
         Expr::ScalarFunction(scalar_fn) => {
-            let fun = if let ScalarFunctionDefinition::BuiltIn { fun, .. } =
-                scalar_fn.func_def
-            {
+            let fun = if let ScalarFunctionDefinition::BuiltIn(fun) = scalar_fn.func_def {
                 fun
             } else {
                 return Ok(None);

diff --git a/datafusion/optimizer/src/push_down_filter.rs b/datafusion/optimizer/src/push_down_filter.rs
@@ -979,7 +979,7 @@ fn is_volatile_expression(e: &Expr) -> bool {
     e.apply(&mut |expr| {
         Ok(match expr {
             Expr::ScalarFunction(f) => match &f.func_def {
-                ScalarFunctionDefinition::BuiltIn { fun, .. }
+                ScalarFunctionDefinition::BuiltIn(fun)
                     if fun.volatility() == Volatility::Volatile =>
                 {
                     is_volatile = true;

diff --git a/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs b/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs
@@ -344,7 +344,7 @@ impl<'a> ConstEvaluator<'a> {
             | Expr::Wildcard { .. }
             | Expr::Placeholder(_) => false,
             Expr::ScalarFunction(ScalarFunction { func_def, .. }) => match func_def {
-                ScalarFunctionDefinition::BuiltIn { fun, .. } => {
+                ScalarFunctionDefinition::BuiltIn(fun) => {
                     Self::volatility_ok(fun.volatility())
                 }
                 ScalarFunctionDefinition::UDF(fun) => {
@@ -1202,41 +1202,28 @@ impl<'a, S: SimplifyInfo> TreeNodeRewriter for Simplifier<'a, S> {
 
             // log
             Expr::ScalarFunction(ScalarFunction {
-                func_def:
-                    ScalarFunctionDefinition::BuiltIn {
-                        fun: BuiltinScalarFunction::Log,
-                        ..
-                    },
+                func_def: ScalarFunctionDefinition::BuiltIn(BuiltinScalarFunction::Log),
                 args,
             }) => simpl_log(args, <&S>::clone(&info))?,
 
             // power
             Expr::ScalarFunction(ScalarFunction {
-                func_def:
-                    ScalarFunctionDefinition::BuiltIn {
-                        fun: BuiltinScalarFunction::Power,
-                        ..
-                    },
+                func_def: ScalarFunctionDefinition::BuiltIn(BuiltinScalarFunction::Power),
                 args,
             }) => simpl_power(args, <&S>::clone(&info))?,
 
             // concat
             Expr::ScalarFunction(ScalarFunction {
-                func_def:
-                    ScalarFunctionDefinition::BuiltIn {
-                        fun: BuiltinScalarFunction::Concat,
-                        ..
-                    },
+                func_def: ScalarFunctionDefinition::BuiltIn(BuiltinScalarFunction::Concat),
                 args,
             }) => simpl_concat(args)?,
 
             // concat_ws
             Expr::ScalarFunction(ScalarFunction {
                 func_def:
-                    ScalarFunctionDefinition::BuiltIn {
-                        fun: BuiltinScalarFunction::ConcatWithSeparator,
-                        ..
-                    },
+                    ScalarFunctionDefinition::BuiltIn(
+                        BuiltinScalarFunction::ConcatWithSeparator,
+                    ),
                 args,
             }) => match &args[..] {
                 [delimiter, vals @ ..] => simpl_concat_ws(delimiter, vals)?,

diff --git a/datafusion/optimizer/src/simplify_expressions/utils.rs b/datafusion/optimizer/src/simplify_expressions/utils.rs
@@ -365,11 +365,7 @@ pub fn simpl_log(current_args: Vec<Expr>, info: &dyn SimplifyInfo) -> Result<Exp
             )?))
         }
         Expr::ScalarFunction(ScalarFunction {
-            func_def:
-                ScalarFunctionDefinition::BuiltIn {
-                    fun: BuiltinScalarFunction::Power,
-                    ..
-                },
+            func_def: ScalarFunctionDefinition::BuiltIn(BuiltinScalarFunction::Power),
             args,
         }) if base == &args[0] => Ok(args[1].clone()),
         _ => {
@@ -409,11 +405,7 @@ pub fn simpl_power(current_args: Vec<Expr>, info: &dyn SimplifyInfo) -> Result<E
             Ok(base.clone())
         }
         Expr::ScalarFunction(ScalarFunction {
-            func_def:
-                ScalarFunctionDefinition::BuiltIn {
-                    fun: BuiltinScalarFunction::Log,
-                    ..
-                },
+            func_def: ScalarFunctionDefinition::BuiltIn(BuiltinScalarFunction::Log),
             args,
         }) if base == &args[0] => Ok(args[1].clone()),
         _ => Ok(Expr::ScalarFunction(ScalarFunction::new(

diff --git a/datafusion/physical-expr/src/planner.rs b/datafusion/physical-expr/src/planner.rs
@@ -349,7 +349,7 @@ pub fn create_physical_expr(
         }
 
         Expr::ScalarFunction(ScalarFunction { func_def, args }) => match func_def {
-            ScalarFunctionDefinition::BuiltIn { fun, .. } => {
+            ScalarFunctionDefinition::BuiltIn(fun) => {
                 let physical_args = args
                     .iter()
                     .map(|e| {

diff --git a/datafusion/proto/src/logical_plan/to_proto.rs b/datafusion/proto/src/logical_plan/to_proto.rs
@@ -793,7 +793,7 @@ impl TryFrom<&Expr> for protobuf::LogicalExprNode {
                 ))
             }
             Expr::ScalarFunction(ScalarFunction { func_def, args }) => match func_def {
-                ScalarFunctionDefinition::BuiltIn { fun, .. } => {
+                ScalarFunctionDefinition::BuiltIn(fun) => {
                     let fun: protobuf::ScalarFunction = fun.try_into()?;
                     let args: Vec<Self> = args
                         .iter()

diff --git a/datafusion/sql/src/expr/value.rs b/datafusion/sql/src/expr/value.rs
@@ -144,7 +144,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
                     values.push(value);
                 }
                 Expr::ScalarFunction(ScalarFunction {
-                    func_def: ScalarFunctionDefinition::BuiltIn { fun, .. },
+                    func_def: ScalarFunctionDefinition::BuiltIn(fun),
                     ..
                 }) => {
                     if fun == BuiltinScalarFunction::MakeArray {