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

Request: Improve Monotoniciy API #9879

Closed
alamb opened this issue Mar 30, 2024 · 2 comments
Closed

Request: Improve Monotoniciy API #9879

alamb opened this issue Mar 30, 2024 · 2 comments
Assignees
Labels
enhancement New feature or request

Comments

@alamb
Copy link
Contributor

alamb commented Mar 30, 2024

Is your feature request related to a problem or challenge?

While reviewing #9869 from @tinfoil-knight I was confused about the ScalarUDFImpl::monotonicity API

// This function specifies monotonicity behaviors for User defined scalar functions.
fn monotonicity(&self) -> Result<Option<Vec<Option<bool>>>, DataFusionError> {..}

The challenge is the cognative complexity of understanding what Result<Option<Vec<Option<bool>>> means

The actual code is defined in terms FuncMonotonicity https://github.com/apache/arrow-datafusion/blob/57c0bc65280c467ef6f5a498c6e78a616401b286/datafusion/expr/src/signature.rs#L350-L356

However, since it is a typedef it doesn't show up in the Rust docs

Describe the solution you'd like

An easy to understand Monotonicity API

Describe alternatives you've considered

I think it would be great to have an API like this:

// This function specifies monotonicity behaviors for User defined scalar functions.
fn monotonicity(&self) -> Result<Monotonicity>, DataFusionError> {..}

And then we could make Monotonicity an enum that captured the relevant data. This would both be easier to understand and likely more efficient as well

Something like

/// Monotonicity of a function with respect to its arguments.
///
/// A function is [monotonic] if it preserves the relative order of its inputs. 
///
/// [monotonic]: https://en.wikipedia.org/wiki/Monotonic_function
pub enum Monotonicity {
  /// not monotonic or unknown monotonicity
  None,
  /// Increasing with respect to all of its arguments
  Increasing,
  /// Decreasing with respect to all of its arguments
  Decreasing,
  /// 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.
  Mixed(Vec<bool>),
}

I think this would be much eaiser to reason about

Additional context

No response

@alamb alamb added the enhancement New feature or request label Mar 30, 2024
@tinfoil-knight
Copy link
Contributor

tinfoil-knight commented Mar 30, 2024

take

@alamb
Copy link
Contributor Author

alamb commented May 21, 2024

#10504 introduces a new API for boundary propagation, so the API challenge described in this ticket is no longer relevant. Thus closing

@alamb alamb closed this as completed May 21, 2024
@alamb alamb reopened this May 21, 2024
@alamb alamb closed this as not planned Won't fix, can't repro, duplicate, stale May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants