alamb commented on code in PR #7804:
URL: https://github.com/apache/arrow-datafusion/pull/7804#discussion_r1356768508
##########
datafusion/physical-expr/src/expressions/negative.rs:
##########
@@ -105,6 +104,30 @@ impl PhysicalExpr for NegativeExpr {
self.hash(&mut s);
}
+ /// Given the child interval of a NegativeExpr, it calculates the
NegativeExpr's interval.
+ /// It replaces the upper and lower bounds after multiplying them with -1.
+ /// Ex: (a, b] => [-b, a)
Review Comment:
I think it is `-a` on the upper bound
```suggestion
/// Ex: `(a, b] => [-b, -a)`
```
##########
datafusion/physical-expr/src/intervals/utils.rs:
##########
@@ -37,16 +36,32 @@ const DT_MS_MASK: i64 = 0xFFFF_FFFF;
/// Currently, we do not support all [`PhysicalExpr`]s for interval
calculations.
/// We do not support every type of [`Operator`]s either. Over time, this check
/// will relax as more types of `PhysicalExpr`s and `Operator`s are supported.
-/// Currently, [`CastExpr`], [`BinaryExpr`], [`Column`] and [`Literal`] are
supported.
-pub fn check_support(expr: &Arc<dyn PhysicalExpr>) -> bool {
+/// Currently, [`CastExpr`], [`NegativeExpr`], [`BinaryExpr`], [`Column`] and
[`Literal`] are supported.
Review Comment:
While reviewing this PR I noticed that to add support for interval analysis
to a `PhysicalExpr` there are at least two places that need to be changed. The
`impl PhysicalExpr` and `check_support`. Besides making it somewhat harder to
add interval analysis to new expression types, I also think it means that
custom `impl`s of `PhysicalExpr` can't be used in interval analysis.
What do you think about making it possible for custom PhysicalExprs to use
interval analysis (likely by moving some part of this check into the `impl
PhysicalExpr`)? I can file a follow on ticket if you like
##########
datafusion/physical-expr/src/expressions/negative.rs:
##########
@@ -105,6 +104,30 @@ impl PhysicalExpr for NegativeExpr {
self.hash(&mut s);
}
+ /// Given the child interval of a NegativeExpr, it calculates the
NegativeExpr's interval.
+ /// It replaces the upper and lower bounds after multiplying them with -1.
+ /// Ex: (a, b] => [-b, a)
+ fn evaluate_bounds(&self, children: &[&Interval]) -> Result<Interval> {
+ Ok(Interval::new(
+ children[0].upper.negate()?,
+ children[0].lower.negate()?,
+ ))
+ }
+
+ /// Updates the child interval of a NegativeExpr by intersecting the
original
Review Comment:
```suggestion
/// Returns a new [`Interval`] of a NegativeExpr that has the existing
`interval` given that
/// given the input interval is known to be `children`
```
##########
datafusion/physical-expr/src/intervals/interval_aritmetic.rs:
##########
@@ -87,6 +86,13 @@ impl IntervalBound {
.map(|value| IntervalBound::new(value, self.open))
}
+ pub fn negate(&self) -> Result<IntervalBound> {
Review Comment:
Could we possible add some doc comments with an example, like
```suggestion
/// returns a new bound with a negated value, if any, and the same
open/closed.
/// For example negating `[5` would return `[-5`.
pub fn negate(&self) -> Result<IntervalBound> {
```
--
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]
