diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryToPipelineTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryToPipelineTest.java index 1b582515c87..1e30eb98a77 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryToPipelineTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryToPipelineTest.java @@ -154,6 +154,68 @@ public void testLimitToLastQueriesWithCursors() { data); } + @Test + public void testNotInRemovesExistenceFilter() { + CollectionReference collection = + testCollectionWithDocs( + map( + "doc1", map("field", 2), + "doc2", map("field", 1), + "doc3", map())); + + Query query = collection.whereNotIn("field", asList(1)); + FirebaseFirestore db = collection.firestore; + Pipeline.Snapshot set = waitFor(db.pipeline().createFrom(query).execute()); + List ids = pipelineSnapshotToIds(set); + assertEquals(asList("doc3", "doc1"), ids); + } + + @Test + public void testNotEqualRemovesExistenceFilter() { + CollectionReference collection = + testCollectionWithDocs( + map( + "doc1", map("field", 2), + "doc2", map("field", 1), + "doc3", map())); + + Query query = collection.whereNotEqualTo("field", 1); + FirebaseFirestore db = collection.firestore; + Pipeline.Snapshot set = waitFor(db.pipeline().createFrom(query).execute()); + List ids = pipelineSnapshotToIds(set); + assertEquals(asList("doc3", "doc1"), ids); + } + + @Test + public void testInequalityMaintainsExistenceFilter() { + CollectionReference collection = + testCollectionWithDocs( + map( + "doc1", map("field", 0), + "doc2", map())); + + Query query = collection.whereLessThan("field", 1); + FirebaseFirestore db = collection.firestore; + Pipeline.Snapshot set = waitFor(db.pipeline().createFrom(query).execute()); + List ids = pipelineSnapshotToIds(set); + assertEquals(asList("doc1"), ids); + } + + @Test + public void testExplicitOrderMaintainsExistenceFilter() { + CollectionReference collection = + testCollectionWithDocs( + map( + "doc1", map("field", 1), + "doc2", map())); + + Query query = collection.orderBy("field"); + FirebaseFirestore db = collection.firestore; + Pipeline.Snapshot set = waitFor(db.pipeline().createFrom(query).execute()); + List ids = pipelineSnapshotToIds(set); + assertEquals(asList("doc1"), ids); + } + @Test public void testKeyOrderIsDescendingForDescendingInequality() { CollectionReference collection = @@ -321,15 +383,14 @@ public void testQueriesCanUseNotEqualFilters() { Map> allDocs = map( - "j", docJ, "a", docA, "b", docB, "c", docC, "d", docD, "e", docE, "f", docF, "g", docG, - "h", docH, "i", docI); + "i", docI, "j", docJ, "a", docA, "b", docB, "c", docC, "d", docD, "e", docE, "f", docF, + "g", docG, "h", docH); CollectionReference collection = testCollectionWithDocs(allDocs); FirebaseFirestore db = collection.firestore; // Search for zips not matching 98101. Map> expectedDocsMap = new LinkedHashMap<>(allDocs); expectedDocsMap.remove("c"); - expectedDocsMap.remove("i"); Pipeline.Snapshot snapshot = waitFor(db.pipeline().createFrom(collection.whereNotEqualTo("zip", 98101L)).execute()); @@ -338,7 +399,6 @@ public void testQueriesCanUseNotEqualFilters() { // With objects. expectedDocsMap = new LinkedHashMap<>(allDocs); expectedDocsMap.remove("h"); - expectedDocsMap.remove("i"); snapshot = waitFor( db.pipeline() @@ -348,7 +408,6 @@ public void testQueriesCanUseNotEqualFilters() { // With Null. expectedDocsMap = new LinkedHashMap<>(allDocs); - expectedDocsMap.remove("i"); expectedDocsMap.remove("j"); snapshot = waitFor(db.pipeline().createFrom(collection.whereNotEqualTo("zip", null)).execute()); assertEquals(Lists.newArrayList(expectedDocsMap.values()), pipelineSnapshotToValues(snapshot)); @@ -356,7 +415,6 @@ public void testQueriesCanUseNotEqualFilters() { // With NaN. expectedDocsMap = new LinkedHashMap<>(allDocs); expectedDocsMap.remove("a"); - expectedDocsMap.remove("i"); snapshot = waitFor(db.pipeline().createFrom(collection.whereNotEqualTo("zip", Double.NaN)).execute()); assertEquals(Lists.newArrayList(expectedDocsMap.values()), pipelineSnapshotToValues(snapshot)); @@ -507,8 +565,8 @@ public void testQueriesCanUseNotInFilters() { Map> allDocs = map( - "j", docJ, "a", docA, "b", docB, "c", docC, "d", docD, "e", docE, "f", docF, "g", docG, - "h", docH, "i", docI); + "i", docI, "j", docJ, "a", docA, "b", docB, "c", docC, "d", docD, "e", docE, "f", docF, + "g", docG, "h", docH); CollectionReference collection = testCollectionWithDocs(allDocs); FirebaseFirestore db = collection.firestore; @@ -517,7 +575,6 @@ public void testQueriesCanUseNotInFilters() { expectedDocsMap.remove("c"); expectedDocsMap.remove("d"); expectedDocsMap.remove("f"); - expectedDocsMap.remove("i"); Pipeline.Snapshot snapshot = waitFor( @@ -530,7 +587,6 @@ public void testQueriesCanUseNotInFilters() { // With objects. expectedDocsMap = new LinkedHashMap<>(allDocs); expectedDocsMap.remove("h"); - expectedDocsMap.remove("i"); snapshot = waitFor( db.pipeline() @@ -540,7 +596,6 @@ public void testQueriesCanUseNotInFilters() { // With Null. expectedDocsMap = new LinkedHashMap<>(allDocs); - expectedDocsMap.remove("i"); expectedDocsMap.remove("j"); snapshot = waitFor(db.pipeline().createFrom(collection.whereNotIn("zip", nullList())).execute()); @@ -549,7 +604,6 @@ public void testQueriesCanUseNotInFilters() { // With NaN. expectedDocsMap = new LinkedHashMap<>(allDocs); expectedDocsMap.remove("a"); - expectedDocsMap.remove("i"); snapshot = waitFor( db.pipeline().createFrom(collection.whereNotIn("zip", asList(Double.NaN))).execute()); @@ -559,7 +613,6 @@ public void testQueriesCanUseNotInFilters() { expectedDocsMap = new LinkedHashMap<>(allDocs); expectedDocsMap.remove("a"); expectedDocsMap.remove("c"); - expectedDocsMap.remove("i"); snapshot = waitFor( db.pipeline() diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FieldFilter.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FieldFilter.java index 4d7f997fe39..986ce4a2f82 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FieldFilter.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FieldFilter.java @@ -192,7 +192,10 @@ BooleanExpression toPipelineExpr() { case EQUAL: return and(exists, x.equal(value)); case NOT_EQUAL: - return and(exists, x.notEqual(value)); + // In Enterprise DBs NOT_EQUAL will match a field that does not exist, + // therefore we do not want an existence filter for the NOT_EQUAL conversion + // so the Query and Pipeline behavior are consistent in Enterprise. + return x.notEqual(value); case GREATER_THAN: return and(exists, x.greaterThan(value)); case GREATER_THAN_OR_EQUAL: @@ -206,7 +209,10 @@ BooleanExpression toPipelineExpr() { case NOT_IN: { List list = value.getArrayValue().getValuesList(); - return and(exists, x.notEqualAny(list)); + // In Enterprise DBs NOT_IN will match a field that does not exist, + // therefore we do not want an existence filter for the NOT_IN conversion + // so the Query and Pipeline behavior are consistent in Enterprise. + return x.notEqualAny(list); } default: // Handle OPERATOR_UNSPECIFIED and UNRECOGNIZED cases as needed diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/OrderBy.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/OrderBy.java index 1fd8a42ada5..7de939d7011 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/OrderBy.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/OrderBy.java @@ -17,7 +17,6 @@ import com.google.firebase.firestore.model.Document; import com.google.firebase.firestore.model.FieldPath; import com.google.firebase.firestore.model.Values; -import com.google.firebase.firestore.util.Assert; import com.google.firestore.v1.Value; /** Represents a sort order for a Firestore Query */ @@ -64,8 +63,6 @@ int compare(Document d1, Document d2) { } else { Value v1 = d1.getField(field); Value v2 = d2.getField(field); - Assert.hardAssert( - v1 != null && v2 != null, "Trying to compare documents on fields that don't exist."); return direction.getComparisonModifier() * Values.compare(v1, v2); } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java index 7883894e1ed..0781f10c1b0 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java @@ -556,9 +556,9 @@ private List> convertToStages(UserDataReader userDataReader) { // Orders List normalizedOrderBy = getNormalizedOrderBy(); - int size = normalizedOrderBy.size(); - List fields = new ArrayList<>(size); - List orderings = new ArrayList<>(size); + List fields = new ArrayList<>(normalizedOrderBy.size()); + List orderings = new ArrayList<>(normalizedOrderBy.size()); + for (OrderBy order : normalizedOrderBy) { Field field = new Field(order.getField()); fields.add(field); @@ -569,12 +569,23 @@ private List> convertToStages(UserDataReader userDataReader) { } } - if (fields.size() == 1) { - stages.add(new WhereStage(fields.get(0).exists(), InternalOptions.EMPTY)); - } else { + // Only add existence filters for fields that are explicitly ordered. + // Implicit order bys (e.g. from inequality filters) should not generate existence filters + // because the inequality filter itself will already generate an existence filter if needed, + // or arguably should not if it's a NOT_IN / NOT_EQUAL filter. + List existenceCheckFields = new ArrayList<>(); + for (OrderBy order : explicitSortOrder) { + existenceCheckFields.add(new Field(order.getField())); + } + if (existenceCheckFields.size() == 1) { + stages.add(new WhereStage(existenceCheckFields.get(0).exists(), InternalOptions.EMPTY)); + } else if (existenceCheckFields.size() > 1) { BooleanExpression[] conditions = - skipFirstToArray(fields, BooleanExpression[]::new, Expression.Companion::exists); - stages.add(new WhereStage(and(fields.get(0).exists(), conditions), InternalOptions.EMPTY)); + skipFirstToArray( + existenceCheckFields, BooleanExpression[]::new, Expression.Companion::exists); + stages.add( + new WhereStage( + and(existenceCheckFields.get(0).exists(), conditions), InternalOptions.EMPTY)); } if (startAt != null) { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/Values.kt b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/Values.kt index 4bd087a600d..c0478f2c1f5 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/Values.kt +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/Values.kt @@ -257,7 +257,7 @@ object Values { } @JvmStatic - fun compare(left: Value, right: Value): Int { + fun compare(left: Value?, right: Value?): Int { val leftType = typeOrder(left) val rightType = typeOrder(right) @@ -265,7 +265,7 @@ object Values { return leftType.compareTo(rightType) } - return compareInternal(leftType, left, right) + return compareInternal(leftType, left ?: NULL_VALUE, right ?: NULL_VALUE) } private fun compareInternal(leftType: Int, left: Value, right: Value): Int = diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/QueryEngineTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/QueryEngineTestCase.java index 4d9b8a76ae7..79619662ec6 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/QueryEngineTestCase.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/QueryEngineTestCase.java @@ -589,12 +589,16 @@ public void orQueryDoesNotIncludeDocumentsWithMissingFields() throws Exception { assertEquals(docSet(query2.comparator(), doc1, doc2, doc4), result2); // Query: a>2 || b==1. - // This query has an implicit 'order by a'. - // doc2 should not be included because it's missing the field 'a'. Query query3 = query("coll").filter(orFilters(filter("a", ">", 2), filter("b", "==", 1))); DocumentSet result3 = expectFullCollectionScan(() -> runQuery(query3, MISSING_LAST_LIMBO_FREE_SNAPSHOT)); - assertEquals(docSet(query3.comparator(), doc3), result3); + if (shouldUsePipeline) { + assertEquals(docSet(query3.comparator(), doc2, doc3), result3); + } else { + // This query has an implicit 'order by a' with standard edition. + // doc2 should not be included because it's missing the field 'a'. + assertEquals(docSet(query3.comparator(), doc3), result3); + } // Query: a>1 || b==1 order by a order by b. // doc6 should not be included because it's missing the field 'b'. @@ -635,13 +639,18 @@ public void orQueryWithInAndNotIn() throws Exception { assertEquals(docSet(query1.comparator(), doc3, doc4, doc6), result1); // a==2 || (b != 2 && b != 3) - // Has implicit "orderBy b" Query query2 = query("coll") .filter(orFilters(filter("a", "==", 2), filter("b", "not-in", Arrays.asList(2, 3)))); DocumentSet result2 = expectFullCollectionScan(() -> runQuery(query2, MISSING_LAST_LIMBO_FREE_SNAPSHOT)); - assertEquals(docSet(query2.comparator(), doc1, doc2), result2); + if (shouldUsePipeline) { + // Has implicit "orderBy b" but no "b exists" when converting to pipelines + assertEquals(docSet(query2.comparator(), doc6, doc1, doc2), result2); + } else { + // Has implicit "orderBy b" and "b exists" with standard edition + assertEquals(docSet(query2.comparator(), doc1, doc2), result2); + } } @Test