alamb commented on code in PR #7804:
URL: https://github.com/apache/arrow-datafusion/pull/7804#discussion_r1356833303
##########
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:
> Adding such method in impl PhysicalExpr is what you're thinking? If it is
so, I can quickly convert it to that form.
Yes that would be one way. Another potential might be to change the
signature of `evaluate_bounds` to an `Option` so that multiple functions didn't
need to kept in sync
```rust
fn evaluate_bounds(&self, _children: &[&Interval]) ->
Result<Option<Interval>> {
Ok(None)
}
```
To be clear, I think we should merge this PR as is -- maybe with
documentation updates -- and then make this change as a follow on PR. That way
we will unblock https://github.com/apache/arrow-datafusion/pull/7793 faster
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
