From 91a18fc09a2034959758d38f1278dc93128c7622 Mon Sep 17 00:00:00 2001 From: Thiago Nunes Date: Wed, 17 Nov 2021 08:49:08 +1100 Subject: [PATCH] feat: NaNs in Mutations are equal and have the same hashcode (#1554) * feat: loosen equality check for NaNs in mutations NaNs in mutations are considered equal * feat: explain why we relaxed mutation equality --- .../com/google/cloud/spanner/Mutation.java | 32 +++++++++- .../google/cloud/spanner/MutationTest.java | 62 +++++++++++++++++++ 2 files changed, 93 insertions(+), 1 deletion(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Mutation.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Mutation.java index 89d989b1c3..dd04365356 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Mutation.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Mutation.java @@ -349,7 +349,7 @@ public boolean equals(Object o) { return operation == that.operation && Objects.equals(table, that.table) && Objects.equals(columns, that.columns) - && Objects.equals(values, that.values) + && areValuesEqual(values, that.values) && Objects.equals(keySet, that.keySet); } @@ -358,6 +358,36 @@ public int hashCode() { return Objects.hash(operation, table, columns, values, keySet); } + /** + * We are relaxing equality values here, making sure that Double.NaNs and Float.NaNs are equal to + * each other. This is because our Cloud Spanner Import / Export template in Apache Beam uses the + * mutation equality to check for modifications before committing. We noticed that when NaNs where + * used the template would always indicate a modification was present, when it turned out not to + * be the case. For more information see b/206339664. + */ + private boolean areValuesEqual(List values, List otherValues) { + if (values == null && otherValues == null) { + return true; + } else if (values == null || otherValues == null) { + return false; + } else if (values.size() != otherValues.size()) { + return false; + } else { + for (int i = 0; i < values.size(); i++) { + final Value value = values.get(i); + final Value otherValue = otherValues.get(i); + if (!value.equals(otherValue) && (!isNaN(value) || !isNaN(otherValue))) { + return false; + } + } + return true; + } + } + + private boolean isNaN(Value value) { + return !value.isNull() && value.getType() == Type.float64() && Double.isNaN(value.getFloat64()); + } + static void toProto(Iterable mutations, List out) { Mutation last = null; // The mutation currently being built. diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/MutationTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/MutationTest.java index 53586641d4..372b2de303 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/MutationTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/MutationTest.java @@ -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; @@ -206,6 +207,67 @@ public void equalsAndHashCode() { tester.addEqualityGroup( Mutation.delete("T1", KeySet.singleKey(Key.of("k"))), Mutation.delete("T1", Key.of("k"))); + // Test NaNs + tester.addEqualityGroup( + Mutation.newInsertBuilder("T1").set("C").to(Double.NaN).build(), + Mutation.newInsertBuilder("T1").set("C").to(Value.float64(Double.NaN)).build(), + Mutation.newInsertBuilder("T1").set("C").to(Float.NaN).build(), + Mutation.newInsertBuilder("T1").set("C").to(Value.float64(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(), + Mutation.newInsertBuilder("T1") + .set("C") + .to(Value.float64Array(new double[] {Double.NaN})) + .build(), + Mutation.newInsertBuilder("T1") + .set("C") + .to(Value.float64Array(new double[] {Float.NaN})) + .build(), + Mutation.newInsertBuilder("T1") + .set("C") + .to(Value.float64Array(new double[] {Double.NaN}, 0, 1)) + .build(), + Mutation.newInsertBuilder("T1") + .set("C") + .to(Value.float64Array(new double[] {Float.NaN}, 0, 1)) + .build(), + Mutation.newInsertBuilder("T1") + .set("C") + .to(Value.float64Array(Collections.singletonList(Double.NaN))) + .build(), + Mutation.newInsertBuilder("T1") + .set("C") + .to(Value.float64Array(Collections.singletonList((double) Float.NaN))) + .build()); + // Test NaNs and nulls + tester.addEqualityGroup( + Mutation.newInsertBuilder("T1") + .set("C") + .toFloat64Array(Arrays.asList(null, Double.NaN)) + .build(), + Mutation.newInsertBuilder("T1") + .set("C") + .toFloat64Array(Arrays.asList(null, (double) Float.NaN)) + .build()); + tester.testEquals(); }