diff --git a/packages/firestore/src/core/query.ts b/packages/firestore/src/core/query.ts index b6898f5f37e..88afd71368c 100644 --- a/packages/firestore/src/core/query.ts +++ b/packages/firestore/src/core/query.ts @@ -19,7 +19,6 @@ import { compareDocumentsByField, Document } from '../model/document'; import { DocumentKey } from '../model/document_key'; import { FieldPath, ResourcePath } from '../model/path'; import { debugAssert, debugCast, fail } from '../util/assert'; -import { isNullOrUndefined } from '../util/types'; import { Bound, @@ -155,7 +154,7 @@ export function asCollectionQueryAtPath( * Returns true if this query does not specify any query constraints that * could remove results. */ -export function matchesAllDocuments(query: Query): boolean { +export function queryMatchesAllDocuments(query: Query): boolean { return ( query.filters.length === 0 && query.limit === null && @@ -167,14 +166,6 @@ export function matchesAllDocuments(query: Query): boolean { ); } -export function hasLimitToFirst(query: Query): boolean { - return !isNullOrUndefined(query.limit) && query.limitType === LimitType.First; -} - -export function hasLimitToLast(query: Query): boolean { - return !isNullOrUndefined(query.limit) && query.limitType === LimitType.Last; -} - export function getFirstOrderByField(query: Query): FieldPath | null { return query.explicitOrderBy.length > 0 ? query.explicitOrderBy[0].field @@ -393,7 +384,7 @@ export function queryWithAddedOrderBy(query: Query, orderBy: OrderBy): Query { export function queryWithLimit( query: Query, - limit: number, + limit: number | null, limitType: LimitType ): Query { return new QueryImpl( diff --git a/packages/firestore/src/core/view.ts b/packages/firestore/src/core/view.ts index 2f460861c2e..21ab049b9af 100644 --- a/packages/firestore/src/core/view.ts +++ b/packages/firestore/src/core/view.ts @@ -27,13 +27,7 @@ import { DocumentSet } from '../model/document_set'; import { TargetChange } from '../remote/remote_event'; import { debugAssert, fail } from '../util/assert'; -import { - hasLimitToFirst, - hasLimitToLast, - newQueryComparator, - Query, - queryMatches -} from './query'; +import { LimitType, newQueryComparator, Query, queryMatches } from './query'; import { OnlineState } from './types'; import { ChangeType, @@ -146,11 +140,13 @@ export class View { // Note that this should never get used in a refill (when previousChanges is // set), because there will only be adds -- no deletes or updates. const lastDocInLimit = - hasLimitToFirst(this.query) && oldDocumentSet.size === this.query.limit + this.query.limitType === LimitType.First && + oldDocumentSet.size === this.query.limit ? oldDocumentSet.last() : null; const firstDocInLimit = - hasLimitToLast(this.query) && oldDocumentSet.size === this.query.limit + this.query.limitType === LimitType.Last && + oldDocumentSet.size === this.query.limit ? oldDocumentSet.first() : null; @@ -228,11 +224,12 @@ export class View { }); // Drop documents out to meet limit/limitToLast requirement. - if (hasLimitToFirst(this.query) || hasLimitToLast(this.query)) { + if (this.query.limit !== null) { while (newDocumentSet.size > this.query.limit!) { - const oldDoc = hasLimitToFirst(this.query) - ? newDocumentSet.last() - : newDocumentSet.first(); + const oldDoc = + this.query.limitType === LimitType.First + ? newDocumentSet.last() + : newDocumentSet.first(); newDocumentSet = newDocumentSet.delete(oldDoc!.key); newMutatedKeys = newMutatedKeys.delete(oldDoc!.key); changeSet.track({ type: ChangeType.Removed, doc: oldDoc! }); diff --git a/packages/firestore/src/lite-api/query.ts b/packages/firestore/src/lite-api/query.ts index df760c9df58..0a762918b33 100644 --- a/packages/firestore/src/lite-api/query.ts +++ b/packages/firestore/src/lite-api/query.ts @@ -22,7 +22,6 @@ import { findFilterOperator, getFirstOrderByField, getInequalityFilterField, - hasLimitToLast, isCollectionGroupQuery, LimitType, Query as InternalQuery, @@ -66,7 +65,10 @@ import { export function validateHasExplicitOrderByForLimitToLast( query: InternalQuery ): void { - if (hasLimitToLast(query) && query.explicitOrderBy.length === 0) { + if ( + query.limitType === LimitType.Last && + query.explicitOrderBy.length === 0 + ) { throw new FirestoreError( Code.UNIMPLEMENTED, 'limitToLast() queries require specifying at least one orderBy() clause' diff --git a/packages/firestore/src/lite-api/reference_impl.ts b/packages/firestore/src/lite-api/reference_impl.ts index d7ec2c62655..e478cc76eb4 100644 --- a/packages/firestore/src/lite-api/reference_impl.ts +++ b/packages/firestore/src/lite-api/reference_impl.ts @@ -21,7 +21,7 @@ import { } from '@firebase/firestore-types'; import { getModularInstance } from '@firebase/util'; -import { hasLimitToLast } from '../core/query'; +import { LimitType } from '../core/query'; import { DeleteMutation, Precondition } from '../model/mutation'; import { invokeBatchGetDocumentsRpc, @@ -172,7 +172,7 @@ export function getDocs(query: Query): Promise> { ) ); - if (hasLimitToLast(query._query)) { + if (query._query.limitType === LimitType.Last) { // Limit to last queries reverse the orderBy constraint that was // specified by the user. As such, we need to reverse the order of the // results to return the documents in the expected order. diff --git a/packages/firestore/src/local/index_manager.ts b/packages/firestore/src/local/index_manager.ts index e65eb743595..6692e3cda38 100644 --- a/packages/firestore/src/local/index_manager.ts +++ b/packages/firestore/src/local/index_manager.ts @@ -155,4 +155,13 @@ export interface IndexManager { transaction: PersistenceTransaction, documents: DocumentMap ): PersistencePromise; + + /** + * Iterates over all field indexes that are used to serve the given target, + * and returns the minimum offset of them all. + */ + getMinOffset( + transaction: PersistenceTransaction, + target: Target + ): PersistencePromise; } diff --git a/packages/firestore/src/local/indexeddb_index_manager.ts b/packages/firestore/src/local/indexeddb_index_manager.ts index 41d5c83df70..e5c65438de5 100644 --- a/packages/firestore/src/local/indexeddb_index_manager.ts +++ b/packages/firestore/src/local/indexeddb_index_manager.ts @@ -44,6 +44,7 @@ import { fieldIndexToString, IndexKind, IndexOffset, + indexOffsetComparator, IndexSegment } from '../model/field_index'; import { FieldPath, ResourcePath } from '../model/path'; @@ -469,15 +470,22 @@ export class IndexedDbIndexManager implements IndexManager { transaction: PersistenceTransaction, target: Target ): PersistencePromise { - // TODO(orqueries): We should look at the subtargets here - return this.getFieldIndex(transaction, target).next(index => { - if (!index) { - return IndexType.NONE as 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; + } + }); } - return index.fields.length < targetGetSegmentCount(target) - ? IndexType.PARTIAL - : IndexType.FULL; - }); + ).next(() => indexType); } /** @@ -972,6 +980,28 @@ export class IndexedDbIndexManager implements IndexManager { } return ranges; } + + getMinOffset( + transaction: PersistenceTransaction, + target: Target + ): PersistencePromise { + let offset: IndexOffset | undefined; + return PersistencePromise.forEach( + this.getSubTargets(target), + (target: Target) => { + return this.getFieldIndex(transaction, target).next(index => { + if (!index) { + offset = IndexOffset.min(); + } else if ( + !offset || + indexOffsetComparator(index.indexState.offset, offset) < 0 + ) { + offset = index.indexState.offset; + } + }); + } + ).next(() => offset!); + } } /** diff --git a/packages/firestore/src/local/indexeddb_schema.ts b/packages/firestore/src/local/indexeddb_schema.ts index 90904b6f717..4f2130b96f6 100644 --- a/packages/firestore/src/local/indexeddb_schema.ts +++ b/packages/firestore/src/local/indexeddb_schema.ts @@ -29,7 +29,7 @@ import { EncodedResourcePath } from './encoded_resource_path'; import { DbTimestampKey } from './indexeddb_sentinels'; // TODO(indexing): Remove this constant -const INDEXING_ENABLED = false; +export const INDEXING_ENABLED = false; export const INDEXING_SCHEMA_VERSION = 14; diff --git a/packages/firestore/src/local/local_documents_view.ts b/packages/firestore/src/local/local_documents_view.ts index 3e0edb081dd..11b3a3e2514 100644 --- a/packages/firestore/src/local/local_documents_view.ts +++ b/packages/firestore/src/local/local_documents_view.ts @@ -52,7 +52,7 @@ export class LocalDocumentsView { constructor( readonly remoteDocumentCache: RemoteDocumentCache, readonly mutationQueue: MutationQueue, - readonly indexManager: IndexManager + private readonly indexManager: IndexManager ) {} /** diff --git a/packages/firestore/src/local/local_store_impl.ts b/packages/firestore/src/local/local_store_impl.ts index 6bc4ad356f7..ef07e853783 100644 --- a/packages/firestore/src/local/local_store_impl.ts +++ b/packages/firestore/src/local/local_store_impl.ts @@ -203,7 +203,7 @@ class LocalStoreImpl implements LocalStore { this.indexManager ); this.remoteDocuments.setIndexManager(this.indexManager); - this.queryEngine.setLocalDocumentsView(this.localDocuments); + this.queryEngine.initialize(this.localDocuments, this.indexManager); } collectGarbage(garbageCollector: LruGarbageCollector): Promise { diff --git a/packages/firestore/src/local/memory_index_manager.ts b/packages/firestore/src/local/memory_index_manager.ts index 574cf841089..5b2f860bce6 100644 --- a/packages/firestore/src/local/memory_index_manager.ts +++ b/packages/firestore/src/local/memory_index_manager.ts @@ -97,6 +97,13 @@ export class MemoryIndexManager implements IndexManager { return PersistencePromise.resolve(null); } + getMinOffset( + transaction: PersistenceTransaction, + target: Target + ): PersistencePromise { + return PersistencePromise.resolve(IndexOffset.min()); + } + updateCollectionGroup( transaction: PersistenceTransaction, collectionGroup: string, diff --git a/packages/firestore/src/local/query_engine.ts b/packages/firestore/src/local/query_engine.ts index 53755663361..3856e9835eb 100644 --- a/packages/firestore/src/local/query_engine.ts +++ b/packages/firestore/src/local/query_engine.ts @@ -16,17 +16,21 @@ */ import { - hasLimitToFirst, - hasLimitToLast, LimitType, - matchesAllDocuments, newQueryComparator, Query, queryMatches, + queryMatchesAllDocuments, + queryToTarget, + queryWithLimit, stringifyQuery } from '../core/query'; import { SnapshotVersion } from '../core/snapshot_version'; -import { DocumentKeySet, DocumentMap } from '../model/collections'; +import { + documentKeySet, + DocumentKeySet, + DocumentMap +} from '../model/collections'; import { Document } from '../model/document'; import { IndexOffset, @@ -34,18 +38,41 @@ import { newIndexOffsetSuccessorFromReadTime } from '../model/field_index'; import { debugAssert } from '../util/assert'; -import { getLogLevel, LogLevel, logDebug } from '../util/log'; +import { getLogLevel, logDebug, LogLevel } from '../util/log'; +import { Iterable } from '../util/misc'; import { SortedSet } from '../util/sorted_set'; +import { IndexManager, IndexType } from './index_manager'; +import { INDEXING_ENABLED } from './indexeddb_schema'; import { LocalDocumentsView } from './local_documents_view'; import { PersistencePromise } from './persistence_promise'; import { PersistenceTransaction } from './persistence_transaction'; /** - * A query engine that takes advantage of the target document mapping in the - * QueryCache. Query execution is optimized by only reading the documents that - * previously matched a query plus any documents that were edited after the - * query was last listened to. + * The Firestore query engine. + * + * Firestore queries can be executed in three modes. The Query Engine determines + * what mode to use based on what data is persisted. The mode only determines + * the runtime complexity of the query - the result set is equivalent across all + * implementations. + * + * The Query engine will use indexed-based execution if a user has configured + * any index that can be used to execute query (via `setIndexConfiguration()`). + * Otherwise, the engine will try to optimize the query by re-using a previously + * persisted query result. If that is not possible, the query will be executed + * via a full collection scan. + * + * Index-based execution is the default when available. The query engine + * supports partial indexed execution and merges the result from the index + * lookup with documents that have not yet been indexed. The index evaluation + * matches the backend's format and as such, the SDK can use indexing for all + * queries that the backend supports. + * + * If no index exists, the query engine tries to take advantage of the target + * document mapping in the TargetCache. These mappings exists for all queries + * that have been synced with the backend at least once and allow the query + * engine to only read documents that previously matched a query plus any + * documents that were edited after the query was last listened to. * * There are some cases when this optimization is not guaranteed to produce * the same results as full collection scans. In these cases, query @@ -60,11 +87,18 @@ import { PersistenceTransaction } from './persistence_transaction'; * - Queries that have never been CURRENT or free of limbo documents. */ export class QueryEngine { - private localDocumentsView: LocalDocumentsView | undefined; + private localDocumentsView!: LocalDocumentsView; + private indexManager!: IndexManager; + private initialized = false; /** Sets the document view to query against. */ - setLocalDocumentsView(localDocuments: LocalDocumentsView): void { + initialize( + localDocuments: LocalDocumentsView, + indexManager: IndexManager + ): void { this.localDocumentsView = localDocuments; + this.indexManager = indexManager; + this.initialized = true; } /** Returns all local documents matching the specified query. */ @@ -74,15 +108,131 @@ export class QueryEngine { lastLimboFreeSnapshotVersion: SnapshotVersion, remoteKeys: DocumentKeySet ): PersistencePromise { - debugAssert( - this.localDocumentsView !== undefined, - 'setLocalDocumentsView() not called' - ); + debugAssert(this.initialized, 'initialize() not called'); + + return this.performQueryUsingIndex(transaction, query) + .next(result => + result + ? result + : this.performQueryUsingRemoteKeys( + transaction, + query, + remoteKeys, + lastLimboFreeSnapshotVersion + ) + ) + .next(result => + result ? result : this.executeFullCollectionScan(transaction, query) + ); + } + + /** + * Performs an indexed query that evaluates the query based on a collection's + * persisted index values. Returns `null` if an index is not available. + */ + private performQueryUsingIndex( + transaction: PersistenceTransaction, + query: Query + ): PersistencePromise { + if (!INDEXING_ENABLED) { + return PersistencePromise.resolve(null); + } + + 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. + return PersistencePromise.resolve(null); + } + + const target = queryToTarget(query); + return this.indexManager + .getIndexType(transaction, target) + .next(indexType => { + if (indexType === IndexType.NONE) { + // The target cannot be served from any index. + return null; + } + + if (indexType === IndexType.PARTIAL) { + // We cannot apply a limit for targets that are served using a partial + // index. If a partial index will be used to serve the target, the + // query may return a superset of documents that match the target + // (e.g. if the index doesn't include all the target's filters), or + // may return the correct set of documents in the wrong order (e.g. if + // the index doesn't include a segment for one of the orderBys). + // Therefore, a limit should not be applied in such cases. + return this.performQueryUsingIndex( + transaction, + queryWithLimit(query, null, LimitType.First) + ); + } + + return this.indexManager + .getDocumentsMatchingTarget(transaction, target) + .next(keys => { + debugAssert( + !!keys, + 'Index manager must return results for partial and full indexes.' + ); + const sortedKeys = documentKeySet(...keys); + return this.localDocumentsView + .getDocuments(transaction, sortedKeys) + .next(indexedDocuments => { + return this.indexManager + .getMinOffset(transaction, target) + .next(offset => { + const previousResults = this.applyQuery( + query, + indexedDocuments + ); + + if ( + this.needsRefill( + query, + previousResults, + sortedKeys, + offset.readTime + ) + ) { + // A limit query whose boundaries change due to local + // edits can be re-run against the cache by excluding the + // limit. This ensures that all documents that match the + // query's filters are included in the result set. The SDK + // can then apply the limit once all local edits are + // incorporated. + return this.performQueryUsingIndex( + transaction, + queryWithLimit(query, null, LimitType.First) + ); + } - // 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. - if (matchesAllDocuments(query)) { + return this.appendRemainingResults( + transaction, + previousResults, + query, + offset + ) as PersistencePromise; + }); + }); + }); + }); + } + + /** + * Performs a query based on the target's persisted query mapping. Returns + * `null` if the mapping is not available or cannot be used. + */ + private performQueryUsingRemoteKeys( + transaction: PersistenceTransaction, + query: Query, + remoteKeys: DocumentKeySet, + lastLimboFreeSnapshotVersion: SnapshotVersion + ): PersistencePromise { + 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. return this.executeFullCollectionScan(transaction, query); } @@ -97,9 +247,8 @@ export class QueryEngine { const previousResults = this.applyQuery(query, documents); if ( - (hasLimitToFirst(query) || hasLimitToLast(query)) && this.needsRefill( - query.limitType, + query, previousResults, remoteKeys, lastLimboFreeSnapshotVersion @@ -119,22 +268,15 @@ export class QueryEngine { // Retrieve all results for documents that were updated since the last // limbo-document free remote snapshot. - return this.localDocumentsView!.getDocumentsMatchingQuery( + return this.appendRemainingResults( transaction, + previousResults, query, newIndexOffsetSuccessorFromReadTime( lastLimboFreeSnapshotVersion, INITIAL_LARGEST_BATCH_ID ) - ).next(updatedResults => { - // We merge `previousResults` into `updateResults`, since - // `updateResults` is already a DocumentMap. If a document is - // contained in both lists, then its contents are the same. - previousResults.forEach(doc => { - updatedResults = updatedResults.insert(doc.key, doc); - }); - return updatedResults; - }); + ); } ); } @@ -159,6 +301,7 @@ export class QueryEngine { * Determines if a limit query needs to be refilled from cache, making it * ineligible for index-free execution. * + * @param query The query. * @param sortedPreviousResults - The documents that matched the query when it * was last synchronized, sorted by the query's comparator. * @param remoteKeys - The document keys that matched the query at the last @@ -167,14 +310,19 @@ export class QueryEngine { * query was last synchronized. */ private needsRefill( - limitType: LimitType, + query: Query, sortedPreviousResults: SortedSet, remoteKeys: DocumentKeySet, limboFreeSnapshotVersion: SnapshotVersion ): boolean { - // The query needs to be refilled if a previously matching document no - // longer matches. + if (query.limit === null) { + // Queries without limits do not need to be refilled. + return false; + } + if (remoteKeys.size !== sortedPreviousResults.size) { + // The query needs to be refilled if a previously matching document no + // longer matches. return true; } @@ -187,7 +335,7 @@ export class QueryEngine { // will continue to be "rejected" by this boundary. Therefore, we can ignore // any modifications that don't affect the last document. const docAtLimitEdge = - limitType === LimitType.First + query.limitType === LimitType.First ? sortedPreviousResults.last() : sortedPreviousResults.first(); if (!docAtLimitEdge) { @@ -218,4 +366,26 @@ export class QueryEngine { IndexOffset.min() ); } + + /** + * Combines the results from an indexed execution with the remaining documents + * that have not yet been indexed. + */ + private appendRemainingResults( + transaction: PersistenceTransaction, + indexedResults: Iterable, + query: Query, + offset: IndexOffset + ): PersistencePromise { + // Retrieve all results for documents that were updated since the offset. + return this.localDocumentsView + .getDocumentsMatchingQuery(transaction, query, offset) + .next(remainingResults => { + // Merge with existing results + indexedResults.forEach(d => { + remainingResults = remainingResults.insert(d.key, d); + }); + return remainingResults; + }); + } } diff --git a/packages/firestore/src/model/collections.ts b/packages/firestore/src/model/collections.ts index de760c6e00b..d377512a205 100644 --- a/packages/firestore/src/model/collections.ts +++ b/packages/firestore/src/model/collections.ts @@ -46,8 +46,12 @@ export type DocumentMap = SortedMap; const EMPTY_DOCUMENT_MAP = new SortedMap( DocumentKey.comparator ); -export function documentMap(): DocumentMap { - return EMPTY_DOCUMENT_MAP; +export function documentMap(...docs: Document[]): DocumentMap { + let map = EMPTY_DOCUMENT_MAP; + for (const doc of docs) { + map = map.insert(doc.key, doc); + } + return map; } export type OverlayMap = ObjectMap; diff --git a/packages/firestore/src/util/misc.ts b/packages/firestore/src/util/misc.ts index 39fcc3a827e..60a18d26ffc 100644 --- a/packages/firestore/src/util/misc.ts +++ b/packages/firestore/src/util/misc.ts @@ -68,6 +68,10 @@ export interface Equatable { isEqual(other: T): boolean; } +export interface Iterable { + forEach: (cb: (v: V) => void) => void; +} + /** Helper to compare arrays using isEqual(). */ export function arrayEquals( left: T[], diff --git a/packages/firestore/test/unit/core/query.test.ts b/packages/firestore/test/unit/core/query.test.ts index 66238adef6d..2acfbc65cda 100644 --- a/packages/firestore/test/unit/core/query.test.ts +++ b/packages/firestore/test/unit/core/query.test.ts @@ -34,7 +34,7 @@ import { queryToTarget, QueryImpl, queryEquals, - matchesAllDocuments, + queryMatchesAllDocuments, queryCollectionGroup, newQueryForCollectionGroup } from '../../../src/core/query'; @@ -779,25 +779,25 @@ describe('Query', () => { it('matchesAllDocuments() considers filters, orders and bounds', () => { const baseQuery = newQueryForPath(ResourcePath.fromString('collection')); - expect(matchesAllDocuments(baseQuery)).to.be.true; + expect(queryMatchesAllDocuments(baseQuery)).to.be.true; let query1 = query('collection', orderBy('__name__')); - expect(matchesAllDocuments(query1)).to.be.true; + expect(queryMatchesAllDocuments(query1)).to.be.true; query1 = query('collection', orderBy('foo')); - expect(matchesAllDocuments(query1)).to.be.false; + expect(queryMatchesAllDocuments(query1)).to.be.false; query1 = query('collection', filter('foo', '==', 'bar')); - expect(matchesAllDocuments(query1)).to.be.false; + expect(queryMatchesAllDocuments(query1)).to.be.false; query1 = queryWithLimit(query('foo'), 1, LimitType.First); - expect(matchesAllDocuments(query1)).to.be.false; + expect(queryMatchesAllDocuments(query1)).to.be.false; query1 = queryWithStartAt(baseQuery, bound([], true)); - expect(matchesAllDocuments(query1)).to.be.false; + expect(queryMatchesAllDocuments(query1)).to.be.false; query1 = queryWithEndAt(baseQuery, bound([], false)); - expect(matchesAllDocuments(query1)).to.be.false; + expect(queryMatchesAllDocuments(query1)).to.be.false; }); function assertImplicitOrderBy(query: Query, ...orderBys: OrderBy[]): void { diff --git a/packages/firestore/test/unit/local/counting_query_engine.ts b/packages/firestore/test/unit/local/counting_query_engine.ts index 784625e9cd3..392693b0165 100644 --- a/packages/firestore/test/unit/local/counting_query_engine.ts +++ b/packages/firestore/test/unit/local/counting_query_engine.ts @@ -79,14 +79,16 @@ export class CountingQueryEngine extends QueryEngine { ); } - setLocalDocumentsView(localDocuments: LocalDocumentsView): void { + initialize( + localDocuments: LocalDocumentsView, + indexManager: IndexManager + ): void { const view = new LocalDocumentsView( this.wrapRemoteDocumentCache(localDocuments.remoteDocumentCache), this.wrapMutationQueue(localDocuments.mutationQueue), - localDocuments.indexManager + indexManager ); - - return super.setLocalDocumentsView(view); + return super.initialize(view, indexManager); } private wrapRemoteDocumentCache( diff --git a/packages/firestore/test/unit/local/index_manager.test.ts b/packages/firestore/test/unit/local/index_manager.test.ts index a4a643f4eb9..7970679a265 100644 --- a/packages/firestore/test/unit/local/index_manager.test.ts +++ b/packages/firestore/test/unit/local/index_manager.test.ts @@ -1600,11 +1600,7 @@ describe('IndexedDbIndexManager', async () => { } function addDocs(...docs: Document[]): Promise { - let data = documentMap(); - for (const doc of docs) { - data = data.insert(doc.key, doc); - } - return indexManager.updateIndexEntries(data); + return indexManager.updateIndexEntries(documentMap(...docs)); } function addDoc(key: string, data: JsonObject): Promise { diff --git a/packages/firestore/test/unit/local/query_engine.test.ts b/packages/firestore/test/unit/local/query_engine.test.ts index f4a829c7e75..82d5c364337 100644 --- a/packages/firestore/test/unit/local/query_engine.test.ts +++ b/packages/firestore/test/unit/local/query_engine.test.ts @@ -17,43 +17,65 @@ import { expect } from 'chai'; +import { Timestamp } from '../../../src'; import { User } from '../../../src/auth/user'; -import { LimitType, Query, queryWithLimit } from '../../../src/core/query'; +import { + LimitType, + Query, + queryWithAddedFilter, + queryWithAddedOrderBy, + queryWithLimit +} from '../../../src/core/query'; import { SnapshotVersion } from '../../../src/core/snapshot_version'; import { View } from '../../../src/core/view'; +import { IndexedDbPersistence } from '../../../src/local/indexeddb_persistence'; +import { + INDEXING_ENABLED, + INDEXING_SCHEMA_VERSION +} from '../../../src/local/indexeddb_schema'; import { LocalDocumentsView } from '../../../src/local/local_documents_view'; -import { MemoryIndexManager } from '../../../src/local/memory_index_manager'; +import { MutationQueue } from '../../../src/local/mutation_queue'; import { Persistence } from '../../../src/local/persistence'; import { PersistencePromise } from '../../../src/local/persistence_promise'; import { PersistenceTransaction } from '../../../src/local/persistence_transaction'; import { QueryEngine } from '../../../src/local/query_engine'; import { RemoteDocumentCache } from '../../../src/local/remote_document_cache'; import { TargetCache } from '../../../src/local/target_cache'; -import { documentKeySet, DocumentMap } from '../../../src/model/collections'; +import { + documentKeySet, + documentMap, + DocumentMap +} from '../../../src/model/collections'; import { Document, MutableDocument } from '../../../src/model/document'; import { DocumentKey } from '../../../src/model/document_key'; import { DocumentSet } from '../../../src/model/document_set'; import { + IndexKind, IndexOffset, - indexOffsetComparator + indexOffsetComparator, + newIndexOffsetFromDocument } from '../../../src/model/field_index'; +import { Mutation } from '../../../src/model/mutation'; import { debugAssert } from '../../../src/util/assert'; -import { doc, filter, key, orderBy, query, version } from '../../util/helpers'; - -import { testMemoryEagerPersistence } from './persistence_test_helpers'; +import { + doc, + fieldIndex, + filter, + key, + orderBy, + patchMutation, + query, + setMutation, + version +} from '../../util/helpers'; + +import * as persistenceHelpers from './persistence_test_helpers'; +import { TestIndexManager } from './test_index_manager'; const TEST_TARGET_ID = 1; const MATCHING_DOC_A = doc('coll/a', 1, { matches: true, order: 1 }); const NON_MATCHING_DOC_A = doc('coll/a', 1, { matches: false, order: 1 }); -const PENDING_MATCHING_DOC_A = doc('coll/a', 1, { - matches: true, - order: 1 -}).setHasLocalMutations(); -const PENDING_NON_MATCHING_DOC_A = doc('coll/a', 1, { - matches: false, - order: 1 -}).setHasLocalMutations(); const UPDATED_DOC_A = doc('coll/a', 11, { matches: true, order: 1 }); const MATCHING_DOC_B = doc('coll/b', 1, { matches: true, order: 2 }); const UPDATED_MATCHING_DOC_B = doc('coll/b', 11, { matches: true, order: 2 }); @@ -85,11 +107,47 @@ class TestLocalDocumentsView extends LocalDocumentsView { } } -describe('QueryEngine', () => { +describe('MemoryQueryEngine', async () => { + genericQueryEngineTest( + /* durable= */ false, + persistenceHelpers.testMemoryEagerPersistence + ); +}); + +describe('IndexedDbQueryEngine', async () => { + if (!IndexedDbPersistence.isAvailable()) { + console.warn('No IndexedDB. Skipping IndexedDbQueryEngine tests.'); + return; + } + + let persistencePromise: Promise; + beforeEach(async () => { + persistencePromise = persistenceHelpers.testIndexedDbPersistence({ + schemaVersion: INDEXING_SCHEMA_VERSION + }); + }); + + genericQueryEngineTest(/* durable= */ true, () => persistencePromise); +}); + +/** + * Defines the set of tests to run against the memory and IndexedDB-backed + * query engine. + * + * @param durable Whether the provided persistence is backed by IndexedDB + * @param persistencePromise A factory function that returns an initialized + * persistence layer. + */ +function genericQueryEngineTest( + durable: boolean, + persistencePromise: () => Promise +): void { let persistence!: Persistence; let remoteDocumentCache!: RemoteDocumentCache; let targetCache!: TargetCache; let queryEngine!: QueryEngine; + let indexManager!: TestIndexManager; + let mutationQueue!: MutationQueue; let localDocuments!: TestLocalDocumentsView; /** Adds the provided documents to the query target mapping. */ @@ -108,13 +166,21 @@ describe('QueryEngine', () => { function addDocument(...docs: MutableDocument[]): Promise { return persistence.runTransaction('addDocument', 'readwrite', txn => { const changeBuffer = remoteDocumentCache.newChangeBuffer(); - for (const doc of docs) { - changeBuffer.addEntry(doc); - } - return changeBuffer.apply(txn); + return PersistencePromise.forEach(docs, (doc: MutableDocument) => + changeBuffer + .getEntry(txn, doc.key) + .next(() => changeBuffer.addEntry(doc)) + ).next(() => changeBuffer.apply(txn)); }); } + /** Adds a mutation to the mutation queue. */ + async function addMutation(mutation: Mutation): Promise { + await persistence.runTransaction('addMutation', 'readwrite', txn => + mutationQueue.addMutationBatch(txn, Timestamp.now(), [], [mutation]) + ); + } + async function expectOptimizedCollectionQuery( op: () => Promise ): Promise { @@ -171,20 +237,35 @@ describe('QueryEngine', () => { } beforeEach(async () => { - persistence = await testMemoryEagerPersistence(); + persistence = await persistencePromise(); targetCache = persistence.getTargetCache(); queryEngine = new QueryEngine(); - const indexManager = persistence.getIndexManager(User.UNAUTHENTICATED); + const underlyingIndexManager = persistence.getIndexManager( + User.UNAUTHENTICATED + ); remoteDocumentCache = persistence.getRemoteDocumentCache(); - remoteDocumentCache.setIndexManager(indexManager); + remoteDocumentCache.setIndexManager(underlyingIndexManager); + mutationQueue = persistence.getMutationQueue( + User.UNAUTHENTICATED, + underlyingIndexManager + ); localDocuments = new TestLocalDocumentsView( remoteDocumentCache, - persistence.getMutationQueue(User.UNAUTHENTICATED, indexManager), - new MemoryIndexManager() + mutationQueue, + underlyingIndexManager ); - queryEngine.setLocalDocumentsView(localDocuments); + queryEngine.initialize(localDocuments, underlyingIndexManager); + + indexManager = new TestIndexManager(persistence, underlyingIndexManager); + }); + + afterEach(async () => { + if (persistence.started) { + await persistence.shutdown(); + await persistenceHelpers.clearTestPersistence(); + } }); it('uses target mapping for initial view', async () => { @@ -206,8 +287,8 @@ describe('QueryEngine', () => { await addDocument(MATCHING_DOC_A, MATCHING_DOC_B); await persistQueryMapping(MATCHING_DOC_A.key, MATCHING_DOC_B.key); - // Add a mutated document that is not yet part of query's set of remote keys. - await addDocument(PENDING_NON_MATCHING_DOC_A); + // Add a mutation that is not yet part of query's set of remote keys. + await addMutation(patchMutation('coll/a', { 'matches': false })); const docs = await expectOptimizedCollectionQuery(() => runQuery(query1, LAST_LIMBO_FREE_SNAPSHOT) @@ -307,8 +388,9 @@ describe('QueryEngine', () => { // Add a query mapping for a document that matches, but that sorts below // another document due to a pending write. - await addDocument(PENDING_MATCHING_DOC_A); - await persistQueryMapping(PENDING_MATCHING_DOC_A.key); + await addDocument(MATCHING_DOC_A); + await addMutation(patchMutation('coll/a', { order: 1 })); + await persistQueryMapping(MATCHING_DOC_A.key); await addDocument(MATCHING_DOC_B); @@ -326,8 +408,9 @@ describe('QueryEngine', () => { ); // Add a query mapping for a document that matches, but that sorts below // another document due to a pending write. - await addDocument(PENDING_MATCHING_DOC_A); - await persistQueryMapping(PENDING_MATCHING_DOC_A.key); + await addDocument(MATCHING_DOC_A); + await addMutation(patchMutation('coll/a', { order: 2 })); + await persistQueryMapping(MATCHING_DOC_A.key); await addDocument(MATCHING_DOC_B); @@ -390,7 +473,7 @@ describe('QueryEngine', () => { await persistQueryMapping(key('coll/a'), key('coll/b')); // Update "coll/a" but make sure it still sorts before "coll/b" - await addDocument(doc('coll/a', 1, { order: 2 }).setHasLocalMutations()); + await addMutation(patchMutation('coll/a', { order: 2 })); // Since the last document in the limit didn't change (and hence we know // that all documents written prior to query execution still sort after @@ -403,7 +486,117 @@ describe('QueryEngine', () => { doc('coll/b', 1, { order: 3 }) ]); }); -}); + + if (!durable) { + return; + } + + if (!INDEXING_ENABLED) { + return; + } + + it('combines indexed with non-indexed results', async () => { + debugAssert(durable, 'Test requires durable persistence'); + + const doc1 = doc('coll/a', 1, { 'foo': true }); + const doc2 = doc('coll/b', 2, { 'foo': true }); + const doc3 = doc('coll/c', 3, { 'foo': true }); + const doc4 = doc('coll/d', 3, { 'foo': true }).setHasLocalMutations(); + + await indexManager.addFieldIndex( + fieldIndex('coll', { fields: [['foo', IndexKind.ASCENDING]] }) + ); + + await addDocument(doc1); + await addDocument(doc2); + await indexManager.updateIndexEntries(documentMap(doc1, doc2)); + await indexManager.updateCollectionGroup( + 'coll', + newIndexOffsetFromDocument(doc2) + ); + + await addDocument(doc3); + await addMutation(setMutation('coll/d', { 'foo': true })); + + const queryWithFilter = queryWithAddedFilter( + query('coll'), + filter('foo', '==', true) + ); + const results = await expectOptimizedCollectionQuery(() => + runQuery(queryWithFilter, SnapshotVersion.min()) + ); + + verifyResult(results, [doc1, doc2, doc3, doc4]); + }); + + it('uses partial index for limit queries', async () => { + debugAssert(durable, 'Test requires durable persistence'); + + const doc1 = doc('coll/1', 1, { 'a': 1, 'b': 0 }); + const doc2 = doc('coll/2', 1, { 'a': 1, 'b': 1 }); + const doc3 = doc('coll/3', 1, { 'a': 1, 'b': 2 }); + const doc4 = doc('coll/4', 1, { 'a': 1, 'b': 3 }); + const doc5 = doc('coll/5', 1, { 'a': 2, 'b': 3 }); + await addDocument(doc1, doc2, doc3, doc4, doc5); + + await indexManager.addFieldIndex( + fieldIndex('coll', { fields: [['a', IndexKind.ASCENDING]] }) + ); + await indexManager.updateIndexEntries( + documentMap(doc1, doc2, doc3, doc4, doc5) + ); + await indexManager.updateCollectionGroup( + 'coll', + newIndexOffsetFromDocument(doc5) + ); + + const q = queryWithLimit( + queryWithAddedFilter( + queryWithAddedFilter(query('coll'), filter('a', '==', 1)), + filter('b', '==', 1) + ), + 3, + LimitType.First + ); + const results = await expectOptimizedCollectionQuery(() => + runQuery(q, SnapshotVersion.min()) + ); + + verifyResult(results, [doc2]); + }); + + it('re-fills indexed limit queries', async () => { + debugAssert(durable, 'Test requires durable persistence'); + + const doc1 = doc('coll/1', 1, { 'a': 1 }); + const doc2 = doc('coll/2', 1, { 'a': 2 }); + const doc3 = doc('coll/3', 1, { 'a': 3 }); + const doc4 = doc('coll/4', 1, { 'a': 4 }); + await addDocument(doc1, doc2, doc3, doc4); + + await indexManager.addFieldIndex( + fieldIndex('coll', { fields: [['a', IndexKind.ASCENDING]] }) + ); + await indexManager.updateIndexEntries(documentMap(doc1, doc2, doc3, doc4)); + await indexManager.updateCollectionGroup( + 'coll', + newIndexOffsetFromDocument(doc4) + ); + + await addMutation(patchMutation('coll/3', { 'a': 5 })); + + const q = queryWithLimit( + queryWithAddedOrderBy(query('coll'), orderBy('a')), + 3, + LimitType.First + ); + const results = await expectOptimizedCollectionQuery(() => + runQuery(q, SnapshotVersion.min()) + ); + + verifyResult(results, [doc1, doc2, doc4]); + }); +} function verifyResult(actualDocs: DocumentSet, expectedDocs: Document[]): void { for (const doc of expectedDocs) { diff --git a/packages/firestore/test/unit/specs/spec_builder.ts b/packages/firestore/test/unit/specs/spec_builder.ts index 838b3263171..87b16734af8 100644 --- a/packages/firestore/test/unit/specs/spec_builder.ts +++ b/packages/firestore/test/unit/specs/spec_builder.ts @@ -17,8 +17,7 @@ import { ExpUserDataWriter } from '../../../src/api/reference_impl'; import { - hasLimitToFirst, - hasLimitToLast, + LimitType, newQueryForPath, Query, queryEquals, @@ -1006,13 +1005,10 @@ export class SpecBuilder { if (query.collectionGroup !== null) { spec.collectionGroup = query.collectionGroup; } - if (hasLimitToFirst(query)) { - spec.limit = query.limit!; - spec.limitType = 'LimitToFirst'; - } - if (hasLimitToLast(query)) { - spec.limit = query.limit!; - spec.limitType = 'LimitToLast'; + if (query.limit !== null) { + spec.limit = query.limit; + spec.limitType = + query.limitType === LimitType.First ? 'LimitToFirst' : 'LimitToLast'; } if (query.filters) { spec.filters = query.filters.map((filter: Filter) => {