From 203e9437202a6a2d7c751ac525cb1bb4d71a2a3a Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 8 Mar 2023 15:42:38 -0800 Subject: [PATCH 1/4] initial code --- .../firestore/src/core/sync_engine_impl.ts | 5 +- .../firestore/src/local/local_store_impl.ts | 2 +- packages/firestore/src/local/target_data.ts | 5 ++ packages/firestore/src/remote/remote_event.ts | 11 ++-- packages/firestore/src/remote/remote_store.ts | 6 +-- packages/firestore/src/remote/serializer.ts | 4 +- packages/firestore/src/remote/watch_change.ts | 53 ++++++++++++++----- .../test/integration/api/query.test.ts | 4 +- .../unit/specs/existence_filter_spec.test.ts | 25 +++++++-- .../firestore/test/unit/specs/spec_builder.ts | 36 +++++++++---- .../test/unit/specs/spec_test_components.ts | 13 ++++- .../test/unit/specs/spec_test_runner.ts | 13 ++++- 12 files changed, 131 insertions(+), 46 deletions(-) diff --git a/packages/firestore/src/core/sync_engine_impl.ts b/packages/firestore/src/core/sync_engine_impl.ts index efe65df6439..60a36d0dcaf 100644 --- a/packages/firestore/src/core/sync_engine_impl.ts +++ b/packages/firestore/src/core/sync_engine_impl.ts @@ -71,7 +71,6 @@ import { primitiveComparator } from '../util/misc'; import { ObjectMap } from '../util/obj_map'; import { Deferred } from '../util/promise'; import { SortedMap } from '../util/sorted_map'; -import { SortedSet } from '../util/sorted_set'; import { BATCHID_UNKNOWN } from '../util/types'; import { @@ -639,7 +638,9 @@ export async function syncEngineRejectListen( const event = new RemoteEvent( SnapshotVersion.min(), /* targetChanges= */ new Map(), - /* targetMismatches= */ new SortedSet(primitiveComparator), + /* targetMismatches= */ new SortedMap( + primitiveComparator + ), documentUpdates, resolvedLimboDocuments ); diff --git a/packages/firestore/src/local/local_store_impl.ts b/packages/firestore/src/local/local_store_impl.ts index ae1760f5930..c547d1e427d 100644 --- a/packages/firestore/src/local/local_store_impl.ts +++ b/packages/firestore/src/local/local_store_impl.ts @@ -601,7 +601,7 @@ export function localStoreApplyRemoteEventToLocalCache( let newTargetData = oldTargetData.withSequenceNumber( txn.currentSequenceNumber ); - if (remoteEvent.targetMismatches.has(targetId)) { + if (remoteEvent.targetMismatches.get(targetId) != null) { newTargetData = newTargetData .withResumeToken( ByteString.EMPTY_BYTE_STRING, diff --git a/packages/firestore/src/local/target_data.ts b/packages/firestore/src/local/target_data.ts index 2893cbf342a..bc822a967d9 100644 --- a/packages/firestore/src/local/target_data.ts +++ b/packages/firestore/src/local/target_data.ts @@ -30,6 +30,11 @@ export const enum TargetPurpose { */ ExistenceFilterMismatch, + /** + * The query target was used if the query is the result of a false positive in the bloom filter. + */ + ExistenceFilterMismatchBloom, + /** The query target was used to resolve a limbo document. */ LimboResolution } diff --git a/packages/firestore/src/remote/remote_event.ts b/packages/firestore/src/remote/remote_event.ts index 5d48d32bb3f..79ac4578267 100644 --- a/packages/firestore/src/remote/remote_event.ts +++ b/packages/firestore/src/remote/remote_event.ts @@ -17,15 +17,16 @@ import { SnapshotVersion } from '../core/snapshot_version'; import { TargetId } from '../core/types'; +import { TargetPurpose } from '../local/target_data'; import { documentKeySet, DocumentKeySet, mutableDocumentMap, - MutableDocumentMap, - targetIdSet + MutableDocumentMap } from '../model/collections'; import { ByteString } from '../util/byte_string'; -import { SortedSet } from '../util/sorted_set'; +import { primitiveComparator } from '../util/misc'; +import { SortedMap } from '../util/sorted_map'; /** * An event from the RemoteStore. It is split into targetChanges (changes to the @@ -46,7 +47,7 @@ export class RemoteEvent { * A set of targets that is known to be inconsistent. Listens for these * targets should be re-established without resume tokens. */ - readonly targetMismatches: SortedSet, + readonly targetMismatches: SortedMap, /** * A set of which documents have changed or been deleted, along with the * doc's new values (if not deleted). @@ -82,7 +83,7 @@ export class RemoteEvent { return new RemoteEvent( SnapshotVersion.min(), targetChanges, - targetIdSet(), + new SortedMap(primitiveComparator), mutableDocumentMap(), documentKeySet() ); diff --git a/packages/firestore/src/remote/remote_store.ts b/packages/firestore/src/remote/remote_store.ts index 68196a50ad2..8cb361bfe09 100644 --- a/packages/firestore/src/remote/remote_store.ts +++ b/packages/firestore/src/remote/remote_store.ts @@ -24,7 +24,7 @@ import { localStoreGetNextMutationBatch } from '../local/local_store_impl'; import { isIndexedDbTransactionError } from '../local/simple_db'; -import { TargetData, TargetPurpose } from '../local/target_data'; +import { TargetData } from '../local/target_data'; import { MutationResult } from '../model/mutation'; import { MutationBatch, MutationBatchResult } from '../model/mutation_batch'; import { debugAssert, debugCast } from '../util/assert'; @@ -587,7 +587,7 @@ function raiseWatchSnapshot( // Re-establish listens for the targets that have been invalidated by // existence filter mismatches. - remoteEvent.targetMismatches.forEach(targetId => { + remoteEvent.targetMismatches.forEach((targetId, targetPurpose) => { const targetData = remoteStoreImpl.listenTargets.get(targetId); if (!targetData) { // A watched target might have been removed already. @@ -615,7 +615,7 @@ function raiseWatchSnapshot( const requestTargetData = new TargetData( targetData.target, targetId, - TargetPurpose.ExistenceFilterMismatch, + targetPurpose, targetData.sequenceNumber ); sendWatchRequest(remoteStoreImpl, requestTargetData); diff --git a/packages/firestore/src/remote/serializer.ts b/packages/firestore/src/remote/serializer.ts index 39a787a943d..e51e9eef7f5 100644 --- a/packages/firestore/src/remote/serializer.ts +++ b/packages/firestore/src/remote/serializer.ts @@ -1018,7 +1018,7 @@ export function toListenRequestLabels( } } -function toLabel( +export function toLabel( serializer: JsonProtoSerializer, purpose: TargetPurpose ): string | null { @@ -1027,6 +1027,8 @@ function toLabel( return null; case TargetPurpose.ExistenceFilterMismatch: return 'existence-filter-mismatch'; + case TargetPurpose.ExistenceFilterMismatchBloom: + return 'existence-filter-mismatch-bloom'; case TargetPurpose.LimboResolution: return 'limbo-document'; default: diff --git a/packages/firestore/src/remote/watch_change.ts b/packages/firestore/src/remote/watch_change.ts index 82ef17213dd..c08370d71e4 100644 --- a/packages/firestore/src/remote/watch_change.ts +++ b/packages/firestore/src/remote/watch_change.ts @@ -87,6 +87,11 @@ export const enum WatchTargetChangeState { Reset } +const enum BloomFilterApplicationStatus { + Success, + Skipped, + FalsePositive +} export class WatchTargetChange { constructor( /** What kind of change occurred to the watch target. */ @@ -284,7 +289,9 @@ export class WatchChangeAggregator { * known to be inconsistent and their listens needs to be re-established by * RemoteStore. */ - private pendingTargetResets = new SortedSet(primitiveComparator); + private pendingTargetResets = new SortedMap( + primitiveComparator + ); /** * Processes and adds the DocumentWatchChange to the current set of changes. @@ -422,31 +429,39 @@ export class WatchChangeAggregator { // raise a snapshot with `isFromCache:true`. if (currentSize !== expectedCount) { // Apply bloom filter to identify and mark removed documents. - const bloomFilterApplied = this.applyBloomFilter( - watchChange, - currentSize - ); - if (!bloomFilterApplied) { + const status = this.applyBloomFilter(watchChange, currentSize); + if (status !== BloomFilterApplicationStatus.Success) { // If bloom filter application fails, we reset the mapping and // trigger re-run of the query. this.resetTarget(targetId); - this.pendingTargetResets = this.pendingTargetResets.add(targetId); + const purpose: TargetPurpose = + status === BloomFilterApplicationStatus.FalsePositive + ? TargetPurpose.ExistenceFilterMismatchBloom + : TargetPurpose.ExistenceFilterMismatch; + + this.pendingTargetResets = this.pendingTargetResets.insert( + targetId, + purpose + ); } } } } } - /** Returns whether a bloom filter removed the deleted documents successfully. */ + /** + * Apply bloom filter to remove the deleted documents, and return the + * application status. + */ private applyBloomFilter( watchChange: ExistenceFilterChange, currentCount: number - ): boolean { + ): BloomFilterApplicationStatus { const { unchangedNames, count: expectedCount } = watchChange.existenceFilter; if (!unchangedNames || !unchangedNames.bits) { - return false; + return BloomFilterApplicationStatus.Skipped; } const { @@ -454,6 +469,10 @@ export class WatchChangeAggregator { hashCount = 0 } = unchangedNames; + if (bitmap.length === 0) { + return BloomFilterApplicationStatus.Skipped; + } + let normalizedBitmap: Uint8Array; try { normalizedBitmap = normalizeByteString(bitmap).toUint8Array(); @@ -464,7 +483,7 @@ export class WatchChangeAggregator { err.message + '); ignoring the bloom filter and falling back to full re-query.' ); - return false; + return BloomFilterApplicationStatus.Skipped; } else { throw err; } @@ -480,7 +499,7 @@ export class WatchChangeAggregator { } else { logWarn('Applying bloom filter failed: ', err); } - return false; + return BloomFilterApplicationStatus.Skipped; } const removedDocumentCount = this.filterRemovedDocuments( @@ -488,7 +507,11 @@ export class WatchChangeAggregator { bloomFilter ); - return expectedCount === currentCount - removedDocumentCount; + if (expectedCount !== currentCount - removedDocumentCount) { + return BloomFilterApplicationStatus.FalsePositive; + } + + return BloomFilterApplicationStatus.Success; } /** @@ -599,7 +622,9 @@ export class WatchChangeAggregator { this.pendingDocumentUpdates = mutableDocumentMap(); this.pendingDocumentTargetMapping = documentTargetMap(); - this.pendingTargetResets = new SortedSet(primitiveComparator); + this.pendingTargetResets = new SortedMap( + primitiveComparator + ); return remoteEvent; } diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index 2438d8bdb6c..94e8221100f 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -2061,8 +2061,8 @@ apiDescribe('Queries', (persistence: boolean) => { }); }); - // TODO(Mila): Skip the test when using emulator as there is a bug related to - // sending existence filter in response: b/270731363. Remove the condition + // TODO(Mila): Skip the test when using emulator as there is a bug related to + // sending existence filter in response: b/270731363. Remove the condition // here once the bug is resolved. // eslint-disable-next-line no-restricted-properties (USE_EMULATOR ? it.skip : it)( 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 334d47ce178..2b67ef8212e 100644 --- a/packages/firestore/test/unit/specs/existence_filter_spec.test.ts +++ b/packages/firestore/test/unit/specs/existence_filter_spec.test.ts @@ -16,6 +16,7 @@ */ import { newQueryForPath } from '../../../src/core/query'; +import { TargetPurpose } from '../../../src/local/target_data'; import { Code } from '../../../src/util/error'; import { deletedDoc, @@ -305,7 +306,11 @@ describeSpec('Existence Filters:', [], () => { // BloomFilter correctly identifies docC is deleted, but yields false // positive results for docB. Re-run query is triggered. .expectEvents(query1, { fromCache: true }) - .expectActiveTargets({ query: query1, resumeToken: '' }) + .expectActiveTargets({ + query: query1, + resumeToken: '', + targetPurpose: TargetPurpose.ExistenceFilterMismatchBloom + }) ); } ); @@ -394,7 +399,11 @@ describeSpec('Existence Filters:', [], () => { .watchSnapshots(2000) // Re-run query is triggered. .expectEvents(query1, { fromCache: true }) - .expectActiveTargets({ query: query1, resumeToken: '' }) + .expectActiveTargets({ + query: query1, + resumeToken: '', + targetPurpose: TargetPurpose.ExistenceFilterMismatch + }) ); } ); @@ -424,7 +433,11 @@ describeSpec('Existence Filters:', [], () => { .watchSnapshots(2000) // Re-run query is triggered. .expectEvents(query1, { fromCache: true }) - .expectActiveTargets({ query: query1, resumeToken: '' }) + .expectActiveTargets({ + query: query1, + resumeToken: '', + targetPurpose: TargetPurpose.ExistenceFilterMismatch + }) ); } ); @@ -452,7 +465,11 @@ describeSpec('Existence Filters:', [], () => { .watchSnapshots(2000) // Re-run query is triggered. .expectEvents(query1, { fromCache: true }) - .expectActiveTargets({ query: query1, resumeToken: '' }) + .expectActiveTargets({ + query: query1, + resumeToken: '', + targetPurpose: TargetPurpose.ExistenceFilterMismatch + }) ); }); diff --git a/packages/firestore/test/unit/specs/spec_builder.ts b/packages/firestore/test/unit/specs/spec_builder.ts index a18e5742256..5cff73f45a3 100644 --- a/packages/firestore/test/unit/specs/spec_builder.ts +++ b/packages/firestore/test/unit/specs/spec_builder.ts @@ -28,6 +28,7 @@ import { import { canonifyTarget, Target, targetEquals } from '../../../src/core/target'; import { TargetIdGenerator } from '../../../src/core/target_id_generator'; import { TargetId } from '../../../src/core/types'; +import { TargetPurpose } from '../../../src/local/target_data'; import { Document } from '../../../src/model/document'; import { DocumentKey } from '../../../src/model/document_key'; import { FieldIndex } from '../../../src/model/field_index'; @@ -77,6 +78,7 @@ export interface ActiveTargetSpec { resumeToken?: string; readTime?: TestSnapshotVersion; expectedCount?: number; + targetPurpose?: TargetPurpose; } export interface ActiveTargetMap { @@ -538,18 +540,26 @@ export class SpecBuilder { resumeToken?: string; readTime?: TestSnapshotVersion; expectedCount?: number; + targetPurpose?: TargetPurpose; }> ): this { this.assertStep('Active target expectation requires previous step'); const currentStep = this.currentStep!; this.clientState.activeTargets = {}; - targets.forEach(({ query, resumeToken, readTime, expectedCount }) => { - this.addQueryToActiveTargets(this.getTargetId(query), query, { - resumeToken, - readTime, - expectedCount - }); - }); + targets.forEach( + ({ query, resumeToken, readTime, expectedCount, targetPurpose }) => { + this.addQueryToActiveTargets( + this.getTargetId(query), + query, + { + resumeToken, + readTime, + expectedCount + }, + targetPurpose + ); + } + ); currentStep.expectedState = currentStep.expectedState || {}; currentStep.expectedState.activeTargets = { ...this.activeTargets }; return this; @@ -1093,7 +1103,8 @@ export class SpecBuilder { private addQueryToActiveTargets( targetId: number, query: Query, - resume?: ResumeSpec + resume?: ResumeSpec, + targetPurpose?: TargetPurpose ): void { if (!(resume?.resumeToken || resume?.readTime) && resume?.expectedCount) { fail('Expected count is present without a resume token or read time.'); @@ -1111,14 +1122,16 @@ export class SpecBuilder { queries: [SpecBuilder.queryToSpec(query), ...activeQueries], resumeToken: resume?.resumeToken || '', readTime: resume?.readTime, - expectedCount: resume?.expectedCount + expectedCount: resume?.expectedCount, + targetPurpose }; } else { this.activeTargets[targetId] = { queries: activeQueries, resumeToken: resume?.resumeToken || '', readTime: resume?.readTime, - expectedCount: resume?.expectedCount + expectedCount: resume?.expectedCount, + targetPurpose }; } } else { @@ -1126,7 +1139,8 @@ export class SpecBuilder { queries: [SpecBuilder.queryToSpec(query)], resumeToken: resume?.resumeToken || '', readTime: resume?.readTime, - expectedCount: resume?.expectedCount + expectedCount: resume?.expectedCount, + targetPurpose }; } } diff --git a/packages/firestore/test/unit/specs/spec_test_components.ts b/packages/firestore/test/unit/specs/spec_test_components.ts index 1436efa3fcd..c51bc1a461b 100644 --- a/packages/firestore/test/unit/specs/spec_test_components.ts +++ b/packages/firestore/test/unit/specs/spec_test_components.ts @@ -53,6 +53,7 @@ import { Mutation } from '../../../src/model/mutation'; import { encodeBase64 } from '../../../src/platform/base64'; import { newSerializer } from '../../../src/platform/serializer'; import * as api from '../../../src/protos/firestore_proto_api'; +import { ApiClientObjectMap } from '../../../src/protos/firestore_proto_api'; import { Connection, Stream } from '../../../src/remote/connection'; import { Datastore, newDatastore } from '../../../src/remote/datastore'; import { WriteRequest } from '../../../src/remote/persistent_stream'; @@ -259,7 +260,12 @@ export class MockConnection implements Connection { * Tracks the currently active watch targets as detected by the mock watch * stream, as a mapping from target ID to query Target. */ - activeTargets: { [targetId: number]: api.Target } = {}; + activeTargets: { + [targetId: number]: { + target: api.Target; + labels?: ApiClientObjectMap; + }; + } = {}; /** A Deferred that is resolved once watch opens. */ watchOpen = new Deferred(); @@ -398,7 +404,10 @@ export class MockConnection implements Connection { ++this.watchStreamRequestCount; if (request.addTarget) { const targetId = request.addTarget.targetId!; - this.activeTargets[targetId] = request.addTarget; + this.activeTargets[targetId] = { + target: request.addTarget, + labels: request.labels + }; } else if (request.removeTarget) { delete this.activeTargets[request.removeTarget]; } else { diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts index 137f51d6d54..3ac624efb6d 100644 --- a/packages/firestore/test/unit/specs/spec_test_runner.ts +++ b/packages/firestore/test/unit/specs/spec_test_runner.ts @@ -105,6 +105,7 @@ import { import { mapCodeFromRpcCode } from '../../../src/remote/rpc_error'; import { JsonProtoSerializer, + toLabel, toMutation, toTarget, toVersion @@ -1095,7 +1096,17 @@ abstract class TestRunner { undefined, 'Expected active target not found: ' + JSON.stringify(expected) ); - const actualTarget = actualTargets[targetId]; + const { target: actualTarget, labels } = actualTargets[targetId]; + + if (expected.targetPurpose) { + debugAssert( + labels !== undefined, + "Actual listen request doesn't have a 'goog-listen-tags'" + ); + expect(labels['goog-listen-tags']).to.equal( + toLabel(this.serializer, expected.targetPurpose) + ); + } // TODO(mcg): populate the purpose of the target once it's possible to // encode that in the spec tests. For now, hard-code that it's a listen From cc17bc9f4b7f6003d36e79274e0d8036ba569e09 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 8 Mar 2023 17:39:41 -0800 Subject: [PATCH 2/4] format --- packages/firestore/src/remote/bloom_filter.ts | 4 ++++ packages/firestore/src/remote/remote_event.ts | 5 +++-- packages/firestore/src/remote/remote_store.ts | 5 ++++- packages/firestore/src/remote/serializer.ts | 7 ++----- packages/firestore/src/remote/watch_change.ts | 13 +++++++------ .../firestore/test/unit/remote/remote_event.test.ts | 6 ++++++ .../firestore/test/unit/specs/spec_test_runner.ts | 2 +- 7 files changed, 27 insertions(+), 15 deletions(-) diff --git a/packages/firestore/src/remote/bloom_filter.ts b/packages/firestore/src/remote/bloom_filter.ts index 4c0e4ef6b7e..8fff2d80db7 100644 --- a/packages/firestore/src/remote/bloom_filter.ts +++ b/packages/firestore/src/remote/bloom_filter.ts @@ -75,6 +75,10 @@ export class BloomFilter { this.bitCountInInteger = Integer.fromNumber(this.bitCount); } + isEmpty(): boolean { + return this.bitCount === 0; + } + // Calculate the ith hash value based on the hashed 64bit integers, // and calculate its corresponding bit index in the bitmap to be checked. private getBitIndex(num1: Integer, num2: Integer, hashIndex: number): number { diff --git a/packages/firestore/src/remote/remote_event.ts b/packages/firestore/src/remote/remote_event.ts index 79ac4578267..49b2ef56a97 100644 --- a/packages/firestore/src/remote/remote_event.ts +++ b/packages/firestore/src/remote/remote_event.ts @@ -44,8 +44,9 @@ export class RemoteEvent { */ readonly targetChanges: Map, /** - * A set of targets that is known to be inconsistent. Listens for these - * targets should be re-established without resume tokens. + * A map of targets that is known to be inconsistent, and the purpose for + * re-listening. Listens for these targets should be re-established without + * resume tokens. */ readonly targetMismatches: SortedMap, /** diff --git a/packages/firestore/src/remote/remote_store.ts b/packages/firestore/src/remote/remote_store.ts index 8cb361bfe09..bd93a1bcb38 100644 --- a/packages/firestore/src/remote/remote_store.ts +++ b/packages/firestore/src/remote/remote_store.ts @@ -611,7 +611,10 @@ function raiseWatchSnapshot( // Mark the target we send as being on behalf of an existence filter // mismatch, but don't actually retain that in listenTargets. This ensures // that we flag the first re-listen this way without impacting future - // listens of this target (that might happen e.g. on reconnect). + // listens of this target (that might happen e.g. on reconnect). The target + // purpose will be `ExistenceFilterMismatchBloom` if there is a bloom filter + // but it yield false positive result, otherwise, it will be set to + // `ExistenceFilterMismatch`. const requestTargetData = new TargetData( targetData.target, targetId, diff --git a/packages/firestore/src/remote/serializer.ts b/packages/firestore/src/remote/serializer.ts index e51e9eef7f5..1945ddbff4e 100644 --- a/packages/firestore/src/remote/serializer.ts +++ b/packages/firestore/src/remote/serializer.ts @@ -1008,7 +1008,7 @@ export function toListenRequestLabels( serializer: JsonProtoSerializer, targetData: TargetData ): ProtoApiClientObjectMap | null { - const value = toLabel(serializer, targetData.purpose); + const value = toLabel(targetData.purpose); if (value == null) { return null; } else { @@ -1018,10 +1018,7 @@ export function toListenRequestLabels( } } -export function toLabel( - serializer: JsonProtoSerializer, - purpose: TargetPurpose -): string | null { +export function toLabel(purpose: TargetPurpose): string | null { switch (purpose) { case TargetPurpose.Listen: return null; diff --git a/packages/firestore/src/remote/watch_change.ts b/packages/firestore/src/remote/watch_change.ts index c08370d71e4..def3fa03299 100644 --- a/packages/firestore/src/remote/watch_change.ts +++ b/packages/firestore/src/remote/watch_change.ts @@ -285,7 +285,7 @@ export class WatchChangeAggregator { private pendingDocumentTargetMapping = documentTargetMap(); /** - * A list of targets with existence filter mismatches. These targets are + * A map of targets with existence filter mismatches. These targets are * known to be inconsistent and their listens needs to be re-established by * RemoteStore. */ @@ -430,15 +430,16 @@ export class WatchChangeAggregator { if (currentSize !== expectedCount) { // Apply bloom filter to identify and mark removed documents. const status = this.applyBloomFilter(watchChange, currentSize); + if (status !== BloomFilterApplicationStatus.Success) { // If bloom filter application fails, we reset the mapping and // trigger re-run of the query. this.resetTarget(targetId); + const purpose: TargetPurpose = status === BloomFilterApplicationStatus.FalsePositive ? TargetPurpose.ExistenceFilterMismatchBloom : TargetPurpose.ExistenceFilterMismatch; - this.pendingTargetResets = this.pendingTargetResets.insert( targetId, purpose @@ -469,10 +470,6 @@ export class WatchChangeAggregator { hashCount = 0 } = unchangedNames; - if (bitmap.length === 0) { - return BloomFilterApplicationStatus.Skipped; - } - let normalizedBitmap: Uint8Array; try { normalizedBitmap = normalizeByteString(bitmap).toUint8Array(); @@ -502,6 +499,10 @@ export class WatchChangeAggregator { return BloomFilterApplicationStatus.Skipped; } + if (bloomFilter.isEmpty()) { + return BloomFilterApplicationStatus.Skipped; + } + const removedDocumentCount = this.filterRemovedDocuments( watchChange.targetId, bloomFilter diff --git a/packages/firestore/test/unit/remote/remote_event.test.ts b/packages/firestore/test/unit/remote/remote_event.test.ts index 36646a5dba6..7003cfb7395 100644 --- a/packages/firestore/test/unit/remote/remote_event.test.ts +++ b/packages/firestore/test/unit/remote/remote_event.test.ts @@ -459,6 +459,9 @@ describe('RemoteEvent', () => { event = aggregator.createRemoteEvent(version(3)); expect(event.documentUpdates.size).to.equal(0); expect(event.targetMismatches.size).to.equal(1); + expect(event.targetMismatches.get(1)).to.equal( + TargetPurpose.ExistenceFilterMismatch + ); expect(event.targetChanges.size).to.equal(1); const expected = updateMapping( @@ -499,6 +502,9 @@ describe('RemoteEvent', () => { const event = aggregator.createRemoteEvent(version(3)); expect(event.documentUpdates.size).to.equal(1); expect(event.targetMismatches.size).to.equal(1); + expect(event.targetMismatches.get(1)).to.equal( + TargetPurpose.ExistenceFilterMismatch + ); expect(event.targetChanges.get(1)!.current).to.be.false; }); diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts index 3ac624efb6d..230297146ed 100644 --- a/packages/firestore/test/unit/specs/spec_test_runner.ts +++ b/packages/firestore/test/unit/specs/spec_test_runner.ts @@ -1104,7 +1104,7 @@ abstract class TestRunner { "Actual listen request doesn't have a 'goog-listen-tags'" ); expect(labels['goog-listen-tags']).to.equal( - toLabel(this.serializer, expected.targetPurpose) + toLabel(expected.targetPurpose) ); } From 160b74283d8137512c591a01317e031bbb7fdb78 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 9 Mar 2023 13:21:12 -0800 Subject: [PATCH 3/4] resolve comments --- packages/firestore/src/local/local_store_impl.ts | 2 +- packages/firestore/src/local/target_data.ts | 6 ++++-- packages/firestore/src/remote/bloom_filter.ts | 4 ---- packages/firestore/src/remote/remote_store.ts | 5 +---- packages/firestore/src/remote/watch_change.ts | 2 +- packages/firestore/test/unit/remote/remote_event.test.ts | 3 +++ 6 files changed, 10 insertions(+), 12 deletions(-) diff --git a/packages/firestore/src/local/local_store_impl.ts b/packages/firestore/src/local/local_store_impl.ts index c547d1e427d..e56ffc3af04 100644 --- a/packages/firestore/src/local/local_store_impl.ts +++ b/packages/firestore/src/local/local_store_impl.ts @@ -601,7 +601,7 @@ export function localStoreApplyRemoteEventToLocalCache( let newTargetData = oldTargetData.withSequenceNumber( txn.currentSequenceNumber ); - if (remoteEvent.targetMismatches.get(targetId) != null) { + if (remoteEvent.targetMismatches.get(targetId) !== null) { newTargetData = newTargetData .withResumeToken( ByteString.EMPTY_BYTE_STRING, diff --git a/packages/firestore/src/local/target_data.ts b/packages/firestore/src/local/target_data.ts index bc822a967d9..0128d1b2322 100644 --- a/packages/firestore/src/local/target_data.ts +++ b/packages/firestore/src/local/target_data.ts @@ -26,12 +26,14 @@ export const enum TargetPurpose { Listen, /** - * The query target was used to refill a query after an existence filter mismatch. + * The query target was used to refill a query after an existence filter + * mismatch. */ ExistenceFilterMismatch, /** - * The query target was used if the query is the result of a false positive in the bloom filter. + * The query target was used if the query is the result of a false positive in + * the bloom filter. */ ExistenceFilterMismatchBloom, diff --git a/packages/firestore/src/remote/bloom_filter.ts b/packages/firestore/src/remote/bloom_filter.ts index 8fff2d80db7..4c0e4ef6b7e 100644 --- a/packages/firestore/src/remote/bloom_filter.ts +++ b/packages/firestore/src/remote/bloom_filter.ts @@ -75,10 +75,6 @@ export class BloomFilter { this.bitCountInInteger = Integer.fromNumber(this.bitCount); } - isEmpty(): boolean { - return this.bitCount === 0; - } - // Calculate the ith hash value based on the hashed 64bit integers, // and calculate its corresponding bit index in the bitmap to be checked. private getBitIndex(num1: Integer, num2: Integer, hashIndex: number): number { diff --git a/packages/firestore/src/remote/remote_store.ts b/packages/firestore/src/remote/remote_store.ts index bd93a1bcb38..8cb361bfe09 100644 --- a/packages/firestore/src/remote/remote_store.ts +++ b/packages/firestore/src/remote/remote_store.ts @@ -611,10 +611,7 @@ function raiseWatchSnapshot( // Mark the target we send as being on behalf of an existence filter // mismatch, but don't actually retain that in listenTargets. This ensures // that we flag the first re-listen this way without impacting future - // listens of this target (that might happen e.g. on reconnect). The target - // purpose will be `ExistenceFilterMismatchBloom` if there is a bloom filter - // but it yield false positive result, otherwise, it will be set to - // `ExistenceFilterMismatch`. + // listens of this target (that might happen e.g. on reconnect). const requestTargetData = new TargetData( targetData.target, targetId, diff --git a/packages/firestore/src/remote/watch_change.ts b/packages/firestore/src/remote/watch_change.ts index def3fa03299..dd201c4ec1f 100644 --- a/packages/firestore/src/remote/watch_change.ts +++ b/packages/firestore/src/remote/watch_change.ts @@ -499,7 +499,7 @@ export class WatchChangeAggregator { return BloomFilterApplicationStatus.Skipped; } - if (bloomFilter.isEmpty()) { + if (bloomFilter.bitCount === 0) { return BloomFilterApplicationStatus.Skipped; } diff --git a/packages/firestore/test/unit/remote/remote_event.test.ts b/packages/firestore/test/unit/remote/remote_event.test.ts index 7003cfb7395..d58bbd3ea78 100644 --- a/packages/firestore/test/unit/remote/remote_event.test.ts +++ b/packages/firestore/test/unit/remote/remote_event.test.ts @@ -431,6 +431,9 @@ describe('RemoteEvent', () => { expectTargetChangeEquals(event.targetChanges.get(1)!, expected); }); + // TODO(Mila): Add test cases for existence filter with bloom filter, one will + // skip the re-query, one will yield false positive result and clears target + // mapping. b/272564458 it('existence filters clears target mapping', () => { const targets = listens(1, 2); From f90ab2ed82136d29110a485f3377adf9c8d55487 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 9 Mar 2023 15:12:17 -0800 Subject: [PATCH 4/4] resolve comments --- packages/firestore/test/unit/remote/remote_event.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/firestore/test/unit/remote/remote_event.test.ts b/packages/firestore/test/unit/remote/remote_event.test.ts index d58bbd3ea78..36596a8997c 100644 --- a/packages/firestore/test/unit/remote/remote_event.test.ts +++ b/packages/firestore/test/unit/remote/remote_event.test.ts @@ -431,9 +431,9 @@ describe('RemoteEvent', () => { expectTargetChangeEquals(event.targetChanges.get(1)!, expected); }); - // TODO(Mila): Add test cases for existence filter with bloom filter, one will - // skip the re-query, one will yield false positive result and clears target - // mapping. b/272564458 + // TODO(b/272564458): Add test cases for existence filter with bloom filter, + // one will skip the re-query, one will yield false positive result and clears + // target mapping. it('existence filters clears target mapping', () => { const targets = listens(1, 2);