You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "tinfoil-knight (via GitHub)" <gi...@apache.org> on 2024/04/17 14:31:56 UTC
[PR] improve monotonicity api [arrow-datafusion]
tinfoil-knight opened a new pull request, #10117:
URL: https://github.com/apache/arrow-datafusion/pull/10117
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #9879 .
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
The `Vec<Option<bool>>` type used to express the monotonicity of scalar functions wrt their arguments was really difficult to understand.
Note: Currently only mathematical and date-time functions are considered for monotonicity.
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
`FuncMonotonicity` has been changed from the type `Vec<Option<bool>>` to an enum.
```
pub enum FuncMonotonicity {
None,
Increasing,
Decreasing,
Mixed(Vec<Option<bool>>),
}
```
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
By existing tests.
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
No.
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] improve monotonicity api [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10117:
URL: https://github.com/apache/datafusion/pull/10117#issuecomment-2122333403
I believe this PR has been superceded by https://github.com/apache/datafusion/pull/10504 which removed the montonicity apu in favor of a more expressive bounds analysis, so closing this pR
Thanks for the work on this @tinfoil-knight , and sorry we went another way.
BTW @berkaysynnada in the future if you are working on a PR that might make another become obselete it might be nice to drop a note on the other PR to given the author(s) a heads up.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] improve monotonicity api [datafusion]
Posted by "tinfoil-knight (via GitHub)" <gi...@apache.org>.
tinfoil-knight commented on code in PR #10117:
URL: https://github.com/apache/datafusion/pull/10117#discussion_r1590813568
##########
datafusion/expr/src/signature.rs:
##########
@@ -346,13 +346,81 @@ 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))
+ }
+
+ /// returns true if this function is monotonically increasing with respect to argument number arg
+ pub fn arg_increasing(&self, arg: usize) -> bool {
+ match &self.0 {
+ FuncMonotonicityPartial::None => false,
+ FuncMonotonicityPartial::Increasing => true,
+ FuncMonotonicityPartial::Decreasing => false,
+ FuncMonotonicityPartial::Mixed(inner) => inner[arg].unwrap_or(false),
+ }
+ }
+
+ /// returns true if this function is monotonically decreasing with respect to argument number arg
+ pub fn arg_decreasing(&self, arg: usize) -> bool {
+ match &self.0 {
+ FuncMonotonicityPartial::None => false,
+ FuncMonotonicityPartial::Increasing => false,
+ FuncMonotonicityPartial::Decreasing => true,
+ FuncMonotonicityPartial::Mixed(inner) => inner[arg].unwrap_or(false),
+ }
Review Comment:
These APIs aren't being used anywhere. I think we can just remove them.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] improve monotonicity api [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10117:
URL: https://github.com/apache/datafusion/pull/10117#issuecomment-2068007077
@tinfoil-knight -- are you interested in completing this PR with the proposed API updates?
--
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: commits-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org
Re: [PR] improve monotonicity api [datafusion]
Posted by "tinfoil-knight (via GitHub)" <gi...@apache.org>.
tinfoil-knight commented on code in PR #10117:
URL: https://github.com/apache/datafusion/pull/10117#discussion_r1595579610
##########
datafusion/expr/src/signature.rs:
##########
@@ -346,13 +346,81 @@ 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))
+ }
+
+ /// returns true if this function is monotonically increasing with respect to argument number arg
+ pub fn arg_increasing(&self, arg: usize) -> bool {
+ match &self.0 {
+ FuncMonotonicityPartial::None => false,
+ FuncMonotonicityPartial::Increasing => true,
+ FuncMonotonicityPartial::Decreasing => false,
+ FuncMonotonicityPartial::Mixed(inner) => inner[arg].unwrap_or(false),
+ }
+ }
+
+ /// returns true if this function is monotonically decreasing with respect to argument number arg
+ pub fn arg_decreasing(&self, arg: usize) -> bool {
+ match &self.0 {
+ FuncMonotonicityPartial::None => false,
+ FuncMonotonicityPartial::Increasing => false,
+ FuncMonotonicityPartial::Decreasing => true,
+ FuncMonotonicityPartial::Mixed(inner) => inner[arg].unwrap_or(false),
+ }
Review Comment:
Resolving as outdated.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] improve monotonicity api [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10117:
URL: https://github.com/apache/datafusion/pull/10117#issuecomment-2125736201
Thanks everyone for the communication. I just want everyone working on this project to have a good experience -- I am glad to hear that it was. Thanks again @tinfoil-knight and @berkaysynnada
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] improve monotonicity api [arrow-datafusion]
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on PR #10117:
URL: https://github.com/apache/arrow-datafusion/pull/10117#issuecomment-2061771725
The problem I see with this representation is that `Monotonicity::Increasing` and `Monotonicity::Mixed(vec![Some(true), ..., Some(true)])` refers to the same actual configuration. If one writes a check like `given_monotonicity == Monotonicity::Increasing` (and I'm sure people will), one will miss the alternative representation (which may easily arise when one generates this information programmatically if no special treatment is done).
I think we can find a refactor that increases readability but doesn't introduce such traps. Maybe a struct with an inner `Vec<Option<bool>>` and methods that define an API structuring how one interacts with it.
@alamb
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] improve monotonicity api [datafusion]
Posted by "tinfoil-knight (via GitHub)" <gi...@apache.org>.
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] improve monotonicity api [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10117:
URL: https://github.com/apache/datafusion/pull/10117#discussion_r1592966390
##########
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:
Maybe that type of comparsion belongs in something other than `equal` -- perhaps
```rust
impl FuncMonotonicity {
fn matches(&self, &other) -> bool {
...
}
}
```
So we can separate the equality check from the both sides match 🤔
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] improve monotonicity api [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10117:
URL: https://github.com/apache/datafusion/pull/10117#issuecomment-2104738501
Thanks @tinfoil-knight -- I will check this out later today
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] improve monotonicity api [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on PR #10117:
URL: https://github.com/apache/datafusion/pull/10117#issuecomment-2122357671
> I believe this PR has been superceded by #10504 which removed the montonicity apu in favor of a more expressive bounds analysis, so closing this pR
>
> Thanks for the work on this @tinfoil-knight , and sorry we went another way.
>
> BTW @berkaysynnada in the future if you are working on a PR that might make another become obselete it might be nice to drop a note on the other PR to given the author(s) a heads up.
Sorry for the inconvenience, @tinfoil-knight. My main motivation for #10504 was not specifically related to the monotonicity API, but I had to make changes in those areas to achieve a better overall design. I apologize for not informing you earlier.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] improve monotonicity api [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10117:
URL: https://github.com/apache/datafusion/pull/10117#discussion_r1598189334
##########
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:
Maybe we could add something like
```rust
enum Monotonicity {
Increasing,
Decreasing,
Unknown,
}
```
And then represent `FuncMonotonicity::Mixed` with a `Vec<Monotonicity>`
I would love to get a chance to pla around with it, but may not have time for a while
##########
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:
I agree the conversion feels confusing --
> 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.
I would be interested in trying to help here as I think the major benefit of this change is to encapsulate the monotonicity calculations. I'll give it a look
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] improve monotonicity api [datafusion]
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] improve monotonicity api [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10117:
URL: https://github.com/apache/datafusion/pull/10117#issuecomment-2068009288
Converting to draft as it is not waiting on feedback
--
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: commits-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org
Re: [PR] improve monotonicity api [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb closed pull request #10117: improve monotonicity api
URL: https://github.com/apache/datafusion/pull/10117
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] improve monotonicity api [datafusion]
Posted by "tinfoil-knight (via GitHub)" <gi...@apache.org>.
tinfoil-knight commented on PR #10117:
URL: https://github.com/apache/datafusion/pull/10117#issuecomment-2122366778
No worries @berkaysynnada and @alamb. It's difficult to keep track of everyone's work in such a large project.
It was fun to work on this and I learnt a few things from everyone's review.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] improve monotonicity api [arrow-datafusion]
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on PR #10117:
URL: https://github.com/apache/arrow-datafusion/pull/10117#issuecomment-2066467305
I like your new proposal. In this one, `new_mixed` would check if inner consists entirely of `true`/`false` values, and return `FuncMonotonicityInner::Increasing`/`FuncMonotonicityInner::Decreasing` in those cases.
Instead of `FuncMonotonicityInner`, I recommend `FuncMonotonicityPartial` to be more in line with math verbiage.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] improve monotonicity api [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10117:
URL: https://github.com/apache/datafusion/pull/10117#issuecomment-2068009219
> @alamb Yeah. I just have some college exams this week so might get delayed a bit but I'll pick this up as soon as possible.
No worries -- good luck with the exams!
--
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: commits-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org
Re: [PR] improve monotonicity api [datafusion]
Posted by "tinfoil-knight (via GitHub)" <gi...@apache.org>.
tinfoil-knight commented on code in PR #10117:
URL: https://github.com/apache/datafusion/pull/10117#discussion_r1590734875
##########
datafusion/expr/src/signature.rs:
##########
@@ -346,13 +346,75 @@ 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, PartialEq)]
Review Comment:
Yeah. Makes sense. I've fixed this in [`d47b896` (#10117)](https://github.com/apache/datafusion/pull/10117/commits/d47b89664df19ebcd59ad2aae4b8f6d0572f08fd)
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] improve monotonicity api [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on code in PR #10117:
URL: https://github.com/apache/datafusion/pull/10117#discussion_r1590633167
##########
datafusion/expr/src/signature.rs:
##########
@@ -346,13 +346,75 @@ 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, PartialEq)]
Review Comment:
Thanks @tinfoil-knight. I can just say this `PartialEq` derivation could be dangerous here. One can expect to see the equality of Increasing and vec![Some(true)].
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] improve monotonicity api [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10117:
URL: https://github.com/apache/arrow-datafusion/pull/10117#issuecomment-2066420390
> The problem I see with this representation is that `Monotonicity::Increasing` and `Monotonicity::Mixed(vec![Some(true), ..., Some(true)])` refers to the same actual configuration. If one writes a check like `given_monotonicity == Monotonicity::Increasing` (and I'm sure people will), one will miss the alternative representation (which may easily arise when one generates this information programmatically if no special treatment is done).
I think this is an excellent point
> I think we can find a refactor that increases readability but doesn't introduce such traps and preserves uniqueness.
Another potential solution that would avoid allocating Vec<bool> so much would be to make the enum private and only allow construction through APIs
Maybe something like
```rust
// Non pub to ensure can only construct valid representations
pub enum FuncMonotonicityInner {
None,
Increasing,
Decreasing,
Mixed(Vec<Option<bool>>),
}
/// Describes function monotonicity
pub struct FuncMonotonicity(FuncMonotonicityInner);
impl FuncMontonicity {
pub fn new_none() -> Self {..}
pub fn new_increasing() -> Self { .. }
pub fn new_decreasing() -> Self { .. }
pub fn new_mixed(inner: Vec<bool>) -> { .. }
/// returns true if this function is monotonically increasing with respect to argument number arg
pub fn arg_increasing(&self, arg: usize) -> bool { .. }
/// returns true if this function is monotonically decreasing with respect to argument number arg
pub fn arg_decreasing(&self, arg: usize) -> bool { .. }
}
```
In my opinion the real value of a struct over a typedef is that it reduces the cognative load of remembering "what does `monotonicity[0] == Some(false)`" mean -- instead it can be `monotonicty.arg_decreasing(0)` which also has documentation explaining it in case you forget.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] improve monotonicity api [datafusion]
Posted by "tinfoil-knight (via GitHub)" <gi...@apache.org>.
tinfoil-knight commented on PR #10117:
URL: https://github.com/apache/datafusion/pull/10117#issuecomment-2068007615
@alamb Yeah. I just have some college exams this week so might get delayed a bit but I'll pick this up as soon as possible.
--
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: commits-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org
Re: [PR] improve monotonicity api [datafusion]
Posted by "tinfoil-knight (via GitHub)" <gi...@apache.org>.
tinfoil-knight commented on code in PR #10117:
URL: https://github.com/apache/datafusion/pull/10117#discussion_r1595579610
##########
datafusion/expr/src/signature.rs:
##########
@@ -346,13 +346,81 @@ 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))
+ }
+
+ /// returns true if this function is monotonically increasing with respect to argument number arg
+ pub fn arg_increasing(&self, arg: usize) -> bool {
+ match &self.0 {
+ FuncMonotonicityPartial::None => false,
+ FuncMonotonicityPartial::Increasing => true,
+ FuncMonotonicityPartial::Decreasing => false,
+ FuncMonotonicityPartial::Mixed(inner) => inner[arg].unwrap_or(false),
+ }
+ }
+
+ /// returns true if this function is monotonically decreasing with respect to argument number arg
+ pub fn arg_decreasing(&self, arg: usize) -> bool {
+ match &self.0 {
+ FuncMonotonicityPartial::None => false,
+ FuncMonotonicityPartial::Increasing => false,
+ FuncMonotonicityPartial::Decreasing => true,
+ FuncMonotonicityPartial::Mixed(inner) => inner[arg].unwrap_or(false),
+ }
Review Comment:
Resolving as outdated.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] improve monotonicity api [datafusion]
Posted by "tinfoil-knight (via GitHub)" <gi...@apache.org>.
tinfoil-knight commented on code in PR #10117:
URL: https://github.com/apache/datafusion/pull/10117#discussion_r1592020936
##########
datafusion/expr/src/signature.rs:
##########
@@ -346,13 +346,81 @@ 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))
+ }
+
+ /// returns true if this function is monotonically increasing with respect to argument number arg
+ pub fn arg_increasing(&self, arg: usize) -> bool {
+ match &self.0 {
+ FuncMonotonicityPartial::None => false,
+ FuncMonotonicityPartial::Increasing => true,
+ FuncMonotonicityPartial::Decreasing => false,
+ FuncMonotonicityPartial::Mixed(inner) => inner[arg].unwrap_or(false),
+ }
+ }
+
+ /// returns true if this function is monotonically decreasing with respect to argument number arg
+ pub fn arg_decreasing(&self, arg: usize) -> bool {
+ match &self.0 {
+ FuncMonotonicityPartial::None => false,
+ FuncMonotonicityPartial::Increasing => false,
+ FuncMonotonicityPartial::Decreasing => true,
+ FuncMonotonicityPartial::Mixed(inner) => inner[arg].unwrap_or(false),
+ }
Review Comment:
Update:
I've removed the un-used `arg_increasing` & `arg_decreasing` public APIs.
For reference, the only place we're currently using FuncMonotonicity is this:
https://github.com/tinfoil-knight/arrow-datafusion/blob/53d9e30a7561c97492e47e3ee1679885b6c510e6/datafusion/physical-expr/src/scalar_function.rs#L250-L263
##########
datafusion/expr/src/signature.rs:
##########
@@ -346,13 +346,81 @@ 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))
+ }
+
+ /// returns true if this function is monotonically increasing with respect to argument number arg
+ pub fn arg_increasing(&self, arg: usize) -> bool {
+ match &self.0 {
+ FuncMonotonicityPartial::None => false,
+ FuncMonotonicityPartial::Increasing => true,
+ FuncMonotonicityPartial::Decreasing => false,
+ FuncMonotonicityPartial::Mixed(inner) => inner[arg].unwrap_or(false),
+ }
+ }
+
+ /// returns true if this function is monotonically decreasing with respect to argument number arg
+ pub fn arg_decreasing(&self, arg: usize) -> bool {
+ match &self.0 {
+ FuncMonotonicityPartial::None => false,
+ FuncMonotonicityPartial::Increasing => false,
+ FuncMonotonicityPartial::Decreasing => true,
+ FuncMonotonicityPartial::Mixed(inner) => inner[arg].unwrap_or(false),
+ }
Review Comment:
Update:
I've removed the unused `arg_increasing` & `arg_decreasing` public APIs.
For reference, the only place we're currently using FuncMonotonicity is this:
https://github.com/tinfoil-knight/arrow-datafusion/blob/53d9e30a7561c97492e47e3ee1679885b6c510e6/datafusion/physical-expr/src/scalar_function.rs#L250-L263
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] improve monotonicity api [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on code in PR #10117:
URL: https://github.com/apache/datafusion/pull/10117#discussion_r1590780255
##########
datafusion/expr/src/signature.rs:
##########
@@ -346,13 +346,81 @@ 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))
+ }
+
+ /// returns true if this function is monotonically increasing with respect to argument number arg
+ pub fn arg_increasing(&self, arg: usize) -> bool {
+ match &self.0 {
+ FuncMonotonicityPartial::None => false,
+ FuncMonotonicityPartial::Increasing => true,
+ FuncMonotonicityPartial::Decreasing => false,
+ FuncMonotonicityPartial::Mixed(inner) => inner[arg].unwrap_or(false),
+ }
+ }
+
+ /// returns true if this function is monotonically decreasing with respect to argument number arg
+ pub fn arg_decreasing(&self, arg: usize) -> bool {
+ match &self.0 {
+ FuncMonotonicityPartial::None => false,
+ FuncMonotonicityPartial::Increasing => false,
+ FuncMonotonicityPartial::Decreasing => true,
+ FuncMonotonicityPartial::Mixed(inner) => inner[arg].unwrap_or(false),
+ }
Review Comment:
These public API's don't check the size of the inner vector. Giving a larger index would panic the code. We can wrap it with a result here.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] improve monotonicity api [datafusion]
Posted by "tinfoil-knight (via GitHub)" <gi...@apache.org>.
tinfoil-knight commented on code in PR #10117:
URL: https://github.com/apache/datafusion/pull/10117#discussion_r1590734875
##########
datafusion/expr/src/signature.rs:
##########
@@ -346,13 +346,75 @@ 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, PartialEq)]
Review Comment:
Yeah. Makes sense. I've fixed this in d47b89664df19ebcd59ad2aae4b8f6d0572f08fd
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] improve monotonicity api [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10117:
URL: https://github.com/apache/datafusion/pull/10117#discussion_r1592934687
##########
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:
Something like this perhaps:
```rust
match (self, other) {
(Self::None, None) => true,
(Self::Increasing, Self::Increasing) => true,
(Self::Decreasing, Self::Decreasing) => true,
(Self::Partial(self_inner), Self::Partial(other_inner)) => self_inner == other_inner,
_ => false
}
```
##########
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 agree with @ozankabak it is confusing to have a way to convert back to `Vec<Option<bool>>` -- I think it would be easier to understand if all comparisons are done directly on `FuncMonotonicity` rather than converting to Vec<Option<Bool>>
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] improve monotonicity api [datafusion]
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on code in PR #10117:
URL: https://github.com/apache/datafusion/pull/10117#discussion_r1592962690
##########
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:
Yes. We also need to do configs like `(Self::Partial(inner), Self::Increasing)` but this is the idea 👍
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] improve monotonicity api [arrow-datafusion]
Posted by "tinfoil-knight (via GitHub)" <gi...@apache.org>.
tinfoil-knight commented on PR #10117:
URL: https://github.com/apache/arrow-datafusion/pull/10117#issuecomment-2062012465
@ozankabak Are you suggesting something like this?
```rust
use std::ops::Deref;
struct LimitedVector(Vec<Option<bool>>);
impl LimitedVector {
fn new(initial: Vec<Option<bool>>) -> Result<Self, &'static str> {
if initial.len() >= 2 {
Ok(LimitedVector(initial))
} else {
Err("Initial vector length must be 2 or more")
}
}
}
impl Deref for LimitedVector {
type Target = Vec<Option<bool>>;
fn deref(&self) -> &Self::Target {
&self.0
}
}
// Example usage:
fn main() {
let initial_vector = vec![Some(true), None];
match LimitedVector::new(initial_vector) {
Ok(limited_vector) => {
println!("Length: {}", limited_vector.len()); // Length: 2
println!("Elements: {:?}", *limited_vector); // Elements: [Some(true), None]
}
Err(err) => println!("Error: {}", err),
}
}
```
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org