Skip to content

Commit 75e2016

Browse files
wu-huiwu-hui
committed
Remove undesired exists() conditions from query-to-pipeline conversion
1 parent c8ada3c commit 75e2016

File tree

6 files changed

+109
-33
lines changed

6 files changed

+109
-33
lines changed

‎firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryToPipelineTest.java‎

Lines changed: 66 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,68 @@ public void testLimitToLastQueriesWithCursors() {
154154
data);
155155
}
156156

157+
@Test
158+
public void testNotInRemovesExistenceFilter() {
159+
CollectionReference collection =
160+
testCollectionWithDocs(
161+
map(
162+
"doc1", map("field", 2),
163+
"doc2", map("field", 1),
164+
"doc3", map()));
165+
166+
Query query = collection.whereNotIn("field", asList(1));
167+
FirebaseFirestore db = collection.firestore;
168+
Pipeline.Snapshot set = waitFor(db.pipeline().createFrom(query).execute());
169+
List<String> ids = pipelineSnapshotToIds(set);
170+
assertEquals(asList("doc3", "doc1"), ids);
171+
}
172+
173+
@Test
174+
public void testNotEqualRemovesExistenceFilter() {
175+
CollectionReference collection =
176+
testCollectionWithDocs(
177+
map(
178+
"doc1", map("field", 2),
179+
"doc2", map("field", 1),
180+
"doc3", map()));
181+
182+
Query query = collection.whereNotEqualTo("field", 1);
183+
FirebaseFirestore db = collection.firestore;
184+
Pipeline.Snapshot set = waitFor(db.pipeline().createFrom(query).execute());
185+
List<String> ids = pipelineSnapshotToIds(set);
186+
assertEquals(asList("doc3", "doc1"), ids);
187+
}
188+
189+
@Test
190+
public void testInequalityMaintainsExistenceFilter() {
191+
CollectionReference collection =
192+
testCollectionWithDocs(
193+
map(
194+
"doc1", map("field", 0),
195+
"doc2", map()));
196+
197+
Query query = collection.whereLessThan("field", 1);
198+
FirebaseFirestore db = collection.firestore;
199+
Pipeline.Snapshot set = waitFor(db.pipeline().createFrom(query).execute());
200+
List<String> ids = pipelineSnapshotToIds(set);
201+
assertEquals(asList("doc1"), ids);
202+
}
203+
204+
@Test
205+
public void testExplicitOrderMaintainsExistenceFilter() {
206+
CollectionReference collection =
207+
testCollectionWithDocs(
208+
map(
209+
"doc1", map("field", 1),
210+
"doc2", map()));
211+
212+
Query query = collection.orderBy("field");
213+
FirebaseFirestore db = collection.firestore;
214+
Pipeline.Snapshot set = waitFor(db.pipeline().createFrom(query).execute());
215+
List<String> ids = pipelineSnapshotToIds(set);
216+
assertEquals(asList("doc1"), ids);
217+
}
218+
157219
@Test
158220
public void testKeyOrderIsDescendingForDescendingInequality() {
159221
CollectionReference collection =
@@ -321,15 +383,14 @@ public void testQueriesCanUseNotEqualFilters() {
321383

322384
Map<String, Map<String, Object>> allDocs =
323385
map(
324-
"j", docJ, "a", docA, "b", docB, "c", docC, "d", docD, "e", docE, "f", docF, "g", docG,
325-
"h", docH, "i", docI);
386+
"i", docI, "j", docJ, "a", docA, "b", docB, "c", docC, "d", docD, "e", docE, "f", docF,
387+
"g", docG, "h", docH);
326388
CollectionReference collection = testCollectionWithDocs(allDocs);
327389
FirebaseFirestore db = collection.firestore;
328390

329391
// Search for zips not matching 98101.
330392
Map<String, Map<String, Object>> expectedDocsMap = new LinkedHashMap<>(allDocs);
331393
expectedDocsMap.remove("c");
332-
expectedDocsMap.remove("i");
333394

334395
Pipeline.Snapshot snapshot =
335396
waitFor(db.pipeline().createFrom(collection.whereNotEqualTo("zip", 98101L)).execute());
@@ -338,7 +399,6 @@ public void testQueriesCanUseNotEqualFilters() {
338399
// With objects.
339400
expectedDocsMap = new LinkedHashMap<>(allDocs);
340401
expectedDocsMap.remove("h");
341-
expectedDocsMap.remove("i");
342402
snapshot =
343403
waitFor(
344404
db.pipeline()
@@ -348,15 +408,13 @@ public void testQueriesCanUseNotEqualFilters() {
348408

349409
// With Null.
350410
expectedDocsMap = new LinkedHashMap<>(allDocs);
351-
expectedDocsMap.remove("i");
352411
expectedDocsMap.remove("j");
353412
snapshot = waitFor(db.pipeline().createFrom(collection.whereNotEqualTo("zip", null)).execute());
354413
assertEquals(Lists.newArrayList(expectedDocsMap.values()), pipelineSnapshotToValues(snapshot));
355414

356415
// With NaN.
357416
expectedDocsMap = new LinkedHashMap<>(allDocs);
358417
expectedDocsMap.remove("a");
359-
expectedDocsMap.remove("i");
360418
snapshot =
361419
waitFor(db.pipeline().createFrom(collection.whereNotEqualTo("zip", Double.NaN)).execute());
362420
assertEquals(Lists.newArrayList(expectedDocsMap.values()), pipelineSnapshotToValues(snapshot));
@@ -507,8 +565,8 @@ public void testQueriesCanUseNotInFilters() {
507565

508566
Map<String, Map<String, Object>> allDocs =
509567
map(
510-
"j", docJ, "a", docA, "b", docB, "c", docC, "d", docD, "e", docE, "f", docF, "g", docG,
511-
"h", docH, "i", docI);
568+
"i", docI, "j", docJ, "a", docA, "b", docB, "c", docC, "d", docD, "e", docE, "f", docF,
569+
"g", docG, "h", docH);
512570
CollectionReference collection = testCollectionWithDocs(allDocs);
513571
FirebaseFirestore db = collection.firestore;
514572

@@ -517,7 +575,6 @@ public void testQueriesCanUseNotInFilters() {
517575
expectedDocsMap.remove("c");
518576
expectedDocsMap.remove("d");
519577
expectedDocsMap.remove("f");
520-
expectedDocsMap.remove("i");
521578

522579
Pipeline.Snapshot snapshot =
523580
waitFor(
@@ -530,7 +587,6 @@ public void testQueriesCanUseNotInFilters() {
530587
// With objects.
531588
expectedDocsMap = new LinkedHashMap<>(allDocs);
532589
expectedDocsMap.remove("h");
533-
expectedDocsMap.remove("i");
534590
snapshot =
535591
waitFor(
536592
db.pipeline()
@@ -540,7 +596,6 @@ public void testQueriesCanUseNotInFilters() {
540596

541597
// With Null.
542598
expectedDocsMap = new LinkedHashMap<>(allDocs);
543-
expectedDocsMap.remove("i");
544599
expectedDocsMap.remove("j");
545600
snapshot =
546601
waitFor(db.pipeline().createFrom(collection.whereNotIn("zip", nullList())).execute());
@@ -549,7 +604,6 @@ public void testQueriesCanUseNotInFilters() {
549604
// With NaN.
550605
expectedDocsMap = new LinkedHashMap<>(allDocs);
551606
expectedDocsMap.remove("a");
552-
expectedDocsMap.remove("i");
553607
snapshot =
554608
waitFor(
555609
db.pipeline().createFrom(collection.whereNotIn("zip", asList(Double.NaN))).execute());
@@ -559,7 +613,6 @@ public void testQueriesCanUseNotInFilters() {
559613
expectedDocsMap = new LinkedHashMap<>(allDocs);
560614
expectedDocsMap.remove("a");
561615
expectedDocsMap.remove("c");
562-
expectedDocsMap.remove("i");
563616
snapshot =
564617
waitFor(
565618
db.pipeline()

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

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,10 @@ BooleanExpression toPipelineExpr() {
192192
case EQUAL:
193193
return and(exists, x.equal(value));
194194
case NOT_EQUAL:
195-
return and(exists, x.notEqual(value));
195+
// In Enterprise DBs NOT_EQUAL will match a field that does not exist,
196+
// therefore we do not want an existence filter for the NOT_EQUAL conversion
197+
// so the Query and Pipeline behavior are consistent in Enterprise.
198+
return x.notEqual(value);
196199
case GREATER_THAN:
197200
return and(exists, x.greaterThan(value));
198201
case GREATER_THAN_OR_EQUAL:
@@ -206,7 +209,10 @@ BooleanExpression toPipelineExpr() {
206209
case NOT_IN:
207210
{
208211
List<Value> list = value.getArrayValue().getValuesList();
209-
return and(exists, x.notEqualAny(list));
212+
// In Enterprise DBs NOT_IN will match a field that does not exist,
213+
// therefore we do not want an existence filter for the NOT_IN conversion
214+
// so the Query and Pipeline behavior are consistent in Enterprise.
215+
return x.notEqualAny(list);
210216
}
211217
default:
212218
// Handle OPERATOR_UNSPECIFIED and UNRECOGNIZED cases as needed

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

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
import com.google.firebase.firestore.model.Document;
1818
import com.google.firebase.firestore.model.FieldPath;
1919
import com.google.firebase.firestore.model.Values;
20-
import com.google.firebase.firestore.util.Assert;
2120
import com.google.firestore.v1.Value;
2221

2322
/** Represents a sort order for a Firestore Query */
@@ -64,8 +63,6 @@ int compare(Document d1, Document d2) {
6463
} else {
6564
Value v1 = d1.getField(field);
6665
Value v2 = d2.getField(field);
67-
Assert.hardAssert(
68-
v1 != null && v2 != null, "Trying to compare documents on fields that don't exist.");
6966
return direction.getComparisonModifier() * Values.compare(v1, v2);
7067
}
7168
}

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

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -556,9 +556,9 @@ private List<Stage<?>> convertToStages(UserDataReader userDataReader) {
556556

557557
// Orders
558558
List<OrderBy> normalizedOrderBy = getNormalizedOrderBy();
559-
int size = normalizedOrderBy.size();
560-
List<Field> fields = new ArrayList<>(size);
561-
List<Ordering> orderings = new ArrayList<>(size);
559+
List<Field> fields = new ArrayList<>(normalizedOrderBy.size());
560+
List<Ordering> orderings = new ArrayList<>(normalizedOrderBy.size());
561+
562562
for (OrderBy order : normalizedOrderBy) {
563563
Field field = new Field(order.getField());
564564
fields.add(field);
@@ -569,12 +569,23 @@ private List<Stage<?>> convertToStages(UserDataReader userDataReader) {
569569
}
570570
}
571571

572-
if (fields.size() == 1) {
573-
stages.add(new WhereStage(fields.get(0).exists(), InternalOptions.EMPTY));
574-
} else {
572+
// Only add existence filters for fields that are explicitly ordered.
573+
// Implicit order bys (e.g. from inequality filters) should not generate existence filters
574+
// because the inequality filter itself will already generate an existence filter if needed,
575+
// or arguably should not if it's a NOT_IN / NOT_EQUAL filter.
576+
List<Field> existenceCheckFields = new ArrayList<>();
577+
for (OrderBy order : explicitSortOrder) {
578+
existenceCheckFields.add(new Field(order.getField()));
579+
}
580+
if (existenceCheckFields.size() == 1) {
581+
stages.add(new WhereStage(existenceCheckFields.get(0).exists(), InternalOptions.EMPTY));
582+
} else if (existenceCheckFields.size() > 1) {
575583
BooleanExpression[] conditions =
576-
skipFirstToArray(fields, BooleanExpression[]::new, Expression.Companion::exists);
577-
stages.add(new WhereStage(and(fields.get(0).exists(), conditions), InternalOptions.EMPTY));
584+
skipFirstToArray(
585+
existenceCheckFields, BooleanExpression[]::new, Expression.Companion::exists);
586+
stages.add(
587+
new WhereStage(
588+
and(existenceCheckFields.get(0).exists(), conditions), InternalOptions.EMPTY));
578589
}
579590

580591
if (startAt != null) {

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -257,15 +257,15 @@ object Values {
257257
}
258258

259259
@JvmStatic
260-
fun compare(left: Value, right: Value): Int {
260+
fun compare(left: Value?, right: Value?): Int {
261261
val leftType = typeOrder(left)
262262
val rightType = typeOrder(right)
263263

264264
if (leftType != rightType) {
265265
return leftType.compareTo(rightType)
266266
}
267267

268-
return compareInternal(leftType, left, right)
268+
return compareInternal(leftType, left ?: NULL_VALUE, right ?: NULL_VALUE)
269269
}
270270

271271
private fun compareInternal(leftType: Int, left: Value, right: Value): Int =

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

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -589,12 +589,16 @@ public void orQueryDoesNotIncludeDocumentsWithMissingFields() throws Exception {
589589
assertEquals(docSet(query2.comparator(), doc1, doc2, doc4), result2);
590590

591591
// Query: a>2 || b==1.
592-
// This query has an implicit 'order by a'.
593-
// doc2 should not be included because it's missing the field 'a'.
594592
Query query3 = query("coll").filter(orFilters(filter("a", ">", 2), filter("b", "==", 1)));
595593
DocumentSet result3 =
596594
expectFullCollectionScan(() -> runQuery(query3, MISSING_LAST_LIMBO_FREE_SNAPSHOT));
597-
assertEquals(docSet(query3.comparator(), doc3), result3);
595+
if (shouldUsePipeline) {
596+
assertEquals(docSet(query3.comparator(), doc2, doc3), result3);
597+
} else {
598+
// This query has an implicit 'order by a' with standard edition.
599+
// doc2 should not be included because it's missing the field 'a'.
600+
assertEquals(docSet(query3.comparator(), doc3), result3);
601+
}
598602

599603
// Query: a>1 || b==1 order by a order by b.
600604
// doc6 should not be included because it's missing the field 'b'.
@@ -635,13 +639,18 @@ public void orQueryWithInAndNotIn() throws Exception {
635639
assertEquals(docSet(query1.comparator(), doc3, doc4, doc6), result1);
636640

637641
// a==2 || (b != 2 && b != 3)
638-
// Has implicit "orderBy b"
639642
Query query2 =
640643
query("coll")
641644
.filter(orFilters(filter("a", "==", 2), filter("b", "not-in", Arrays.asList(2, 3))));
642645
DocumentSet result2 =
643646
expectFullCollectionScan(() -> runQuery(query2, MISSING_LAST_LIMBO_FREE_SNAPSHOT));
644-
assertEquals(docSet(query2.comparator(), doc1, doc2), result2);
647+
if (shouldUsePipeline) {
648+
// Has implicit "orderBy b" but no "b exists" when converting to pipelines
649+
assertEquals(docSet(query2.comparator(), doc6, doc1, doc2), result2);
650+
} else {
651+
// Has implicit "orderBy b" and "b exists" with standard edition
652+
assertEquals(docSet(query2.comparator(), doc1, doc2), result2);
653+
}
645654
}
646655

647656
@Test

0 commit comments

Comments
 (0)