Skip to content
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

Handle NaN in value comparers for double/float #22168

Merged
merged 1 commit into from
Aug 25, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/EFCore.Relational/Storage/DoubleTypeMapping.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Data;
using System.Globalization;
using Microsoft.EntityFrameworkCore.ChangeTracking;
using JetBrains.Annotations;

namespace Microsoft.EntityFrameworkCore.Storage
Expand All @@ -27,7 +28,10 @@ public class DoubleTypeMapping : RelationalTypeMapping
public DoubleTypeMapping(
[NotNull] string storeType,
DbType? dbType = null)
: base(storeType, typeof(double), dbType)
: base(
new RelationalTypeMappingParameters(
new CoreTypeMappingParameters(typeof(double), comparer: new DoubleValueComparer()),
storeType, StoreTypePostfix.None, dbType, unicode: false, size: null, fixedLength: false, precision: null, scale: null))
{
}

Expand Down
6 changes: 5 additions & 1 deletion src/EFCore.Relational/Storage/FloatTypeMapping.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Data;
using System.Globalization;
using JetBrains.Annotations;
using Microsoft.EntityFrameworkCore.ChangeTracking;

namespace Microsoft.EntityFrameworkCore.Storage
{
Expand All @@ -27,7 +28,10 @@ public class FloatTypeMapping : RelationalTypeMapping
public FloatTypeMapping(
[NotNull] string storeType,
DbType? dbType = null)
: base(storeType, typeof(float), dbType)
: base(
new RelationalTypeMappingParameters(
new CoreTypeMappingParameters(typeof(float), comparer: new FloatValueComparer()),
storeType, StoreTypePostfix.None, dbType, unicode: false, size: null, fixedLength: false, precision: null, scale: null))
{
}

Expand Down
4 changes: 2 additions & 2 deletions src/EFCore/ChangeTracking/ArrayStructuralComparer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ namespace Microsoft.EntityFrameworkCore.ChangeTracking
{
/// <summary>
/// <para>
/// Specifies value snapshotting and comparison for arrays where each element is compared
/// a new array is constructed when snapshotting.
/// Specifies value comparison for arrays where each element pair is compared.
/// A new array is constructed when snapshotting.
/// </para>
/// </summary>
/// <typeparam name="TElement"> The array element type. </typeparam>
Expand Down
25 changes: 25 additions & 0 deletions src/EFCore/ChangeTracking/DoubleValueComparer.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Linq.Expressions;
using JetBrains.Annotations;

namespace Microsoft.EntityFrameworkCore.ChangeTracking
{
/// <summary>
/// Defines value comparison for <see cref="double" /> which correctly takes <see cref="double.NaN" />
/// into account.
/// </summary>
public class DoubleValueComparer : ValueComparer<double>
{
/// <summary>
/// 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),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see it's merged now, but

Suggested change
(x, y) => double.IsNaN(x) ? double.IsNaN(y) : x.Equals(y),
(x, y) => x.Equals(y),

will yield the same result.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, did not know that... Do you want to submit a cleanup PR for this (and the same for FloatValueComparer)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, will send one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--> #22212

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the value comparer are just doing Equals then do we need a custom value comparer?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @smitpatel. What's the actual issue we're trying to fix here?

Copy link
Member Author

@roji roji Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's what's happening.

  • The result of double.NaN == double.NaN is false, but the result of double.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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@roji Yes, do that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@roji maybe it makes sense to close #22212 and incorporate that change into your coming PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gfoidl, submitted #22242 to clean this up.

d => d.GetHashCode())
{
}
}
}
25 changes: 25 additions & 0 deletions src/EFCore/ChangeTracking/FloatValueComparer.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Linq.Expressions;
using JetBrains.Annotations;

namespace Microsoft.EntityFrameworkCore.ChangeTracking
{
/// <summary>
/// Defines value comparison for <see cref="float" /> which correctly takes <see cref="float.NaN" />
/// into account.
/// </summary>
public class FloatValueComparer : ValueComparer<float>
{
/// <summary>
/// Initializes a new instance of the <see cref="FloatValueComparer" /> class.
/// </summary>
public FloatValueComparer() : base(
(x, y) => float.IsNaN(x) ? float.IsNaN(y) : x.Equals(y),
d => d.GetHashCode())
{
}
}
}
20 changes: 20 additions & 0 deletions test/EFCore.Relational.Tests/Storage/RelationalTypeMappingTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -529,6 +529,26 @@ public virtual void UShort_literal_generated_correctly()
Test_GenerateSqlLiteral_helper(typeMapping, ushort.MaxValue, "65535");
}

[ConditionalFact]
public virtual void Double_value_comparer_handles_NaN()
{
var typeMapping = new DoubleTypeMapping("double precision", DbType.Double);

Assert.True(typeMapping.Comparer.Equals(3.0, 3.0));
Assert.True(typeMapping.Comparer.Equals(double.NaN, double.NaN));
Assert.False(typeMapping.Comparer.Equals(3.0, double.NaN));
}

[ConditionalFact]
public virtual void Float_value_comparer_handles_NaN()
{
var typeMapping = new FloatTypeMapping("float", DbType.Single);

Assert.True(typeMapping.Comparer.Equals(3.0f, 3.0f));
Assert.True(typeMapping.Comparer.Equals(float.NaN, float.NaN));
Assert.False(typeMapping.Comparer.Equals(3.0f, float.NaN));
}

[ConditionalFact]
public virtual void Primary_key_type_mapping_is_picked_up_by_FK_without_going_through_store_type()
{
Expand Down