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.

Sign up for GitHub

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

Jump to bottom

Implement BloomFilter class #10846

Merged
merged 18 commits into from
Feb 28, 2023
Merged

Implement BloomFilter class #10846

merged 18 commits into from
Feb 28, 2023

Conversation

milaGGL
Copy link
Contributor

@milaGGL milaGGL commented Feb 18, 2023 •
edited
Loading

#no-changelog

  • Implement BloomFilter class.
  • Add basic positive/negative test cases for instantiating BloomFilter object.
@milaGGL milaGGL self-assigned this Feb 18, 2023
@milaGGL milaGGL marked this pull request as ready for review February 22, 2023 18:51
@milaGGL milaGGL requested a review from dconeybe February 22, 2023 18:51
dconeybe
dconeybe requested changes Feb 22, 2023
View reviewed changes
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.

I haven't completely finished my review but wanted to give the feedback I have so far. PTAL.
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/bloom_filter.h Outdated Show resolved Hide resolved
Firestore/core/src/remote/bloom_filter.h Outdated Show resolved Hide resolved
Firestore/core/src/remote/bloom_filter.h Outdated Show resolved Hide resolved
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.cc Outdated Show resolved Hide resolved
Firestore/core/src/remote/bloom_filter.cc Outdated Show resolved Hide resolved
Firestore/core/src/remote/bloom_filter.cc Outdated Show resolved Hide resolved
dconeybe
dconeybe requested changes Feb 23, 2023
View reviewed changes
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
Firestore/core/src/remote/bloom_filter.h Outdated Show resolved Hide resolved
Firestore/core/src/remote/bloom_filter.cc Show resolved Hide resolved
@milaGGL milaGGL requested a review from dconeybe February 23, 2023 17:49
dconeybe
dconeybe requested changes Feb 24, 2023
View reviewed changes
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.

Overall looks great! I've left a bunch of minor comments.
Firestore/core/src/remote/bloom_filter.h Outdated Show resolved Hide resolved
Firestore/core/src/remote/bloom_filter.h Outdated Show resolved Hide resolved
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.cc Outdated Show resolved Hide resolved
Firestore/core/test/unit/remote/bloom_filter_test.cc Outdated Show resolved Hide resolved
Firestore/core/test/unit/remote/bloom_filter_test.cc Outdated Show resolved Hide resolved
Firestore/core/test/unit/remote/bloom_filter_test.cc Outdated Show resolved Hide resolved
Firestore/core/src/remote/bloom_filter.cc Outdated Show resolved Hide resolved
Firestore/core/test/unit/remote/bloom_filter_test.cc Outdated Show resolved Hide resolved
@milaGGL milaGGL requested a review from dconeybe February 25, 2023 00:11
dconeybe
dconeybe requested changes Feb 25, 2023
View reviewed changes
Firestore/core/src/remote/bloom_filter.cc Show resolved Hide resolved
Firestore/core/src/remote/bloom_filter.cc Outdated Show resolved Hide resolved
Firestore/core/src/remote/bloom_filter.cc Show resolved Hide resolved
Firestore/core/test/unit/remote/bloom_filter_test.cc Outdated Show resolved Hide resolved
Firestore/core/test/unit/remote/bloom_filter_test.cc Outdated Show resolved Hide resolved
Firestore/core/test/unit/remote/bloom_filter_test.cc Outdated Show resolved Hide resolved
@milaGGL milaGGL requested a review from dconeybe February 27, 2023 18:22
@milaGGL milaGGL merged commit 024df8c into mila/BloomFilter Feb 28, 2023
@milaGGL milaGGL deleted the mila/BloomFilter-add-BloomFilter-cpp-file branch February 28, 2023 02:25
@firebase firebase locked and limited conversation to collaborators Mar 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api: firestore
3 participants
@milaGGL @dconeybe @google-oss-bot