tinfoil-knight commented on code in PR #10117:
URL: https://github.com/apache/datafusion/pull/10117#discussion_r1595938806
##########
datafusion/expr/src/signature.rs:
##########
@@ -346,13 +346,60 @@ 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>>;
+/// 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
+#[derive(Debug, Clone)]
+pub enum FuncMonotonicity {
+ /// 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<Option<bool>>),
+}
+
+impl PartialEq for FuncMonotonicity {
+ fn eq(&self, other: &Self) -> bool {
+ match (self, other) {
+ (FuncMonotonicity::None, FuncMonotonicity::None) => true,
+ (FuncMonotonicity::Increasing, FuncMonotonicity::Increasing) =>
true,
+ (FuncMonotonicity::Decreasing, FuncMonotonicity::Decreasing) =>
true,
+ (FuncMonotonicity::Mixed(vec1), FuncMonotonicity::Mixed(vec2)) => {
+ vec1 == vec2
+ }
+ _ => false,
+ }
+ }
+}
+
+impl FuncMonotonicity {
+ pub fn matches(&self, other: &Self) -> bool {
Review Comment:
Note: The `matches` method isn't currently being used anywhere in the
codebase. It has been added to make comparisons easier for any future purposes.
---
Also, I don't think we need the arity of the function anymore for comparison
b/w `FuncMonotonicity::Mixed` and other types.
For eg: `FuncMonotonicity::Increasing` is "Increasing with respect to all of
its arguments" which is the same as the `FuncMonotonicity::Mixed`'s inner
vector being `Some(true)` for all elements from what I understand.
##########
datafusion/physical-expr/src/scalar_function.rs:
##########
@@ -251,10 +251,23 @@ pub fn out_ordering(
func: &FuncMonotonicity,
arg_orderings: &[SortProperties],
) -> SortProperties {
- func.iter().zip(arg_orderings).fold(
+ arg_orderings.iter().enumerate().fold(
SortProperties::Singleton,
- |prev_sort, (item, arg)| {
- let current_sort = func_order_in_one_dimension(item, arg);
+ |prev_sort, (index, arg)| {
+ let arg_monotonicity: Option<bool> = match func {
+ FuncMonotonicity::None => None,
+ FuncMonotonicity::Increasing => Some(true),
+ FuncMonotonicity::Decreasing => Some(false),
+ FuncMonotonicity::Mixed(inner_vec) => {
Review Comment:
The earlier `impl From<&FuncMonotonicity> for Vec<Option<bool>>` was
confusing & incorrect so it has been removed now.
I do think that the conversion from `FuncMonotonicity` to `Option<bool>` is
required in this function.
I tried a few different ways (without conversion) but it was resulting in
really messy code.
If the conversion still feels confusing, we can leave a note here mentioning
this for clarity:
```
/// - `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.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
