Skip to content

Conversation

2010YOUY01
Copy link
Contributor

Which issue does this PR close?

NA

Rationale for this change

I'm working on something related to functions and would like to use Monotonicity inside expr crate.
But now Monotonicity is defined in physical-expr crate, which depends on expr
Since it should be part of the function definition, it's better to move it into expr crate.

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates labels Oct 13, 2023
@2010YOUY01 2010YOUY01 changed the title Move Monotonicity to expr crate Minor: Move Monotonicity to expr crate Oct 13, 2023
alamb
alamb previously approved these changes Oct 14, 2023
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @2010YOUY01 -- this looks like a good idea to me. I think we should avoid a breaking API change, either through pub use or (better in my opinion) a deprecated function

/// - `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?

/// 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

@alamb alamb dismissed their stale review October 14, 2023 10:38

Clicked wrong button -- please avoid breaking API change

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @2010YOUY01

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants