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 unchanged names to existence filter and watchFilters spec test runner #10862

Merged
merged 18 commits into from
Mar 18, 2023

Conversation

milaGGL
Copy link
Contributor

@milaGGL milaGGL commented Feb 24, 2023

#no-changelog

Ported from Web SDK: firebase/firebase-js-sdk#6839

  • mark unchangedNames and bits optional in proto
  • Add bloom_filter to ExistenceFilter class
  • Update serializer to pass unchangedNames to ExistenceFilter
  • update the Spec test runner doWatchFilter to accept NSDictionary instead of NSArray
    • In web SDK spec test runner doWatchFilter(watchFilter: SpecWatchFilter), the specWatchFilter input type changed from an array to object:
      Screenshot 2023-02-23 at 5 46 34 PM
  • Ported the spec tests that are changed due to the doWatchFilter change above(no bloom filter proto json values are included)
@milaGGL milaGGL self-assigned this Feb 24, 2023
@milaGGL milaGGL requested a review from dconeybe February 24, 2023 01:49
Copy link
Contributor

@dconeybe dconeybe left a comment

Choose a reason for hiding this comment

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

We need to fix the proto passing and memory management issue in this PR. I haven't reviewed all of this PR but wanted to send the feedback now so you can have a chance to review prior to our meeting in an hour. The logic you want to achieve in this PR is a bit more difficult than it had appeared on the surface!

Firestore/core/src/remote/existence_filter.h Outdated Show resolved Hide resolved
Firestore/core/src/remote/existence_filter.h Outdated Show resolved Hide resolved
Firestore/core/src/remote/existence_filter.h Outdated Show resolved Hide resolved
@dconeybe dconeybe self-requested a review March 11, 2023 05:17
Firestore/core/src/remote/bloom_filter.cc Show resolved Hide resolved
Firestore/core/src/remote/bloom_filter.h Outdated Show resolved Hide resolved
Firestore/core/src/remote/bloom_filter.h Show resolved Hide resolved
Firestore/core/src/remote/serializer.cc Show resolved Hide resolved
Firestore/core/src/remote/serializer.cc Outdated Show resolved Hide resolved
Firestore/Example/Tests/SpecTests/FSTSpecTests.mm Outdated Show resolved Hide resolved
Firestore/Example/Tests/SpecTests/FSTSpecTests.mm Outdated Show resolved Hide resolved
Firestore/core/test/unit/remote/bloom_filter_test.cc Outdated Show resolved Hide resolved
Firestore/core/test/unit/remote/remote_event_test.cc Outdated Show resolved Hide resolved
@milaGGL milaGGL requested a review from dconeybe March 16, 2023 22:42
Firestore/core/src/remote/bloom_filter.cc Outdated Show resolved Hide resolved
Firestore/core/src/remote/bloom_filter.cc Outdated Show resolved Hide resolved
Firestore/core/src/remote/bloom_filter.h Outdated Show resolved Hide resolved
@milaGGL milaGGL requested a review from dconeybe March 17, 2023 19:36
Copy link
Contributor

@dconeybe dconeybe left a comment

Choose a reason for hiding this comment

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

Just one tiny comment!

Firestore/core/src/remote/bloom_filter.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@dconeybe dconeybe left a comment

Choose a reason for hiding this comment

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

Ok, a few more comments!

Firestore/Example/Tests/SpecTests/FSTSpecTests.mm Outdated Show resolved Hide resolved
Firestore/core/src/remote/serializer.cc Outdated Show resolved Hide resolved
Firestore/core/src/remote/serializer.cc Outdated Show resolved Hide resolved
@milaGGL milaGGL requested a review from dconeybe March 17, 2023 21:18
@milaGGL milaGGL merged commit 9dc9f3e into mila/BloomFilter Mar 18, 2023
@milaGGL milaGGL deleted the mila/BloomFilter-add--existence-filter-class branch March 18, 2023 08:50
@firebase firebase locked and limited conversation to collaborators Apr 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
3 participants