Handle NaN in value comparers for double/float #22168
Conversation
|
Note: this does add the NaN check for all databases, including those which don't support it. If we think that's significant we can just have the comparers and leave it to providers to use them. |
|
@AndriySvyryd Any Cosmos changes needed here to handle NaN? |
Yes. But I don't think it's high enough of a priority |
|
In triage we mentioning possibly having core-level floating point mappings with the right comparer, but I see we have no mappings at all in core. I've put the comparers themselves in core, so it's trivial for any non-relational provider to use them if they want, am going to go ahead and merge as-is. |
| /// Initializes a new instance of the <see cref="DoubleValueComparer" /> class. | ||
| /// </summary> | ||
| public DoubleValueComparer() : base( | ||
| (x, y) => double.IsNaN(x) ? double.IsNaN(y) : x.Equals(y), |
There was a problem hiding this comment.
I see it's merged now, but
| (x, y) => double.IsNaN(x) ? double.IsNaN(y) : x.Equals(y), | |
| (x, y) => x.Equals(y), |
will yield the same result.
There was a problem hiding this comment.
Thanks, did not know that... Do you want to submit a cleanup PR for this (and the same for FloatValueComparer)?
There was a problem hiding this comment.
If the value comparer are just doing Equals then do we need a custom value comparer?
There was a problem hiding this comment.
Agree with @smitpatel. What's the actual issue we're trying to fix here?
There was a problem hiding this comment.
Here's what's happening.
- The result of
double.NaN == double.NaNis false, but the result ofdouble.NaN.Equals(double.NaN)is true. - Previous to this PR, floating point properties would get a default ValueComparer, whose implementation of Equals is Expression.Equal. That calls the "operator" version which returns false.
- This PR (and DoubleValueComparer / FloatValueComparer cleaned up NaN-handling #22212) fixes that by having a specialized value comparer which calls double.Equals instead of ==, which works correctly.
It may be better to fix this by having a DefaultDoubleValueComparer which is the same as DefaultValueComparer, but calls Equals instead of ==: it's cleaner and would work universally (e.g. Cosmos). @ajcvickers if that makes sense I'll submit that. If we don't do that, we probably want to clean up and remove double/float from DefaultValueComparer as it doesn't work correctly for NaN.
Fixes #22167