Skip to content

Commit d8ec3fb

Browse files
wu-huiwu-hui
authored
Fix bug. (#3673)
1 parent 849b645 commit d8ec3fb

File tree

5 files changed

+91
-74
lines changed

5 files changed

+91
-74
lines changed

‎firebase-firestore/src/main/java/com/google/firebase/firestore/core/Target.java

Lines changed: 17 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,10 @@
1616

1717
import static com.google.firebase.firestore.core.FieldFilter.Operator.ARRAY_CONTAINS;
1818
import static com.google.firebase.firestore.core.FieldFilter.Operator.ARRAY_CONTAINS_ANY;
19-
import static com.google.firebase.firestore.model.Values.max;
20-
import static com.google.firebase.firestore.model.Values.min;
19+
import static com.google.firebase.firestore.model.Values.MAX_VALUE;
20+
import static com.google.firebase.firestore.model.Values.MIN_VALUE;
21+
import static com.google.firebase.firestore.model.Values.lowerBoundCompare;
22+
import static com.google.firebase.firestore.model.Values.upperBoundCompare;
2123

2224
import android.util.Pair;
2325
import androidx.annotation.Nullable;
@@ -182,9 +184,8 @@ private List<FieldFilter> getFieldFiltersForPath(FieldPath path) {
182184

183185
/**
184186
* Returns a lower bound of field values that can be used as a starting point to scan the index
185-
* defined by {@code fieldIndex}. Returns {@code null} if no lower bound exists.
187+
* defined by {@code fieldIndex}. Returns {@link Values#MIN_VALUE} if no lower bound exists.
186188
*/
187-
@Nullable
188189
public Bound getLowerBound(FieldIndex fieldIndex) {
189190
List<Value> values = new ArrayList<>();
190191
boolean inclusive = true;
@@ -196,11 +197,6 @@ public Bound getLowerBound(FieldIndex fieldIndex) {
196197
? getAscendingBound(segment, startAt)
197198
: getDescendingBound(segment, startAt);
198199

199-
if (segmentBound.first == null) {
200-
// No lower bound exists
201-
return null;
202-
}
203-
204200
values.add(segmentBound.first);
205201
inclusive &= segmentBound.second;
206202
}
@@ -210,9 +206,9 @@ public Bound getLowerBound(FieldIndex fieldIndex) {
210206

211207
/**
212208
* Returns an upper bound of field values that can be used as an ending point when scanning the
213-
* index defined by {@code fieldIndex}. Returns {@code null} if no upper bound exists.
209+
* index defined by {@code fieldIndex}. Returns {@link Values#MAX_VALUE} if no upper bound exists.
214210
*/
215-
public @Nullable Bound getUpperBound(FieldIndex fieldIndex) {
211+
public Bound getUpperBound(FieldIndex fieldIndex) {
216212
List<Value> values = new ArrayList<>();
217213
boolean inclusive = true;
218214

@@ -223,11 +219,6 @@ public Bound getLowerBound(FieldIndex fieldIndex) {
223219
? getDescendingBound(segment, endAt)
224220
: getAscendingBound(segment, endAt);
225221

226-
if (segmentBound.first == null) {
227-
// No upper bound exists
228-
return null;
229-
}
230-
231222
values.add(segmentBound.first);
232223
inclusive &= segmentBound.second;
233224
}
@@ -244,12 +235,12 @@ public Bound getLowerBound(FieldIndex fieldIndex) {
244235
*/
245236
private Pair<Value, Boolean> getAscendingBound(
246237
FieldIndex.Segment segment, @Nullable Bound bound) {
247-
Value segmentValue = null;
238+
Value segmentValue = MIN_VALUE;
248239
boolean segmentInclusive = true;
249240

250241
// Process all filters to find a value for the current field segment
251242
for (FieldFilter fieldFilter : getFieldFiltersForPath(segment.getFieldPath())) {
252-
Value filterValue = null;
243+
Value filterValue = MIN_VALUE;
253244
boolean filterInclusive = true;
254245

255246
switch (fieldFilter.getOperator()) {
@@ -274,7 +265,7 @@ private Pair<Value, Boolean> getAscendingBound(
274265
// Remaining filters cannot be used as bound.
275266
}
276267

277-
if (max(segmentValue, filterValue) == filterValue) {
268+
if (lowerBoundCompare(segmentValue, segmentInclusive, filterValue, filterInclusive) < 0) {
278269
segmentValue = filterValue;
279270
segmentInclusive = filterInclusive;
280271
}
@@ -287,7 +278,8 @@ private Pair<Value, Boolean> getAscendingBound(
287278
OrderBy orderBy = this.orderBys.get(i);
288279
if (orderBy.getField().equals(segment.getFieldPath())) {
289280
Value cursorValue = bound.getPosition().get(i);
290-
if (max(segmentValue, cursorValue) == cursorValue) {
281+
if (lowerBoundCompare(segmentValue, segmentInclusive, cursorValue, bound.isInclusive())
282+
< 0) {
291283
segmentValue = cursorValue;
292284
segmentInclusive = bound.isInclusive();
293285
}
@@ -308,12 +300,12 @@ private Pair<Value, Boolean> getAscendingBound(
308300
*/
309301
private Pair<Value, Boolean> getDescendingBound(
310302
FieldIndex.Segment segment, @Nullable Bound bound) {
311-
Value segmentValue = null;
303+
Value segmentValue = MAX_VALUE;
312304
boolean segmentInclusive = true;
313305

314306
// Process all filters to find a value for the current field segment
315307
for (FieldFilter fieldFilter : getFieldFiltersForPath(segment.getFieldPath())) {
316-
Value filterValue = null;
308+
Value filterValue = MAX_VALUE;
317309
boolean filterInclusive = true;
318310

319311
switch (fieldFilter.getOperator()) {
@@ -339,7 +331,7 @@ private Pair<Value, Boolean> getDescendingBound(
339331
// Remaining filters cannot be used as bound.
340332
}
341333

342-
if (min(segmentValue, filterValue) == filterValue) {
334+
if (upperBoundCompare(segmentValue, segmentInclusive, filterValue, filterInclusive) > 0) {
343335
segmentValue = filterValue;
344336
segmentInclusive = filterInclusive;
345337
}
@@ -352,7 +344,8 @@ private Pair<Value, Boolean> getDescendingBound(
352344
OrderBy orderBy = this.orderBys.get(i);
353345
if (orderBy.getField().equals(segment.getFieldPath())) {
354346
Value cursorValue = bound.getPosition().get(i);
355-
if (min(segmentValue, cursorValue) == cursorValue) {
347+
if (upperBoundCompare(segmentValue, segmentInclusive, cursorValue, bound.isInclusive())
348+
> 0) {
356349
segmentValue = cursorValue;
357350
segmentInclusive = bound.isInclusive();
358351
}

‎firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java

Lines changed: 16 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -475,8 +475,8 @@ public List<DocumentKey> getDocumentsMatchingTarget(Target target) {
475475

476476
@Nullable List<Value> arrayValues = subTarget.getArrayValues(fieldIndex);
477477
@Nullable Collection<Value> notInValues = subTarget.getNotInValues(fieldIndex);
478-
@Nullable Bound lowerBound = subTarget.getLowerBound(fieldIndex);
479-
@Nullable Bound upperBound = subTarget.getUpperBound(fieldIndex);
478+
Bound lowerBound = subTarget.getLowerBound(fieldIndex);
479+
Bound upperBound = subTarget.getUpperBound(fieldIndex);
480480

481481
if (Logger.isDebugEnabled()) {
482482
Logger.debug(
@@ -490,9 +490,9 @@ public List<DocumentKey> getDocumentsMatchingTarget(Target target) {
490490
}
491491

492492
Object[] lowerBoundEncoded = encodeBound(fieldIndex, subTarget, lowerBound);
493-
String lowerBoundOp = lowerBound != null && lowerBound.isInclusive() ? ">=" : ">";
493+
String lowerBoundOp = lowerBound.isInclusive() ? ">=" : ">";
494494
Object[] upperBoundEncoded = encodeBound(fieldIndex, subTarget, upperBound);
495-
String upperBoundOp = upperBound != null && upperBound.isInclusive() ? "<=" : "<";
495+
String upperBoundOp = upperBound.isInclusive() ? "<=" : "<";
496496
Object[] notInEncoded = encodeValues(fieldIndex, subTarget, notInValues);
497497

498498
Object[] subQueryAndBindings =
@@ -536,32 +536,26 @@ private Object[] generateQueryAndBindings(
536536
Target target,
537537
int indexId,
538538
@Nullable List<Value> arrayValues,
539-
@Nullable Object[] lowerBounds,
539+
Object[] lowerBounds,
540540
String lowerBoundOp,
541-
@Nullable Object[] upperBounds,
541+
Object[] upperBounds,
542542
String upperBoundOp,
543543
@Nullable Object[] notIn) {
544544
// The number of total statements we union together. This is similar to a distributed normal
545545
// form, but adapted for array values. We create a single statement per value in an
546546
// ARRAY_CONTAINS or ARRAY_CONTAINS_ANY filter combined with the values from the query bounds.
547547
int statementCount =
548548
(arrayValues != null ? arrayValues.size() : 1)
549-
* max(
550-
lowerBounds != null ? lowerBounds.length : 1,
551-
upperBounds != null ? upperBounds.length : 1);
549+
* max(lowerBounds.length, upperBounds.length);
552550

553551
// Build the statement. We always include the lower bound, and optionally include an array value
554552
// and an upper bound.
555553
StringBuilder statement = new StringBuilder();
556554
statement.append("SELECT document_key, directional_value FROM index_entries ");
557555
statement.append("WHERE index_id = ? AND uid = ? ");
558556
statement.append("AND array_value = ? ");
559-
if (lowerBounds != null) {
560-
statement.append("AND directional_value ").append(lowerBoundOp).append(" ? ");
561-
}
562-
if (upperBounds != null) {
563-
statement.append("AND directional_value ").append(upperBoundOp).append(" ? ");
564-
}
557+
statement.append("AND directional_value ").append(lowerBoundOp).append(" ? ");
558+
statement.append("AND directional_value ").append(upperBoundOp).append(" ? ");
565559

566560
// Create the UNION statement by repeating the above generated statement. We can then add
567561
// ordering and a limit clause.
@@ -592,10 +586,11 @@ private Object[] fillBounds(
592586
int statementCount,
593587
int indexId,
594588
@Nullable List<Value> arrayValues,
595-
@Nullable Object[] lowerBounds,
596-
@Nullable Object[] upperBounds,
589+
Object[] lowerBounds,
590+
Object[] upperBounds,
597591
@Nullable Object[] notInValues) {
598-
int bindsPerStatement = 3 + (lowerBounds != null ? 1 : 0) + (upperBounds != null ? 1 : 0);
592+
// Every SQL statement we union together have 5 binds, see generateQueryAndBindings.
593+
int bindsPerStatement = 5;
599594
int statementsPerArrayValue = statementCount / (arrayValues != null ? arrayValues.size() : 1);
600595

601596
Object[] bindArgs =
@@ -610,12 +605,8 @@ private Object[] fillBounds(
610605
? encodeSingleElement(arrayValues.get(i / statementsPerArrayValue))
611606
: EMPTY_BYTES_VALUE;
612607

613-
if (lowerBounds != null) {
614-
bindArgs[offset++] = lowerBounds[i % statementsPerArrayValue];
615-
}
616-
if (upperBounds != null) {
617-
bindArgs[offset++] = upperBounds[i % statementsPerArrayValue];
618-
}
608+
bindArgs[offset++] = lowerBounds[i % statementsPerArrayValue];
609+
bindArgs[offset++] = upperBounds[i % statementsPerArrayValue];
619610
}
620611
if (notInValues != null) {
621612
for (Object notInValue : notInValues) {
@@ -712,9 +703,7 @@ private byte[] encodeSingleElement(Value value) {
712703
* Encodes the given bounds according to the specification in {@code target}. For IN queries, a
713704
* list of possible values is returned.
714705
*/
715-
private @Nullable Object[] encodeBound(
716-
FieldIndex fieldIndex, Target target, @Nullable Bound bound) {
717-
if (bound == null) return null;
706+
private Object[] encodeBound(FieldIndex fieldIndex, Target target, Bound bound) {
718707
return encodeValues(fieldIndex, target, bound.getPosition());
719708
}
720709

‎firebase-firestore/src/main/java/com/google/firebase/firestore/model/Values.java

Lines changed: 26 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -228,28 +228,36 @@ public static int compare(Value left, Value right) {
228228
}
229229
}
230230

231-
public static @Nullable Value max(@Nullable Value left, @Nullable Value right) {
232-
if (left == null && right == null) {
233-
return null;
234-
} else if (left == null) {
235-
return right;
236-
} else if (right == null) {
237-
return left;
238-
} else {
239-
return compare(left, right) > 0 ? left : right;
231+
public static int lowerBoundCompare(
232+
Value left, boolean leftInclusive, Value right, boolean rightInclusive) {
233+
int cmp = compare(left, right);
234+
if (cmp != 0) {
235+
return cmp;
240236
}
237+
238+
if (leftInclusive && !rightInclusive) {
239+
return -1;
240+
} else if (!leftInclusive && rightInclusive) {
241+
return 1;
242+
}
243+
244+
return 0;
241245
}
242246

243-
public static @Nullable Value min(@Nullable Value left, @Nullable Value right) {
244-
if (left == null && right == null) {
245-
return null;
246-
} else if (left == null) {
247-
return right;
248-
} else if (right == null) {
249-
return left;
250-
} else {
251-
return compare(left, right) < 0 ? left : right;
247+
public static int upperBoundCompare(
248+
Value left, boolean leftInclusive, Value right, boolean rightInclusive) {
249+
int cmp = compare(left, right);
250+
if (cmp != 0) {
251+
return cmp;
252252
}
253+
254+
if (leftInclusive && !rightInclusive) {
255+
return 1;
256+
} else if (!leftInclusive && rightInclusive) {
257+
return -1;
258+
}
259+
260+
return 0;
253261
}
254262

255263
private static int compareNumbers(Value left, Value right) {

‎firebase-firestore/src/test/java/com/google/firebase/firestore/core/TargetTest.java

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323
import static com.google.firebase.firestore.testutil.TestUtil.query;
2424
import static com.google.firebase.firestore.testutil.TestUtil.wrap;
2525
import static org.junit.Assert.assertEquals;
26-
import static org.junit.Assert.assertNull;
2726
import static org.junit.Assert.assertTrue;
2827

2928
import com.google.firebase.firestore.model.FieldIndex;
@@ -151,10 +150,14 @@ public void orderByQueryBound() {
151150
FieldIndex index = fieldIndex("c", "foo", FieldIndex.Segment.Kind.ASCENDING);
152151

153152
Bound lowerBound = target.getLowerBound(index);
154-
assertNull(lowerBound);
153+
assertEquals(1, lowerBound.getPosition().size());
154+
assertTrue(Values.equals(lowerBound.getPosition().get(0), Values.MIN_VALUE));
155+
assertTrue(lowerBound.isInclusive());
155156

156157
Bound upperBound = target.getUpperBound(index);
157-
assertNull(upperBound);
158+
assertEquals(1, upperBound.getPosition().size());
159+
assertTrue(Values.equals(upperBound.getPosition().get(0), Values.MAX_VALUE));
160+
assertTrue(upperBound.isInclusive());
158161
}
159162

160163
@Test
@@ -179,7 +182,9 @@ public void startAtQueryBound() {
179182
verifyBound(lowerBound, true, "bar");
180183

181184
Bound upperBound = target.getUpperBound(index);
182-
assertNull(upperBound);
185+
assertEquals(1, upperBound.getPosition().size());
186+
assertTrue(Values.equals(upperBound.getPosition().get(0), Values.MAX_VALUE));
187+
assertTrue(upperBound.isInclusive());
183188
}
184189

185190
@Test
@@ -253,7 +258,9 @@ public void endAtQueryBound() {
253258
FieldIndex index = fieldIndex("c", "foo", FieldIndex.Segment.Kind.ASCENDING);
254259

255260
Bound lowerBound = target.getLowerBound(index);
256-
assertNull(lowerBound);
261+
assertEquals(1, lowerBound.getPosition().size());
262+
assertTrue(Values.equals(lowerBound.getPosition().get(0), Values.MIN_VALUE));
263+
assertTrue(lowerBound.isInclusive());
257264

258265
Bound upperBound = target.getUpperBound(index);
259266
verifyBound(upperBound, true, "bar");

‎firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteIndexManagerTest.java

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -603,6 +603,26 @@ public void testIndexEntriesAreUpdatedWithDeletedDoc() {
603603
verifyResults(query);
604604
}
605605

606+
@Test
607+
public void testCursorsDoNoExpandResultSet() {
608+
indexManager.addFieldIndex(fieldIndex("coll", "c", Kind.ASCENDING));
609+
indexManager.addFieldIndex(fieldIndex("coll", "c", Kind.DESCENDING));
610+
611+
addDoc("coll/val1", map("a", 1, "b", 1, "c", 3));
612+
addDoc("coll/val2", map("a", 2, "b", 2, "c", 2));
613+
614+
Query query =
615+
query("coll").filter(filter("c", ">", 2)).orderBy(orderBy("c")).startAt(bound(true, 2));
616+
verifyResults(query, "coll/val1");
617+
618+
query =
619+
query("coll")
620+
.filter(filter("c", "<", 3))
621+
.orderBy(orderBy("c", "desc"))
622+
.startAt(bound(true, 3));
623+
verifyResults(query, "coll/val2");
624+
}
625+
606626
@Test
607627
public void testAdvancedQueries() {
608628
// This test compares local query results with those received from the Java Server SDK.

0 commit comments

Comments
 (0)