dragonfly icon indicating copy to clipboard operation
dragonfly copied to clipboard

Published 20 hours ago • verified publisher icon dragonflydb

Add support for Sparse HLL PFADD

Open azuredream opened this issue 11 months ago • 2 comments

#2679

azuredream avatar Mar 22 '24 01:03 azuredream

It's working as expected now. Please let me know if you have any advice of the implementation. Not sure if I can use sds in .cc, maybe there is better practice. Todo:

  1. Add more test cases.
  2. Code cleanup.
  3. make HLL_SPARSE_MAX_BYTES configurable
  4. memory usage test

azuredream avatar Mar 22 '24 02:03 azuredream

@azuredream thank you, again, for your contribution!

chakaz avatar Mar 24 '24 12:03 chakaz

Sorry for the delay. I think the code is ready to be reviewed.

We also need to verify that the new change addresses the memory issue in #2679. Please let me know if you have any advice.

azuredream avatar Apr 04 '24 14:04 azuredream

Updated.

  1. Please let me know if want to keep sds operations in spares path only. We'll need to update string obj after each loop.
  2. I think there is not a double free problem. Please correct me if I was wrong.
  3. Added a random insertion test case as you suggested.
  4. Do we need to check line by line of the code for the license concern? I'm sure there is not any identical function in this PR.

azuredream avatar Apr 08 '24 02:04 azuredream

  1. Please let me know if want to keep sds operations in spares path only. We'll need to update string obj after each loop.

I'm not sure what you mean? It looks like you only allocate in the sparse path now, which is great.

  1. I think there is not a double free problem. Please correct me if I was wrong.

You are right, I was wrong :)

  1. Added a random insertion test case as you suggested.

Thanks!

  1. Do we need to check line by line of the code for the license concern? I'm sure there is not any identical function in this PR.

No need to check line by line, I just want to verify that:

  1. The code that you copied from Redis is from a commit before they changed license. Redis recently changed their source code license (March 21st), so if you copied functions from before that date, we're good. If the code was copied after, then we need to make sure it's identical to the one before (or just copy from before, which is the same).
  2. The code you created is fine, no issues there

chakaz avatar Apr 08 '24 06:04 chakaz

Updated.

  1. Please see my reply to your comment at line 94.
  2. The last commit of Hyperloglog.c in my Redis repo is on 11.26.2023. I think it's OK?

azuredream avatar Apr 10 '24 04:04 azuredream

  1. Please see my reply to your comment at line 94.

I don't see a comment there, but your code is robust. I proposed last final edit (I promise this is the last one! :smile:), and we can then merge this PR

  1. The last commit of Hyperloglog.c in my Redis repo is on 11.26.2023. I think it's OK?

Yes, that should be good, thanks for verifying!

chakaz avatar Apr 10 '24 05:04 chakaz

Thanks again @azuredream !

romange avatar Apr 11 '24 08:04 romange