Review

Author: schmidt-sebastian

firebase-firestore/src/main/java/com/google/firebase/firestore/core/Target.java

@@ -217,11 +217,11 @@ public Bound getLowerBound(FieldIndex fieldIndex) {
             case ARRAY_CONTAINS:
             case LESS_THAN_OR_EQUAL:
               largestValue = fieldFilter.getValue();
-              before = true;
+              before = false;
               break;
             case LESS_THAN:
               largestValue = fieldFilter.getValue();
-              before = false;
+              before = true;
               break;
           }
         }

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

@@ -110,8 +110,8 @@ public void addFieldIndex(FieldIndex index) {
 
   @Override
   public void addIndexEntries(Document document) {
-    ResourcePath documentPath = document.getKey().getPath();
-    String collectionGroup = documentPath.getSegment(documentPath.length() - 2);
+    DocumentKey documentKey = document.getKey();
+    String collectionGroup = documentKey.getCollectionGroup();
     db.query(
             "SELECT index_id, index_proto FROM index_configuration WHERE collection_group = ? AND active = 1")
         .binding(collectionGroup)
@@ -127,10 +127,10 @@ public void addIndexEntries(Document document) {
                 if (values == null) return;
 
                 if (Logger.isDebugEnabled()) {
-                  Logger.warn(
+                  Logger.debug(
                       TAG,
                       "Adding index values for document '%s' to index '%s'",
-                      documentPath,
+                      documentKey,
                       fieldIndex);
                 }
 
@@ -144,7 +144,7 @@ public void addIndexEntries(Document document) {
                           + "document_name) VALUES(?, ?, ?)",
                       indexId,
                       encoded,
-                      documentPath.canonicalString());
+                      documentKey.toString());
                 }
               } catch (InvalidProtocolBufferException e) {
                 throw fail("Invalid index: " + e);
@@ -176,6 +176,8 @@ public Set<DocumentKey> getDocumentsMatchingTarget(Target target) {
     if (fieldIndex == null) return null;
 
     Bound lowerBound = target.getLowerBound(fieldIndex);
+    String lowerBoundOp = lowerBound.isBefore() ? ">=" : ">"; // `startAt()` versus `startAfter()`
+
     @Nullable Bound upperBound = target.getUpperBound(fieldIndex);
 
     if (Logger.isDebugEnabled()) {
@@ -200,13 +202,15 @@ public Set<DocumentKey> getDocumentsMatchingTarget(Target target) {
           lowerBoundValues.size() == upperBoundValues.size(),
           "Expected upper and lower bound size to match");
 
+      String upperBoundOp = upperBound.isBefore() ? "<" : "<="; // `endBefore()` versus `endAt()`
+
       // TODO(indexing): To avoid reading the same documents multiple times, we should ideally only
       // send one query that combines all clauses.
       for (int i = 0; i < lowerBoundValues.size(); ++i) {
         db.query(
                 String.format(
                     "SELECT document_name from index_entries WHERE index_id = ? AND index_value %s ? AND index_value %s ?",
-                    lowerBound.isBefore() ? ">=" : ">", upperBound.isBefore() ? "<=" : "<"))
+                    lowerBoundOp, upperBoundOp))
             .binding(fieldIndex.getIndexId(), lowerBoundValues.get(i), upperBoundValues.get(i))
             .forEach(
                 row -> result.add(DocumentKey.fromPath(ResourcePath.fromString(row.getString(0)))));
@@ -218,7 +222,7 @@ public Set<DocumentKey> getDocumentsMatchingTarget(Target target) {
         db.query(
                 String.format(
                     "SELECT document_name from index_entries WHERE index_id = ? AND index_value %s  ?",
-                    lowerBound.isBefore() ? ">=" : ">"))
+                    lowerBoundOp))
             .binding(fieldIndex.getIndexId(), lowerBoundValue)
             .forEach(
                 row -> result.add(DocumentKey.fromPath(ResourcePath.fromString(row.getString(0)))));
@@ -259,15 +263,14 @@ public Set<DocumentKey> getDocumentsMatchingTarget(Target target) {
                 throw fail("Failed to decode index: " + e);
               }
             });
-    ;
 
     if (activeIndices.isEmpty()) {
       return null;
     }
 
     // Return the index with the most number of segments
-    Collections.sort(activeIndices, (l, r) -> Integer.compare(r.segmentCount(), l.segmentCount()));
-    return activeIndices.get(0);
+    return Collections.max(
+        activeIndices, (l, r) -> Integer.compare(l.segmentCount(), r.segmentCount()));
   }
 
   /**

firebase-firestore/src/main/java/com/google/firebase/firestore/model/DocumentKey.java

@@ -108,6 +108,11 @@ public ResourcePath getPath() {
     return path;
   }
 
+  /** Returns the collection group (i.e. the name of the parent collection) for this key. */
+  public String getCollectionGroup() {
+    return path.getSegment(path.length() - 2);
+  }
+
   /** Returns true if the document is in the specified collectionId. */
   public boolean hasCollectionId(String collectionId) {
     return path.length() >= 2 && path.segments.get(path.length() - 2).equals(collectionId);

firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteIndexManagerTest.java

@@ -77,13 +77,31 @@ Persistence getPersistence() {
     return PersistenceTestHelpers.createSQLitePersistence();
   }
 
+  @Test
+  public void addsDocuments() {
+    indexManager.addFieldIndex(
+        new FieldIndex("coll").withAddedField(field("exists"), FieldIndex.Segment.Kind.ORDERED));
+    addDoc("coll/doc1", map("exists", 1));
+    addDoc("coll/doc2", map());
+  }
+
   @Test
   public void testEqualityFilter() {
     setUpSingleValueFilter();
     Query query = query("coll").filter(filter("count", "==", 2));
     verifyResults(query, "coll/doc2");
   }
 
+  @Test
+  public void testNestedFieldEqualityFilter() {
+    indexManager.addFieldIndex(
+        new FieldIndex("coll").withAddedField(field("a.b"), FieldIndex.Segment.Kind.ORDERED));
+    addDoc("coll/doc1", map("a", map("b", 1)));
+    addDoc("coll/doc2", map("a", map("b", 2)));
+    Query query = query("coll").filter(filter("a.b", "==", 2));
+    verifyResults(query, "coll/doc2");
+  }
+
   @Test
   public void testNotEqualityFilter() {
     // TODO(indexing): Optimize != filters. We currently return all documents and do not exclude
@@ -145,14 +163,14 @@ public void testStartAfterFilter() {
   @Test
   public void testEndAtFilter() {
     setUpSingleValueFilter();
-    Query query = query("coll").orderBy(orderBy("count")).endAt(bound(true, 2));
+    Query query = query("coll").orderBy(orderBy("count")).endAt(bound(false, 2));
     verifyResults(query, "coll/doc1", "coll/doc2");
   }
 
   @Test
   public void testEndBeforeFilter() {
     setUpSingleValueFilter();
-    Query query = query("coll").orderBy(orderBy("count")).endAt(bound(false, 2));
+    Query query = query("coll").orderBy(orderBy("count")).endAt(bound(true, 2));
     verifyResults(query, "coll/doc1");
   }
 
@@ -165,7 +183,7 @@ public void testRangeWithBoundFilter() {
             .filter(filter("count", "<=", 3))
             .orderBy(orderBy("count"))
             .startAt(bound(false, 1))
-            .endAt(bound(true, 2));
+            .endAt(bound(false, 2));
     verifyResults(startAt, "coll/doc2");
   }