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.

Sign up for GitHub

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

Jump to bottom

feat: NaNs in Mutations are equal and have the same hashcode #1554

Merged
merged 6 commits into from Nov 16, 2021
Merged

feat: NaNs in Mutations are equal and have the same hashcode #1554

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bf25689
feat: NaNs in Values are considered equal
thiagotnunes Nov 15, 2021
2ef2bf7
test: test float64array NaN equals and hashcode
thiagotnunes Nov 15, 2021
112ba82
Revert "test: test float64array NaN equals and hashcode"
thiagotnunes Nov 16, 2021
575b5fe
Revert "feat: NaNs in Values are considered equal"
thiagotnunes Nov 16, 2021
8b62b9a
feat: loosen equality check for NaNs in mutations
thiagotnunes Nov 16, 2021
e622306
feat: explain why we relaxed mutation equality
thiagotnunes Nov 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion google-cloud-spanner/src/main/java/com/google/cloud/spanner/Value.java
View file
Open in desktop
Expand Up @@ -1028,7 +1028,8 @@ void valueToString(StringBuilder b) {

@Override
boolean valueEquals(Value v) {
return ((Float64Impl) v).value == value;
final Float64Impl float64Value = (Float64Impl) v;
return Double.isNaN(value) && Double.isNaN(float64Value.value) || float64Value.value == value;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we want to deviate from the IEEE 754 standard here? According to that (and also according to the Cloud Spanner backend) NaN == NaN should return false.

The following SQL statement for example returns false:

select (IEEE_DIVIDE(0.0, 0.0) = IEEE_DIVIDE(0.0, 0.0)) as b

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a good point and I considered this.

The issue that we have is that on import / export in Apache Beam (see here) we compare mutations before committing. Since we allow for PGNumerics that allow for such a value, import / export is failing and that is why we are loosening the restriction here.

If you feel strong that we should not make changes here, we could try and explore a change in Apache Beam itself, but I guess we would just be re-implementing equality checks over there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is maybe a slightly hypothetical problem, but I don't think we should implement this in Value. However, it might make sense to implement it in Mutation (if doable). That is:

If someone executes the following query:

SELECT FloatValue1, FloatValue2
FROM SomeTable
WHERE FloatValue1 != FloatValue2

Then resultSet.getValue("FloatValue1).equals(resultSet.getValue("FloatValue2")) should return false for all rows.

On the other hand, for Mutations the difference between two NaN values is void, as they will both set the value of a row to the same. So:

      Mutation mutation1 = Mutation.newInsertBuilder("SomeTable")
          .set("FloatValue").to(Value.float64(Double.NaN))
          .build();
      Mutation mutation2 = Mutation.newInsertBuilder("SomeTable")
          .set("FloatValue").to(Value.float64(Double.NaN))
          .build();
      // The following could (should?) be true as both mutations will have the same effect.
      assertTrue(mutation1.equals(mutation2));

Would that solve the problem in Beam?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this would solve the problem for us. In addition, is it possible to see Double.NaN in PgNumeric Value?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In addition, is it possible to see Double.NaN in PgNumeric Value?

Yes, that is possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the implementation to check for NaNs only in mutations

Copy link
Member

Choose a reason for hiding this comment

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

In addition, is it possible to see Double.NaN in PgNumeric Value?

Yes, that is possible.

Hi @thiagotnunes , is this case considered? I see we only try to cast the type to float64 but I am not an expert on client libraries. Ignore me if I am wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey Michael, since this fix is on the public repository we don't have pg numeric here yet. I will make the necessary changes in the other repository as well.

Copy link
Member

Choose a reason for hiding this comment

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

oh, yes. Sorry, you are absolutely right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries!

}

@Override
Expand Down
27 changes: 27 additions & 0 deletions google-cloud-spanner/src/test/java/com/google/cloud/spanner/MutationTest.java
View file
Open in desktop
Expand Up @@ -28,6 +28,7 @@
import com.google.common.testing.EqualsTester;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import org.hamcrest.Matcher;
import org.hamcrest.MatcherAssert;
Expand Down Expand Up @@ -201,6 +202,32 @@ public void equalsAndHashCode() {
Mutation.newInsertBuilder("T1").set("C").to("V").build(),
Mutation.newInsertBuilder("T1").set("C").to("V").build());

// Test NaNs are equal
tester.addEqualityGroup(
Mutation.newInsertBuilder("T1").set("C").to(Double.NaN).build(),
Mutation.newInsertBuilder("T1").set("C").to(Double.NaN).build(),
Mutation.newInsertBuilder("T1").set("C").to(Float.NaN).build());

tester.addEqualityGroup(
Mutation.newInsertBuilder("T1").set("C").toFloat64Array(new double[] {Double.NaN}).build(),
Mutation.newInsertBuilder("T1").set("C").toFloat64Array(new double[] {Float.NaN}).build(),
Mutation.newInsertBuilder("T1")
.set("C")
.toFloat64Array(new double[] {Double.NaN}, 0, 1)
.build(),
Mutation.newInsertBuilder("T1")
.set("C")
.toFloat64Array(new double[] {Float.NaN}, 0, 1)
.build(),
Mutation.newInsertBuilder("T1")
.set("C")
.toFloat64Array(Collections.singletonList(Double.NaN))
.build(),
Mutation.newInsertBuilder("T1")
.set("C")
.toFloat64Array(Collections.singletonList((double) Float.NaN))
.build());

// Deletes consider the key set.
tester.addEqualityGroup(Mutation.delete("T1", KeySet.all()));
tester.addEqualityGroup(
Expand Down
10 changes: 10 additions & 0 deletions google-cloud-spanner/src/test/java/com/google/cloud/spanner/ValueTest.java
View file
Open in desktop
Expand Up @@ -155,6 +155,16 @@ public void float64Wrapper() {
assertThat(v.toString()).isEqualTo("1.23");
}

@Test
public void float64WrapperNaN() {
final Value value = Value.float64(Double.NaN);
assertEquals(Type.float64(), value.getType());
assertFalse(value.isNull());
assertEquals(Value.float64(Double.NaN), value);
assertEquals(Value.float64(Float.NaN), value);
assertEquals("NaN", value.toString());
}

@Test
public void float64WrapperNull() {
Value v = Value.float64(null);
Expand Down