Interval Arithmetic NegativeExpr Support#7804
Interval Arithmetic NegativeExpr Support#7804alamb merged 2 commits intoapache:mainfrom synnada-ai:feature/negative-expr-interval-arithmetic
Conversation
github-actions
Bot
added
physical-expr
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @berkaysynnada -- this PR looks really nice to me. I had a few comment suggestions, but otherwise this looks ready to merge to me.
I also had a question about improving the interface for analysis, but that can be done separately I think
| /// 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. |
There was a problem hiding this 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 impls 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
There was a problem hiding this comment.
fn supports_interval_analysis(&self) -> bool {
false
}
Adding such method in impl PhysicalExpr is what you're thinking? If it is so, I can quickly convert it to that form.
There was a problem hiding this 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
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 #7793 faster
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Since this is an incremental feature improvement and has no API change or anything I think will cause controversy I plan to merge it in after the CI checks pass
|
Thanks again @berkaysynnada |
3 participants
Which issue does this PR close?
Closes #.
Rationale for this change
If there is a NegativeExpr in a PhysicalExpr, during the pruning operations of joins, we encounter a "not implemented" error. This PR enables us to use NegativeExpr's in join predicates if there is a pruning operation.
What changes are included in this PR?
NegativeExpr's evaluate_bounds() and propagate_constraints() functions are implemented.
Are these changes tested?
Yes, unit tests of propagation over NegativeExpr's are added.
Are there any user-facing changes?