Skip to content

Minor: Move Monotonicity to expr crate #7820

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.

Sign up for GitHub

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
Oct 15, 2023
Merged

Minor: Move Monotonicity to expr crate #7820

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df3ad6b
move Monotonicity datafusion-physical-expr -> datafusion-expr
2010YOUY01 Oct 13, 2023
30aea24
review comments
2010YOUY01 Oct 15, 2023
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
45 changes: 43 additions & 2 deletions datafusion/expr/src/built_in_function.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ use crate::nullif::SUPPORTED_NULLIF_TYPES;
use crate::signature::TIMEZONE_WILDCARD;
use crate::type_coercion::functions::data_types;
use crate::{
conditional_expressions, struct_expressions, utils, Signature, TypeSignature,
Volatility,
conditional_expressions, struct_expressions, utils, FuncMonotonicity, Signature,
TypeSignature, Volatility,
};
use arrow::datatypes::{DataType, Field, Fields, IntervalUnit, TimeUnit};
use datafusion_common::{internal_err, plan_err, DataFusionError, Result};
Expand Down Expand Up @@ -1283,6 +1283,47 @@ impl BuiltinScalarFunction {
}
}
}

/// This function specifies monotonicity behaviors for built-in scalar functions.
/// The list can be extended, only mathematical and datetime functions are
/// considered for the initial implementation of this feature.
pub fn monotonicity(&self) -> Option<FuncMonotonicity> {
if matches!(
&self,
BuiltinScalarFunction::Atan
| BuiltinScalarFunction::Acosh
| BuiltinScalarFunction::Asinh
| BuiltinScalarFunction::Atanh
| BuiltinScalarFunction::Ceil
| BuiltinScalarFunction::Degrees
| BuiltinScalarFunction::Exp
| BuiltinScalarFunction::Factorial
| BuiltinScalarFunction::Floor
| BuiltinScalarFunction::Ln
| BuiltinScalarFunction::Log10
| BuiltinScalarFunction::Log2
| BuiltinScalarFunction::Radians
| BuiltinScalarFunction::Round
| BuiltinScalarFunction::Signum
| BuiltinScalarFunction::Sinh
| BuiltinScalarFunction::Sqrt
| BuiltinScalarFunction::Cbrt
| BuiltinScalarFunction::Tanh
| BuiltinScalarFunction::Trunc
| BuiltinScalarFunction::Pi
) {
Some(vec![Some(true)])
} else if matches!(
&self,
BuiltinScalarFunction::DateTrunc | BuiltinScalarFunction::DateBin
) {
Some(vec![None, Some(true)])
} else if *self == BuiltinScalarFunction::Log {
Some(vec![Some(true), Some(false)])
} else {
None
}
}
}

fn aliases(func: &BuiltinScalarFunction) -> &'static [&'static str] {
Expand Down
4 changes: 3 additions & 1 deletion datafusion/expr/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,9 @@ pub use logical_plan::*;
pub use nullif::SUPPORTED_NULLIF_TYPES;
pub use operator::Operator;
pub use partition_evaluator::PartitionEvaluator;
pub use signature::{Signature, TypeSignature, Volatility, TIMEZONE_WILDCARD};
pub use signature::{
FuncMonotonicity, Signature, TypeSignature, Volatility, TIMEZONE_WILDCARD,
};
pub use table_source::{TableProviderFilterPushDown, TableSource, TableType};
pub use udaf::AggregateUDF;
pub use udf::ScalarUDF;
Expand Down
8 changes: 8 additions & 0 deletions datafusion/expr/src/signature.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,3 +226,11 @@ impl Signature {
}
}
}

/// Monotonicity of the `ScalarFunctionExpr` with respect to its arguments.
/// Each element of this vector corresponds to an argument and indicates whether
/// the function's behavior is monotonic, or non-monotonic/unknown for that argument, namely:
/// - `None` signifies unknown monotonicity or non-monotonicity.
/// - `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>>;
58 changes: 9 additions & 49 deletions datafusion/physical-expr/src/functions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ use arrow::{
datatypes::{DataType, Int32Type, Int64Type, Schema},
};
use datafusion_common::{internal_err, DataFusionError, Result, ScalarValue};
pub use datafusion_expr::FuncMonotonicity;
use datafusion_expr::{
BuiltinScalarFunction, ColumnarValue, ScalarFunctionImplementation,
};
Expand Down Expand Up @@ -180,7 +181,7 @@ pub fn create_physical_expr(
_ => create_physical_fun(fun, execution_props)?,
};

let monotonicity = get_func_monotonicity(fun);
let monotonicity = fun.monotonicity();

Ok(Arc::new(ScalarFunctionExpr::new(
&format!("{fun}"),
Expand Down Expand Up @@ -903,13 +904,13 @@ pub fn create_physical_fun(
})
}

/// Monotonicity of the `ScalarFunctionExpr` with respect to its arguments.
/// Each element of this vector corresponds to an argument and indicates whether
/// the function's behavior is monotonic, or non-monotonic/unknown for that argument, namely:
/// - `None` signifies unknown monotonicity or non-monotonicity.
/// - `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>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we can leave a pub use datafusion_expr::FuncMonotonicty in this module to ease backwards compatibility?

#[deprecated(
since = "32.0.0",
note = "Moved to `expr` crate. Please use `BuiltinScalarFunction::monotonicity()` instead"
)]
pub fn get_func_monotonicity(fun: &BuiltinScalarFunction) -> Option<FuncMonotonicity> {
fun.monotonicity()
}

/// Determines a [`ScalarFunctionExpr`]'s monotonicity for the given arguments
/// and the function's behavior depending on its arguments.
Expand Down Expand Up @@ -964,47 +965,6 @@ fn func_order_in_one_dimension(
}
}

/// This function specifies monotonicity behaviors for built-in scalar functions.
/// The list can be extended, only mathematical and datetime functions are
/// considered for the initial implementation of this feature.
pub fn get_func_monotonicity(fun: &BuiltinScalarFunction) -> Option<FuncMonotonicity> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please avoid a breaking api change by at least adding pub use datafusion_expr::get_func_monotonicity in this file?

However, since we are going to move this code anyways, what would you think about putting this code as a method on BuiltinScalarFunction itself, so that it was easier to find

Something like

impl BuiltinScalarFunction {
  pub fn monotonicity(&self) -> Option<FuncMonotonicity> {
  ...
   }
}

And leaving the get_func_monotonicity as a deprecated function, perhaps like

#[deprecated(message="use BuiltinScalarFunction::monotonicity")]
pub fn get_func_monotonicity(fun: &BuiltinScalarFunction) -> Option<FuncMonotonicity> {
  fun.monotonicity()
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I also think deprecation is better

if matches!(
fun,
BuiltinScalarFunction::Atan
| BuiltinScalarFunction::Acosh
| BuiltinScalarFunction::Asinh
| BuiltinScalarFunction::Atanh
| BuiltinScalarFunction::Ceil
| BuiltinScalarFunction::Degrees
| BuiltinScalarFunction::Exp
| BuiltinScalarFunction::Factorial
| BuiltinScalarFunction::Floor
| BuiltinScalarFunction::Ln
| BuiltinScalarFunction::Log10
| BuiltinScalarFunction::Log2
| BuiltinScalarFunction::Radians
| BuiltinScalarFunction::Round
| BuiltinScalarFunction::Signum
| BuiltinScalarFunction::Sinh
| BuiltinScalarFunction::Sqrt
| BuiltinScalarFunction::Cbrt
| BuiltinScalarFunction::Tanh
| BuiltinScalarFunction::Trunc
| BuiltinScalarFunction::Pi
) {
Some(vec![Some(true)])
} else if matches!(
fun,
BuiltinScalarFunction::DateTrunc | BuiltinScalarFunction::DateBin
) {
Some(vec![None, Some(true)])
} else if *fun == BuiltinScalarFunction::Log {
Some(vec![Some(true), Some(false)])
} else {
None
}
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down
2 changes: 1 addition & 1 deletion datafusion/physical-expr/src/scalar_function.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
//! to a function that supports f64, it is coerced to f64.

use crate::functions::out_ordering;
use crate::functions::FuncMonotonicity;
use crate::physical_expr::down_cast_any_ref;
use crate::sort_properties::SortProperties;
use crate::utils::expr_list_eq_strict_order;
Expand All @@ -41,6 +40,7 @@ use datafusion_common::Result;
use datafusion_expr::expr_vec_fmt;
use datafusion_expr::BuiltinScalarFunction;
use datafusion_expr::ColumnarValue;
use datafusion_expr::FuncMonotonicity;
use datafusion_expr::ScalarFunctionImplementation;
use std::any::Any;
use std::fmt::Debug;
Expand Down