-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Implement Eq trait for Expr and nested types #3381
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3381 +/- ##
==========================================
- Coverage 85.58% 85.57% -0.01%
==========================================
Files 296 296
Lines 54179 54179
==========================================
- Hits 46367 46366 -1
- Misses 7812 7813 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
I think this looks good to me. Thanks @jdye64
@@ -1430,6 +1430,8 @@ impl PartialEq for Subquery { | |||
} | |||
} | |||
|
|||
impl Eq for Subquery {} |
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.
well I suppose Eq
is satisfied given that PartialEq always returns false the requirements
https://doc.rust-lang.org/std/cmp/trait.Eq.html
are satisfied in a strange sort of way
@@ -56,6 +56,8 @@ impl PartialEq for AggregateUDF { | |||
} | |||
} | |||
|
|||
impl Eq for AggregateUDF {} |
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.
👍
Benchmark runs are scheduled for baseline = 43e2d91 and contender = e6d1364. e6d1364 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Rationale for this change
Expr is currently not compatible with the
HashSet
container because it does not implement tostd::cmp::Eq
trait. This PR implements that trait forExpr
and any nested components