-
Notifications
You must be signed in to change notification settings - Fork 34
Fix tuple equality with null values #327
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
Conversation
513c539 to
2f61fef
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #327 +/- ##
=======================================
Coverage 87.23% 87.24%
=======================================
Files 52 52
Lines 4537 4538 +1
Branches 1279 1279
=======================================
+ Hits 3958 3959 +1
Misses 369 369
Partials 210 210 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
hossenlopp
left a comment
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.
Looks good and I don't foresee any issues with this after a rebase.
Previously, if tuples had the same fields, and at least one field had a null value on one tuple and a non-null value on the other, then: * if the null/non-null field came after a field with different values, it returned false (which is correct) * if the null/non-null field came before a field with different values, it returned null (which is incorrect) Now it will return the correct answer regardless of where the null/non-null field is.
3a4c785 to
a752e2b
Compare
elsaperelli
left a comment
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.
Lgtm!
hossenlopp
left a comment
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.
A few more suggested tests from when I attempted to "break" the code.
Co-authored-by: hossenlopp <hossenlopp@mitre.org>
Co-authored-by: hossenlopp <hossenlopp@mitre.org>
|
@hossenlopp - I accepted your suggestions so this is ready for your approval! |
hossenlopp
left a comment
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.
👍
Previously, if tuples had the same fields, and at least one field had a null value on one tuple and a non-null value on the other, then:
Now it will return the correct answer regardless of where the null/non-null field is.
NOTE: This PR does not fix npm audit issues or the codecov issue, as those have already been fixed in #326. As a result, this should be merged after #326.
Submitter:
npm run test:plusto run tests, lint, and prettier)cql4browsers.jsbuilt withnpm run build:browserifyif source changed.Reviewer:
Name: