From 601f001f046ea0db50bac84274517ed3f05af51c Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Tue, 15 Feb 2022 09:39:24 -0700 Subject: [PATCH] Backport indexing changes --- .../firebase/firestore/core/Target.java | 28 ++++++++----------- .../firestore/local/IndexManager.java | 8 +++--- .../firestore/local/MemoryIndexManager.java | 7 +---- .../firebase/firestore/local/QueryEngine.java | 5 ++-- .../firestore/local/SQLiteIndexManager.java | 25 +++++------------ .../firestore/local/IndexBackfillerTest.java | 8 +++--- .../local/SQLiteIndexManagerTest.java | 15 ++++++++-- 7 files changed, 43 insertions(+), 53 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Target.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Target.java index 20d59c5c93c..7b8e49898ea 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Target.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Target.java @@ -14,6 +14,8 @@ package com.google.firebase.firestore.core; +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.max; import static com.google.firebase.firestore.model.Values.min; @@ -212,14 +214,11 @@ public Bound getLowerBound(FieldIndex fieldIndex) { filterValue = Values.MIN_VALUE; break; case NOT_IN: - { - ArrayValue.Builder arrayValue = ArrayValue.newBuilder(); - for (int i = 0; i < fieldFilter.getValue().getArrayValue().getValuesCount(); ++i) { - arrayValue.addValues(Values.MIN_VALUE); - } - filterValue = Value.newBuilder().setArrayValue(arrayValue).build(); - break; - } + filterValue = + Value.newBuilder() + .setArrayValue(ArrayValue.newBuilder().addValues(MIN_VALUE)) + .build(); + break; default: // Remaining filters cannot be used as lower bounds. } @@ -295,14 +294,11 @@ public Bound getLowerBound(FieldIndex fieldIndex) { filterValue = Values.MAX_VALUE; break; case NOT_IN: - { - ArrayValue.Builder arrayValue = ArrayValue.newBuilder(); - for (int i = 0; i < fieldFilter.getValue().getArrayValue().getValuesCount(); ++i) { - arrayValue.addValues(Values.MAX_VALUE); - } - filterValue = Value.newBuilder().setArrayValue(arrayValue).build(); - break; - } + filterValue = + Value.newBuilder() + .setArrayValue(ArrayValue.newBuilder().addValues(MAX_VALUE)) + .build(); + break; default: // Remaining filters cannot be used as upper bounds. } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexManager.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexManager.java index d83c6b599dc..3735a912053 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexManager.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexManager.java @@ -75,9 +75,6 @@ public interface IndexManager { /** Returns all configured field indexes. */ Collection getFieldIndexes(); - /** Returns whether we can serve the given target from an index. */ - boolean canServeFromIndex(Target target); - /** * Iterates over all field indexes that are used to serve the given target, and returns the * minimum offset of them all. Asserts that the target can be served from index. @@ -94,7 +91,10 @@ public interface IndexManager { @Nullable FieldIndex getFieldIndex(Target target); - /** Returns the documents that match the given target based on the provided index. */ + /** + * Returns the documents that match the given target based on the provided index or {@code null} + * if the query cannot be served from an index. + */ Set getDocumentsMatchingTarget(Target target); /** Returns the next collection group to update. Returns {@code null} if no group exists. */ diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryIndexManager.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryIndexManager.java index 6b0d6e1f5e0..d4a28b04887 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryIndexManager.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryIndexManager.java @@ -72,7 +72,7 @@ public FieldIndex getFieldIndex(Target target) { @Nullable public Set getDocumentsMatchingTarget(Target target) { // Field indices are not supported with memory persistence. - return Collections.emptySet(); + return null; } @Nullable @@ -99,11 +99,6 @@ public Collection getFieldIndexes() { return Collections.emptyList(); } - @Override - public boolean canServeFromIndex(Target target) { - return false; - } - @Override public IndexOffset getMinOffset(Target target) { return IndexOffset.NONE; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java index bdb822e4578..bba66e80c93 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java @@ -110,14 +110,13 @@ public ImmutableSortedMap getDocumentsMatchingQuery( return null; } - if (!indexManager.canServeFromIndex(target)) { + Set keys = indexManager.getDocumentsMatchingTarget(target); + if (keys == null) { return null; } - Set keys = indexManager.getDocumentsMatchingTarget(target); ImmutableSortedMap indexedDocuments = localDocumentsView.getDocuments(keys); - return appendRemainingResults( values(indexedDocuments), query, indexManager.getMinOffset(target)); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java index 5fc07b85750..f58f7a5328f 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java @@ -279,18 +279,6 @@ public Collection getFieldIndexes() { return allIndices; } - @Override - public boolean canServeFromIndex(Target target) { - for (Target subTarget : getSubTargets(target)) { - // If any of the sub-queries cannot be served from the index, the target as a whole cannot be - // served from the index. - if (getFieldIndex(subTarget) == null) { - return false; - } - } - return true; - } - private IndexOffset getMinOffset(Collection fieldIndexes) { hardAssert( !fieldIndexes.isEmpty(), @@ -319,9 +307,6 @@ public IndexOffset getMinOffset(String collectionGroup) { @Override public IndexOffset getMinOffset(Target target) { - hardAssert( - canServeFromIndex(target), - "Cannot find least recent index offset if target cannot be served from index."); List fieldIndexes = new ArrayList<>(); for (Target subTarget : getSubTargets(target)) { fieldIndexes.add(getFieldIndex(subTarget)); @@ -458,6 +443,10 @@ public Set getDocumentsMatchingTarget(Target target) { for (Target subTarget : getSubTargets(target)) { FieldIndex fieldIndex = getFieldIndex(subTarget); + if (fieldIndex == null) { + return null; + } + @Nullable List arrayValues = subTarget.getArrayValues(fieldIndex); @Nullable List notInValues = subTarget.getNotInValues(fieldIndex); @Nullable Bound lowerBound = subTarget.getLowerBound(fieldIndex); @@ -684,13 +673,13 @@ private byte[] encodeSingleElement(Value value) { * queries, a list of possible values is returned. */ private @Nullable Object[] encodeValues( - FieldIndex fieldIndex, Target target, @Nullable List bound) { - if (bound == null) return null; + FieldIndex fieldIndex, Target target, @Nullable List values) { + if (values == null) return null; List encoders = new ArrayList<>(); encoders.add(new IndexByteEncoder()); - Iterator position = bound.iterator(); + Iterator position = values.iterator(); for (FieldIndex.Segment segment : fieldIndex.getDirectionalSegments()) { Value value = position.next(); for (IndexByteEncoder encoder : encoders) { diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexBackfillerTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexBackfillerTest.java index 6e770218c3e..c7199e74dd1 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexBackfillerTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexBackfillerTest.java @@ -510,12 +510,12 @@ private void addFieldIndex(String collectionGroup, String fieldName, long sequen private void verifyQueryResults(Query query, String... expectedKeys) { Target target = query.toTarget(); - if (indexManager.canServeFromIndex(target)) { - Set actualKeys = indexManager.getDocumentsMatchingTarget(target); + Set actualKeys = indexManager.getDocumentsMatchingTarget(target); + if (actualKeys == null) { + assertEquals(0, expectedKeys.length); + } else { assertThat(actualKeys) .containsExactlyElementsIn(Arrays.stream(expectedKeys).map(TestUtil::key).toArray()); - } else { - assertEquals(0, expectedKeys.length); } } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteIndexManagerTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteIndexManagerTest.java index d76149d0ecd..ce94e9de29c 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteIndexManagerTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteIndexManagerTest.java @@ -31,8 +31,8 @@ import static com.google.firebase.firestore.testutil.TestUtil.version; import static com.google.firebase.firestore.testutil.TestUtil.wrap; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertTrue; import com.google.firebase.firestore.auth.User; import com.google.firebase.firestore.core.Query; @@ -88,6 +88,16 @@ public void addsDocuments() { addDoc("coll/doc2", map()); } + @Test + public void testOrderByFilter() { + indexManager.addFieldIndex(fieldIndex("coll", "count", Kind.ASCENDING)); + addDoc("coll/val1", map("count", 1)); + addDoc("coll/val2", map("not-count", 2)); + addDoc("coll/val3", map("count", 3)); + Query query = query("coll").orderBy(orderBy("count")); + verifyResults(query, "coll/val1", "coll/val3"); + } + @Test public void testEqualityFilter() { setUpSingleValueFilter(); @@ -248,6 +258,7 @@ public void testNoMatchingFilter() { setUpSingleValueFilter(); Query query = query("coll").filter(filter("unknown", "==", true)); assertNull(indexManager.getFieldIndex(query.toTarget())); + assertNull(indexManager.getDocumentsMatchingTarget(query.toTarget())); } @Test @@ -709,8 +720,8 @@ private void addDoc(String key, Map data) { private void verifyResults(Query query, String... documents) { Target target = query.toTarget(); - assertTrue("Target cannot be served from index.", indexManager.canServeFromIndex(target)); Iterable results = indexManager.getDocumentsMatchingTarget(target); + assertNotNull("Target cannot be served from index.", results); List keys = Arrays.stream(documents).map(s -> key(s)).collect(Collectors.toList()); assertWithMessage("Result for %s", query).that(results).containsExactlyElementsIn(keys); }