Skip to content

Commit 8cb4dc8

Browse files
milaGGLmilaGGL
authored
Fix: comparing double with long (#1960)
1 parent 7388291 commit 8cb4dc8

File tree

3 files changed

+145
-72
lines changed

3 files changed

+145
-72
lines changed

‎google-cloud-firestore/src/main/java/com/google/cloud/firestore/Order.java‎

Lines changed: 39 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -250,31 +250,60 @@ private int compareVectors(Value left, Value right) {
250250
}
251251

252252
private int compareNumbers(Value left, Value right) {
253+
// NaN is smaller than any other numbers
254+
if (isNaN(left)) {
255+
return isNaN(right) ? 0 : -1;
256+
} else if (isNaN(right)) {
257+
return 1;
258+
}
259+
253260
if (left.getValueTypeCase() == ValueTypeCase.DOUBLE_VALUE) {
254261
if (right.getValueTypeCase() == ValueTypeCase.DOUBLE_VALUE) {
255262
return compareDoubles(left.getDoubleValue(), right.getDoubleValue());
256263
} else {
257-
return compareDoubles(left.getDoubleValue(), right.getIntegerValue());
264+
return compareDoubleAndLong(left.getDoubleValue(), right.getIntegerValue());
258265
}
259266
} else {
260267
if (right.getValueTypeCase() == ValueTypeCase.INTEGER_VALUE) {
261268
return Long.compare(left.getIntegerValue(), right.getIntegerValue());
262269
} else {
263-
return compareDoubles(left.getIntegerValue(), right.getDoubleValue());
270+
return -compareDoubleAndLong(right.getDoubleValue(), left.getIntegerValue());
264271
}
265272
}
266273
}
267274

275+
private boolean isNaN(Value value) {
276+
return value.hasDoubleValue() && Double.isNaN(value.getDoubleValue());
277+
}
278+
268279
private int compareDoubles(double left, double right) {
269-
// Firestore orders NaNs before all other numbers and treats -0.0, 0.0 and +0.0 as equal.
270-
if (Double.isNaN(left)) {
271-
return Double.isNaN(right) ? 0 : -1;
272-
}
280+
// Firestore treats -0.0, 0.0 and +0.0 as equal.
281+
return Double.compare(left == -0.0 ? 0 : left, right == -0.0 ? 0 : right);
282+
}
273283

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

278-
return Double.compare(left == -0.0 ? 0 : left, right == -0.0 ? 0 : right);
291+
private int compareDoubleAndLong(double doubleValue, long longValue) {
292+
if (Math.abs(longValue) <= MAX_INTEGER_TO_DOUBLE_PRECISION) {
293+
// Enough precision to compare as double, the cast will not be lossy.
294+
return compareDoubles(doubleValue, (double) longValue);
295+
} else if (doubleValue < ((double) Long.MAX_VALUE)
296+
&& doubleValue >= ((double) Long.MIN_VALUE)) {
297+
// The above condition captures all doubles that belong to [min long, max long] inclusive.
298+
// Java long to double conversion rounds-to-nearest, so Long.MAX_VALUE casts to 2^63, hence
299+
// the use of "<" operator.
300+
// The cast to long below may be lossy, but only for absolute values < 2^52 so the loss of
301+
// precision does not affect the comparison, as longValue is outside that range.
302+
return Long.compare((long) doubleValue, longValue);
303+
} else {
304+
// doubleValue is outside the representable range for longs, so always smaller if negative,
305+
// and always greater otherwise.
306+
return doubleValue < 0 ? -1 : 1;
307+
}
279308
}
280309
}

‎google-cloud-firestore/src/test/java/com/google/cloud/firestore/OrderTest.java‎

Lines changed: 64 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ public class OrderTest {
3232

3333
@Test
3434
public void verifyOrder() {
35-
Value[][] groups = new Value[65][];
35+
Value[][] groups = new Value[67][];
3636

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

@@ -42,83 +42,85 @@ public void verifyOrder() {
4242
// numbers
4343
groups[3] = new Value[] {doubleValue(Double.NaN), doubleValue(Double.NaN)};
4444
groups[4] = new Value[] {doubleValue(Double.NEGATIVE_INFINITY)};
45-
groups[5] = new Value[] {intValue((long) Integer.MIN_VALUE - 1)};
46-
groups[6] = new Value[] {intValue(Integer.MIN_VALUE)};
47-
groups[7] = new Value[] {doubleValue(-1.1)};
45+
groups[5] = new Value[] {doubleValue((double) Long.MIN_VALUE - 100)};
46+
groups[6] = new Value[] {intValue((long) Integer.MIN_VALUE - 1)};
47+
groups[7] = new Value[] {intValue(Integer.MIN_VALUE)};
48+
groups[8] = new Value[] {doubleValue(-1.1)};
4849
// Integers and Doubles order the same.
49-
groups[8] = new Value[] {intValue(-1), doubleValue(-1.0)};
50-
groups[9] = new Value[] {doubleValue(-Double.MIN_VALUE)};
50+
groups[9] = new Value[] {intValue(-1), doubleValue(-1.0)};
51+
groups[10] = new Value[] {doubleValue(-Double.MIN_VALUE)};
5152
// zeros all compare the same.
52-
groups[10] = new Value[] {intValue(0), doubleValue(-0.0), doubleValue(0.0), doubleValue(+0.0)};
53-
groups[11] = new Value[] {doubleValue(Double.MIN_VALUE)};
54-
groups[12] = new Value[] {intValue(1), doubleValue(1.0)};
55-
groups[13] = new Value[] {doubleValue(1.1)};
56-
groups[14] = new Value[] {intValue(Integer.MAX_VALUE)};
57-
groups[15] = new Value[] {intValue((long) Integer.MAX_VALUE + 1)};
58-
groups[16] = new Value[] {doubleValue(Double.POSITIVE_INFINITY)};
59-
60-
groups[17] = new Value[] {timestampValue(123, 0)};
61-
groups[18] = new Value[] {timestampValue(123, 123)};
62-
groups[19] = new Value[] {timestampValue(345, 0)};
53+
groups[11] = new Value[] {intValue(0), doubleValue(-0.0), doubleValue(0.0), doubleValue(+0.0)};
54+
groups[12] = new Value[] {doubleValue(Double.MIN_VALUE)};
55+
groups[13] = new Value[] {intValue(1), doubleValue(1.0)};
56+
groups[14] = new Value[] {doubleValue(1.1)};
57+
groups[15] = new Value[] {intValue(Integer.MAX_VALUE)};
58+
groups[16] = new Value[] {intValue((long) Integer.MAX_VALUE + 1)};
59+
groups[17] = new Value[] {doubleValue(((double) Long.MAX_VALUE) + 100)};
60+
groups[18] = new Value[] {doubleValue(Double.POSITIVE_INFINITY)};
61+
62+
groups[19] = new Value[] {timestampValue(123, 0)};
63+
groups[20] = new Value[] {timestampValue(123, 123)};
64+
groups[21] = new Value[] {timestampValue(345, 0)};
6365

6466
// strings
65-
groups[20] = new Value[] {stringValue("")};
66-
groups[21] = new Value[] {stringValue("\u0000\ud7ff\ue000\uffff")};
67-
groups[22] = new Value[] {stringValue("(╯°□°）╯︵ ┻━┻")};
68-
groups[23] = new Value[] {stringValue("a")};
69-
groups[24] = new Value[] {stringValue("abc def")};
67+
groups[22] = new Value[] {stringValue("")};
68+
groups[23] = new Value[] {stringValue("\u0000\ud7ff\ue000\uffff")};
69+
groups[24] = new Value[] {stringValue("(╯°□°）╯︵ ┻━┻")};
70+
groups[25] = new Value[] {stringValue("a")};
71+
groups[26] = new Value[] {stringValue("abc def")};
7072
// latin small letter e + combining acute accent + latin small letter b
71-
groups[25] = new Value[] {stringValue("e\u0301b")};
72-
groups[26] = new Value[] {stringValue("æ")};
73+
groups[27] = new Value[] {stringValue("e\u0301b")};
74+
groups[28] = new Value[] {stringValue("æ")};
7375
// latin small letter e with acute accent + latin small letter a
74-
groups[27] = new Value[] {stringValue("\u00e9a")};
76+
groups[29] = new Value[] {stringValue("\u00e9a")};
7577

7678
// blobs
77-
groups[28] = new Value[] {blobValue(new byte[] {})};
78-
groups[29] = new Value[] {blobValue(new byte[] {0})};
79-
groups[30] = new Value[] {blobValue(new byte[] {0, 1, 2, 3, 4})};
80-
groups[31] = new Value[] {blobValue(new byte[] {0, 1, 2, 4, 3})};
81-
groups[32] = new Value[] {blobValue(new byte[] {127})};
79+
groups[30] = new Value[] {blobValue(new byte[] {})};
80+
groups[31] = new Value[] {blobValue(new byte[] {0})};
81+
groups[32] = new Value[] {blobValue(new byte[] {0, 1, 2, 3, 4})};
82+
groups[33] = new Value[] {blobValue(new byte[] {0, 1, 2, 4, 3})};
83+
groups[34] = new Value[] {blobValue(new byte[] {127})};
8284

8385
// resource names
84-
groups[33] = new Value[] {referenceValue("projects/p1/databases/d1/documents/c1/doc1")};
85-
groups[34] = new Value[] {referenceValue("projects/p1/databases/d1/documents/c1/doc2")};
86-
groups[35] = new Value[] {referenceValue("projects/p1/databases/d1/documents/c1/doc2/c2/doc1")};
87-
groups[36] = new Value[] {referenceValue("projects/p1/databases/d1/documents/c1/doc2/c2/doc2")};
88-
groups[37] = new Value[] {referenceValue("projects/p1/databases/d1/documents/c10/doc1")};
89-
groups[38] = new Value[] {referenceValue("projects/p1/databases/d1/documents/c2/doc1")};
90-
groups[39] = new Value[] {referenceValue("projects/p2/databases/d2/documents/c1/doc1")};
91-
groups[40] = new Value[] {referenceValue("projects/p2/databases/d2/documents/c1-/doc1")};
92-
groups[41] = new Value[] {referenceValue("projects/p2/databases/d3/documents/c1-/doc1")};
86+
groups[35] = new Value[] {referenceValue("projects/p1/databases/d1/documents/c1/doc1")};
87+
groups[36] = new Value[] {referenceValue("projects/p1/databases/d1/documents/c1/doc2")};
88+
groups[37] = new Value[] {referenceValue("projects/p1/databases/d1/documents/c1/doc2/c2/doc1")};
89+
groups[38] = new Value[] {referenceValue("projects/p1/databases/d1/documents/c1/doc2/c2/doc2")};
90+
groups[39] = new Value[] {referenceValue("projects/p1/databases/d1/documents/c10/doc1")};
91+
groups[40] = new Value[] {referenceValue("projects/p1/databases/d1/documents/c2/doc1")};
92+
groups[41] = new Value[] {referenceValue("projects/p2/databases/d2/documents/c1/doc1")};
93+
groups[42] = new Value[] {referenceValue("projects/p2/databases/d2/documents/c1-/doc1")};
94+
groups[43] = new Value[] {referenceValue("projects/p2/databases/d3/documents/c1-/doc1")};
9395

9496
// geo points
95-
groups[42] = new Value[] {geoPointValue(-90, -180)};
96-
groups[43] = new Value[] {geoPointValue(-90, 0)};
97-
groups[44] = new Value[] {geoPointValue(-90, 180)};
98-
groups[45] = new Value[] {geoPointValue(0, -180)};
99-
groups[46] = new Value[] {geoPointValue(0, 0)};
100-
groups[47] = new Value[] {geoPointValue(0, 180)};
101-
groups[48] = new Value[] {geoPointValue(1, -180)};
102-
groups[49] = new Value[] {geoPointValue(1, 0)};
103-
groups[50] = new Value[] {geoPointValue(1, 180)};
104-
groups[51] = new Value[] {geoPointValue(90, -180)};
105-
groups[52] = new Value[] {geoPointValue(90, 0)};
106-
groups[53] = new Value[] {geoPointValue(90, 180)};
97+
groups[44] = new Value[] {geoPointValue(-90, -180)};
98+
groups[45] = new Value[] {geoPointValue(-90, 0)};
99+
groups[46] = new Value[] {geoPointValue(-90, 180)};
100+
groups[47] = new Value[] {geoPointValue(0, -180)};
101+
groups[48] = new Value[] {geoPointValue(0, 0)};
102+
groups[49] = new Value[] {geoPointValue(0, 180)};
103+
groups[50] = new Value[] {geoPointValue(1, -180)};
104+
groups[51] = new Value[] {geoPointValue(1, 0)};
105+
groups[52] = new Value[] {geoPointValue(1, 180)};
106+
groups[53] = new Value[] {geoPointValue(90, -180)};
107+
groups[54] = new Value[] {geoPointValue(90, 0)};
108+
groups[55] = new Value[] {geoPointValue(90, 180)};
107109

108110
// arrays
109-
groups[54] = new Value[] {arrayValue()};
110-
groups[55] = new Value[] {arrayValue(stringValue("bar"))};
111-
groups[56] = new Value[] {arrayValue(stringValue("foo"))};
112-
groups[57] = new Value[] {arrayValue(stringValue("foo"), intValue(0))};
113-
groups[58] = new Value[] {arrayValue(stringValue("foo"), intValue(1))};
114-
groups[59] = new Value[] {arrayValue(stringValue("foo"), stringValue("0"))};
111+
groups[56] = new Value[] {arrayValue()};
112+
groups[57] = new Value[] {arrayValue(stringValue("bar"))};
113+
groups[58] = new Value[] {arrayValue(stringValue("foo"))};
114+
groups[59] = new Value[] {arrayValue(stringValue("foo"), intValue(0))};
115+
groups[60] = new Value[] {arrayValue(stringValue("foo"), intValue(1))};
116+
groups[61] = new Value[] {arrayValue(stringValue("foo"), stringValue("0"))};
115117

116118
// objects
117-
groups[60] = new Value[] {objectValue("bar", intValue(0))};
118-
groups[61] = new Value[] {objectValue("bar", intValue(0), "foo", intValue(1))};
119-
groups[62] = new Value[] {objectValue("bar", intValue(1))};
120-
groups[63] = new Value[] {objectValue("bar", intValue(2))};
121-
groups[64] = new Value[] {objectValue("bar", stringValue("0"))};
119+
groups[62] = new Value[] {objectValue("bar", intValue(0))};
120+
groups[63] = new Value[] {objectValue("bar", intValue(0), "foo", intValue(1))};
121+
groups[64] = new Value[] {objectValue("bar", intValue(1))};
122+
groups[65] = new Value[] {objectValue("bar", intValue(2))};
123+
groups[66] = new Value[] {objectValue("bar", stringValue("0"))};
122124

123125
for (int left = 0; left < groups.length; left++) {
124126
for (int right = 0; right < groups.length; right++) {

‎google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryTest.java‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,12 +28,14 @@
2828
import com.google.cloud.firestore.*;
2929
import com.google.cloud.firestore.Query.Direction;
3030
import java.time.Duration;
31+
import java.util.ArrayList;
3132
import java.util.Arrays;
3233
import java.util.Collections;
3334
import java.util.Iterator;
3435
import java.util.LinkedHashMap;
3536
import java.util.List;
3637
import java.util.Map;
38+
import java.util.concurrent.CountDownLatch;
3739
import java.util.concurrent.ExecutionException;
3840
import java.util.concurrent.TimeUnit;
3941
import java.util.concurrent.TimeoutException;
@@ -1113,4 +1115,44 @@ public void testAggregateQueryProfile() throws Exception {
11131115
assertThat(stats.getResultsReturned()).isEqualTo(1);
11141116
assertThat(stats.getExecutionDuration()).isGreaterThan(Duration.ZERO);
11151117
}
1118+
1119+
@Test
1120+
public void snapshotListenerSortsNumbersSameWayAsServer() throws Exception {
1121+
CollectionReference col = createEmptyCollection();
1122+
firestore
1123+
.batch()
1124+
.set(col.document("intMin"), map("value", Long.MIN_VALUE))
1125+
.set(col.document("doubleMin"), map("value", ((double) Long.MIN_VALUE) - 100))
1126+
.set(col.document("intMax"), map("value", Long.MAX_VALUE))
1127+
.set(col.document("doubleMax"), map("value", ((double) Long.MAX_VALUE) + 100))
1128+
.set(col.document("NaN"), map("value", Double.NaN))
1129+
.set(col.document("integerMax"), map("value", (long) Integer.MAX_VALUE))
1130+
.set(col.document("integerMin"), map("value", (long) Integer.MIN_VALUE))
1131+
.set(col.document("negativeInfinity"), map("value", Double.NEGATIVE_INFINITY))
1132+
.set(col.document("positiveInfinity"), map("value", Double.POSITIVE_INFINITY))
1133+
.commit()
1134+
.get();
1135+
1136+
Query query = col.orderBy("value", Direction.ASCENDING);
1137+
1138+
QuerySnapshot snapshot = query.get().get();
1139+
List<String> queryOrder =
1140+
snapshot.getDocuments().stream().map(doc -> doc.getId()).collect(Collectors.toList());
1141+
1142+
CountDownLatch latch = new CountDownLatch(1);
1143+
List<String> listenerOrder = new ArrayList<>();
1144+
ListenerRegistration registration =
1145+
query.addSnapshotListener(
1146+
(value, error) -> {
1147+
listenerOrder.addAll(
1148+
value.getDocuments().stream()
1149+
.map(doc -> doc.getId())
1150+
.collect(Collectors.toList()));
1151+
latch.countDown();
1152+
});
1153+
latch.await();
1154+
registration.remove();
1155+
1156+
assertEquals(queryOrder, listenerOrder); // Assert order in the SDK
1157+
}
11161158
}

0 commit comments

Comments
 (0)