During expression equality, check for new ordering information #10434
Conversation
|
@suremarc, I would appreciate your review if you have time |
There was a problem hiding this comment.
Looks good to me -- thank you @mustafasrepo
cc @suremarc -- can you also give this a review if you have a chance?
Co-authored-by: Mehmet Ozan Kabak <ozankabak@gmail.com>
|
Given this is a fairly small piece of code, two reviewers already took a look and all tests pass, I will go ahead and merge it. If there is any issue we discover later on, or @suremarc points out something we are missing, we will address with a follow-on PR. Thanks all |
|
Sorry @alamb @mustafasrepo, I completely missed this. My GH notifications tend to get drowned out by stuff from work, but I've discovered my personal email doesn't have this issue. I will check it every day from now on. As for this PR, looks great and I'm glad we don't have to declare equivalences backwards ( |
* discover new orderings when constants are added * more comments * reduce nesting + describe argument * lint?
* discover new orderings when constants are added * more comments * reduce nesting + describe argument * lint?
…e#10434) * Move ordering discovery to equality check * Minor changes * Minor changes * Apply suggestions from code review Co-authored-by: Mehmet Ozan Kabak <ozankabak@gmail.com> --------- Co-authored-by: Mehmet Ozan Kabak <ozankabak@gmail.com>
* discover new orderings when constants are added * more comments * reduce nesting + describe argument * lint?
* discover new orderings when constants are added * more comments * reduce nesting + describe argument * lint?
Which issue does this PR close?
Closes #.
Rationale for this change
See issue #9812 for rationale.
What changes are included in this PR?
When an equality expression is added to the
EquivalenceProperties. In light of this information, we can deduce new information about ordering and constantness. This PR adds this support.With the PR #9813 we have this support, in terms of behaviour. However, current solution doesn't update state and cannot generalize well.
Are these changes tested?
There is an additional TestCase, that showcases increased coverage with this implementation (That test case fails in main branch).
Are there any user-facing changes?