improve monotonicity api #10117

tinfoil-knight · 2024-04-17T14:31:53Z

Which issue does this PR close?

Closes #9879 .

Rationale for this change

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?

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?

By existing tests.

Are there any user-facing changes?

No.

ozankabak · 2024-04-17T16:57:44Z

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 and preserves uniqueness. Maybe a struct with an inner Vec<Option<bool>> and methods that define an API structuring how one interacts with it.

@alamb

alamb · 2024-04-19T11:58:15Z

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 so much would be to make the enum private and only allow construction through APIs

Maybe something like

// 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.

ozankabak · 2024-04-19T12:28:39Z

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.

alamb · 2024-04-21T11:16:00Z

@tinfoil-knight -- are you interested in completing this PR with the proposed API updates?

tinfoil-knight · 2024-04-21T11:17:41Z

@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.

alamb · 2024-04-21T11:24:06Z

@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!

alamb · 2024-04-21T11:24:21Z

Converting to draft as it is not waiting on feedback

This reverts commit fc0db97.

berkaysynnada · 2024-05-06T07:24:55Z

datafusion/expr/src/signature.rs

-/// - `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)]


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)].

Yeah. Makes sense. I've fixed this in d47b896 (#10117)

berkaysynnada · 2024-05-06T09:49:11Z

datafusion/expr/src/signature.rs

+            FuncMonotonicityPartial::Increasing => false,
+            FuncMonotonicityPartial::Decreasing => true,
+            FuncMonotonicityPartial::Mixed(inner) => inner[arg].unwrap_or(false),
+        }


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.

These APIs aren't being used anywhere. I think we can just remove them.

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

ozankabak

Left my comments for things to fix/improve. Thanks @tinfoil-knight

ozankabak · 2024-05-07T11:30:47Z

datafusion/expr/src/signature.rs

+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(),
+        }
+    }
+}


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.

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>

ozankabak · 2024-05-07T11:32:30Z

datafusion/expr/src/signature.rs

+
+impl PartialEq for FuncMonotonicity {
+    fn eq(&self, other: &Self) -> bool {
+        Into::<Vec<Option<bool>>>::into(self) == Into::<Vec<Option<bool>>>::into(other)


Given my comment on From, you will need explicit logic here.

Something like this perhaps:

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 }

Yes. We also need to do configs like (Self::Partial(inner), Self::Increasing) but this is the idea 👍

Maybe that type of comparsion belongs in something other than equal -- perhaps

impl FuncMonotonicity { fn matches(&self, &other) -> bool { ... } }

So we can separate the equality check from the both sides match 🤔

ozankabak · 2024-05-07T11:33:51Z

datafusion/physical-expr/src/scalar_function.rs

+    let monotonicity_vec: Vec<Option<bool>> = func.into();
+
+    monotonicity_vec.iter().zip(arg_orderings).fold(


This logic will unfortunately get a little complicated given the issue with From.

ozankabak · 2024-05-07T11:40:47Z

datafusion/expr/src/signature.rs

+#[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))
+    }
+}


Can we use just one data type? Consider:

/// 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>>), }

alamb

Thanks @tinfoil-knight and @ozankabak -- the PR is making progress (and it is a good example of iterative API design). Thank you for sticking with it

alamb · 2024-05-07T18:57:16Z

datafusion/expr/src/signature.rs

+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(),
+        }
+    }
+}


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>

alamb · 2024-05-07T18:59:04Z

datafusion/expr/src/signature.rs

+
+impl PartialEq for FuncMonotonicity {
+    fn eq(&self, other: &Self) -> bool {
+        Into::<Vec<Option<bool>>>::into(self) == Into::<Vec<Option<bool>>>::into(other)


Something like this perhaps:

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 }

tinfoil-knight

Left a few comments to clarify things.

tinfoil-knight · 2024-05-09T20:16:01Z

datafusion/physical-expr/src/scalar_function.rs

+            let arg_monotonicity: Option<bool> = match func {
+                FuncMonotonicity::None => None,
+                FuncMonotonicity::Increasing => Some(true),
+                FuncMonotonicity::Decreasing => Some(false),
+                FuncMonotonicity::Mixed(inner_vec) => {


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.

I agree the conversion feels confusing --

I do think that the conversion from FuncMonotonicity to Option 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

Maybe we could add something like

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

tinfoil-knight · 2024-05-09T20:25:30Z

datafusion/expr/src/signature.rs

+}
+
+impl FuncMonotonicity {
+    pub fn matches(&self, other: &Self) -> bool {


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.

alamb · 2024-05-10T14:44:46Z

Thanks @tinfoil-knight -- I will check this out later today

alamb

Thanks @tinfoil-knight

alamb · 2024-05-10T20:46:05Z

datafusion/physical-expr/src/scalar_function.rs

+            let arg_monotonicity: Option<bool> = match func {
+                FuncMonotonicity::None => None,
+                FuncMonotonicity::Increasing => Some(true),
+                FuncMonotonicity::Decreasing => Some(false),
+                FuncMonotonicity::Mixed(inner_vec) => {


I agree the conversion feels confusing --

I do think that the conversion from FuncMonotonicity to Option 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

alamb · 2024-05-13T09:47:03Z

datafusion/physical-expr/src/scalar_function.rs

+            let arg_monotonicity: Option<bool> = match func {
+                FuncMonotonicity::None => None,
+                FuncMonotonicity::Increasing => Some(true),
+                FuncMonotonicity::Decreasing => Some(false),
+                FuncMonotonicity::Mixed(inner_vec) => {


Maybe we could add something like

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

alamb · 2024-05-21T10:43:57Z

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.

berkaysynnada · 2024-05-21T10:53:23Z

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.

tinfoil-knight · 2024-05-21T10:58:38Z

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.

alamb · 2024-05-22T21:01:02Z

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

improve monotonicity api

fc0db97

github-actions bot added logical-expr Logical plan and expressions physical-expr Physical Expressions labels Apr 17, 2024

This comment was marked as outdated.

Sign in to view

alamb marked this pull request as draft April 21, 2024 11:24

tinfoil-knight added 3 commits May 5, 2024 19:45

revert "improve monotonicity api"

349e12f

This reverts commit fc0db97.

Merge branch 'main' into 9879-monotonicity-api

cbc08c1

implement new api

b77eabb

tinfoil-knight marked this pull request as ready for review May 5, 2024 20:31

berkaysynnada reviewed May 6, 2024

View reviewed changes

implement PartialEq for FuncMonotonicity

d47b896

berkaysynnada reviewed May 6, 2024

View reviewed changes

alamb mentioned this pull request May 6, 2024

DataFusion weekly project plan (Andrew Lamb) - May 6, 2024 #10395

Closed

7 tasks

remove unused apis

1266ed8

tinfoil-knight requested a review from berkaysynnada May 7, 2024 08:27

fix merge conflict

8379e7c

tinfoil-knight force-pushed the 9879-monotonicity-api branch from 53d9e30 to 8379e7c Compare May 7, 2024 09:14

ozankabak reviewed May 7, 2024

View reviewed changes

alamb reviewed May 7, 2024

View reviewed changes

implement matches, fix PartialEq for monotoncity

ea21acd

tinfoil-knight marked this pull request as draft May 9, 2024 12:13

fix clippy nit

e5f72bb

tinfoil-knight commented May 9, 2024

View reviewed changes

tinfoil-knight marked this pull request as ready for review May 9, 2024 20:33

tinfoil-knight requested a review from alamb May 9, 2024 20:34

alamb reviewed May 13, 2024

View reviewed changes

alamb mentioned this pull request May 14, 2024

PhysicalExpr Orderings with Range Information #10504

Merged

alamb closed this May 21, 2024

tinfoil-knight deleted the 9879-monotonicity-api branch May 21, 2024 10:54

		let monotonicity_vec: Vec<Option<bool>> = func.into();

		monotonicity_vec.iter().zip(arg_orderings).fold(

improve monotonicity api #10117

improve monotonicity api #10117

Conversation

tinfoil-knight commented Apr 17, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

ozankabak commented Apr 17, 2024 • edited

This comment was marked as outdated.

alamb commented Apr 19, 2024

ozankabak commented Apr 19, 2024

alamb commented Apr 21, 2024

tinfoil-knight commented Apr 21, 2024

alamb commented Apr 21, 2024

alamb commented Apr 21, 2024

Choose a reason for hiding this comment

tinfoil-knight May 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tinfoil-knight May 7, 2024 • edited

Choose a reason for hiding this comment

ozankabak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tinfoil-knight left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented May 10, 2024

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented May 21, 2024

berkaysynnada commented May 21, 2024

tinfoil-knight commented May 21, 2024

alamb commented May 22, 2024

ozankabak commented Apr 17, 2024 •

edited

tinfoil-knight May 6, 2024 •

edited

tinfoil-knight May 7, 2024 •

edited