alamb commented on code in PR #8258:
URL: https://github.com/apache/arrow-datafusion/pull/8258#discussion_r1398209542
##########
datafusion/expr/src/expr.rs:
##########
@@ -338,37 +336,43 @@ impl Between {
}
}
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+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(built_in_function::BuiltinScalarFunction),
+ /// Resolved to a user defined function
+ UDF(Arc<crate::ScalarUDF>),
+ /// A scalar function constructed with name, could be resolved with
registered functions during
Review Comment:
```suggestion
/// 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.
```
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -345,12 +345,15 @@ impl<'a> ConstEvaluator<'a> {
| Expr::GroupingSet(_)
| Expr::Wildcard { .. }
| Expr::Placeholder(_) => false,
- Expr::ScalarFunction(ScalarFunction { fun, .. }) => {
- Self::volatility_ok(fun.volatility())
- }
- Expr::ScalarUDF(expr::ScalarUDF { fun, .. }) => {
- Self::volatility_ok(fun.signature().volatility)
- }
+ Expr::ScalarFunction(ScalarFunction { func_def, .. }) => match
func_def {
+ ScalarFunctionDefinition::BuiltIn(fun) => {
+ Self::volatility_ok(fun.volatility())
+ }
+ ScalarFunctionDefinition::UDF(fun) => {
+ Self::volatility_ok(fun.signature().volatility)
+ }
+ ScalarFunctionDefinition::Name(_) => true,
Review Comment:
I think this should return false (as we can't evaluate a function without
its definition)
##########
datafusion/substrait/src/logical_plan/producer.rs:
##########
@@ -834,7 +834,16 @@ pub fn to_substrait_rex(
)?)),
});
}
- let function_anchor = _register_function(fun.to_string(),
extension_info);
+
+ let func_name = match &func_def {
Review Comment:
This again could ideally call `func_def.name()` if we implemented that
##########
datafusion/expr/src/tree_node/expr.rs:
##########
@@ -276,12 +276,19 @@ impl TreeNode for Expr {
asc,
nulls_first,
)),
- Expr::ScalarFunction(ScalarFunction { args, fun }) =>
Expr::ScalarFunction(
- ScalarFunction::new(fun, transform_vec(args, &mut transform)?),
- ),
- Expr::ScalarUDF(ScalarUDF { args, fun }) => {
- Expr::ScalarUDF(ScalarUDF::new(fun, transform_vec(args, &mut
transform)?))
- }
+ Expr::ScalarFunction(ScalarFunction { func_def, args }) => match
func_def {
+ ScalarFunctionDefinition::BuiltIn(fun) => Expr::ScalarFunction(
+ ScalarFunction::new(fun, transform_vec(args, &mut
transform)?),
+ ),
+ ScalarFunctionDefinition::UDF(fun) => Expr::ScalarFunction(
+ ScalarFunction::new_udf(fun, transform_vec(args, &mut
transform)?),
+ ),
+ ScalarFunctionDefinition::Name(_) => {
+ return internal_err!(
Review Comment:
I think it is probably ok to support tree walks on unresolved function
names. I don't see any reason to throw an error
##########
datafusion/expr/src/expr.rs:
##########
@@ -338,37 +336,43 @@ impl Between {
}
}
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+pub enum ScalarFunctionDefinition {
Review Comment:
```suggestion
/// Defines which implementation of a function for DataFusion to call.
pub enum ScalarFunctionDefinition {
```
##########
datafusion/expr/src/expr.rs:
##########
@@ -1198,12 +1201,21 @@ impl fmt::Display for Expr {
write!(f, " NULLS LAST")
}
}
- Expr::ScalarFunction(func) => {
- fmt_function(f, &func.fun.to_string(), false, &func.args, true)
- }
- Expr::ScalarUDF(ScalarUDF { fun, args }) => {
- fmt_function(f, fun.name(), false, args, true)
- }
+ Expr::ScalarFunction(func_expr) => match &func_expr.func_def {
Review Comment:
We could add a function like
```rust
impl ScalarFunctionDefinition {
pub fn name(&self) -> &str {
..
}
}
```
And could reduce the duplication in this method more
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
