Merge 2ddc068 into bf21b93

Author: MarkDuckworth

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;
 
@@ -324,6 +327,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/package.json

@@ -30,6 +30,7 @@
     "test:all:ci": "run-p test:browser test:lite:browser test:travis",
     "test:all": "run-p test:browser test:lite:browser test:travis test:minified",
     "test:browser": "karma start --single-run",
+    "test:browser:emulator": "karma start --single-run --local",
     "test:browser:unit": "karma start --single-run --unit",
     "test:browser:debug": "karma start --browsers=Chrome --auto-watch",
     "test:node": "node ./scripts/run-tests.js  --main=test/register.ts  --emulator 'test/{,!(browser|lite)/**/}*.test.ts'",

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,13 @@ function queryMatchesPathAndCollectionGroup(
  * in the results.
  */
 function queryMatchesOrderBy(query: Query, doc: Document): boolean {
-  for (const orderBy of query.explicitOrderBy) {
+  // 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) {
       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 {
@@ -782,12 +786,31 @@ export class CompositeFilter extends Filter {
   }
 }
 
+/**
+ * Returns a new composite filter that contains all filter from
+ * `compositeFilter` plus all the given filters in `otherFilters`.
+ * 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, compositeFilter.op);
+}
+
 export function compositeFilterIsConjunction(
   compositeFilter: CompositeFilter
 ): boolean {
   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.
  */
@@ -881,9 +904,28 @@ export function compositeFilterEquals(
 /** Returns a debug description for `filter`. */
 export function stringifyFilter(filter: Filter): string {
   debugAssert(
-    filter instanceof FieldFilter,
-    'stringifyFilter() only supports FieldFilters'
+    filter instanceof FieldFilter || filter instanceof CompositeFilter,
+    'stringifyFilter() only supports FieldFilters and CompositeFilters'
   );
+  if (filter instanceof FieldFilter) {
+    return stringifyFieldFilter(filter);
+  } else if (filter instanceof CompositeFilter) {
+    return stringifyCompositeFilter(filter);
+  } else {
+    return 'Filter';
+  }
+}
+
+export function stringifyCompositeFilter(filter: CompositeFilter): string {
+  return (
+    filter.op.toString() +
+    ` {` +
+    filter.getFilters().map(stringifyFilter).join(' ,') +
+    '}'
+  );
+}
+
+export function stringifyFieldFilter(filter: FieldFilter): string {
   return `${filter.field.canonicalString()} ${filter.op} ${canonicalId(
     filter.value
   )}`;

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,18 +570,18 @@ 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 [];
     }
 
     let encoders: IndexByteEncoder[] = [];
     encoders.push(new IndexByteEncoder());
 
-    let boundIdx = 0;
+    let valueIdx = 0;
     for (const segment of fieldIndexGetDirectionalSegments(fieldIndex)) {
-      const value = bound[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/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.