Add PFADD, PFCOUNT, PFMERGE commands

[ilbaktin]

Hi there! We're using your awesome miniredis library for redis-related unit tests in the company I'm working at. Once we started using HLL-related commands of redis, we also definitely wanted to cover the corresponding logic with unit tests but unfortunately found out that HLL-related commands are not supported in miniredis.

I'm adding HLL-related commands to miniredis with this PR using one quite popular implementation of HLL in golang https://github.com/axiomhq/hyperloglog but not reimplementing the actual C++ implementation of HLL https://download.redis.io/redis-stable/src/hyperloglog.c. Why I did this choice - in fact the miniredis is mainly used for testing purposes and hence it's not necessary to have the 100% compatibility of all the internal details, it's enough to have the compatibility of the protocol and the functionality. Basically the implementation of HLL used in this PR behaves almost the same as redis' variant of HLL. One potential difference I could think of is that the set cardinality estimation accuracy could slightly differ on big sets (containing over 1M of elements), but anyway I'm not sure that somebody plans to have sets of over 1M cardinality in unit tests. Please let me know what you think about it.

Also one arguable question - not sure if it's OK for you that I'm adding a new dependency. I could have added the implementation of HLL explicitly as it's been done for geohash library.

Thanks, please let's discuss all the things. If you have any concern about what might be improved, please let me know.

[alicebob]

On Sun, May 30, 2021 at 03:25:22PM -0700, Konstantin Ilbaktin wrote: Hi there! We're using your awesome miniredis library for redis-related unit tests in the company I'm working at. Once we started using HLL-related commands of redis, we also definitely wanted to cover the corresponding logic with unit tests but unfortunately found out that HLL-related commands are not supported in miniredis.

Hi! Thanks for the PR! I'll have a proper look in a few days.

[alicebob]

Thanks, I had a look and the code looks nice and simple. I never used the PF* redis commands, but they are one of the few which do not have a ton of options and arguments and special cases :)

slightly differ on big sets

That's absolutely totally fine :)

About the included hyperloglog library: it's MIT licensed, so how about we add the relevant files to a subdir in miniredis? (hyperloglog.go, registers.go, LICENSE ?). It's straightforward code which I don't expect to change a lot, and saves a dependency.

I assume we can't avoid the murmur dependency. What would happen if we use another hashing function?

Can you add some tests to the ./integration/ subdir? If the numbers don't match up 100% you can use c.DoLoosly() which still compares structures (or maybe c.DoRounded()). I'm happy to help there if you want.

[ilbaktin]

Thanks for looking into the PR!

No problem, I can add relevant files from hyperloglog lib to miniredis subdir.

Regarding a hash function - before using Redis HLL commands we tried using some golang HLL lib (several options) and store these HLLs in a database. Trying golang HLL libs I found out that the most of them have better accuracy if I use murmur as a hash function. But again in tests we don't need better accuracy (and moreover HLL lib that I used in this PR has a default hash func - metro hash which is still not bad) so we can get rid of murmur I think.

And regarding integration tests - no problem, I will take a look and try to handle.

Will handle these changes when I have a chance, presumably on the weekend.

[alicebob]

Will handle these changes when I have a chance, presumably on the weekend.

Thanks! The comment in the readme about Hyperloglog is from 2015, so there doesn't seem to be much urgency ;)

[ilbaktin]

Update: metro lib is a dependency of hyperloglog lib, but metro lib is under MIT license, so we probably can take the necessary hash function right from metro lib to miniredis.

[ilbaktin]

@alicebob Hi again!

Added a few changes as discussed before:

1. Got rid of hyperloglog and murmur dependencies. Instead I added hyperloglog's and metro's hash source code directly into miniredis repo since they're under MIT license.
2. Added integration tests for HLL related commands (and found a small bug in response message meanwhile). I handled unstable approximate nature of HLLs in integration tests with a new function DoApprox() comparing ints with a threshold.

Again, let me know what you think!

[alicebob]

That's a bit outdated :)

[alicebob]

Looks great! I'll have a more proper look in a few days, but I don't expect any issues. Really nice it worked out without adding dependencies.

[alicebob]

Thanks for the very easy PR :)

I merged it to master, will make it a proper release in a few days or so.