Skip to content

Commit e445aca

Browse files
schmidt-sebastianschmidt-sebastian
authored
Change Bound's before to inclusive (#2981)
1 parent d56db2a commit e445aca

File tree

13 files changed

+141
-128
lines changed

13 files changed

+141
-128
lines changed

‎firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -705,7 +705,7 @@ public Query limitToLast(long limit) {
705705
*/
706706
@NonNull
707707
public Query startAt(@NonNull DocumentSnapshot snapshot) {
708-
Bound bound = boundFromDocumentSnapshot("startAt", snapshot, /*before=*/ true);
708+
Bound bound = boundFromDocumentSnapshot("startAt", snapshot, /*inclusive=*/ true);
709709
return new Query(query.startAt(bound), firestore);
710710
}
711711

@@ -719,7 +719,7 @@ public Query startAt(@NonNull DocumentSnapshot snapshot) {
719719
*/
720720
@NonNull
721721
public Query startAt(Object... fieldValues) {
722-
Bound bound = boundFromFields("startAt", fieldValues, /*before=*/ true);
722+
Bound bound = boundFromFields("startAt", fieldValues, /*inclusive=*/ true);
723723
return new Query(query.startAt(bound), firestore);
724724
}
725725

@@ -733,7 +733,7 @@ public Query startAt(Object... fieldValues) {
733733
*/
734734
@NonNull
735735
public Query startAfter(@NonNull DocumentSnapshot snapshot) {
736-
Bound bound = boundFromDocumentSnapshot("startAfter", snapshot, /*before=*/ false);
736+
Bound bound = boundFromDocumentSnapshot("startAfter", snapshot, /*inclusive=*/ false);
737737
return new Query(query.startAt(bound), firestore);
738738
}
739739

@@ -748,7 +748,7 @@ public Query startAfter(@NonNull DocumentSnapshot snapshot) {
748748
*/
749749
@NonNull
750750
public Query startAfter(Object... fieldValues) {
751-
Bound bound = boundFromFields("startAfter", fieldValues, /*before=*/ false);
751+
Bound bound = boundFromFields("startAfter", fieldValues, /*inclusive=*/ false);
752752
return new Query(query.startAt(bound), firestore);
753753
}
754754

@@ -762,7 +762,7 @@ public Query startAfter(Object... fieldValues) {
762762
*/
763763
@NonNull
764764
public Query endBefore(@NonNull DocumentSnapshot snapshot) {
765-
Bound bound = boundFromDocumentSnapshot("endBefore", snapshot, /*before=*/ true);
765+
Bound bound = boundFromDocumentSnapshot("endBefore", snapshot, /*inclusive=*/ false);
766766
return new Query(query.endAt(bound), firestore);
767767
}
768768

@@ -776,7 +776,7 @@ public Query endBefore(@NonNull DocumentSnapshot snapshot) {
776776
*/
777777
@NonNull
778778
public Query endBefore(Object... fieldValues) {
779-
Bound bound = boundFromFields("endBefore", fieldValues, /*before=*/ true);
779+
Bound bound = boundFromFields("endBefore", fieldValues, /*inclusive=*/ false);
780780
return new Query(query.endAt(bound), firestore);
781781
}
782782

@@ -790,7 +790,7 @@ public Query endBefore(Object... fieldValues) {
790790
*/
791791
@NonNull
792792
public Query endAt(@NonNull DocumentSnapshot snapshot) {
793-
Bound bound = boundFromDocumentSnapshot("endAt", snapshot, /*before=*/ false);
793+
Bound bound = boundFromDocumentSnapshot("endAt", snapshot, /*inclusive=*/ true);
794794
return new Query(query.endAt(bound), firestore);
795795
}
796796

@@ -804,7 +804,7 @@ public Query endAt(@NonNull DocumentSnapshot snapshot) {
804804
*/
805805
@NonNull
806806
public Query endAt(Object... fieldValues) {
807-
Bound bound = boundFromFields("endAt", fieldValues, /*before=*/ false);
807+
Bound bound = boundFromFields("endAt", fieldValues, /*inclusive=*/ true);
808808
return new Query(query.endAt(bound), firestore);
809809
}
810810

@@ -818,7 +818,7 @@ public Query endAt(Object... fieldValues) {
818818
* any of the fields in the order by are an uncommitted server timestamp.
819819
*/
820820
private Bound boundFromDocumentSnapshot(
821-
String methodName, DocumentSnapshot snapshot, boolean before) {
821+
String methodName, DocumentSnapshot snapshot, boolean inclusive) {
822822
checkNotNull(snapshot, "Provided snapshot must not be null.");
823823
if (!snapshot.exists()) {
824824
throw new IllegalArgumentException(
@@ -857,11 +857,11 @@ private Bound boundFromDocumentSnapshot(
857857
}
858858
}
859859
}
860-
return new Bound(components, before);
860+
return new Bound(components, inclusive);
861861
}
862862

863863
/** Converts a list of field values to Bound. */
864-
private Bound boundFromFields(String methodName, Object[] values, boolean before) {
864+
private Bound boundFromFields(String methodName, Object[] values, boolean inclusive) {
865865
// Use explicit order by's because it has to match the query the user made
866866
List<OrderBy> explicitOrderBy = query.getExplicitOrderBy();
867867
if (values.length > explicitOrderBy.size()) {
@@ -913,7 +913,7 @@ private Bound boundFromFields(String methodName, Object[] values, boolean before
913913
}
914914
}
915915

916-
return new Bound(components, before);
916+
return new Bound(components, inclusive);
917917
}
918918

919919
/**

‎firebase-firestore/src/main/java/com/google/firebase/firestore/bundle/BundleSerializer.java

Lines changed: 24 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -146,8 +146,8 @@ private BundledQuery decodeBundledQuery(JSONObject bundledQuery) throws JSONExce
146146

147147
List<Filter> filters = decodeWhere(structuredQuery.optJSONObject("where"));
148148
List<OrderBy> orderBys = decodeOrderBy(structuredQuery.optJSONArray("orderBy"));
149-
@Nullable Bound startAt = decodeBound(structuredQuery.optJSONObject("startAt"));
150-
@Nullable Bound endAt = decodeBound(structuredQuery.optJSONObject("endAt"));
149+
@Nullable Bound startAt = decodeStartAtBound(structuredQuery.optJSONObject("startAt"));
150+
@Nullable Bound endAt = decodeEndAtBound(structuredQuery.optJSONObject("endAt"));
151151

152152
verifyNoOffset(structuredQuery);
153153
int limit = decodeLimit(structuredQuery);
@@ -167,24 +167,35 @@ private int decodeLimit(JSONObject structuredQuery) {
167167
}
168168
}
169169

170-
private Bound decodeBound(@Nullable JSONObject bound) throws JSONException {
170+
private Bound decodeStartAtBound(@Nullable JSONObject bound) throws JSONException {
171171
if (bound != null) {
172-
List<Value> cursor = new ArrayList<>();
173172
boolean before = bound.optBoolean("before", false);
174-
175-
JSONArray values = bound.optJSONArray("values");
176-
if (values != null) {
177-
for (int i = 0; i < values.length(); ++i) {
178-
cursor.add(decodeValue(values.getJSONObject(i)));
179-
}
180-
}
181-
182-
return new Bound(cursor, before);
173+
List<Value> position = decodePosition(bound);
174+
return new Bound(position, before);
183175
}
176+
return null;
177+
}
184178

179+
private Bound decodeEndAtBound(@Nullable JSONObject bound) throws JSONException {
180+
if (bound != null) {
181+
boolean before = bound.optBoolean("before", false);
182+
List<Value> position = decodePosition(bound);
183+
return new Bound(position, !before);
184+
}
185185
return null;
186186
}
187187

188+
private List<Value> decodePosition(JSONObject bound) throws JSONException {
189+
List<Value> cursor = new ArrayList<>();
190+
JSONArray values = bound.optJSONArray("values");
191+
if (values != null) {
192+
for (int i = 0; i < values.length(); ++i) {
193+
cursor.add(decodeValue(values.getJSONObject(i)));
194+
}
195+
}
196+
return cursor;
197+
}
198+
188199
private List<OrderBy> decodeOrderBy(@Nullable JSONArray orderBys) throws JSONException {
189200
List<OrderBy> result = new ArrayList<>();
190201

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

Lines changed: 31 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -38,33 +38,36 @@
3838
*/
3939
public final class Bound {
4040

41-
/** Whether this bound is just before or just after the provided position */
42-
private final boolean before;
41+
/**
42+
* Whether this bound includes the provided position (e.g. for {#code startAt()} or {#code
43+
* endAt()})
44+
*/
45+
private final boolean inclusive;
4346

4447
/** The index position of this bound */
4548
private final List<Value> position;
4649

47-
public Bound(List<Value> position, boolean before) {
50+
public Bound(List<Value> position, boolean inclusive) {
4851
this.position = position;
49-
this.before = before;
52+
this.inclusive = inclusive;
5053
}
5154

5255
public List<Value> getPosition() {
5356
return position;
5457
}
5558

56-
public boolean isBefore() {
57-
return before;
59+
/**
60+
* Whether the bound includes the documents that lie directly on the bound. Returns {@code true}
61+
* for {@code startAt()} and {@code endAt()} and false for {@code startAfter()} and {@code
62+
* endBefore()}.
63+
*/
64+
public boolean isInclusive() {
65+
return inclusive;
5866
}
5967

60-
public String canonicalString() {
68+
public String positionString() {
6169
// TODO: Make this collision robust.
6270
StringBuilder builder = new StringBuilder();
63-
if (before) {
64-
builder.append("b:");
65-
} else {
66-
builder.append("a:");
67-
}
6871
boolean first = true;
6972
for (Value indexComponent : position) {
7073
if (!first) {
@@ -78,6 +81,17 @@ public String canonicalString() {
7881

7982
/** Returns true if a document sorts before a bound using the provided sort order. */
8083
public boolean sortsBeforeDocument(List<OrderBy> orderBy, Document document) {
84+
int comparison = compareToDocument(orderBy, document);
85+
return inclusive ? comparison <= 0 : comparison < 0;
86+
}
87+
88+
/** Returns true if a document sorts after a bound using the provided sort order. */
89+
public boolean sortsAfterDocument(List<OrderBy> orderBy, Document document) {
90+
int comparison = compareToDocument(orderBy, document);
91+
return inclusive ? comparison >= 0 : comparison > 0;
92+
}
93+
94+
private int compareToDocument(List<OrderBy> orderBy, Document document) {
8195
hardAssert(position.size() <= orderBy.size(), "Bound has more components than query's orderBy");
8296
int comparison = 0;
8397
for (int i = 0; i < position.size(); i++) {
@@ -105,8 +119,7 @@ public boolean sortsBeforeDocument(List<OrderBy> orderBy, Document document) {
105119
break;
106120
}
107121
}
108-
109-
return before ? comparison <= 0 : comparison < 0;
122+
return comparison;
110123
}
111124

112125
@Override
@@ -120,21 +133,21 @@ public boolean equals(Object o) {
120133

121134
Bound bound = (Bound) o;
122135

123-
return before == bound.before && position.equals(bound.position);
136+
return inclusive == bound.inclusive && position.equals(bound.position);
124137
}
125138

126139
@Override
127140
public int hashCode() {
128-
int result = (before ? 1 : 0);
141+
int result = (inclusive ? 1 : 0);
129142
result = 31 * result + position.hashCode();
130143
return result;
131144
}
132145

133146
@Override
134147
public String toString() {
135148
StringBuilder builder = new StringBuilder();
136-
builder.append("Bound(before=");
137-
builder.append(before);
149+
builder.append("Bound(inclusive=");
150+
builder.append(inclusive);
138151
builder.append(", position=");
139152
for (int i = 0; i < position.size(); i++) {
140153
if (i > 0) {

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

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -448,7 +448,7 @@ private boolean matchesBounds(Document doc) {
448448
if (startAt != null && !startAt.sortsBeforeDocument(getOrderBy(), doc)) {
449449
return false;
450450
}
451-
if (endAt != null && endAt.sortsBeforeDocument(getOrderBy(), doc)) {
451+
if (endAt != null && !endAt.sortsAfterDocument(getOrderBy(), doc)) {
452452
return false;
453453
}
454454
return true;
@@ -520,10 +520,12 @@ public Target toTarget() {
520520

521521
// We need to swap the cursors to match the now-flipped query ordering.
522522
Bound newStartAt =
523-
this.endAt != null ? new Bound(this.endAt.getPosition(), !this.endAt.isBefore()) : null;
523+
this.endAt != null
524+
? new Bound(this.endAt.getPosition(), !this.endAt.isInclusive())
525+
: null;
524526
Bound newEndAt =
525527
this.startAt != null
526-
? new Bound(this.startAt.getPosition(), !this.startAt.isBefore())
528+
? new Bound(this.startAt.getPosition(), !this.startAt.isInclusive())
527529
: null;
528530

529531
this.memoizedTarget =

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

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,7 @@ public Bound getLowerBound(FieldIndex fieldIndex) {
180180
Value cursorValue = startAt.getPosition().get(i);
181181
if (max(segmentValue, cursorValue) == cursorValue) {
182182
segmentValue = cursorValue;
183-
segmentInclusive = startAt.isBefore();
183+
segmentInclusive = startAt.isInclusive();
184184
}
185185
break;
186186
}
@@ -256,7 +256,7 @@ public Bound getLowerBound(FieldIndex fieldIndex) {
256256
Value cursorValue = endAt.getPosition().get(i);
257257
if (min(segmentValue, cursorValue) == cursorValue) {
258258
segmentValue = cursorValue;
259-
segmentInclusive = !endAt.isBefore();
259+
segmentInclusive = endAt.isInclusive();
260260
}
261261
break;
262262
}
@@ -276,7 +276,7 @@ public Bound getLowerBound(FieldIndex fieldIndex) {
276276
return null;
277277
}
278278

279-
return new Bound(values, !inclusive);
279+
return new Bound(values, inclusive);
280280
}
281281

282282
public List<OrderBy> getOrderBy() {
@@ -318,12 +318,14 @@ public String getCanonicalId() {
318318

319319
if (startAt != null) {
320320
builder.append("|lb:");
321-
builder.append(startAt.canonicalString());
321+
builder.append(startAt.isInclusive() ? "b:" : "a:");
322+
builder.append(startAt.positionString());
322323
}
323324

324325
if (endAt != null) {
325326
builder.append("|ub:");
326-
builder.append(endAt.canonicalString());
327+
builder.append(endAt.isInclusive() ? "a:" : "b:");
328+
builder.append(endAt.positionString());
327329
}
328330

329331
memoizedCannonicalId = builder.toString();

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,8 @@ public void addFieldIndex(FieldIndex index) {
9696
db.query("SELECT MAX(index_id) FROM index_configuration")
9797
.firstValue(input -> input.isNull(0) ? 0 : input.getInt(0));
9898

99+
// TODO(indexing): Properly dedupe indices to avoid duplicate index entries (by comparing
100+
// collection_group+index_proto)
99101
db.execute(
100102
"INSERT OR IGNORE INTO index_configuration ("
101103
+ "index_id, "
@@ -176,7 +178,7 @@ public Set<DocumentKey> getDocumentsMatchingTarget(Target target) {
176178
if (fieldIndex == null) return null;
177179

178180
Bound lowerBound = target.getLowerBound(fieldIndex);
179-
String lowerBoundOp = lowerBound.isBefore() ? ">=" : ">"; // `startAt()` versus `startAfter()`
181+
String lowerBoundOp = lowerBound.isInclusive() ? ">=" : ">";
180182

181183
@Nullable Bound upperBound = target.getUpperBound(fieldIndex);
182184

@@ -202,7 +204,7 @@ public Set<DocumentKey> getDocumentsMatchingTarget(Target target) {
202204
lowerBoundValues.size() == upperBoundValues.size(),
203205
"Expected upper and lower bound size to match");
204206

205-
String upperBoundOp = upperBound.isBefore() ? "<" : "<="; // `endBefore()` versus `endAt()`
207+
String upperBoundOp = upperBound.isInclusive() ? "<=" : "<";
206208

207209
// TODO(indexing): To avoid reading the same documents multiple times, we should ideally only
208210
// send one query that combines all clauses.

0 commit comments

Comments
 (0)