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: Move Monotonicity to expr crate #7820

Merged
merged 2 commits into from
Oct 15, 2023
Merged

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 Physical Expressions 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

@alamb alamb merged commit 4819e7a into apache:main Oct 15, 2023
22 checks passed
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 Physical Expressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants