Skip to content

Remove ScalarFunctionDefinition::Name #10277

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

Merged
merged 2 commits into from
Apr 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions datafusion/core/src/datasource/listing/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ use futures::{stream::BoxStream, StreamExt, TryStreamExt};
use log::{debug, trace};

use datafusion_common::tree_node::{TreeNode, TreeNodeRecursion};
use datafusion_common::{internal_err, Column, DFSchema, DataFusionError};
use datafusion_common::{Column, DFSchema, DataFusionError};
use datafusion_expr::{Expr, ScalarFunctionDefinition, Volatility};
use datafusion_physical_expr::create_physical_expr;
use object_store::path::Path;
Expand Down Expand Up @@ -100,9 +100,6 @@ pub fn expr_applicable_for_cols(col_names: &[String], expr: &Expr) -> bool {
}
}
}
ScalarFunctionDefinition::Name(_) => {
internal_err!("Function `Expr` with name should be resolved.")
}
}
}

Expand Down
9 changes: 2 additions & 7 deletions datafusion/core/src/physical_planner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,8 @@ use datafusion_expr::expr_rewriter::unnormalize_cols;
use datafusion_expr::expr_vec_fmt;
use datafusion_expr::logical_plan::builder::wrap_projection_for_join_if_necessary;
use datafusion_expr::{
DescribeTable, DmlStatement, Extension, Filter, RecursiveQuery,
ScalarFunctionDefinition, StringifiedPlan, WindowFrame, WindowFrameBound, WriteOp,
DescribeTable, DmlStatement, Extension, Filter, RecursiveQuery, StringifiedPlan,
WindowFrame, WindowFrameBound, WriteOp,
};
use datafusion_physical_expr::expressions::Literal;
use datafusion_physical_expr::LexOrdering;
Expand Down Expand Up @@ -240,11 +240,6 @@ fn create_physical_name(e: &Expr, is_first_expr: bool) -> Result<String> {
};
}
Expr::ScalarFunction(fun) => {
// function should be resolved during `AnalyzerRule`s
if let ScalarFunctionDefinition::Name(_) = fun.func_def {
return internal_err!("Function `Expr` with name should be resolved.");
}

create_function_physical_name(fun.name(), false, &fun.args, None)
}
Expr::WindowFunction(WindowFunction {
Expand Down
14 changes: 0 additions & 14 deletions datafusion/expr/src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -404,9 +404,6 @@ impl Between {
pub enum ScalarFunctionDefinition {
/// Resolved to a user defined function
UDF(Arc<crate::ScalarUDF>),
/// A scalar function constructed with name. This variant can not be executed directly
/// and instead must be resolved to one of the other variants prior to physical planning.
Name(Arc<str>),
}

/// ScalarFunction expression invokes a built-in scalar function
Expand All @@ -430,7 +427,6 @@ impl ScalarFunctionDefinition {
pub fn name(&self) -> &str {
match self {
ScalarFunctionDefinition::UDF(udf) => udf.name(),
ScalarFunctionDefinition::Name(func_name) => func_name.as_ref(),
}
}

Expand All @@ -441,11 +437,6 @@ impl ScalarFunctionDefinition {
ScalarFunctionDefinition::UDF(udf) => {
Ok(udf.signature().volatility == crate::Volatility::Volatile)
}
ScalarFunctionDefinition::Name(func) => {
internal_err!(
"Cannot determine volatility of unresolved function: {func}"
)
}
}
}
}
Expand Down Expand Up @@ -2100,11 +2091,6 @@ mod test {
),
}));
assert!(ScalarFunctionDefinition::UDF(udf).is_volatile().unwrap());

// Unresolved function
ScalarFunctionDefinition::Name(Arc::from("UnresolvedFunc"))
.is_volatile()
.expect_err("Shouldn't determine volatility of unresolved function");
}

use super::*;
Expand Down
3 changes: 0 additions & 3 deletions datafusion/expr/src/expr_schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,9 +156,6 @@ impl ExprSchemable for Expr {
// expressiveness of `TypeSignature`), then infer return type
Ok(fun.return_type_from_exprs(args, schema, &arg_data_types)?)
}
ScalarFunctionDefinition::Name(_) => {
internal_err!("Function `Expr` with name should be resolved.")
}
}
}
Expr::WindowFunction(WindowFunction { fun, args, .. }) => {
Expand Down
3 changes: 0 additions & 3 deletions datafusion/expr/src/tree_node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -286,9 +286,6 @@ impl TreeNode for Expr {
ScalarFunctionDefinition::UDF(fun) => {
Ok(Expr::ScalarFunction(ScalarFunction::new_udf(fun, new_args)))
}
ScalarFunctionDefinition::Name(_) => {
internal_err!("Function `Expr` with name should be resolved.")
}
})?
}
Expr::WindowFunction(WindowFunction {
Expand Down
9 changes: 4 additions & 5 deletions datafusion/functions/src/math/log.rs
Original file line number Diff line number Diff line change
Expand Up @@ -208,14 +208,13 @@ impl ScalarUDFImpl for LogFunc {

/// Returns true if the function is `PowerFunc`
fn is_pow(func_def: &ScalarFunctionDefinition) -> bool {
if let ScalarFunctionDefinition::UDF(fun) = func_def {
fun.as_ref()
match func_def {
ScalarFunctionDefinition::UDF(fun) => fun
.as_ref()
.inner()
.as_any()
.downcast_ref::<PowerFunc>()
.is_some()
} else {
false
.is_some(),
}
}

Expand Down
9 changes: 4 additions & 5 deletions datafusion/functions/src/math/power.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,14 +153,13 @@ impl ScalarUDFImpl for PowerFunc {

/// Return true if this function call is a call to `Log`
fn is_log(func_def: &ScalarFunctionDefinition) -> bool {
if let ScalarFunctionDefinition::UDF(fun) = func_def {
fun.as_ref()
match func_def {
ScalarFunctionDefinition::UDF(fun) => fun
.as_ref()
.inner()
.as_any()
.downcast_ref::<LogFunc>()
.is_some()
} else {
false
.is_some(),
}
}

Expand Down
3 changes: 0 additions & 3 deletions datafusion/optimizer/src/analyzer/type_coercion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -318,9 +318,6 @@ impl TreeNodeRewriter for TypeCoercionRewriter {
ScalarFunction::new_udf(fun, new_expr),
)))
}
ScalarFunctionDefinition::Name(_) => {
internal_err!("Function `Expr` with name should be resolved.")
}
},
Expr::AggregateFunction(expr::AggregateFunction {
func_def,
Expand Down
1 change: 0 additions & 1 deletion datafusion/optimizer/src/push_down_filter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,6 @@ fn can_evaluate_as_join_condition(predicate: &Expr) -> Result<bool> {
| Expr::Case(_)
| Expr::Cast(_)
| Expr::TryCast(_)
| Expr::ScalarFunction(..)
| Expr::InList { .. } => Ok(TreeNodeRecursion::Continue),
Expr::Sort(_)
| Expr::AggregateFunction(_)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -528,7 +528,6 @@ impl<'a> ConstEvaluator<'a> {
ScalarFunctionDefinition::UDF(fun) => {
Self::volatility_ok(fun.signature().volatility)
}
ScalarFunctionDefinition::Name(_) => false,
},
Expr::Literal(_)
| Expr::Unnest(_)
Expand Down
3 changes: 0 additions & 3 deletions datafusion/physical-expr/src/planner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -316,9 +316,6 @@ pub fn create_physical_expr(
args,
input_dfschema,
),
ScalarFunctionDefinition::Name(_) => {
internal_err!("Function `Expr` with name should be resolved.")
}
}
}
Expr::Between(Between {
Expand Down
7 changes: 1 addition & 6 deletions datafusion/physical-expr/src/scalar_function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ use std::sync::Arc;
use arrow::datatypes::{DataType, Schema};
use arrow::record_batch::RecordBatch;

use datafusion_common::{internal_err, Result};
use datafusion_common::Result;
use datafusion_expr::{
expr_vec_fmt, ColumnarValue, FuncMonotonicity, ScalarFunctionDefinition,
};
Expand Down Expand Up @@ -152,11 +152,6 @@ impl PhysicalExpr for ScalarFunctionExpr {
fun.invoke(&inputs)
}
}
ScalarFunctionDefinition::Name(_) => {
internal_err!(
"Name function must be resolved to one of the other variants prior to physical planning"
)
}
}
}

Expand Down
6 changes: 0 additions & 6 deletions datafusion/proto/src/logical_plan/to_proto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -786,12 +786,6 @@ pub fn serialize_expr(
)),
}
}
ScalarFunctionDefinition::Name(_) => {
return Err(Error::NotImplemented(
"Proto serialization error: Trying to serialize a unresolved function"
.to_string(),
));
}
}
}
Expr::Not(expr) => {
Expand Down
5 changes: 0 additions & 5 deletions datafusion/proto/src/physical_plan/to_proto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -546,11 +546,6 @@ pub fn serialize_physical_expr(
ScalarFunctionDefinition::UDF(udf) => {
codec.try_encode_udf(udf, &mut buf)?;
}
_ => {
return not_impl_err!(
"Proto serialization error: Trying to serialize a unresolved function"
);
}
}

let fun_definition = if buf.is_empty() { None } else { Some(buf) };
Expand Down
7 changes: 1 addition & 6 deletions datafusion/substrait/src/logical_plan/producer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ use datafusion::common::{substrait_err, DFSchemaRef};
use datafusion::logical_expr::aggregate_function;
use datafusion::logical_expr::expr::{
AggregateFunctionDefinition, Alias, BinaryExpr, Case, Cast, GroupingSet, InList,
InSubquery, ScalarFunctionDefinition, Sort, WindowFunction,
InSubquery, Sort, WindowFunction,
};
use datafusion::logical_expr::{expr, Between, JoinConstraint, LogicalPlan, Operator};
use datafusion::prelude::Expr;
Expand Down Expand Up @@ -941,11 +941,6 @@ pub fn to_substrait_rex(
});
}

// function should be resolved during `AnalyzerRule`
if let ScalarFunctionDefinition::Name(_) = fun.func_def {
return internal_err!("Function `Expr` with name should be resolved.");
}

let function_anchor =
_register_function(fun.name().to_string(), extension_info);
Ok(Expression {
Expand Down