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 all 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
32 changes: 31 additions & 1 deletion google-cloud-spanner/src/main/java/com/google/cloud/spanner/Mutation.java
View file
Open in desktop
Expand Up @@ -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);
}

Expand All @@ -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<Value> values, List<Value> 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))) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: maybe for future reference we should add a short comment on why we are doing this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, added a comment here

return false;
}
}
return true;
}
}

private boolean isNaN(Value value) {
return !value.isNull() && value.getType() == Type.float64() && Double.isNaN(value.getFloat64());
}

static void toProto(Iterable<Mutation> mutations, List<com.google.spanner.v1.Mutation> out) {
Mutation last = null;
// The mutation currently being built.
Expand Down
62 changes: 62 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 @@ -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();
}

Expand Down