ozankabak commented on code in PR #10117:
URL: https://github.com/apache/datafusion/pull/10117#discussion_r1592322084
##########
datafusion/expr/src/signature.rs:
##########
@@ -346,13 +346,61 @@ 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>>;
+#[derive(Debug, Clone)]
+enum FuncMonotonicityPartial {
+ /// 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>>),
+}
+
+/// 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 struct FuncMonotonicity(FuncMonotonicityPartial);
+
+impl FuncMonotonicity {
+ pub fn new_none() -> Self {
+ Self(FuncMonotonicityPartial::None)
+ }
+ pub fn new_increasing() -> Self {
+ Self(FuncMonotonicityPartial::Increasing)
+ }
+ pub fn new_decreasing() -> Self {
+ Self(FuncMonotonicityPartial::Decreasing)
+ }
+ pub fn new_mixed(inner: Vec<Option<bool>>) -> Self {
+ Self(FuncMonotonicityPartial::Mixed(inner))
+ }
+}
+
+impl From<&FuncMonotonicity> for Vec<Option<bool>> {
+ fn from(val: &FuncMonotonicity) -> Self {
+ match &val.0 {
+ FuncMonotonicityPartial::None => vec![None],
+ FuncMonotonicityPartial::Increasing => vec![Some(true)],
+ FuncMonotonicityPartial::Decreasing => vec![Some(false)],
+ FuncMonotonicityPartial::Mixed(inner) => inner.to_vec(),
+ }
+ }
+}
Review Comment:
I don't think this conversion is possible to do. Given only `Increasing`,
you can not produce the vector without knowing the arity of the function.
Assuming an arity of one (which seems to be the case here) can lead to subtle
bugs and/or compile-time errors in some cases.
##########
datafusion/expr/src/signature.rs:
##########
@@ -346,13 +346,61 @@ 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>>;
+#[derive(Debug, Clone)]
+enum FuncMonotonicityPartial {
+ /// 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>>),
+}
+
+/// 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 struct FuncMonotonicity(FuncMonotonicityPartial);
+
+impl FuncMonotonicity {
+ pub fn new_none() -> Self {
+ Self(FuncMonotonicityPartial::None)
+ }
+ pub fn new_increasing() -> Self {
+ Self(FuncMonotonicityPartial::Increasing)
+ }
+ pub fn new_decreasing() -> Self {
+ Self(FuncMonotonicityPartial::Decreasing)
+ }
+ pub fn new_mixed(inner: Vec<Option<bool>>) -> Self {
+ Self(FuncMonotonicityPartial::Mixed(inner))
+ }
+}
Review Comment:
Can we use just one data type? Consider:
```rust
/// 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 the 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.
Partial(Vec<Option<bool>>),
}
```
##########
datafusion/expr/src/signature.rs:
##########
@@ -346,13 +346,61 @@ 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>>;
+#[derive(Debug, Clone)]
+enum FuncMonotonicityPartial {
+ /// 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>>),
+}
+
+/// 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 struct FuncMonotonicity(FuncMonotonicityPartial);
+
+impl FuncMonotonicity {
+ pub fn new_none() -> Self {
+ Self(FuncMonotonicityPartial::None)
+ }
+ pub fn new_increasing() -> Self {
+ Self(FuncMonotonicityPartial::Increasing)
+ }
+ pub fn new_decreasing() -> Self {
+ Self(FuncMonotonicityPartial::Decreasing)
+ }
+ pub fn new_mixed(inner: Vec<Option<bool>>) -> Self {
+ Self(FuncMonotonicityPartial::Mixed(inner))
+ }
+}
+
+impl From<&FuncMonotonicity> for Vec<Option<bool>> {
+ fn from(val: &FuncMonotonicity) -> Self {
+ match &val.0 {
+ FuncMonotonicityPartial::None => vec![None],
+ FuncMonotonicityPartial::Increasing => vec![Some(true)],
+ FuncMonotonicityPartial::Decreasing => vec![Some(false)],
+ FuncMonotonicityPartial::Mixed(inner) => inner.to_vec(),
+ }
+ }
+}
+
+impl PartialEq for FuncMonotonicity {
+ fn eq(&self, other: &Self) -> bool {
+ Into::<Vec<Option<bool>>>::into(self) ==
Into::<Vec<Option<bool>>>::into(other)
Review Comment:
Given my comment on `From`, you will need explicit logic here.
##########
datafusion/physical-expr/src/scalar_function.rs:
##########
@@ -251,7 +251,9 @@ pub fn out_ordering(
func: &FuncMonotonicity,
arg_orderings: &[SortProperties],
) -> SortProperties {
- func.iter().zip(arg_orderings).fold(
+ let monotonicity_vec: Vec<Option<bool>> = func.into();
+
+ monotonicity_vec.iter().zip(arg_orderings).fold(
Review Comment:
This logic will unfortunately get a little complicated given the issue with
`From`.
--
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
