Addressing code comments.

Author: MarkDuckworth

packages/firestore/src/core/query.ts

@@ -468,6 +468,12 @@ function queryMatchesPathAndCollectionGroup(
  * in the results.
  */
 function queryMatchesOrderBy(query: Query, doc: Document): boolean {
+  // We must use `queryOrderBy()` to get the list of all orderBys (both implicit and explicit).
+  // Note that for OR queries, orderBy applies to all disjunction terms and implicit orderBys must
+  // be taken into account. For example, the query "a > 1 || b==1" has an implicit "orderBy a" due
+  // to the inequality, and is evaluated as "a > 1 orderBy a || b==1 orderBy a".
+  // A document with content of {b:1} matches the filters, but does not match the orderBy because
+  // it's missing the field 'a'.
   for (const orderBy of queryOrderBy(query)) {
     // order by key always matches
     if (!orderBy.field.isKeyField() && doc.data.field(orderBy.field) === null) {

packages/firestore/src/core/target.ts

@@ -786,6 +786,15 @@ export class CompositeFilter extends Filter {
   }
 }
 
+// TODO(orquery) move compositeFilterWithAddedFilters to filter.ts in future refactor
+export function compositeFilterWithAddedFilters(
+  compositeFilter: CompositeFilter,
+  otherFilters: Filter[]
+): CompositeFilter {
+  const mergedFilters = compositeFilter.filters.concat(otherFilters);
+  return CompositeFilter.create(mergedFilters, CompositeOperator.AND);
+}
+
 export function compositeFilterIsConjunction(
   compositeFilter: CompositeFilter
 ): boolean {

packages/firestore/src/local/indexeddb_index_manager.ts

@@ -579,9 +579,9 @@ export class IndexedDbIndexManager implements IndexManager {
     let encoders: IndexByteEncoder[] = [];
     encoders.push(new IndexByteEncoder());
 
-    let boundIdx = 0;
+    let valueIdx = 0;
     for (const segment of fieldIndexGetDirectionalSegments(fieldIndex)) {
-      const value = values[boundIdx++];
+      const value = values[valueIdx++];
       for (const encoder of encoders) {
         if (this.isInFilter(target, segment.fieldPath) && isArray(value)) {
           encoders = this.expandIndexValues(encoders, segment, value);

packages/firestore/src/util/logic_utils.ts

@@ -21,6 +21,7 @@ import {
   compositeFilterIsDisjunction,
   compositeFilterIsFlat,
   compositeFilterIsFlatConjunction,
+  compositeFilterWithAddedFilters,
   CompositeOperator,
   FieldFilter,
   Filter
@@ -219,10 +220,7 @@ function applyDistributionCompositeFilters(
 
   // Case 1 is a merge.
   if (compositeFilterIsConjunction(lhs) && compositeFilterIsConjunction(rhs)) {
-    return CompositeFilter.create(
-      lhs.filters.concat(rhs.filters),
-      CompositeOperator.AND
-    );
+    return compositeFilterWithAddedFilters(lhs, rhs.getFilters());
   }
 
   // Case 2,3,4 all have at least one side (lhs or rhs) that is a disjunction. In all three cases
@@ -245,9 +243,9 @@ function applyDistributionFieldAndCompositeFilters(
   // A & (B | C) --> (A & B) | (A & C)
   if (compositeFilterIsConjunction(compositeFilter)) {
     // Case 1
-    return CompositeFilter.create(
-      compositeFilter.filters.concat(fieldFilter.getFilters()),
-      CompositeOperator.AND
+    return compositeFilterWithAddedFilters(
+      compositeFilter,
+      fieldFilter.getFilters()
     );
   } else {
     // Case 2

packages/firestore/test/unit/core/query.test.ts

@@ -861,10 +861,10 @@ describe('Query', () => {
     nonMatching: MutableDocument[]
   ): void {
     for (const doc of matching) {
-      expect(queryMatches(query, doc)).true;
+      expect(queryMatches(query, doc)).to.be.true;
     }
     for (const doc of nonMatching) {
-      expect(queryMatches(query, doc)).false;
+      expect(queryMatches(query, doc)).to.be.false;
     }
   }
 

packages/firestore/test/unit/util/logic_utils.test.ts

@@ -178,7 +178,7 @@ describe('LogicUtils', () => {
         applyDistribution(orFilter(A, B), orFilter(C, D)),
         expectedResult
       )
-    );
+    ).to.be.true;
   });
 
   it('implements field filter compute DNF', () => {