Skip to content
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

Add 'existence-filter-mismatch-bloom' to listen request labels and test with spec tests #7107

Merged
merged 5 commits into from
Mar 9, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 1 addition & 1 deletion packages/firestore/src/local/local_store_impl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
6 changes: 4 additions & 2 deletions packages/firestore/src/local/target_data.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,

Expand Down
4 changes: 0 additions & 4 deletions packages/firestore/src/remote/bloom_filter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
5 changes: 1 addition & 4 deletions packages/firestore/src/remote/remote_store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/src/remote/watch_change.ts
Original file line number Diff line number Diff line change
Expand Up @@ -499,7 +499,7 @@ export class WatchChangeAggregator {
return BloomFilterApplicationStatus.Skipped;
}

if (bloomFilter.isEmpty()) {
if (bloomFilter.bitCount === 0) {
return BloomFilterApplicationStatus.Skipped;
}

Expand Down
3 changes: 3 additions & 0 deletions packages/firestore/test/unit/remote/remote_event.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Put b/272564458 in the "TODO" rather than "Mila".

// 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);

Expand Down