Skip to content

Add SimpleQueryEngine #2160

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion packages/firestore/src/core/firestore_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { IndexedDbPersistence } from '../local/indexeddb_persistence';
import { LocalStore } from '../local/local_store';
import { MemoryPersistence } from '../local/memory_persistence';
import { Persistence } from '../local/persistence';
import { SimpleQueryEngine } from '../local/simple_query_engine';
import {
DocumentKeySet,
documentKeySet,
Expand Down Expand Up @@ -406,7 +407,9 @@ export class FirestoreClient {
return this.platform
.loadConnection(this.databaseInfo)
.then(async connection => {
this.localStore = new LocalStore(this.persistence, user);
// TODO(index-free): Use IndexFreeQueryEngine/IndexedQueryEngine as appropriate.
const queryEngine = new SimpleQueryEngine();
this.localStore = new LocalStore(this.persistence, queryEngine, user);
if (maybeLruGc) {
// We're running LRU Garbage collection. Set up the scheduler.
this.lruScheduler = new LruScheduler(
Expand Down
6 changes: 5 additions & 1 deletion packages/firestore/src/local/local_store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ import { Persistence, PersistenceTransaction } from './persistence';
import { PersistencePromise } from './persistence_promise';
import { QueryCache } from './query_cache';
import { QueryData, QueryPurpose } from './query_data';
import { QueryEngine } from './query_engine';
import { ReferenceSet } from './reference_set';
import { RemoteDocumentCache } from './remote_document_cache';
import { RemoteDocumentChangeBuffer } from './remote_document_change_buffer';
Expand Down Expand Up @@ -158,6 +159,7 @@ export class LocalStore {
constructor(
/** Manages our in-memory or durable persistence. */
private persistence: Persistence,
private queryEngine: QueryEngine,
initialUser: User
) {
assert(
Expand All @@ -175,6 +177,7 @@ export class LocalStore {
this.mutationQueue,
this.persistence.getIndexManager()
);
this.queryEngine.setLocalDocumentsView(this.localDocuments);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come we construct the QueryEngine in FirestoreClient and pass it into LocalStore, rather than just construct it here in LocalStore? If we could do that, then we wouldn't need this awkward setLocalDocumentsView() method that must be called before use...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It allows to parameterize the tests for the LocalStore. Fun times.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's tempting to suggest that we just have a useIndexFreeQueryEngine boolean argument or something that tests can use... But I don't care that much.

}

/**
Expand Down Expand Up @@ -207,6 +210,7 @@ export class LocalStore {
this.mutationQueue,
this.persistence.getIndexManager()
);
this.queryEngine.setLocalDocumentsView(this.localDocuments);
return this.mutationQueue.getAllMutationBatches(txn);
})
.next(newBatches => {
Expand Down Expand Up @@ -819,7 +823,7 @@ export class LocalStore {
*/
executeQuery(query: Query): Promise<DocumentMap> {
return this.persistence.runTransaction('Execute query', 'readonly', txn => {
return this.localDocuments.getDocumentsMatchingQuery(
return this.queryEngine.getDocumentsMatchingQuery(
txn,
query,
SnapshotVersion.MIN
Expand Down
39 changes: 39 additions & 0 deletions packages/firestore/src/local/query_engine.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/**
* @license
* Copyright 2019 Google Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import { Query } from '../core/query';
import { SnapshotVersion } from '../core/snapshot_version';
import { DocumentMap } from '../model/collections';
import { LocalDocumentsView } from './local_documents_view';
import { PersistenceTransaction } from './persistence';
import { PersistencePromise } from './persistence_promise';

/**
* Represents a query engine capable of performing queries over the local
* document cache. You must call `setLocalDocumentsView()` before using.
*/
export interface QueryEngine {
/** Sets the document view to query against. */
setLocalDocumentsView(localDocuments: LocalDocumentsView): void;

/** Returns all local documents matching the specified query. */
getDocumentsMatchingQuery(
transaction: PersistenceTransaction,
query: Query,
sinceReadTime: SnapshotVersion
): PersistencePromise<DocumentMap>;
}
57 changes: 57 additions & 0 deletions packages/firestore/src/local/simple_query_engine.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/**
* @license
* Copyright 2019 Google Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import { SnapshotVersion } from '../core/snapshot_version';
import { Query } from '../core/query';
import { DocumentMap } from '../model/collections';
import { assert } from '../util/assert';
import { LocalDocumentsView } from './local_documents_view';
import { QueryEngine } from './query_engine';
import { PersistenceTransaction } from './persistence';
import { PersistencePromise } from './persistence_promise';

/**
* A naive implementation of QueryEngine that just loads all the documents in
* the queried collection and then filters them in memory.
*/
export class SimpleQueryEngine implements QueryEngine {
private localDocumentsView: LocalDocumentsView | undefined;

setLocalDocumentsView(localDocuments: LocalDocumentsView): void {
this.localDocumentsView = localDocuments;
}

/** Returns all local documents matching the specified query. */
getDocumentsMatchingQuery(
transaction: PersistenceTransaction,
query: Query,
sinceReadTime: SnapshotVersion
): PersistencePromise<DocumentMap> {
assert(
this.localDocumentsView !== undefined,
'setLocalDocumentsView() not called'
);

// TODO: Once LocalDocumentsView provides a getCollectionDocuments()
// method, we should call that here and then filter the results.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrm. What's the context for this TODO (other than the fact that it exists in Android too)? Why do we expect LocalDocumentsView is going to provide a getCollectionDocuments() method in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. I don't know. I ported it cause it was there an Android. Unfortunately, all traces of who added it are gone by now. I could invest some research and dig this up on Codesearch, but I suspect you would not like the outcome :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohhh... GRRRR... I forgot about that. I suspect it used to be TODO(indexing) which would imply it doesn't apply to web. So I would prefer if you don't port it. But that means you have to run CI again, so maybe not worth removing (it's friday, YOLO, etc.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll drop it in the next PR.

return this.localDocumentsView!.getDocumentsMatchingQuery(
transaction,
query,
SnapshotVersion.MIN
);
}
}
7 changes: 6 additions & 1 deletion packages/firestore/test/unit/local/local_store.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import { IndexedDbPersistence } from '../../../src/local/indexeddb_persistence';
import { LocalStore, LocalWriteResult } from '../../../src/local/local_store';
import { LocalViewChanges } from '../../../src/local/local_view_changes';
import { Persistence } from '../../../src/local/persistence';
import { QueryEngine } from '../../../src/local/query_engine';
import { SimpleQueryEngine } from '../../../src/local/simple_query_engine';
import {
documentKeySet,
DocumentMap,
Expand Down Expand Up @@ -280,6 +282,7 @@ describe('LocalStore w/ Memory Persistence', () => {
addEqualityMatcher();
genericLocalStoreTests(
persistenceHelpers.testMemoryEagerPersistence,
new SimpleQueryEngine(),
/* gcIsEager= */ true
);
});
Expand All @@ -295,20 +298,22 @@ describe('LocalStore w/ IndexedDB Persistence', () => {
addEqualityMatcher();
genericLocalStoreTests(
persistenceHelpers.testIndexedDbPersistence,
new SimpleQueryEngine(),
/* gcIsEager= */ false
);
});

function genericLocalStoreTests(
getPersistence: () => Promise<Persistence>,
queryEngine: QueryEngine,
gcIsEager: boolean
): void {
let persistence: Persistence;
let localStore: LocalStore;

beforeEach(async () => {
persistence = await getPersistence();
localStore = new LocalStore(persistence, User.UNAUTHENTICATED);
localStore = new LocalStore(persistence, queryEngine, User.UNAUTHENTICATED);
});

afterEach(async () => {
Expand Down
5 changes: 4 additions & 1 deletion packages/firestore/test/unit/specs/spec_test_runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ import {
WebStorageSharedClientState
} from '../../../src/local/shared_client_state';
import { SimpleDb } from '../../../src/local/simple_db';
import { SimpleQueryEngine } from '../../../src/local/simple_query_engine';
import { DocumentOptions } from '../../../src/model/document';
import { DocumentKey } from '../../../src/model/document_key';
import { JsonObject } from '../../../src/model/field_value';
Expand Down Expand Up @@ -434,7 +435,9 @@ abstract class TestRunner {
this.useGarbageCollection
);

this.localStore = new LocalStore(this.persistence, this.user);
// TODO(index-free): Update to index-free query engine when it becomes default.
const queryEngine = new SimpleQueryEngine();
this.localStore = new LocalStore(this.persistence, queryEngine, this.user);

this.connection = new MockConnection(this.queue);
this.datastore = new Datastore(
Expand Down