Markduckworth/or queries pr 4

common/api-review/firestore.api.md

@@ -17,6 +17,9 @@ export type AddPrefixToKeys<Prefix extends string, T extends Record<string, unkn
     [K in keyof T & string as `${Prefix}.${K}`]+?: T[K];
 };
 
+// @public
+export function and(...queryConstraints: QueryConstraint[]): QueryConstraint;
+
 // @public
 export function arrayRemove(...elements: unknown[]): FieldValue;
 
@@ -321,6 +324,9 @@ export function onSnapshotsInSync(firestore: Firestore, observer: {
 // @public
 export function onSnapshotsInSync(firestore: Firestore, onSync: () => void): Unsubscribe;
 
+// @public
+export function or(...queryConstraints: QueryConstraint[]): QueryConstraint;
+
 // @public
 export function orderBy(fieldPath: string | FieldPath, directionStr?: OrderByDirection): QueryConstraint;
 

packages/firestore/src/api.ts

@@ -72,13 +72,15 @@ export {
 } from './api/reference';
 
 export {
+  and,
   endAt,
   endBefore,
   startAt,
   startAfter,
   limit,
   limitToLast,
   where,
+  or,
   orderBy,
   query,
   QueryConstraint,

packages/firestore/src/api/filter.ts

@@ -16,12 +16,14 @@
  */
 
 export {
+  and,
   endAt,
   endBefore,
   startAfter,
   startAt,
   limitToLast,
   limit,
+  or,
   orderBy,
   OrderByDirection,
   where,

packages/firestore/src/core/query.ts

@@ -468,7 +468,7 @@ function queryMatchesPathAndCollectionGroup(
  * in the results.
  */
 function queryMatchesOrderBy(query: Query, doc: Document): boolean {
-  for (const orderBy of query.explicitOrderBy) {
+  for (const orderBy of queryOrderBy(query)) {
     // order by key always matches
     if (!orderBy.field.isKeyField() && doc.data.field(orderBy.field) === null) {
       return false;

packages/firestore/src/core/target.ts

@@ -500,26 +500,25 @@ export function targetGetSegmentCount(target: Target): number {
   let hasArraySegment = false;
 
   for (const filter of target.filters) {
-    // TODO(orquery): Use the flattened filters here
-    const fieldFilter = filter as FieldFilter;
-
-    // __name__ is not an explicit segment of any index, so we don't need to
-    // count it.
-    if (fieldFilter.field.isKeyField()) {
-      continue;
-    }
+    for (const subFilter of filter.getFlattenedFilters()) {
+      // __name__ is not an explicit segment of any index, so we don't need to
+      // count it.
+      if (subFilter.field.isKeyField()) {
+        continue;
+      }
 
-    // ARRAY_CONTAINS or ARRAY_CONTAINS_ANY filters must be counted separately.
-    // For instance, it is possible to have an index for "a ARRAY a ASC". Even
-    // though these are on the same field, they should be counted as two
-    // separate segments in an index.
-    if (
-      fieldFilter.op === Operator.ARRAY_CONTAINS ||
-      fieldFilter.op === Operator.ARRAY_CONTAINS_ANY
-    ) {
-      hasArraySegment = true;
-    } else {
-      fields = fields.add(fieldFilter.field);
+      // ARRAY_CONTAINS or ARRAY_CONTAINS_ANY filters must be counted separately.
+      // For instance, it is possible to have an index for "a ARRAY a ASC". Even
+      // though these are on the same field, they should be counted as two
+      // separate segments in an index.
+      if (
+        subFilter.op === Operator.ARRAY_CONTAINS ||
+        subFilter.op === Operator.ARRAY_CONTAINS_ANY
+      ) {
+        hasArraySegment = true;
+      } else {
+        fields = fields.add(subFilter.field);
+      }
     }
   }
 
@@ -534,12 +533,16 @@ export function targetGetSegmentCount(target: Target): number {
   return fields.size + (hasArraySegment ? 1 : 0);
 }
 
+export function targetHasLimit(target: Target): boolean {
+  return target.limit !== null;
+}
+
 export abstract class Filter {
   abstract matches(doc: Document): boolean;
 
   abstract getFlattenedFilters(): readonly FieldFilter[];
 
-  abstract getFilters(): readonly Filter[];
+  abstract getFilters(): Filter[];
 
   abstract getFirstInequalityField(): FieldPath | null;
 }
@@ -702,7 +705,7 @@ export class FieldFilter extends Filter {
     return [this];
   }
 
-  getFilters(): readonly Filter[] {
+  getFilters(): Filter[] {
     return [this];
   }
 
@@ -753,8 +756,9 @@ export class CompositeFilter extends Filter {
     return this.memoizedFlattenedFilters;
   }
 
-  getFilters(): readonly Filter[] {
-    return this.filters;
+  // Returns a mutable copy of `this.filters`
+  getFilters(): Filter[] {
+    return Object.assign([], this.filters);
   }
 
   getFirstInequalityField(): FieldPath | null {
@@ -788,6 +792,12 @@ export function compositeFilterIsConjunction(
   return compositeFilter.op === CompositeOperator.AND;
 }
 
+export function compositeFilterIsDisjunction(
+  compositeFilter: CompositeFilter
+): boolean {
+  return compositeFilter.op === CompositeOperator.OR;
+}
+
 /**
  * Returns true if this filter is a conjunction of field filters only. Returns false otherwise.
  */

packages/firestore/src/lite-api/query.ts

@@ -994,7 +994,10 @@ export function validateQueryFilterConstraint(
   functionName: string,
   queryConstraint: QueryConstraint
 ): void {
-  if (!(queryConstraint instanceof QueryFilterConstraint)) {
+  if (
+    !(queryConstraint instanceof QueryFilterConstraint) &&
+    !(queryConstraint instanceof QueryCompositeFilterConstraint)
+  ) {
     throw new FirestoreError(
       Code.INVALID_ARGUMENT,
       `Function ${functionName}() requires QueryContraints created with a call to 'where(...)'.`

packages/firestore/src/local/indexeddb_index_manager.ts

@@ -20,15 +20,20 @@ import { DatabaseId } from '../core/database_info';
 import {
   Bound,
   canonifyTarget,
+  CompositeFilter,
+  CompositeOperator,
   FieldFilter,
+  Filter,
+  newTarget,
   Operator,
   Target,
   targetEquals,
   targetGetArrayValues,
   targetGetLowerBound,
   targetGetNotInValues,
   targetGetSegmentCount,
-  targetGetUpperBound
+  targetGetUpperBound,
+  targetHasLimit
 } from '../core/target';
 import { FirestoreIndexValueWriter } from '../index/firestore_index_value_writer';
 import { IndexByteEncoder } from '../index/index_byte_encoder';
@@ -53,6 +58,7 @@ import { isArray, refValue } from '../model/values';
 import { Value as ProtoValue } from '../protos/firestore_proto_api';
 import { debugAssert, fail, hardAssert } from '../util/assert';
 import { logDebug } from '../util/log';
+import { getDnfTerms } from '../util/logic_utils';
 import { immediateSuccessor, primitiveComparator } from '../util/misc';
 import { ObjectMap } from '../util/obj_map';
 import { diffSortedSets, SortedSet } from '../util/sorted_set';
@@ -333,8 +339,28 @@ export class IndexedDbIndexManager implements IndexManager {
     if (subTargets) {
       return subTargets;
     }
-    // TODO(orquery): Implement DNF transform
-    subTargets = [target];
+
+    if (target.filters.length === 0) {
+      subTargets = [target];
+    } else {
+      // There is an implicit AND operation between all the filters stored in the target
+      const dnf: Filter[] = getDnfTerms(
+        CompositeFilter.create(target.filters, CompositeOperator.AND)
+      );
+
+      subTargets = dnf.map(term =>
+        newTarget(
+          target.path,
+          target.collectionGroup,
+          target.orderBy,
+          term.getFilters(),
+          target.limit,
+          target.startAt,
+          target.endAt
+        )
+      );
+    }
+
     this.targetToDnfSubTargets.set(target, subTargets);
     return subTargets;
   }
@@ -459,21 +485,32 @@ export class IndexedDbIndexManager implements IndexManager {
     target: Target
   ): PersistencePromise<IndexType> {
     let indexType = IndexType.FULL;
-    return PersistencePromise.forEach(
-      this.getSubTargets(target),
-      (target: Target) => {
-        return this.getFieldIndex(transaction, target).next(index => {
-          if (!index) {
-            indexType = IndexType.NONE;
-          } else if (
-            indexType !== IndexType.NONE &&
-            index.fields.length < targetGetSegmentCount(target)
-          ) {
-            indexType = IndexType.PARTIAL;
-          }
-        });
+    const subTargets = this.getSubTargets(target);
+    return PersistencePromise.forEach(subTargets, (target: Target) => {
+      return this.getFieldIndex(transaction, target).next(index => {
+        if (!index) {
+          indexType = IndexType.NONE;
+        } else if (
+          indexType !== IndexType.NONE &&
+          index.fields.length < targetGetSegmentCount(target)
+        ) {
+          indexType = IndexType.PARTIAL;
+        }
+      });
+    }).next(() => {
+      // OR queries have more than one sub-target (one sub-target per DNF term). We currently consider
+      // OR queries that have a `limit` to have a partial index. For such queries we perform sorting
+      // and apply the limit in memory as a post-processing step.
+      if (
+        targetHasLimit(target) &&
+        subTargets.length > 1 &&
+        indexType === IndexType.FULL
+      ) {
+        return IndexType.PARTIAL;
       }
-    ).next(() => indexType);
+
+      return indexType;
+    });
   }
 
   /**
@@ -533,9 +570,9 @@ export class IndexedDbIndexManager implements IndexManager {
   private encodeValues(
     fieldIndex: FieldIndex,
     target: Target,
-    bound: ProtoValue[] | null
+    values: ProtoValue[] | null
   ): Uint8Array[] {
-    if (bound === null) {
+    if (values === null) {
       return [];
     }
 
@@ -544,7 +581,7 @@ export class IndexedDbIndexManager implements IndexManager {
 
     let boundIdx = 0;
     for (const segment of fieldIndexGetDirectionalSegments(fieldIndex)) {
-      const value = bound[boundIdx++];
+      const value = values[boundIdx++];
       for (const encoder of encoders) {
         if (this.isInFilter(target, segment.fieldPath) && isArray(value)) {
           encoders = this.expandIndexValues(encoders, segment, value);

packages/firestore/src/local/query_engine.ts

@@ -19,7 +19,6 @@ import {
   LimitType,
   newQueryComparator,
   Query,
-  queryContainsCompositeFilters,
   queryMatches,
   queryMatchesAllDocuments,
   queryToTarget,
@@ -134,10 +133,7 @@ export class QueryEngine {
     transaction: PersistenceTransaction,
     query: Query
   ): PersistencePromise<DocumentMap | null> {
-
-    if (
-      queryMatchesAllDocuments(query)
-    ) {
+    if (queryMatchesAllDocuments(query)) {
       // Queries that match all documents don't benefit from using
       // key-based lookups. It is more efficient to scan all documents in a
       // collection, rather than to perform individual lookups.
@@ -226,10 +222,7 @@ export class QueryEngine {
     remoteKeys: DocumentKeySet,
     lastLimboFreeSnapshotVersion: SnapshotVersion
   ): PersistencePromise<DocumentMap> {
-    if (
-      queryMatchesAllDocuments(query) ||
-      queryContainsCompositeFilters(query)
-    ) {
+    if (queryMatchesAllDocuments(query)) {
       // Queries that match all documents don't benefit from using
       // key-based lookups. It is more efficient to scan all documents in a
       // collection, rather than to perform individual lookups.

packages/firestore/src/model/target_index_matcher.ts

@@ -22,7 +22,7 @@ import {
   OrderBy,
   Target
 } from '../core/target';
-import { debugAssert } from '../util/assert';
+import { debugAssert, hardAssert } from '../util/assert';
 
 import {
   FieldIndex,
@@ -107,7 +107,7 @@ export class TargetIndexMatcher {
    *   omitted.
    */
   servedByIndex(index: FieldIndex): boolean {
-    debugAssert(
+    hardAssert(
       index.collectionGroup === this.collectionId,
       'Collection IDs do not match'
     );

packages/firestore/src/remote/serializer.ts

@@ -1003,8 +1003,7 @@ function fromFilters(filter: ProtoFilter): Filter[] {
     result instanceof CompositeFilter &&
     compositeFilterIsFlatConjunction(result)
   ) {
-    // Copy the readonly array into a mutable array
-    return Object.assign([], result.getFilters());
+    return result.getFilters();
   }
 
   return [result];