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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
