-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Inconsistent BF.ADD
: modify the implementation of bloom filters to achieve similar result as redis
#1287
Comments
@JyotinderSingh @apoorvyadav1111 I am going to work on this. pls assign |
@JyotinderSingh @apoorvyadav1111 from above you can see in |
@shashi-sah2003 , yes please go ahead. Please consider that while we aim at being redis-compliant, finer values can differ based on the implementation of the data structure. That said, bloom filters do need an upgrade especially in expansion (currently its fixed). Thank you for working on this and lets stay connected on discord for this issue. |
hey @JyotinderSingh @apoorvyadav1111 reducing the number of filters will lead to increase in false positive rate which we dont want right? but I wonder why redis uses only one filter? |
We would need to investigate why they went with it. I'm not aware of the reasoning for that. |
@JyotinderSingh @apoorvyadav1111 I have done some more research from my side and found out that |
In that case we can adjust our filters accordingly |
Then how should I go about this? adjust the capacity and filters accordinly as redis? |
We don't need to match redis output in this case. |
yeah also in the codebase, the optimized formula is being used to calculate number of filters which is calculated using |
That could be a separate discussion, it would depend on a lot of factors which are unclear for now. |
Steps to reproduce
Expected output
The expected output when the above set of commands when run on Redis
Observed output
The observed output when the above set of commands when run on DiceDB
The steps to run the test cases are mentioned in the README of the dice-tests repository.
Expectations for resolution
This issue will be considered resolved when the following things are done
dice
code to meet the expected behaviorYou can find the tests under the
tests
directory of thedice
repository and the steps to run are in the README file. Refer to the following links to set up DiceDB and Redis 7.2.5 locallyThe text was updated successfully, but these errors were encountered: