From bf256890e9647f958af8c2b0bbe619aeefb2ee54 Mon Sep 17 00:00:00 2001 From: Thiago Nunes Date: Mon, 15 Nov 2021 16:56:14 +1100 Subject: [PATCH 1/6] feat: NaNs in Values are considered equal Double.NaN and Float.NaN are considered equal when wrapped with the Float64 Value implementation class. --- .../src/main/java/com/google/cloud/spanner/Value.java | 3 ++- .../java/com/google/cloud/spanner/MutationTest.java | 6 ++++++ .../test/java/com/google/cloud/spanner/ValueTest.java | 10 ++++++++++ 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Value.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Value.java index 090c636b59..c5cc514421 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Value.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Value.java @@ -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; } @Override 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..2882926540 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 @@ -201,6 +201,12 @@ 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()); + // Deletes consider the key set. tester.addEqualityGroup(Mutation.delete("T1", KeySet.all())); tester.addEqualityGroup( diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/ValueTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/ValueTest.java index 7c2d7d0a95..198cf65517 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/ValueTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/ValueTest.java @@ -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); From 2ef2bf72430ab279a201ed4d14306236d031acaa Mon Sep 17 00:00:00 2001 From: Thiago Nunes Date: Mon, 15 Nov 2021 17:03:42 +1100 Subject: [PATCH 2/6] test: test float64array NaN equals and hashcode --- .../google/cloud/spanner/MutationTest.java | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) 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 2882926540..948839e6f1 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; @@ -207,6 +208,26 @@ public void equalsAndHashCode() { 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( From 112ba827ee87114fe340fcb0c2b59e6079c4046e Mon Sep 17 00:00:00 2001 From: Thiago Nunes Date: Tue, 16 Nov 2021 14:28:58 +1100 Subject: [PATCH 3/6] Revert "test: test float64array NaN equals and hashcode" This reverts commit 2ef2bf72430ab279a201ed4d14306236d031acaa. --- .../google/cloud/spanner/MutationTest.java | 21 ------------------- 1 file changed, 21 deletions(-) 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 948839e6f1..2882926540 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,7 +28,6 @@ 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; @@ -208,26 +207,6 @@ public void equalsAndHashCode() { 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( From 575b5fe8bcae6ad6bf386aa2d61e51ab3fedbbc0 Mon Sep 17 00:00:00 2001 From: Thiago Nunes Date: Tue, 16 Nov 2021 14:29:08 +1100 Subject: [PATCH 4/6] Revert "feat: NaNs in Values are considered equal" This reverts commit bf256890e9647f958af8c2b0bbe619aeefb2ee54. --- .../src/main/java/com/google/cloud/spanner/Value.java | 3 +-- .../java/com/google/cloud/spanner/MutationTest.java | 6 ------ .../test/java/com/google/cloud/spanner/ValueTest.java | 10 ---------- 3 files changed, 1 insertion(+), 18 deletions(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Value.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Value.java index c5cc514421..090c636b59 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Value.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Value.java @@ -1028,8 +1028,7 @@ void valueToString(StringBuilder b) { @Override boolean valueEquals(Value v) { - final Float64Impl float64Value = (Float64Impl) v; - return Double.isNaN(value) && Double.isNaN(float64Value.value) || float64Value.value == value; + return ((Float64Impl) v).value == value; } @Override 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 2882926540..53586641d4 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 @@ -201,12 +201,6 @@ 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()); - // Deletes consider the key set. tester.addEqualityGroup(Mutation.delete("T1", KeySet.all())); tester.addEqualityGroup( diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/ValueTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/ValueTest.java index 198cf65517..7c2d7d0a95 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/ValueTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/ValueTest.java @@ -155,16 +155,6 @@ 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); From 8b62b9a18fe40d6d74bace57d3bb7eb808bc4a01 Mon Sep 17 00:00:00 2001 From: Thiago Nunes Date: Tue, 16 Nov 2021 15:51:19 +1100 Subject: [PATCH 5/6] feat: loosen equality check for NaNs in mutations NaNs in mutations are considered equal --- .../com/google/cloud/spanner/Mutation.java | 25 +++++++- .../google/cloud/spanner/MutationTest.java | 62 +++++++++++++++++++ 2 files changed, 86 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..157d679c57 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,29 @@ public int hashCode() { return Objects.hash(operation, table, columns, values, keySet); } + 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(); } From e622306bc4eae03a745957b1c0991352fdc0d15c Mon Sep 17 00:00:00 2001 From: Thiago Nunes Date: Tue, 16 Nov 2021 21:55:54 +1100 Subject: [PATCH 6/6] feat: explain why we relaxed mutation equality --- .../src/main/java/com/google/cloud/spanner/Mutation.java | 7 +++++++ 1 file changed, 7 insertions(+) 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 157d679c57..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 @@ -358,6 +358,13 @@ 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;