Skip to content

Commit

Permalink
Fix: comparing double with long (#1960)
Browse files Browse the repository at this point in the history
  • Loading branch information
@milaGGL
milaGGL authored Jan 15, 2025
1 parent 7388291 commit 8cb4dc8
Show file tree
Hide file tree
Showing 3 changed files with 145 additions and 72 deletions.
49 changes: 39 additions & 10 deletions google-cloud-firestore/src/main/java/com/google/cloud/firestore/Order.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -250,31 +250,60 @@ private int compareVectors(Value left, Value right) {
}

private int compareNumbers(Value left, Value right) {
// NaN is smaller than any other numbers
if (isNaN(left)) {
return isNaN(right) ? 0 : -1;
} else if (isNaN(right)) {
return 1;
}

if (left.getValueTypeCase() == ValueTypeCase.DOUBLE_VALUE) {
if (right.getValueTypeCase() == ValueTypeCase.DOUBLE_VALUE) {
return compareDoubles(left.getDoubleValue(), right.getDoubleValue());
} else {
return compareDoubles(left.getDoubleValue(), right.getIntegerValue());
return compareDoubleAndLong(left.getDoubleValue(), right.getIntegerValue());
}
} else {
if (right.getValueTypeCase() == ValueTypeCase.INTEGER_VALUE) {
return Long.compare(left.getIntegerValue(), right.getIntegerValue());
} else {
return compareDoubles(left.getIntegerValue(), right.getDoubleValue());
return -compareDoubleAndLong(right.getDoubleValue(), left.getIntegerValue());
}
}
}

private boolean isNaN(Value value) {
return value.hasDoubleValue() && Double.isNaN(value.getDoubleValue());
}

private int compareDoubles(double left, double right) {
// Firestore orders NaNs before all other numbers and treats -0.0, 0.0 and +0.0 as equal.
if (Double.isNaN(left)) {
return Double.isNaN(right) ? 0 : -1;
}
// Firestore treats -0.0, 0.0 and +0.0 as equal.
return Double.compare(left == -0.0 ? 0 : left, right == -0.0 ? 0 : right);
}

if (Double.isNaN(right)) {
return 1;
}
/**
* The maximum integer absolute number that can be represented as a double without loss of
* precision. This is 2^53 because double-precision floating point numbers have 53 bits
* significand precision (52 explicit bit + 1 hidden bit).
*/
private static final long MAX_INTEGER_TO_DOUBLE_PRECISION = 1L << 53;

return Double.compare(left == -0.0 ? 0 : left, right == -0.0 ? 0 : right);
private int compareDoubleAndLong(double doubleValue, long longValue) {
if (Math.abs(longValue) <= MAX_INTEGER_TO_DOUBLE_PRECISION) {
// Enough precision to compare as double, the cast will not be lossy.
return compareDoubles(doubleValue, (double) longValue);
} else if (doubleValue < ((double) Long.MAX_VALUE)
&& doubleValue >= ((double) Long.MIN_VALUE)) {
// The above condition captures all doubles that belong to [min long, max long] inclusive.
// Java long to double conversion rounds-to-nearest, so Long.MAX_VALUE casts to 2^63, hence
// the use of "<" operator.
// The cast to long below may be lossy, but only for absolute values < 2^52 so the loss of
// precision does not affect the comparison, as longValue is outside that range.
return Long.compare((long) doubleValue, longValue);
} else {
// doubleValue is outside the representable range for longs, so always smaller if negative,
// and always greater otherwise.
return doubleValue < 0 ? -1 : 1;
}
}
}
126 changes: 64 additions & 62 deletions google-cloud-firestore/src/test/java/com/google/cloud/firestore/OrderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public class OrderTest {

@Test
public void verifyOrder() {
Value[][] groups = new Value[65][];
Value[][] groups = new Value[67][];

groups[0] = new Value[] {nullValue()};

Expand All @@ -42,83 +42,85 @@ public void verifyOrder() {
// numbers
groups[3] = new Value[] {doubleValue(Double.NaN), doubleValue(Double.NaN)};
groups[4] = new Value[] {doubleValue(Double.NEGATIVE_INFINITY)};
groups[5] = new Value[] {intValue((long) Integer.MIN_VALUE - 1)};
groups[6] = new Value[] {intValue(Integer.MIN_VALUE)};
groups[7] = new Value[] {doubleValue(-1.1)};
groups[5] = new Value[] {doubleValue((double) Long.MIN_VALUE - 100)};
groups[6] = new Value[] {intValue((long) Integer.MIN_VALUE - 1)};
groups[7] = new Value[] {intValue(Integer.MIN_VALUE)};
groups[8] = new Value[] {doubleValue(-1.1)};
// Integers and Doubles order the same.
groups[8] = new Value[] {intValue(-1), doubleValue(-1.0)};
groups[9] = new Value[] {doubleValue(-Double.MIN_VALUE)};
groups[9] = new Value[] {intValue(-1), doubleValue(-1.0)};
groups[10] = new Value[] {doubleValue(-Double.MIN_VALUE)};
// zeros all compare the same.
groups[10] = new Value[] {intValue(0), doubleValue(-0.0), doubleValue(0.0), doubleValue(+0.0)};
groups[11] = new Value[] {doubleValue(Double.MIN_VALUE)};
groups[12] = new Value[] {intValue(1), doubleValue(1.0)};
groups[13] = new Value[] {doubleValue(1.1)};
groups[14] = new Value[] {intValue(Integer.MAX_VALUE)};
groups[15] = new Value[] {intValue((long) Integer.MAX_VALUE + 1)};
groups[16] = new Value[] {doubleValue(Double.POSITIVE_INFINITY)};

groups[17] = new Value[] {timestampValue(123, 0)};
groups[18] = new Value[] {timestampValue(123, 123)};
groups[19] = new Value[] {timestampValue(345, 0)};
groups[11] = new Value[] {intValue(0), doubleValue(-0.0), doubleValue(0.0), doubleValue(+0.0)};
groups[12] = new Value[] {doubleValue(Double.MIN_VALUE)};
groups[13] = new Value[] {intValue(1), doubleValue(1.0)};
groups[14] = new Value[] {doubleValue(1.1)};
groups[15] = new Value[] {intValue(Integer.MAX_VALUE)};
groups[16] = new Value[] {intValue((long) Integer.MAX_VALUE + 1)};
groups[17] = new Value[] {doubleValue(((double) Long.MAX_VALUE) + 100)};
groups[18] = new Value[] {doubleValue(Double.POSITIVE_INFINITY)};

groups[19] = new Value[] {timestampValue(123, 0)};
groups[20] = new Value[] {timestampValue(123, 123)};
groups[21] = new Value[] {timestampValue(345, 0)};

// strings
groups[20] = new Value[] {stringValue("")};
groups[21] = new Value[] {stringValue("\u0000\ud7ff\ue000\uffff")};
groups[22] = new Value[] {stringValue("(╯°□°）╯︵ ┻━┻")};
groups[23] = new Value[] {stringValue("a")};
groups[24] = new Value[] {stringValue("abc def")};
groups[22] = new Value[] {stringValue("")};
groups[23] = new Value[] {stringValue("\u0000\ud7ff\ue000\uffff")};
groups[24] = new Value[] {stringValue("(╯°□°）╯︵ ┻━┻")};
groups[25] = new Value[] {stringValue("a")};
groups[26] = new Value[] {stringValue("abc def")};
// latin small letter e + combining acute accent + latin small letter b
groups[25] = new Value[] {stringValue("e\u0301b")};
groups[26] = new Value[] {stringValue("æ")};
groups[27] = new Value[] {stringValue("e\u0301b")};
groups[28] = new Value[] {stringValue("æ")};
// latin small letter e with acute accent + latin small letter a
groups[27] = new Value[] {stringValue("\u00e9a")};
groups[29] = new Value[] {stringValue("\u00e9a")};

// blobs
groups[28] = new Value[] {blobValue(new byte[] {})};
groups[29] = new Value[] {blobValue(new byte[] {0})};
groups[30] = new Value[] {blobValue(new byte[] {0, 1, 2, 3, 4})};
groups[31] = new Value[] {blobValue(new byte[] {0, 1, 2, 4, 3})};
groups[32] = new Value[] {blobValue(new byte[] {127})};
groups[30] = new Value[] {blobValue(new byte[] {})};
groups[31] = new Value[] {blobValue(new byte[] {0})};
groups[32] = new Value[] {blobValue(new byte[] {0, 1, 2, 3, 4})};
groups[33] = new Value[] {blobValue(new byte[] {0, 1, 2, 4, 3})};
groups[34] = new Value[] {blobValue(new byte[] {127})};

// resource names
groups[33] = new Value[] {referenceValue("projects/p1/databases/d1/documents/c1/doc1")};
groups[34] = new Value[] {referenceValue("projects/p1/databases/d1/documents/c1/doc2")};
groups[35] = new Value[] {referenceValue("projects/p1/databases/d1/documents/c1/doc2/c2/doc1")};
groups[36] = new Value[] {referenceValue("projects/p1/databases/d1/documents/c1/doc2/c2/doc2")};
groups[37] = new Value[] {referenceValue("projects/p1/databases/d1/documents/c10/doc1")};
groups[38] = new Value[] {referenceValue("projects/p1/databases/d1/documents/c2/doc1")};
groups[39] = new Value[] {referenceValue("projects/p2/databases/d2/documents/c1/doc1")};
groups[40] = new Value[] {referenceValue("projects/p2/databases/d2/documents/c1-/doc1")};
groups[41] = new Value[] {referenceValue("projects/p2/databases/d3/documents/c1-/doc1")};
groups[35] = new Value[] {referenceValue("projects/p1/databases/d1/documents/c1/doc1")};
groups[36] = new Value[] {referenceValue("projects/p1/databases/d1/documents/c1/doc2")};
groups[37] = new Value[] {referenceValue("projects/p1/databases/d1/documents/c1/doc2/c2/doc1")};
groups[38] = new Value[] {referenceValue("projects/p1/databases/d1/documents/c1/doc2/c2/doc2")};
groups[39] = new Value[] {referenceValue("projects/p1/databases/d1/documents/c10/doc1")};
groups[40] = new Value[] {referenceValue("projects/p1/databases/d1/documents/c2/doc1")};
groups[41] = new Value[] {referenceValue("projects/p2/databases/d2/documents/c1/doc1")};
groups[42] = new Value[] {referenceValue("projects/p2/databases/d2/documents/c1-/doc1")};
groups[43] = new Value[] {referenceValue("projects/p2/databases/d3/documents/c1-/doc1")};

// geo points
groups[42] = new Value[] {geoPointValue(-90, -180)};
groups[43] = new Value[] {geoPointValue(-90, 0)};
groups[44] = new Value[] {geoPointValue(-90, 180)};
groups[45] = new Value[] {geoPointValue(0, -180)};
groups[46] = new Value[] {geoPointValue(0, 0)};
groups[47] = new Value[] {geoPointValue(0, 180)};
groups[48] = new Value[] {geoPointValue(1, -180)};
groups[49] = new Value[] {geoPointValue(1, 0)};
groups[50] = new Value[] {geoPointValue(1, 180)};
groups[51] = new Value[] {geoPointValue(90, -180)};
groups[52] = new Value[] {geoPointValue(90, 0)};
groups[53] = new Value[] {geoPointValue(90, 180)};
groups[44] = new Value[] {geoPointValue(-90, -180)};
groups[45] = new Value[] {geoPointValue(-90, 0)};
groups[46] = new Value[] {geoPointValue(-90, 180)};
groups[47] = new Value[] {geoPointValue(0, -180)};
groups[48] = new Value[] {geoPointValue(0, 0)};
groups[49] = new Value[] {geoPointValue(0, 180)};
groups[50] = new Value[] {geoPointValue(1, -180)};
groups[51] = new Value[] {geoPointValue(1, 0)};
groups[52] = new Value[] {geoPointValue(1, 180)};
groups[53] = new Value[] {geoPointValue(90, -180)};
groups[54] = new Value[] {geoPointValue(90, 0)};
groups[55] = new Value[] {geoPointValue(90, 180)};

// arrays
groups[54] = new Value[] {arrayValue()};
groups[55] = new Value[] {arrayValue(stringValue("bar"))};
groups[56] = new Value[] {arrayValue(stringValue("foo"))};
groups[57] = new Value[] {arrayValue(stringValue("foo"), intValue(0))};
groups[58] = new Value[] {arrayValue(stringValue("foo"), intValue(1))};
groups[59] = new Value[] {arrayValue(stringValue("foo"), stringValue("0"))};
groups[56] = new Value[] {arrayValue()};
groups[57] = new Value[] {arrayValue(stringValue("bar"))};
groups[58] = new Value[] {arrayValue(stringValue("foo"))};
groups[59] = new Value[] {arrayValue(stringValue("foo"), intValue(0))};
groups[60] = new Value[] {arrayValue(stringValue("foo"), intValue(1))};
groups[61] = new Value[] {arrayValue(stringValue("foo"), stringValue("0"))};

// objects
groups[60] = new Value[] {objectValue("bar", intValue(0))};
groups[61] = new Value[] {objectValue("bar", intValue(0), "foo", intValue(1))};
groups[62] = new Value[] {objectValue("bar", intValue(1))};
groups[63] = new Value[] {objectValue("bar", intValue(2))};
groups[64] = new Value[] {objectValue("bar", stringValue("0"))};
groups[62] = new Value[] {objectValue("bar", intValue(0))};
groups[63] = new Value[] {objectValue("bar", intValue(0), "foo", intValue(1))};
groups[64] = new Value[] {objectValue("bar", intValue(1))};
groups[65] = new Value[] {objectValue("bar", intValue(2))};
groups[66] = new Value[] {objectValue("bar", stringValue("0"))};

for (int left = 0; left < groups.length; left++) {
for (int right = 0; right < groups.length; right++) {
Expand Down
42 changes: 42 additions & 0 deletions google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,14 @@
import com.google.cloud.firestore.*;
import com.google.cloud.firestore.Query.Direction;
import java.time.Duration;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.Iterator;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
Expand Down Expand Up @@ -1113,4 +1115,44 @@ public void testAggregateQueryProfile() throws Exception {
assertThat(stats.getResultsReturned()).isEqualTo(1);
assertThat(stats.getExecutionDuration()).isGreaterThan(Duration.ZERO);
}

@Test
public void snapshotListenerSortsNumbersSameWayAsServer() throws Exception {
CollectionReference col = createEmptyCollection();
firestore
.batch()
.set(col.document("intMin"), map("value", Long.MIN_VALUE))
.set(col.document("doubleMin"), map("value", ((double) Long.MIN_VALUE) - 100))
.set(col.document("intMax"), map("value", Long.MAX_VALUE))
.set(col.document("doubleMax"), map("value", ((double) Long.MAX_VALUE) + 100))
.set(col.document("NaN"), map("value", Double.NaN))
.set(col.document("integerMax"), map("value", (long) Integer.MAX_VALUE))
.set(col.document("integerMin"), map("value", (long) Integer.MIN_VALUE))
.set(col.document("negativeInfinity"), map("value", Double.NEGATIVE_INFINITY))
.set(col.document("positiveInfinity"), map("value", Double.POSITIVE_INFINITY))
.commit()
.get();

Query query = col.orderBy("value", Direction.ASCENDING);

QuerySnapshot snapshot = query.get().get();
List<String> queryOrder =
snapshot.getDocuments().stream().map(doc -> doc.getId()).collect(Collectors.toList());

CountDownLatch latch = new CountDownLatch(1);
List<String> listenerOrder = new ArrayList<>();
ListenerRegistration registration =
query.addSnapshotListener(
(value, error) -> {
listenerOrder.addAll(
value.getDocuments().stream()
.map(doc -> doc.getId())
.collect(Collectors.toList()));
latch.countDown();
});
latch.await();
registration.remove();

assertEquals(queryOrder, listenerOrder); // Assert order in the SDK
}
}

0 comments on commit 8cb4dc8

Please sign in to comment.