Skip to content

Fix cursor expanding result set bug. #3673

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

Merged
merged 1 commit into from
Apr 25, 2022
Merged

Fix cursor expanding result set bug. #3673

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 17 additions & 24 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/core/Target.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@

import static com.google.firebase.firestore.core.FieldFilter.Operator.ARRAY_CONTAINS;
import static com.google.firebase.firestore.core.FieldFilter.Operator.ARRAY_CONTAINS_ANY;
import static com.google.firebase.firestore.model.Values.max;
import static com.google.firebase.firestore.model.Values.min;
import static com.google.firebase.firestore.model.Values.MAX_VALUE;
import static com.google.firebase.firestore.model.Values.MIN_VALUE;
import static com.google.firebase.firestore.model.Values.lowerBoundCompare;
import static com.google.firebase.firestore.model.Values.upperBoundCompare;

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

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

if (segmentBound.first == null) {
// No lower bound exists
return null;
}

values.add(segmentBound.first);
inclusive &= segmentBound.second;
}
Expand All @@ -210,9 +206,9 @@ public Bound getLowerBound(FieldIndex fieldIndex) {

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

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

if (segmentBound.first == null) {
// No upper bound exists
return null;
}

values.add(segmentBound.first);
inclusive &= segmentBound.second;
}
Expand All @@ -244,12 +235,12 @@ public Bound getLowerBound(FieldIndex fieldIndex) {
*/
private Pair<Value, Boolean> getAscendingBound(
FieldIndex.Segment segment, @Nullable Bound bound) {
Value segmentValue = null;
Value segmentValue = MIN_VALUE;
boolean segmentInclusive = true;

// Process all filters to find a value for the current field segment
for (FieldFilter fieldFilter : getFieldFiltersForPath(segment.getFieldPath())) {
Value filterValue = null;
Value filterValue = MIN_VALUE;
boolean filterInclusive = true;

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

if (max(segmentValue, filterValue) == filterValue) {
if (lowerBoundCompare(segmentValue, segmentInclusive, filterValue, filterInclusive) < 0) {
segmentValue = filterValue;
segmentInclusive = filterInclusive;
}
Expand All @@ -287,7 +278,8 @@ private Pair<Value, Boolean> getAscendingBound(
OrderBy orderBy = this.orderBys.get(i);
if (orderBy.getField().equals(segment.getFieldPath())) {
Value cursorValue = bound.getPosition().get(i);
if (max(segmentValue, cursorValue) == cursorValue) {
if (lowerBoundCompare(segmentValue, segmentInclusive, cursorValue, bound.isInclusive())
< 0) {
segmentValue = cursorValue;
segmentInclusive = bound.isInclusive();
}
Expand All @@ -308,12 +300,12 @@ private Pair<Value, Boolean> getAscendingBound(
*/
private Pair<Value, Boolean> getDescendingBound(
FieldIndex.Segment segment, @Nullable Bound bound) {
Value segmentValue = null;
Value segmentValue = MAX_VALUE;
boolean segmentInclusive = true;

// Process all filters to find a value for the current field segment
for (FieldFilter fieldFilter : getFieldFiltersForPath(segment.getFieldPath())) {
Value filterValue = null;
Value filterValue = MAX_VALUE;
boolean filterInclusive = true;

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

if (min(segmentValue, filterValue) == filterValue) {
if (upperBoundCompare(segmentValue, segmentInclusive, filterValue, filterInclusive) > 0) {
segmentValue = filterValue;
segmentInclusive = filterInclusive;
}
Expand All @@ -352,7 +344,8 @@ private Pair<Value, Boolean> getDescendingBound(
OrderBy orderBy = this.orderBys.get(i);
if (orderBy.getField().equals(segment.getFieldPath())) {
Value cursorValue = bound.getPosition().get(i);
if (min(segmentValue, cursorValue) == cursorValue) {
if (upperBoundCompare(segmentValue, segmentInclusive, cursorValue, bound.isInclusive())
> 0) {
segmentValue = cursorValue;
segmentInclusive = bound.isInclusive();
}
Expand Down
43 changes: 16 additions & 27 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,8 +475,8 @@ public List<DocumentKey> getDocumentsMatchingTarget(Target target) {

@Nullable List<Value> arrayValues = subTarget.getArrayValues(fieldIndex);
@Nullable Collection<Value> notInValues = subTarget.getNotInValues(fieldIndex);
@Nullable Bound lowerBound = subTarget.getLowerBound(fieldIndex);
@Nullable Bound upperBound = subTarget.getUpperBound(fieldIndex);
Bound lowerBound = subTarget.getLowerBound(fieldIndex);
Bound upperBound = subTarget.getUpperBound(fieldIndex);

if (Logger.isDebugEnabled()) {
Logger.debug(
Expand All @@ -490,9 +490,9 @@ public List<DocumentKey> getDocumentsMatchingTarget(Target target) {
}

Object[] lowerBoundEncoded = encodeBound(fieldIndex, subTarget, lowerBound);
String lowerBoundOp = lowerBound != null && lowerBound.isInclusive() ? ">=" : ">";
String lowerBoundOp = lowerBound.isInclusive() ? ">=" : ">";
Object[] upperBoundEncoded = encodeBound(fieldIndex, subTarget, upperBound);
String upperBoundOp = upperBound != null && upperBound.isInclusive() ? "<=" : "<";
String upperBoundOp = upperBound.isInclusive() ? "<=" : "<";
Object[] notInEncoded = encodeValues(fieldIndex, subTarget, notInValues);

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

// Build the statement. We always include the lower bound, and optionally include an array value
// and an upper bound.
StringBuilder statement = new StringBuilder();
statement.append("SELECT document_key, directional_value FROM index_entries ");
statement.append("WHERE index_id = ? AND uid = ? ");
statement.append("AND array_value = ? ");
if (lowerBounds != null) {
statement.append("AND directional_value ").append(lowerBoundOp).append(" ? ");
}
if (upperBounds != null) {
statement.append("AND directional_value ").append(upperBoundOp).append(" ? ");
}
statement.append("AND directional_value ").append(lowerBoundOp).append(" ? ");
statement.append("AND directional_value ").append(upperBoundOp).append(" ? ");

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

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

if (lowerBounds != null) {
bindArgs[offset++] = lowerBounds[i % statementsPerArrayValue];
}
if (upperBounds != null) {
bindArgs[offset++] = upperBounds[i % statementsPerArrayValue];
}
bindArgs[offset++] = lowerBounds[i % statementsPerArrayValue];
bindArgs[offset++] = upperBounds[i % statementsPerArrayValue];
}
if (notInValues != null) {
for (Object notInValue : notInValues) {
Expand Down Expand Up @@ -712,9 +703,7 @@ private byte[] encodeSingleElement(Value value) {
* Encodes the given bounds according to the specification in {@code target}. For IN queries, a
* list of possible values is returned.
*/
private @Nullable Object[] encodeBound(
FieldIndex fieldIndex, Target target, @Nullable Bound bound) {
if (bound == null) return null;
private Object[] encodeBound(FieldIndex fieldIndex, Target target, Bound bound) {
return encodeValues(fieldIndex, target, bound.getPosition());
}

Expand Down
44 changes: 26 additions & 18 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/model/Values.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,28 +228,36 @@ public static int compare(Value left, Value right) {
}
}

public static @Nullable Value max(@Nullable Value left, @Nullable Value right) {
if (left == null && right == null) {
return null;
} else if (left == null) {
return right;
} else if (right == null) {
return left;
} else {
return compare(left, right) > 0 ? left : right;
public static int lowerBoundCompare(
Value left, boolean leftInclusive, Value right, boolean rightInclusive) {
int cmp = compare(left, right);
if (cmp != 0) {
return cmp;
}

if (leftInclusive && !rightInclusive) {
return -1;
} else if (!leftInclusive && rightInclusive) {
return 1;
}

return 0;
}

public static @Nullable Value min(@Nullable Value left, @Nullable Value right) {
if (left == null && right == null) {
return null;
} else if (left == null) {
return right;
} else if (right == null) {
return left;
} else {
return compare(left, right) < 0 ? left : right;
public static int upperBoundCompare(
Value left, boolean leftInclusive, Value right, boolean rightInclusive) {
int cmp = compare(left, right);
if (cmp != 0) {
return cmp;
}

if (leftInclusive && !rightInclusive) {
return 1;
} else if (!leftInclusive && rightInclusive) {
return -1;
}

return 0;
}

private static int compareNumbers(Value left, Value right) {
Expand Down
17 changes: 12 additions & 5 deletions firebase-firestore/src/test/java/com/google/firebase/firestore/core/TargetTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import static com.google.firebase.firestore.testutil.TestUtil.query;
import static com.google.firebase.firestore.testutil.TestUtil.wrap;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;

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

Bound lowerBound = target.getLowerBound(index);
assertNull(lowerBound);
assertEquals(1, lowerBound.getPosition().size());
assertTrue(Values.equals(lowerBound.getPosition().get(0), Values.MIN_VALUE));
assertTrue(lowerBound.isInclusive());

Bound upperBound = target.getUpperBound(index);
assertNull(upperBound);
assertEquals(1, upperBound.getPosition().size());
assertTrue(Values.equals(upperBound.getPosition().get(0), Values.MAX_VALUE));
assertTrue(upperBound.isInclusive());
}

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

Bound upperBound = target.getUpperBound(index);
assertNull(upperBound);
assertEquals(1, upperBound.getPosition().size());
assertTrue(Values.equals(upperBound.getPosition().get(0), Values.MAX_VALUE));
assertTrue(upperBound.isInclusive());
}

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

Bound lowerBound = target.getLowerBound(index);
assertNull(lowerBound);
assertEquals(1, lowerBound.getPosition().size());
assertTrue(Values.equals(lowerBound.getPosition().get(0), Values.MIN_VALUE));
assertTrue(lowerBound.isInclusive());

Bound upperBound = target.getUpperBound(index);
verifyBound(upperBound, true, "bar");
Expand Down
20 changes: 20 additions & 0 deletions ...e-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteIndexManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -603,6 +603,26 @@ public void testIndexEntriesAreUpdatedWithDeletedDoc() {
verifyResults(query);
}

@Test
public void testCursorsDoNoExpandResultSet() {
indexManager.addFieldIndex(fieldIndex("coll", "c", Kind.ASCENDING));
indexManager.addFieldIndex(fieldIndex("coll", "c", Kind.DESCENDING));

addDoc("coll/val1", map("a", 1, "b", 1, "c", 3));
addDoc("coll/val2", map("a", 2, "b", 2, "c", 2));

Query query =
query("coll").filter(filter("c", ">", 2)).orderBy(orderBy("c")).startAt(bound(true, 2));
verifyResults(query, "coll/val1");

query =
query("coll")
.filter(filter("c", "<", 3))
.orderBy(orderBy("c", "desc"))
.startAt(bound(true, 3));
verifyResults(query, "coll/val2");
}

@Test
public void testAdvancedQueries() {
// This test compares local query results with those received from the Java Server SDK.
Expand Down