Update the hashCode logic to account for NaN equality

Author: arawind

google-cloud-spanner/src/main/java/com/google/cloud/spanner/Mutation.java

@@ -364,6 +364,8 @@ public int hashCode() {
    * 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.
+   *
+   * <p>Similar change is being done while calculating `Value.hashCode()`.
    */
   private boolean areValuesEqual(List<Value> values, List<Value> otherValues) {
     if (values == null && otherValues == null) {
@@ -386,8 +388,10 @@ private boolean areValuesEqual(List<Value> values, List<Value> otherValues) {
 
   private boolean isNaN(Value value) {
     return !value.isNull()
-        && ((value.getType().equals(Type.float64()) && Double.isNaN(value.getFloat64()))
-            || (value.getType().equals(Type.float32()) && Float.isNaN(value.getFloat32())));
+        && ((value.getType().equals(Type.float64())
+                && (Double.isNaN(value.getFloat64()) || Float.isNaN((float) value.getFloat64())))
+            || (value.getType().equals(Type.float32())
+                && (Float.isNaN(value.getFloat32()) || Double.isNaN(value.getFloat32()))));
   }
 
   static void toProto(Iterable<Mutation> mutations, List<com.google.spanner.v1.Mutation> out) {

google-cloud-spanner/src/main/java/com/google/cloud/spanner/Value.java

@@ -1375,9 +1375,29 @@ public final boolean equals(Object o) {
 
     @Override
     public final int hashCode() {
-      int result = Objects.hash(getType(), isNull);
+      Type typeToHash = getType();
+      int valueHash = isNull ? 0 : valueHash();
+
+      /**
+       * 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.
+       *
+       * <p>With FLOAT32 being introduced, we want to ensure the backward compatibility of the NaN
+       * equality checks that existed for FLOAT64. We're promoting the type to FLOAT64 while
+       * calculating the type hash when the value is a NaN. We're doing a similar type promotion
+       * while calculating valueHash of Float32 type. Note that this is not applicable for composite
+       * types containing FLOAT32.
+       */
+      if (type.getCode() == Type.Code.FLOAT32 && !isNull && Float.isNaN(getFloat32())) {
+        typeToHash = Type.float64();
+      }
+
+      int result = Objects.hash(typeToHash, isNull);
       if (!isNull) {
-        result = 31 * result + valueHash();
+        result = 31 * result + valueHash;
       }
       return result;
     }
@@ -1621,6 +1641,11 @@ boolean valueEquals(Value v) {
 
     @Override
     int valueHash() {
+      // For backward compatibility of NaN equality checks with Float64 NaNs.
+      // Refer the comment in `Value.hashCode()` for more details.
+      if (!isNull() && Float.isNaN(value)) {
+        return Double.valueOf(Double.NaN).hashCode();
+      }
       return Float.valueOf(value).hashCode();
     }
   }

google-cloud-spanner/src/test/java/com/google/cloud/spanner/MutationTest.java

@@ -211,40 +211,13 @@ public void equalsAndHashCode() {
         Mutation.delete("T1", KeySet.singleKey(Key.of("k"))), Mutation.delete("T1", Key.of("k")));
 
     // Test NaNs
-    // tester.addEqualityGroup(
-    //   // DO NOT SUBMIT
-    // // DO NOT SUBMIT
-    // // DO NOT SUBMIT
-    // // DO NOT SUBMIT
-    // // DO NOT SUBMIT
-    // // DO NOT SUBMIT
-    // // DO NOT SUBMIT
-    // // DO NOT SUBMIT
-    // // DO NOT SUBMIT
-    // // DO NOT SUBMIT
-    // // DO NOT SUBMIT
-    // // DO NOT SUBMIT
-    // // DO NOT SUBMIT
-    // // DO NOT SUBMIT
-    // // DO NOT SUBMIT
-    // // DO NOT SUBMIT
-    // // DO NOT SUBMIT
-    // // DO NOT SUBMIT
-    // // DO NOT SUBMIT
-    // // DO NOT SUBMIT
-    // // DO NOT SUBMIT
-    // // DO NOT SUBMIT
-    // // DO NOT SUBMIT
-    // // DO NOT SUBMIT
-    // // DO NOT SUBMIT
-    // // DO NOT SUBMIT
-    // // DO NOT SUBMIT
-    //   Mutation.newInsertBuilder("T1").set("C").to(Float.NaN).build(),
-    //     Mutation.newInsertBuilder("T1").set("C").to(Value.float32(Float.NaN)).build());
+    // Refer the comment in `Value.hashCode()` for more details on NaN equality.
     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(Value.float64(Float.NaN)).build());
+        Mutation.newInsertBuilder("T1").set("C").to(Float.NaN).build(),
+        Mutation.newInsertBuilder("T1").set("C").to(Value.float64(Float.NaN)).build(),
+        Mutation.newInsertBuilder("T1").set("C").to(Value.float32(Float.NaN)).build());
 
     // Test NaN arrays
     tester.addEqualityGroup(