From 513fd052bdbf5c7a73de544a876961f780b90a92 Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Thu, 30 Nov 2023 17:11:19 -0500 Subject: [PATCH] Minor: Remove uncessary name field in ScalarFunctionDefintion (#8365) --- .../core/src/datasource/listing/helpers.rs | 2 +- datafusion/expr/src/built_in_function.rs | 9 +++++-- datafusion/expr/src/expr.rs | 14 +++------- datafusion/expr/src/expr_fn.rs | 14 +++++----- datafusion/expr/src/expr_schema.rs | 2 +- datafusion/expr/src/tree_node/expr.rs | 2 +- .../optimizer/src/analyzer/type_coercion.rs | 2 +- .../optimizer/src/optimize_projections.rs | 4 +-- datafusion/optimizer/src/push_down_filter.rs | 2 +- .../simplify_expressions/expr_simplifier.rs | 27 +++++-------------- .../src/simplify_expressions/utils.rs | 12 ++------- datafusion/physical-expr/src/planner.rs | 2 +- datafusion/proto/src/logical_plan/to_proto.rs | 2 +- datafusion/sql/src/expr/value.rs | 2 +- 14 files changed, 36 insertions(+), 60 deletions(-) diff --git a/datafusion/core/src/datasource/listing/helpers.rs b/datafusion/core/src/datasource/listing/helpers.rs index 0c39877cd11e..a4505cf62d6a 100644 --- 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 index 2f67783201f5..a51941fdee11 100644 --- 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 index 256f5b210ec2..ee9b0ad6f967 100644 --- 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, - }, + BuiltIn(BuiltinScalarFunction), /// Resolved to a user defined function UDF(Arc), /// 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) -> 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 index 1f4ab7bb4ad3..6148226f6b1a 100644 --- 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 index 99b27e8912bc..2795ac5f0962 100644 --- 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 index fcb0a4cd93f3..1098842716b9 100644 --- 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 index bedc86e2f4f1..e3b86f5db78f 100644 --- 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 index b6d026279aa6..bbf704a83c55 100644 --- 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> { ))) } 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 index bad6e24715c9..e8f116d89466 100644 --- 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 index c7366e17619c..41c71c9d9aff 100644 --- 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 index e69207b6889a..fa91a3ace2a2 100644 --- 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, info: &dyn SimplifyInfo) -> Result Ok(args[1].clone()), _ => { @@ -409,11 +405,7 @@ pub fn simpl_power(current_args: Vec, info: &dyn SimplifyInfo) -> Result 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 index 5c5cc8e36fa7..5501647da2c3 100644 --- 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 index b619339674fd..ab8e850014e5 100644 --- 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 = args .iter() diff --git a/datafusion/sql/src/expr/value.rs b/datafusion/sql/src/expr/value.rs index f33e9e8ddf78..a3f29da488ba 100644 --- 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 {