Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Interval Arithmetic NegativeExpr Support #7804
Interval Arithmetic NegativeExpr Support #7804
Changes from all commits
88356f0
c98e27b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. Theimpl PhysicalExpr
andcheck_support
. Besides making it somewhat harder to add interval analysis to new expression types, I also think it means that customimpl
s ofPhysicalExpr
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 likeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that would be one way. Another potential might be to change the signature of
evaluate_bounds
to anOption
so that multiple functions didn't need to kept in syncTo 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