From 3d6d92b8b3fc3aef30bf8b98469e541b273b88fd Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 30 Nov 2022 11:20:39 -0800 Subject: [PATCH 1/3] add bloom filter to existence filter, and watchFilters spec builder --- .../src/protos/firestore_proto_api.ts | 2 + .../firestore/src/remote/existence_filter.ts | 5 +- packages/firestore/src/remote/serializer.ts | 5 +- .../unit/specs/existence_filter_spec.test.ts | 58 ++++++++++++++++--- .../test/unit/specs/limbo_spec.test.ts | 2 +- .../test/unit/specs/limit_spec.test.ts | 2 +- .../firestore/test/unit/specs/spec_builder.ts | 12 ++-- .../test/unit/specs/spec_test_runner.ts | 15 ++--- packages/firestore/test/util/helpers.ts | 8 ++- .../firestore/test/util/spec_test_helpers.ts | 3 +- 10 files changed, 81 insertions(+), 31 deletions(-) diff --git a/packages/firestore/src/protos/firestore_proto_api.ts b/packages/firestore/src/protos/firestore_proto_api.ts index 08ed4f676be..5777a3fb860 100644 --- a/packages/firestore/src/protos/firestore_proto_api.ts +++ b/packages/firestore/src/protos/firestore_proto_api.ts @@ -456,6 +456,8 @@ export declare type BeginTransactionRequest = firestoreV1ApiClientInterfaces.BeginTransactionRequest; export declare type BeginTransactionResponse = firestoreV1ApiClientInterfaces.BeginTransactionResponse; + export declare type BloomFilter = + firestoreV1ApiClientInterfaces.BloomFilter; export declare type CollectionSelector = firestoreV1ApiClientInterfaces.CollectionSelector; export declare type CommitRequest = diff --git a/packages/firestore/src/remote/existence_filter.ts b/packages/firestore/src/remote/existence_filter.ts index ec2240851ed..0d5ec72eb7e 100644 --- a/packages/firestore/src/remote/existence_filter.ts +++ b/packages/firestore/src/remote/existence_filter.ts @@ -15,7 +15,10 @@ * limitations under the License. */ +import { BloomFilter as ProtoBloomFilter } from '../protos/firestore_proto_api'; + export class ExistenceFilter { // TODO(b/33078163): just use simplest form of existence filter for now - constructor(public count: number) {} + constructor(public count: number, public unchangedNames?: ProtoBloomFilter) { + } } diff --git a/packages/firestore/src/remote/serializer.ts b/packages/firestore/src/remote/serializer.ts index c537648775f..8dc771d85d4 100644 --- a/packages/firestore/src/remote/serializer.ts +++ b/packages/firestore/src/remote/serializer.ts @@ -15,6 +15,7 @@ * limitations under the License. */ +import { resumeTokenForSnapshot } from '../../test/util/helpers'; import { Bound } from '../core/bound'; import { DatabaseId } from '../core/database_info'; import { @@ -537,8 +538,8 @@ export function fromWatchChange( assertPresent(change.filter, 'filter'); const filter = change.filter; assertPresent(filter.targetId, 'filter.targetId'); - const count = filter.count || 0; - const existenceFilter = new ExistenceFilter(count); + const { count = 0, unchangedNames } = filter; + const existenceFilter = new ExistenceFilter(count, unchangedNames); const targetId = filter.targetId; watchChange = new ExistenceFilterChange(targetId, existenceFilter); } else { diff --git a/packages/firestore/test/unit/specs/existence_filter_spec.test.ts b/packages/firestore/test/unit/specs/existence_filter_spec.test.ts index 4943d986bd2..45df01edef7 100644 --- a/packages/firestore/test/unit/specs/existence_filter_spec.test.ts +++ b/packages/firestore/test/unit/specs/existence_filter_spec.test.ts @@ -23,7 +23,7 @@ import { describeSpec, specTest } from './describe_spec'; import { spec } from './spec_builder'; import { RpcError } from './spec_rpc_error'; -describeSpec('Existence Filters:', [], () => { +describeSpec('Existence Filters:', ['exclusive'], () => { specTest('Existence filter match', [], () => { const query1 = query('collection'); const doc1 = doc('collection/1', 1000, { v: 1 }); @@ -31,10 +31,24 @@ describeSpec('Existence Filters:', [], () => { .userListens(query1) .watchAcksFull(query1, 1000, doc1) .expectEvents(query1, { added: [doc1] }) - .watchFilters([query1], doc1.key) + .watchFilters([query1], [doc1.key]) .watchSnapshots(2000); }); + // TODO:(mila) update the tests when bloom filter is + specTest('Existence filter with bloom filter match', ['exclusive'], () => { + const query1 = query('collection'); + const doc1 = doc('collection/1', 1000, { v: 1 }); + return spec() + .userListens(query1) + .watchAcksFull(query1, 1000, doc1) + .expectEvents(query1, { added: [doc1] }) + .watchFilters([query1], [doc1.key],{bits:{bitmap:"a",padding:1}, hashCount:1}) + .watchSnapshots(2000) + .watchFilters([query1]) + .watchSnapshots(3000); + }); + specTest('Existence filter match after pending update', [], () => { const query1 = query('collection'); const doc1 = doc('collection/1', 2000, { v: 2 }); @@ -45,7 +59,7 @@ describeSpec('Existence Filters:', [], () => { .watchSnapshots(2000) .expectEvents(query1, {}) .watchSends({ affects: [query1] }, doc1) - .watchFilters([query1], doc1.key) + .watchFilters([query1], [doc1.key]) .watchSnapshots(2000) .expectEvents(query1, { added: [doc1] }); }); @@ -59,7 +73,7 @@ describeSpec('Existence Filters:', [], () => { .watchCurrents(query1, 'resume-token-1000') .watchSnapshots(2000) .expectEvents(query1, {}) - .watchFilters([query1], doc1.key) + .watchFilters([query1], [doc1.key]) .watchSnapshots(2000) .expectEvents(query1, { fromCache: true }); }); @@ -96,7 +110,33 @@ describeSpec('Existence Filters:', [], () => { .userListens(query1) .watchAcksFull(query1, 1000, doc1, doc2) .expectEvents(query1, { added: [doc1, doc2] }) - .watchFilters([query1], doc1.key) // in the next sync doc2 was deleted + .watchFilters([query1], [doc1.key]) // in the next sync doc2 was deleted + .watchSnapshots(2000) + // query is now marked as "inconsistent" because of filter mismatch + .expectEvents(query1, { fromCache: true }) + .expectActiveTargets({ query: query1, resumeToken: '' }) + .watchRemoves(query1) // Acks removal of query + .watchAcksFull(query1, 2000, doc1) + .expectLimboDocs(doc2.key) // doc2 is now in limbo + .ackLimbo(2000, deletedDoc('collection/2', 2000)) + .expectLimboDocs() // doc2 is no longer in limbo + .expectEvents(query1, { + removed: [doc2] + }) + ); + }); + + // todo + specTest('Existence filter mismatch triggers bloom filter', ['exclusive'], () => { + const query1 = query('collection'); + const doc1 = doc('collection/1', 1000, { v: 1 }); + const doc2 = doc('collection/2', 1000, { v: 2 }); + return ( + spec() + .userListens(query1) + .watchAcksFull(query1, 1000, doc1, doc2) + .expectEvents(query1, { added: [doc1, doc2] }) + .watchFilters([query1], [doc1.key],{bits:{bitmap:"a",padding:1},hashCount:3}) // in the next sync doc2 was deleted .watchSnapshots(2000) // query is now marked as "inconsistent" because of filter mismatch .expectEvents(query1, { fromCache: true }) @@ -130,7 +170,7 @@ describeSpec('Existence Filters:', [], () => { resumeToken: 'existence-filter-resume-token' }) .watchAcks(query1) - .watchFilters([query1], doc1.key) // in the next sync doc2 was deleted + .watchFilters([query1], [doc1.key]) // in the next sync doc2 was deleted .watchSnapshots(2000) // query is now marked as "inconsistent" because of filter mismatch .expectEvents(query1, { fromCache: true }) @@ -159,7 +199,7 @@ describeSpec('Existence Filters:', [], () => { // Send a mismatching existence filter with two documents, but don't // send a new global snapshot. We should not see an event until we // receive the snapshot. - .watchFilters([query1], doc1.key, doc2.key) + .watchFilters([query1], [doc1.key, doc2.key]) .watchSends({ affects: [query1] }, doc3) .watchSnapshots(2000) // The query result includes doc3, but is marked as "inconsistent" @@ -193,7 +233,7 @@ describeSpec('Existence Filters:', [], () => { .userListens(query1) .watchAcksFull(query1, 1000, doc1, doc2) .expectEvents(query1, { added: [doc1, doc2] }) - .watchFilters([query1], doc1.key) // in the next sync doc2 was deleted + .watchFilters([query1], [doc1.key]) // in the next sync doc2 was deleted .watchSnapshots(2000) // query is now marked as "inconsistent" because of filter mismatch .expectEvents(query1, { fromCache: true }) @@ -229,7 +269,7 @@ describeSpec('Existence Filters:', [], () => { .userListens(query1) .watchAcksFull(query1, 1000, doc1, doc2) .expectEvents(query1, { added: [doc1, doc2] }) - .watchFilters([query1], doc1.key) // doc2 was deleted + .watchFilters([query1], [doc1.key]) // doc2 was deleted .watchSnapshots(2000) .expectEvents(query1, { fromCache: true }) // The SDK is unable to re-run the query, and does not remove doc2 diff --git a/packages/firestore/test/unit/specs/limbo_spec.test.ts b/packages/firestore/test/unit/specs/limbo_spec.test.ts index b7db2049ec9..4451fc5a80a 100644 --- a/packages/firestore/test/unit/specs/limbo_spec.test.ts +++ b/packages/firestore/test/unit/specs/limbo_spec.test.ts @@ -880,7 +880,7 @@ describeSpec('Limbo Documents:', [], () => { // documents that changed since the resume token. This will cause it // to just send the docBs with an existence filter with a count of 3. .watchSends({ affects: [query1] }, docB1, docB2, docB3) - .watchFilters([query1], docB1.key, docB2.key, docB3.key) + .watchFilters([query1], [docB1.key, docB2.key, docB3.key]) .watchSnapshots(1001) .expectEvents(query1, { added: [docB1, docB2, docB3], diff --git a/packages/firestore/test/unit/specs/limit_spec.test.ts b/packages/firestore/test/unit/specs/limit_spec.test.ts index 678f929743d..133fd1b1b22 100644 --- a/packages/firestore/test/unit/specs/limit_spec.test.ts +++ b/packages/firestore/test/unit/specs/limit_spec.test.ts @@ -341,7 +341,7 @@ describeSpec('Limits:', [], () => { // we receive an existence filter, which indicates that our view is // out of sync. .watchSends({ affects: [limitQuery] }, secondDocument) - .watchFilters([limitQuery], secondDocument.key) + .watchFilters([limitQuery], [secondDocument.key]) .watchSnapshots(1004) .expectActiveTargets({ query: limitQuery, resumeToken: '' }) .watchRemoves(limitQuery) diff --git a/packages/firestore/test/unit/specs/spec_builder.ts b/packages/firestore/test/unit/specs/spec_builder.ts index fe3f079edb4..9d34b09875f 100644 --- a/packages/firestore/test/unit/specs/spec_builder.ts +++ b/packages/firestore/test/unit/specs/spec_builder.ts @@ -33,6 +33,7 @@ import { DocumentKey } from '../../../src/model/document_key'; import { FieldIndex } from '../../../src/model/field_index'; import { JsonObject } from '../../../src/model/object_value'; import { ResourcePath } from '../../../src/model/path'; +import { BloomFilter as ProtoBloomFilter} from '../../../src/protos/firestore_proto_api'; import { isPermanentWriteError, mapCodeFromRpcCode, @@ -769,7 +770,11 @@ export class SpecBuilder { return this; } - watchFilters(queries: Query[], ...docs: DocumentKey[]): this { + watchFilters( + queries: Query[], + docs: DocumentKey[] = [], + bloomFilter?: ProtoBloomFilter + ): this { this.nextStep(); const targetIds = queries.map(query => { return this.getTargetId(query); @@ -777,10 +782,7 @@ export class SpecBuilder { const keys = docs.map(key => { return key.path.canonicalString(); }); - const filter: SpecWatchFilter = [targetIds] as SpecWatchFilter; - for (const key of keys) { - filter.push(key); - } + const filter = { targetIds, keys, bloomFilter } as SpecWatchFilter; this.currentStep = { watchFilter: filter }; diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts index 8a6909b3f2e..f011856c860 100644 --- a/packages/firestore/test/unit/specs/spec_test_runner.ts +++ b/packages/firestore/test/unit/specs/spec_test_runner.ts @@ -693,13 +693,12 @@ abstract class TestRunner { } private doWatchFilter(watchFilter: SpecWatchFilter): Promise { - const targetIds: TargetId[] = watchFilter[0]; + const { targetIds, keys, bloomFilter } = watchFilter; debugAssert( targetIds.length === 1, 'ExistenceFilters currently support exactly one target only.' ); - const keys = watchFilter.slice(1); - const filter = new ExistenceFilter(keys.length); + const filter = new ExistenceFilter(keys.length, bloomFilter); const change = new ExistenceFilterChange(targetIds[0], filter); return this.doWatchEvent(change); } @@ -1577,14 +1576,12 @@ export interface SpecClientState { } /** - * [[, ...], , ...] - * Note that the last parameter is really of type ...string (spread operator) * The filter is based of a list of keys to match in the existence filter */ -export interface SpecWatchFilter - extends Array { - '0': TargetId[]; - '1': string | undefined; +export interface SpecWatchFilter { + targetIds: TargetId[]; + keys: string[]; + bloomFilter?: api.BloomFilter; } export type SpecLimitType = 'LimitToFirst' | 'LimitToLast'; diff --git a/packages/firestore/test/util/helpers.ts b/packages/firestore/test/util/helpers.ts index 24cb7bccf0d..2d6fbb69fc8 100644 --- a/packages/firestore/test/util/helpers.ts +++ b/packages/firestore/test/util/helpers.ts @@ -429,7 +429,8 @@ export function existenceFilterEvent( targetId: number, syncedKeys: DocumentKeySet, remoteCount: number, - snapshotVersion: number + snapshotVersion: number, + bloomFilter?: api.BloomFilter ): RemoteEvent { const aggregator = new WatchChangeAggregator({ getRemoteKeysForTarget: () => syncedKeys, @@ -437,7 +438,10 @@ export function existenceFilterEvent( targetData(targetId, TargetPurpose.Listen, 'foo') }); aggregator.handleExistenceFilter( - new ExistenceFilterChange(targetId, new ExistenceFilter(remoteCount)) + new ExistenceFilterChange( + targetId, + new ExistenceFilter(remoteCount, bloomFilter) + ) ); return aggregator.createRemoteEvent(version(snapshotVersion)); } diff --git a/packages/firestore/test/util/spec_test_helpers.ts b/packages/firestore/test/util/spec_test_helpers.ts index c721d0e0675..cc4793caaa9 100644 --- a/packages/firestore/test/util/spec_test_helpers.ts +++ b/packages/firestore/test/util/spec_test_helpers.ts @@ -44,8 +44,9 @@ export function encodeWatchChange( if (watchChange instanceof ExistenceFilterChange) { return { filter: { + targetId: watchChange.targetId, count: watchChange.existenceFilter.count, - targetId: watchChange.targetId + unchangedNames:watchChange.existenceFilter.unchangedNames } }; } From b02d4a9405f09b1d9c7bf1cf40e0d2f99b8b06b0 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 30 Nov 2022 11:32:28 -0800 Subject: [PATCH 2/3] remove 'exclusive' --- .../firestore/src/remote/existence_filter.ts | 3 +-- packages/firestore/src/remote/serializer.ts | 1 - .../unit/specs/existence_filter_spec.test.ts | 26 ++++++++++++------- .../firestore/test/unit/specs/spec_builder.ts | 2 +- .../firestore/test/util/spec_test_helpers.ts | 2 +- 5 files changed, 19 insertions(+), 15 deletions(-) diff --git a/packages/firestore/src/remote/existence_filter.ts b/packages/firestore/src/remote/existence_filter.ts index 0d5ec72eb7e..6b6eeb2d49d 100644 --- a/packages/firestore/src/remote/existence_filter.ts +++ b/packages/firestore/src/remote/existence_filter.ts @@ -19,6 +19,5 @@ import { BloomFilter as ProtoBloomFilter } from '../protos/firestore_proto_api'; export class ExistenceFilter { // TODO(b/33078163): just use simplest form of existence filter for now - constructor(public count: number, public unchangedNames?: ProtoBloomFilter) { - } + constructor(public count: number, public unchangedNames?: ProtoBloomFilter) {} } diff --git a/packages/firestore/src/remote/serializer.ts b/packages/firestore/src/remote/serializer.ts index 8dc771d85d4..bbf6a112642 100644 --- a/packages/firestore/src/remote/serializer.ts +++ b/packages/firestore/src/remote/serializer.ts @@ -15,7 +15,6 @@ * limitations under the License. */ -import { resumeTokenForSnapshot } from '../../test/util/helpers'; import { Bound } from '../core/bound'; import { DatabaseId } from '../core/database_info'; import { diff --git a/packages/firestore/test/unit/specs/existence_filter_spec.test.ts b/packages/firestore/test/unit/specs/existence_filter_spec.test.ts index 45df01edef7..32346c31af5 100644 --- a/packages/firestore/test/unit/specs/existence_filter_spec.test.ts +++ b/packages/firestore/test/unit/specs/existence_filter_spec.test.ts @@ -23,7 +23,7 @@ import { describeSpec, specTest } from './describe_spec'; import { spec } from './spec_builder'; import { RpcError } from './spec_rpc_error'; -describeSpec('Existence Filters:', ['exclusive'], () => { +describeSpec('Existence Filters:', [], () => { specTest('Existence filter match', [], () => { const query1 = query('collection'); const doc1 = doc('collection/1', 1000, { v: 1 }); @@ -35,18 +35,20 @@ describeSpec('Existence Filters:', ['exclusive'], () => { .watchSnapshots(2000); }); - // TODO:(mila) update the tests when bloom filter is - specTest('Existence filter with bloom filter match', ['exclusive'], () => { + // This test is only to make sure watchFilters can accept bloom filter. + // TODO:(mila) update the tests when bloom filter logic is implemented. + specTest('Existence filter with bloom filter match', [], () => { const query1 = query('collection'); const doc1 = doc('collection/1', 1000, { v: 1 }); return spec() .userListens(query1) .watchAcksFull(query1, 1000, doc1) .expectEvents(query1, { added: [doc1] }) - .watchFilters([query1], [doc1.key],{bits:{bitmap:"a",padding:1}, hashCount:1}) - .watchSnapshots(2000) - .watchFilters([query1]) - .watchSnapshots(3000); + .watchFilters([query1], [doc1.key], { + bits: { bitmap: 'a', padding: 1 }, + hashCount: 1 + }) + .watchSnapshots(2000); }); specTest('Existence filter match after pending update', [], () => { @@ -126,8 +128,9 @@ describeSpec('Existence Filters:', ['exclusive'], () => { ); }); - // todo - specTest('Existence filter mismatch triggers bloom filter', ['exclusive'], () => { + // This test is only to make sure watchFilters can accept bloom filter. + // TODO:(mila) update the tests when bloom filter logic is implemented. + specTest('Existence filter mismatch triggers bloom filter', [], () => { const query1 = query('collection'); const doc1 = doc('collection/1', 1000, { v: 1 }); const doc2 = doc('collection/2', 1000, { v: 2 }); @@ -136,7 +139,10 @@ describeSpec('Existence Filters:', ['exclusive'], () => { .userListens(query1) .watchAcksFull(query1, 1000, doc1, doc2) .expectEvents(query1, { added: [doc1, doc2] }) - .watchFilters([query1], [doc1.key],{bits:{bitmap:"a",padding:1},hashCount:3}) // in the next sync doc2 was deleted + .watchFilters([query1], [doc1.key], { + bits: { bitmap: 'a', padding: 1 }, + hashCount: 3 + }) // in the next sync doc2 was deleted .watchSnapshots(2000) // query is now marked as "inconsistent" because of filter mismatch .expectEvents(query1, { fromCache: true }) diff --git a/packages/firestore/test/unit/specs/spec_builder.ts b/packages/firestore/test/unit/specs/spec_builder.ts index 9d34b09875f..a53015058db 100644 --- a/packages/firestore/test/unit/specs/spec_builder.ts +++ b/packages/firestore/test/unit/specs/spec_builder.ts @@ -33,7 +33,7 @@ import { DocumentKey } from '../../../src/model/document_key'; import { FieldIndex } from '../../../src/model/field_index'; import { JsonObject } from '../../../src/model/object_value'; import { ResourcePath } from '../../../src/model/path'; -import { BloomFilter as ProtoBloomFilter} from '../../../src/protos/firestore_proto_api'; +import { BloomFilter as ProtoBloomFilter } from '../../../src/protos/firestore_proto_api'; import { isPermanentWriteError, mapCodeFromRpcCode, diff --git a/packages/firestore/test/util/spec_test_helpers.ts b/packages/firestore/test/util/spec_test_helpers.ts index cc4793caaa9..66d4eae34c6 100644 --- a/packages/firestore/test/util/spec_test_helpers.ts +++ b/packages/firestore/test/util/spec_test_helpers.ts @@ -46,7 +46,7 @@ export function encodeWatchChange( filter: { targetId: watchChange.targetId, count: watchChange.existenceFilter.count, - unchangedNames:watchChange.existenceFilter.unchangedNames + unchangedNames: watchChange.existenceFilter.unchangedNames } }; } From 0b76b5e76f2f0eb7ee38a64e1b534fcc78070ff2 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 30 Nov 2022 12:28:35 -0800 Subject: [PATCH 3/3] resolve comments --- packages/firestore/src/protos/firestore_proto_api.ts | 3 +-- packages/firestore/src/remote/existence_filter.ts | 1 - 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/firestore/src/protos/firestore_proto_api.ts b/packages/firestore/src/protos/firestore_proto_api.ts index 5777a3fb860..4e184f91071 100644 --- a/packages/firestore/src/protos/firestore_proto_api.ts +++ b/packages/firestore/src/protos/firestore_proto_api.ts @@ -456,8 +456,7 @@ export declare type BeginTransactionRequest = firestoreV1ApiClientInterfaces.BeginTransactionRequest; export declare type BeginTransactionResponse = firestoreV1ApiClientInterfaces.BeginTransactionResponse; - export declare type BloomFilter = - firestoreV1ApiClientInterfaces.BloomFilter; +export declare type BloomFilter = firestoreV1ApiClientInterfaces.BloomFilter; export declare type CollectionSelector = firestoreV1ApiClientInterfaces.CollectionSelector; export declare type CommitRequest = diff --git a/packages/firestore/src/remote/existence_filter.ts b/packages/firestore/src/remote/existence_filter.ts index 6b6eeb2d49d..377b1f017da 100644 --- a/packages/firestore/src/remote/existence_filter.ts +++ b/packages/firestore/src/remote/existence_filter.ts @@ -18,6 +18,5 @@ import { BloomFilter as ProtoBloomFilter } from '../protos/firestore_proto_api'; export class ExistenceFilter { - // TODO(b/33078163): just use simplest form of existence filter for now constructor(public count: number, public unchangedNames?: ProtoBloomFilter) {} }