Address comments.

Author: ehsannas

firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexBackfiller.java

@@ -20,11 +20,9 @@
 import androidx.annotation.VisibleForTesting;
 import com.google.firebase.firestore.model.Document;
 import com.google.firebase.firestore.model.DocumentKey;
-import com.google.firebase.firestore.model.FieldIndex;
 import com.google.firebase.firestore.model.FieldIndex.IndexOffset;
 import com.google.firebase.firestore.util.AsyncQueue;
 import com.google.firebase.firestore.util.Logger;
-import java.util.Collection;
 import java.util.HashSet;
 import java.util.Map;
 import java.util.Set;
@@ -131,9 +129,7 @@ private int writeIndexEntries() {
   private int writeEntriesForCollectionGroup(
       String collectionGroup, int documentsRemainingUnderCap) {
     // Use the earliest offset of all field indexes to query the local cache.
-    Collection<FieldIndex> fieldIndexes = indexManager.getFieldIndexes(collectionGroup);
-    hardAssert(!fieldIndexes.isEmpty(), "Updating collection without indexes");
-    IndexOffset existingOffset = indexManager.getLeastRecentIndexOffset(fieldIndexes);
+    IndexOffset existingOffset = indexManager.minOffset(collectionGroup);
 
     LocalDocumentsResult nextBatch =
         localDocumentsView.getNextDocuments(

firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexManager.java

@@ -20,6 +20,7 @@
 import com.google.firebase.firestore.model.Document;
 import com.google.firebase.firestore.model.DocumentKey;
 import com.google.firebase.firestore.model.FieldIndex;
+import com.google.firebase.firestore.model.FieldIndex.IndexOffset;
 import com.google.firebase.firestore.model.ResourcePath;
 import java.util.Collection;
 import java.util.List;
@@ -74,17 +75,17 @@ public interface IndexManager {
   /** Returns all configured field indexes. */
   Collection<FieldIndex> getFieldIndexes();
 
-  /** Returns whether we can serve the given target from the index. */
+  /** 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 least
    * recent offset of them all. Asserts that the target can be served from index.
    */
-  FieldIndex.IndexOffset getLeastRecentIndexOffset(Target target);
+  IndexOffset minOffset(Target target);
 
-  /** Returns the lowest offset for the provided index group. */
-  FieldIndex.IndexOffset getLeastRecentIndexOffset(Collection<FieldIndex> fieldIndexes);
+  /** Returns the lowest offset for the given collection group. */
+  IndexOffset minOffset(String collectionGroup);
 
   /**
    * Returns an index that can be used to serve the provided target. Returns {@code null} if no

firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryIndexManager.java

@@ -21,6 +21,7 @@
 import com.google.firebase.firestore.model.Document;
 import com.google.firebase.firestore.model.DocumentKey;
 import com.google.firebase.firestore.model.FieldIndex;
+import com.google.firebase.firestore.model.FieldIndex.IndexOffset;
 import com.google.firebase.firestore.model.ResourcePath;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -82,7 +83,7 @@ public String getNextCollectionGroupToUpdate() {
   }
 
   @Override
-  public void updateCollectionGroup(String collectionGroup, FieldIndex.IndexOffset offset) {
+  public void updateCollectionGroup(String collectionGroup, IndexOffset offset) {
     // Field indices are not supported with memory persistence.
   }
 
@@ -104,13 +105,13 @@ public boolean canServeFromIndex(Target target) {
   }
 
   @Override
-  public FieldIndex.IndexOffset getLeastRecentIndexOffset(Target target) {
-    return FieldIndex.IndexOffset.NONE;
+  public IndexOffset minOffset(Target target) {
+    return IndexOffset.NONE;
   }
 
   @Override
-  public FieldIndex.IndexOffset getLeastRecentIndexOffset(Collection<FieldIndex> fieldIndexes) {
-    return FieldIndex.IndexOffset.NONE;
+  public IndexOffset minOffset(String collectionGroup) {
+    return IndexOffset.NONE;
   }
 
   @Override

firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java

@@ -118,8 +118,7 @@ public ImmutableSortedMap<DocumentKey, Document> getDocumentsMatchingQuery(
     ImmutableSortedMap<DocumentKey, Document> indexedDocuments =
         localDocumentsView.getDocuments(keys);
 
-    return appendRemainingResults(
-        values(indexedDocuments), query, indexManager.getLeastRecentIndexOffset(target));
+    return appendRemainingResults(values(indexedDocuments), query, indexManager.minOffset(target));
   }
 
   /**
@@ -238,11 +237,7 @@ private ImmutableSortedMap<DocumentKey, Document> appendRemainingResults(
     ImmutableSortedMap<DocumentKey, Document> remainingResults =
         localDocumentsView.getDocumentsMatchingQuery(query, offset);
     for (Document entry : indexedResults) {
-      // For OR queries, it is possible that a document that's been indexed also shows up in
-      // "remaining results" since we use the least recent IndexOffset of all DNF terms.
-      if (!remainingResults.containsKey(entry.getKey())) {
-        remainingResults = remainingResults.insert(entry.getKey(), entry);
-      }
+      remainingResults = remainingResults.insert(entry.getKey(), entry);
     }
     return remainingResults;
   }

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

@@ -23,7 +23,6 @@
 
 import android.text.TextUtils;
 import androidx.annotation.Nullable;
-import com.google.common.collect.ObjectArrays;
 import com.google.firebase.Timestamp;
 import com.google.firebase.database.collection.ImmutableSortedMap;
 import com.google.firebase.firestore.auth.User;
@@ -39,6 +38,7 @@
 import com.google.firebase.firestore.model.Document;
 import com.google.firebase.firestore.model.DocumentKey;
 import com.google.firebase.firestore.model.FieldIndex;
+import com.google.firebase.firestore.model.FieldIndex.IndexOffset;
 import com.google.firebase.firestore.model.FieldPath;
 import com.google.firebase.firestore.model.ResourcePath;
 import com.google.firebase.firestore.model.SnapshotVersion;
@@ -76,6 +76,7 @@ final class SQLiteIndexManager implements IndexManager {
    * Maps from a target to its equivalent list of sub-targets. Each sub-target contains only one
    * term from the target's disjunctive normal form (DNF).
    */
+  // TODO(orquery): Find a way for the GC algorithm to remove the mapping once we remove a target.
   private final Map<Target, List<Target>> targetToDnfSubTargets = new HashMap<>();
 
   /**
@@ -296,29 +297,34 @@ public boolean canServeFromIndex(Target target) {
     return true;
   }
 
-  @Override
-  public FieldIndex.IndexOffset getLeastRecentIndexOffset(Collection<FieldIndex> fieldIndexes) {
+  private IndexOffset getLeastRecentIndexOffset(Collection<FieldIndex> fieldIndexes) {
     hardAssert(
         !fieldIndexes.isEmpty(),
         "Found empty index group when looking for least recent index offset.");
 
     Iterator<FieldIndex> it = fieldIndexes.iterator();
-    FieldIndex.IndexOffset minOffset = it.next().getIndexState().getOffset();
+    IndexOffset minOffset = it.next().getIndexState().getOffset();
     int minBatchId = minOffset.getLargestBatchId();
     while (it.hasNext()) {
-      FieldIndex.IndexOffset newOffset = it.next().getIndexState().getOffset();
+      IndexOffset newOffset = it.next().getIndexState().getOffset();
       if (newOffset.compareTo(minOffset) < 0) {
         minOffset = newOffset;
       }
       minBatchId = Math.max(newOffset.getLargestBatchId(), minBatchId);
     }
 
-    return FieldIndex.IndexOffset.create(
-        minOffset.getReadTime(), minOffset.getDocumentKey(), minBatchId);
+    return IndexOffset.create(minOffset.getReadTime(), minOffset.getDocumentKey(), minBatchId);
+  }
+
+  @Override
+  public IndexOffset minOffset(String collectionGroup) {
+    Collection<FieldIndex> fieldIndexes = getFieldIndexes(collectionGroup);
+    hardAssert(!fieldIndexes.isEmpty(), "minOffset was called for collection without indexes");
+    return getLeastRecentIndexOffset(fieldIndexes);
   }
 
   @Override
-  public FieldIndex.IndexOffset getLeastRecentIndexOffset(Target target) {
+  public IndexOffset minOffset(Target target) {
     hardAssert(
         canServeFromIndex(target),
         "Cannot find least recent index offset if target cannot be served from index.");
@@ -494,14 +500,19 @@ public Set<DocumentKey> getDocumentsMatchingTarget(Target target) {
       bindings.addAll(Arrays.asList(subQueryAndBindings).subList(1, subQueryAndBindings.length));
     }
 
-    String queryString =
-        subQueries.size() == 1
-            ?
-            // If there's only one subQuery, just execute the one subQuery.
-            subQueries.get(0)
-            :
-            // Construct "subQuery1 UNION subQuery2 UNION ... LIMIT N"
-            TextUtils.join(" UNION ", subQueries) + " LIMIT " + target.getLimit();
+    String queryString;
+    if (subQueries.size() == 1) {
+      // If there's only one subQuery, just execute the one subQuery.
+      queryString = subQueries.get(0);
+    } else {
+      // Construct "SELECT * FROM (subQuery1 UNION subQuery2 UNION ...) LIMIT N"
+      queryString = "SELECT * FROM (" + TextUtils.join(" UNION ", subQueries) + ")";
+      if (target.getLimit() != -1) {
+        queryString = queryString + " LIMIT " + target.getLimit();
+      }
+    }
+
+    hardAssert(bindings.size() < 1000, "Cannot perform query with more than 999 bind elements");
 
     SQLitePersistence.Query query = db.query(queryString).binding(bindings.toArray());
 
@@ -569,7 +580,11 @@ private Object[] generateQueryAndBindings(
     // Fill in the bind ("question marks") variables.
     Object[] bindArgs =
         fillBounds(statementCount, indexId, arrayValues, lowerBounds, upperBounds, notIn);
-    return ObjectArrays.concat(sql.toString(), bindArgs);
+
+    List<Object> result = new ArrayList<Object>();
+    result.add(sql.toString());
+    result.addAll(Arrays.asList(bindArgs));
+    return result.toArray();
   }
 
   /** Returns the bind arguments for all {@code statementCount} statements. */
@@ -758,7 +773,7 @@ private byte[] encodeSegments(FieldIndex fieldIndex) {
   }
 
   @Override
-  public void updateCollectionGroup(String collectionGroup, FieldIndex.IndexOffset offset) {
+  public void updateCollectionGroup(String collectionGroup, IndexOffset offset) {
     hardAssert(started, "IndexManager not started");
 
     ++memoizedMaxSequenceNumber;

firebase-firestore/src/main/java/com/google/firebase/firestore/util/LogicUtils.java

@@ -27,17 +27,10 @@ public class LogicUtils {
   /**
    * Given a composite filter, returns the list of terms in its disjunctive normal form.
    *
-   * <p>Each element in the return value is one term of the resulting DNF.
-   *
-   * <p>For instance, for the input: (A || B) && C
-   *
-   * <p>The DNF form is: (A && C) || (B && C)
-   *
-   * <p>The return value is a list with two elements:
-   *
-   * <p>The first element is a composite filter that performs (A && C).
-   *
-   * <p>The second element is a composite filter that performs (B && C).
+   * <p>Each element in the return value is one term of the resulting DNF. For instance: For the
+   * input: (A || B) && C, the DNF form is: (A && C) || (B && C), and the return value is a list
+   * with two elements: a composite filter that performs (A && C), and a composite filter that
+   * performs (B && C).
    *
    * @param filter the composite filter to calculate DNF transform for.
    * @return the terms in the DNF transform.