Skip to content
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

Minor: Remove redundant BuiltinScalarFunction::supports_zero_argument() #8059

Merged
merged 2 commits into from
Nov 6, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
12 changes: 10 additions & 2 deletions datafusion/expr/src/built_in_function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,10 @@ fn function_to_name() -> &'static HashMap<BuiltinScalarFunction, &'static str> {
impl BuiltinScalarFunction {
/// an allowlist of functions to take zero arguments, so that they will get special treatment
/// while executing.
#[deprecated(
since = "32.0.0",
note = "please use TypeSignature::supports_zero_argument instead"
)]
pub fn supports_zero_argument(&self) -> bool {
matches!(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also please change this method to call into self.type_signature().supports_zero_argument so it is clear they are equivalent and so the implementations don't drift out of sync?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense, updated. Thank you.

self,
Expand All @@ -337,6 +341,7 @@ impl BuiltinScalarFunction {
| BuiltinScalarFunction::MakeArray
)
}

/// Returns the [Volatility] of the builtin function.
pub fn volatility(&self) -> Volatility {
match self {
Expand Down Expand Up @@ -494,7 +499,9 @@ impl BuiltinScalarFunction {
// Note that this function *must* return the same type that the respective physical expression returns
// or the execution panics.

if input_expr_types.is_empty() && !self.supports_zero_argument() {
if input_expr_types.is_empty()
&& !self.signature().type_signature.supports_zero_argument()
{
return plan_err!(
"{}",
utils::generate_signature_error_msg(
Expand Down Expand Up @@ -904,7 +911,8 @@ impl BuiltinScalarFunction {
}
BuiltinScalarFunction::Cardinality => Signature::any(1, self.volatility()),
BuiltinScalarFunction::MakeArray => {
Signature::variadic_any(self.volatility())
// 0 or more arguments of arbitrary type
Signature::one_of(vec![VariadicAny, Any(0)], self.volatility())
}
BuiltinScalarFunction::Struct => Signature::variadic(
struct_expressions::SUPPORTED_STRUCT_TYPES.to_vec(),
Expand Down
75 changes: 70 additions & 5 deletions datafusion/expr/src/signature.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,31 +82,36 @@ pub enum Volatility {
/// ```
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub enum TypeSignature {
/// arbitrary number of arguments of an common type out of a list of valid types.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the current functions' signatures, Variadic* actually means supports 1 or more arguments instead of "arbitrary number" for current implementations, and use supports_zero_arguments() to determine whether 0 arg case is allowed.

So the doc comment is updated for clarification:
Variadic* means support 1 or more args
OneOf(Any(0), Variadic*) means support 0 or more args

/// One or more arguments of an common type out of a list of valid types.
///
/// # Examples
/// A function such as `concat` is `Variadic(vec![DataType::Utf8, DataType::LargeUtf8])`
Variadic(Vec<DataType>),
/// arbitrary number of arguments of an arbitrary but equal type.
/// One or more arguments of an arbitrary but equal type.
/// DataFusion attempts to coerce all argument types to match the first argument's type
///
/// # Examples
/// A function such as `array` is `VariadicEqual`
VariadicEqual,
/// arbitrary number of arguments with arbitrary types
/// One or more arguments with arbitrary types
VariadicAny,
/// fixed number of arguments of an arbitrary but equal type out of a list of valid types.
///
/// # Examples
/// 1. A function of one argument of f64 is `Uniform(1, vec![DataType::Float64])`
/// 2. A function of one argument of f64 or f32 is `Uniform(1, vec![DataType::Float32, DataType::Float64])`
Uniform(usize, Vec<DataType>),
/// exact number of arguments of an exact type
/// Exact number of arguments of an exact type
Exact(Vec<DataType>),
/// fixed number of arguments of arbitrary types
/// Fixed number of arguments of arbitrary types
/// If a function takes 0 argument, its `TypeSignature` should be `Any(0)`
Any(usize),
/// Matches exactly one of a list of [`TypeSignature`]s. Coercion is attempted to match
/// the signatures in order, and stops after the first success, if any.
///
/// # Examples
/// Function `make_array` takes 0 or more arguments with arbitrary types, its `TypeSignature`
/// is `OneOf(vec![Any(0), VariadicAny])`.
OneOf(Vec<TypeSignature>),
}

Expand Down Expand Up @@ -150,6 +155,18 @@ impl TypeSignature {
.collect::<Vec<String>>()
.join(delimiter)
}

/// Check whether 0 input argument is valid for given `TypeSignature`
pub fn supports_zero_argument(&self) -> bool {
match &self {
TypeSignature::Exact(vec) => vec.is_empty(),
TypeSignature::Uniform(0, _) | TypeSignature::Any(0) => true,
TypeSignature::OneOf(types) => types
.iter()
.any(|type_sig| type_sig.supports_zero_argument()),
_ => false,
}
}
}

/// Defines the supported argument types ([`TypeSignature`]) and [`Volatility`] for a function.
Expand Down Expand Up @@ -234,3 +251,51 @@ impl Signature {
/// - `Some(true)` indicates that the function is monotonically increasing w.r.t. the argument in question.
/// - Some(false) indicates that the function is monotonically decreasing w.r.t. the argument in question.
pub type FuncMonotonicity = Vec<Option<bool>>;

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn supports_zero_argument_tests() {
// Testing `TypeSignature`s which supports 0 arg
let positive_cases = vec![
TypeSignature::Exact(vec![]),
TypeSignature::Uniform(0, vec![DataType::Float64]),
TypeSignature::Any(0),
TypeSignature::OneOf(vec![
TypeSignature::Exact(vec![DataType::Int8]),
TypeSignature::Any(0),
TypeSignature::Uniform(1, vec![DataType::Int8]),
]),
];

for case in positive_cases {
assert!(
case.supports_zero_argument(),
"Expected {:?} to support zero arguments",
case
);
}

// Testing `TypeSignature`s which doesn't support 0 arg
let negative_cases = vec![
TypeSignature::Exact(vec![DataType::Utf8]),
TypeSignature::Uniform(1, vec![DataType::Float64]),
TypeSignature::Any(1),
TypeSignature::VariadicAny,
TypeSignature::OneOf(vec![
TypeSignature::Exact(vec![DataType::Int8]),
TypeSignature::Uniform(1, vec![DataType::Int8]),
]),
];

for case in negative_cases {
assert!(
!case.supports_zero_argument(),
"Expected {:?} not to support zero arguments",
case
);
}
}
}
5 changes: 4 additions & 1 deletion datafusion/physical-expr/src/scalar_function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,10 @@ impl PhysicalExpr for ScalarFunctionExpr {
let inputs = match (self.args.len(), self.name.parse::<BuiltinScalarFunction>()) {
// MakeArray support zero argument but has the different behavior from the array with one null.
(0, Ok(scalar_fun))
if scalar_fun.supports_zero_argument()
if scalar_fun
.signature()
.type_signature
.supports_zero_argument()
&& scalar_fun != BuiltinScalarFunction::MakeArray =>
{
vec![ColumnarValue::create_null_array(batch.num_rows())]
Expand Down