Merged
Conversation
- The test filter's max_elements parameter is set to twice the number of elements that are being tested as false positives. This doesn't test the functionality of the filter, since max_elements should be related to the number of elements that we expect to put into the filter. - The tests currently aren't aggressive enough wrt the magnitude of false positives being checked, added a test for checking a million false positives that fails given expectations.
- The previous linear combination hashing algorithm was flawed because the Hamming weight of the entire probe is guaranteed to be equal to min(num_bits_n / gcd(num_bits_n, hash2), num_probes_k) since we're treating hash2 as a generator of the group num_bits_n/hash2. Since the order of that group is the above, that means that the Hamming weight of the probe may be significantly smaller than num_probes_k, especially if hash2 is a multiple of num_bits_n (which will happen 1 out of num_bits_n times). A low Hamming weight probe is much more likely to be a positive, and therefore also much more likely to be a false positive.
|
@hiway Any chance of merging this... This bug bites lots of users in ways that are painful... |
remram44
added a commit
to remram44/python-bloom-filter
that referenced
this pull request
May 5, 2021
Fix hash probes See pull request hiway#5
remram44
added a commit
to remram44/python-bloom-filter
that referenced
this pull request
May 5, 2021
Fix hash probes See pull request hiway#5
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I changed the probe logic to make collisions less likely. Using a linear combination is susceptible to false positives because if hash2 has a high GCD with bloom_filter.num_bits_m, then it will not produce very many different hash values (it will produce precisely min(num_probes_k, num_bits_m/GCD(num_bits_m, hash2)), meaning that there are fewer opportunities for it to produce a hash that's not in the filter.
Ideally this should use python's random() functionality, so long as the random generator uses all the entropy of the seeds, but it looks like it uses only an int's worth: https://docs.python.org/3/library/random.html#random.seed