ozankabak commented on issue #8078:
URL:
https://github.com/apache/arrow-datafusion/issues/8078#issuecomment-1810981010
I like your proposal for the new type (reproduced below).
```rust
pub enum Precision<T: Debug + Clone + PartialEq + Eq + PartialOrd> {
/// The exact value is known
Exact(T),
/// The exact value is not known, but the real value is **known** to be
within
/// the specified range: `lower <= value <= upper` and furthermore is
estimated
/// to be `estimate`. However, the real value may fall anywhere in the
range
/// TODO: we could use `Interval` here instead, which could represent
more complex cases (like
/// open/closed bounds rather than using T::MIN/T::MAX)
Bounded { lower: T, estimate: T upper: T},
/// Nothing is known about the value
#[default]
Absent,
}
```
A few questions and remarks:
1. Do we need the `Exact` variant anymore? It seems no. If `lower`, `upper`
and `estimate` values are the same, it would be an exact estimate. This struct
can implement a method `is_exact` to check this.
2. Do we need the`Absent` variant anymore? I think yes. We may want to
differentiate between "no information" and "all possible values" (i.e. [MIN,
MAX]) in certain cases.
3. Should we use `Interval` in the bounded variant? I think yes, but we
should probably start off without it and migrate when we move the interval
library outside of the physical expression crate (this week hopefully).
4. Should we use the name `Precision` still? IMO it is not a good name for
this. We are making estimates after all. I think we should use the right name
for what we are doing and use the name `Estimate`.
Assuming these tweaks, the type would look like:
```rust
pub enum Estimate<T: Debug + Clone + PartialEq + Eq + PartialOrd> {
Range { lower: T, value: T, upper: T },
/// Nothing is known about the value
#[default]
Absent,
}
```
or with `ScalarValue`, we'd have:
```rust
pub enum Estimate {
Range { lower: ScalarValue, value: ScalarValue, upper: ScalarValue },
/// Nothing is known about the value
#[default]
Absent,
}
```
Once `Interval` moves out of `physical-expr`, we can use:
```rust
pub enum Estimate {
Range { bounds: Interval, value: ScalarValue },
/// Nothing is known about the value
#[default]
Absent,
}
```
--
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]
