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

feat(server): Add support for PFADD and PFCOUNT #1152

Merged
merged 8 commits into from Apr 29, 2023

Conversation

chakaz
Copy link
Collaborator

@chakaz chakaz commented Apr 27, 2023

This version does not create sparse-encoded HLLs, however it is fully compatible with such ones created by Redis as it converts them to the dense encoding.

#925

This version does not create sparse-encoded HLLs, however it is fully compatible with such ones created by Redis as it converts them to the dense encoding.

Note that PFMERGE is not yet implemented.

Signed-off-by: chakaz <chakaz@chakaz>
Copy link
Collaborator

@romange romange left a comment

Choose a reason for hiding this comment

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

Wow, super cool!

src/server/hll_family.cc Outdated Show resolved Hide resolved
src/server/hll_family.cc Outdated Show resolved Hide resolved
src/server/hll_family.cc Outdated Show resolved Hide resolved
src/server/hll_family.cc Outdated Show resolved Hide resolved
@chakaz
Copy link
Collaborator Author

chakaz commented Apr 28, 2023

It should be enough to check just after inserts, as it doesn't mutate between reads

It's true that all HLLs created by Dragonfly should always be in the dense format, but in order to be compatible with Redis (like if Redis is the master) we need to support reading strings not crafted by Dragonfly which may use sparse encoding.
Or did I miss your intention? Sorry if so!

@romange
Copy link
Collaborator

romange commented Apr 28, 2023

You are right that it can happen during the rdb load or using the replication from Redis master.

  1. Please add a comment about the case
  2. Please add a test to rdb_test as well. This is how I did it. I run redis 6.2, added a couple of items , in this case pf counts, and then shutdown the server which produces dump.rdb. Then I rename it, move it to testdata and use it in rdb_test to show compatibility. We want both dense and sparse items be present in the rdb, I guess.

@chakaz
Copy link
Collaborator Author

chakaz commented Apr 28, 2023

I'm happy to add the rdb_test, but just making sure before: have you looked at the WorkWithSparseEncoding test that I added? I took that string from Redis, and so I'm not sure what additional coverage will rdb_test add?
I guess I could also add a Redis-created dense-encoding string, would that do?

Signed-off-by: chakaz <chakaz@chakaz>
@romange
Copy link
Collaborator

romange commented Apr 28, 2023

I guess rdb_test is not needed then. Yes, adding a dense string would suffice, thanks!

@romange romange requested a review from dranikpg April 28, 2023 08:51
Signed-off-by: chakaz <chakaz@chakaz>
@chakaz
Copy link
Collaborator Author

chakaz commented Apr 28, 2023

Great, done!

dranikpg
dranikpg previously approved these changes Apr 28, 2023
src/server/hll_family.cc Outdated Show resolved Hide resolved
Signed-off-by: chakaz <chakaz@chakaz>
@chakaz
Copy link
Collaborator Author

chakaz commented Apr 28, 2023

I switched to use rdb_test indeed, as explicitly writing the dense-encoded string was just too long (took > 1000 lines!).
In addition, I switched to use GetSlice() which should save a bunch of allocations, so nice optimization!

PTAL.

romange
romange previously approved these changes Apr 29, 2023
Copy link
Collaborator

@romange romange left a comment

Choose a reason for hiding this comment

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

Looks great!

src/server/hll_family.cc Show resolved Hide resolved
This will allow dense-encoded HLL to *not* fit within the small string,
which will make it contiguous in memory, thus GetSlice() will not
allocate.

Signed-off-by: chakaz <chakaz@chakaz>
@romange romange merged commit fa39c18 into dragonflydb:main Apr 29, 2023
6 checks passed
@chakaz chakaz mentioned this pull request Apr 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants