-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Move coalesce to datafusion-functions and remove BuiltInScalarFunction #10098
Changes from all commits
e298c1c
3cb053c
5c7c3e2
a422b3a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,8 +28,8 @@ use crate::logical_plan::Subquery; | |
use crate::utils::expr_to_columns; | ||
use crate::window_frame; | ||
use crate::{ | ||
aggregate_function, built_in_function, built_in_window_function, udaf, | ||
BuiltinScalarFunction, ExprSchemable, Operator, Signature, | ||
aggregate_function, built_in_window_function, udaf, ExprSchemable, Operator, | ||
Signature, | ||
}; | ||
|
||
use arrow::datatypes::DataType; | ||
|
@@ -362,10 +362,6 @@ impl Between { | |
#[derive(Debug, Clone, PartialEq, Eq, Hash)] | ||
/// Defines which implementation of a function for DataFusion to call. | ||
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(BuiltinScalarFunction), | ||
/// Resolved to a user defined function | ||
UDF(Arc<crate::ScalarUDF>), | ||
/// A scalar function constructed with name. This variant can not be executed directly | ||
|
@@ -393,7 +389,6 @@ impl ScalarFunctionDefinition { | |
/// Function's name for display | ||
pub fn name(&self) -> &str { | ||
match self { | ||
ScalarFunctionDefinition::BuiltIn(fun) => fun.name(), | ||
ScalarFunctionDefinition::UDF(udf) => udf.name(), | ||
ScalarFunctionDefinition::Name(func_name) => func_name.as_ref(), | ||
} | ||
|
@@ -403,9 +398,6 @@ impl ScalarFunctionDefinition { | |
/// when evaluated multiple times with the same input. | ||
pub fn is_volatile(&self) -> Result<bool> { | ||
match self { | ||
ScalarFunctionDefinition::BuiltIn(fun) => { | ||
Ok(fun.volatility() == crate::Volatility::Volatile) | ||
} | ||
ScalarFunctionDefinition::UDF(udf) => { | ||
Ok(udf.signature().volatility == crate::Volatility::Volatile) | ||
} | ||
|
@@ -419,14 +411,6 @@ impl ScalarFunctionDefinition { | |
} | ||
|
||
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), | ||
args, | ||
} | ||
} | ||
|
||
/// Create a new ScalarFunction expression with a user-defined function (UDF) | ||
pub fn new_udf(udf: Arc<crate::ScalarUDF>, args: Vec<Expr>) -> Self { | ||
Self { | ||
|
@@ -1282,7 +1266,7 @@ impl Expr { | |
pub fn short_circuits(&self) -> bool { | ||
match self { | ||
Expr::ScalarFunction(ScalarFunction { func_def, .. }) => { | ||
matches!(func_def, ScalarFunctionDefinition::BuiltIn(fun) if *fun == BuiltinScalarFunction::Coalesce) | ||
matches!(func_def, ScalarFunctionDefinition::UDF(fun) if fun.name().eq("coalesce")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I couldn't find a good way to directly refer to the coalesce function here so went with checking by name. There is another by-name check in /physical-expr/src/scalar_function.rs for make_array so I believe this is acceptable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I"m thinking if its possible if user creates udf with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought about that and my thought was that if that was done it would replace the existing coalesce function and one would think it would (hopefully) have similar functionality. If there is another way to determine it here it should be a reasonable change. Just importing the crate though isn't possible as best as I can tell because that would cause a circular dependency. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding the new API There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is an option but I think if we go that route it should be it's own PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that @haohuaijin 's idea is a good one (and keeping with the spirit of "anything we can do with built in functions can be done with ScalarAPIs"). I filed #10162 to track |
||
} | ||
Expr::BinaryExpr(BinaryExpr { op, .. }) => { | ||
matches!(op, Operator::And | Operator::Or) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestingly, I don't think Name is ever used anymore -- we could eventually simply remove
ScalarFunctionDefinition
to avoid another level of wrapping.However, I think there has been enough API churn for a while. We can maybe clean that up as a follow on PR in a few months or somthing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On argument against keeping it: if it's unused and you keep it around, someone will likely use it again and make the removal even harder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracked in #10175