Skip to content

Remove undesired exists() conditions from query-to-pipeline conversion #7634

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

Open
wu-hui wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Remove undesired exists() conditions from query-to-pipeline conversion #7634

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
79 changes: 66 additions & 13 deletions ...ase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryToPipelineTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<String> 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<String> 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<String> 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<String> ids = pipelineSnapshotToIds(set);
assertEquals(asList("doc1"), ids);
}

@Test
public void testKeyOrderIsDescendingForDescendingInequality() {
CollectionReference collection =
Expand Down Expand Up @@ -321,15 +383,14 @@ public void testQueriesCanUseNotEqualFilters() {

Map<String, Map<String, Object>> 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<String, Map<String, Object>> expectedDocsMap = new LinkedHashMap<>(allDocs);
expectedDocsMap.remove("c");
expectedDocsMap.remove("i");

Pipeline.Snapshot snapshot =
waitFor(db.pipeline().createFrom(collection.whereNotEqualTo("zip", 98101L)).execute());
Expand All @@ -338,7 +399,6 @@ public void testQueriesCanUseNotEqualFilters() {
// With objects.
expectedDocsMap = new LinkedHashMap<>(allDocs);
expectedDocsMap.remove("h");
expectedDocsMap.remove("i");
snapshot =
waitFor(
db.pipeline()
Expand All @@ -348,15 +408,13 @@ 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));

// 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));
Expand Down Expand Up @@ -507,8 +565,8 @@ public void testQueriesCanUseNotInFilters() {

Map<String, Map<String, Object>> 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;

Expand All @@ -517,7 +575,6 @@ public void testQueriesCanUseNotInFilters() {
expectedDocsMap.remove("c");
expectedDocsMap.remove("d");
expectedDocsMap.remove("f");
expectedDocsMap.remove("i");

Pipeline.Snapshot snapshot =
waitFor(
Expand All @@ -530,7 +587,6 @@ public void testQueriesCanUseNotInFilters() {
// With objects.
expectedDocsMap = new LinkedHashMap<>(allDocs);
expectedDocsMap.remove("h");
expectedDocsMap.remove("i");
snapshot =
waitFor(
db.pipeline()
Expand All @@ -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());
Expand All @@ -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());
Expand All @@ -559,7 +613,6 @@ public void testQueriesCanUseNotInFilters() {
expectedDocsMap = new LinkedHashMap<>(allDocs);
expectedDocsMap.remove("a");
expectedDocsMap.remove("c");
expectedDocsMap.remove("i");
snapshot =
waitFor(
db.pipeline()
Expand Down
10 changes: 8 additions & 2 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/core/FieldFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -206,7 +209,10 @@ BooleanExpression toPipelineExpr() {
case NOT_IN:
{
List<Value> 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
Expand Down
3 changes: 0 additions & 3 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/core/OrderBy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down Expand Up @@ -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);
}
}
Expand Down
27 changes: 19 additions & 8 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -556,9 +556,9 @@ private List<Stage<?>> convertToStages(UserDataReader userDataReader) {

// Orders
List<OrderBy> normalizedOrderBy = getNormalizedOrderBy();
int size = normalizedOrderBy.size();
List<Field> fields = new ArrayList<>(size);
List<Ordering> orderings = new ArrayList<>(size);
List<Field> fields = new ArrayList<>(normalizedOrderBy.size());
List<Ordering> orderings = new ArrayList<>(normalizedOrderBy.size());

for (OrderBy order : normalizedOrderBy) {
Field field = new Field(order.getField());
fields.add(field);
Expand All @@ -569,12 +569,23 @@ private List<Stage<?>> 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<Field> 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) {
Expand Down
4 changes: 2 additions & 2 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/model/Values.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,15 +257,15 @@ 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)

if (leftType != rightType) {
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 =
Expand Down
19 changes: 14 additions & 5 deletions ...base-firestore/src/test/java/com/google/firebase/firestore/local/QueryEngineTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'.
Expand Down Expand Up @@ -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
Expand Down
Loading