Add support of the Hyperloglog data structure

[tutububug]

storage format description: https://github.com/apache/kvrocks-website/pull/207/files.

[mapleFU]

I'll take a careful review this week

[torwig]

@tutububug Thank you for your contribution. Running ./x.py format should eliminate the linting issues.

[Yangsx-1]

Thank you for your contribution! Maybe you need to add a command parser to use the data structure with actual command like redis.

[PragmaTwice]

Thank you for your contribution!

Could you include your design in the PR description? For example, explain how to encode the metadata and HLL data (subkeys), similar to what is shown on https://kvrocks.apache.org/community/data-structure-on-rocksdb.

[tutububug]

Thank you for your contribution! Maybe you need to add a command parser to use the data structure with actual command like redis.

Yes, I will give the commit later.

Thank you for your contribution!

Could you include your design in the PR description? For example, explain how to encode the metadata and HLL data (subkeys), similar to what is shown on https://kvrocks.apache.org/community/data-structure-on-rocksdb.

OK.

[tutububug]

@PragmaTwice I create a PR(apache/kvrocks-website#207) for describe hyperloglog storage format.

[PragmaTwice]

@PragmaTwice I create a PR(apache/kvrocks-website#207) for describe hyperloglog storage format.

Thank you!
Currently, it is only necessary to include your design in the PR description, not on the website as the design is not finalized yet.

Regarding your design, I have some questions:

1. Based on the current design, typically one Redis key will introduce a maximum of 16384 RocksDB keys (registers). Each value corresponding to a RocksDB key contains only one integer. This may be inefficient; merging multiple registers onto one key could reduce the number of keys introduced. WDYT?
2. I noticed that integers are stored as string representations (std::to_string) rather than their binary form (e.g., subkey and register value). What is the reason for this approach?
3. Having a constant size seems illogical since the number of subkeys linked to this Redis key varies. (However, if every write operation modifies the metadata, it may lead to a decrease in performance. I don't have a clear idea about this aspect.)

Concerning the code, although I haven't reviewed it thoroughly yet, there are some points worth mentioning:

1. The complete source code of MurmurHash can be placed in the vendor directory.
2. It appears that using PFADD in the code leads to an increasing number of RocksDB keys (registers). There seems to be no operation that reduces these keys until deleting this Redis key. How can we prevent an increase in RocksDB keys without a mechanism to decrease them?

cc @git-hulk @mapleFU

[git-hulk]

@tutububug As @PragmaTwice mentioned in #2142 (comment), it's unnecessary to use a static number of 16384 since it may heavily affect the read performance while using PFMERGE, I guess a smaller one like 16 is enough and every subkey has 1000 integers.

[PragmaTwice]

I suggest that we can store the number of registers in one rocksdb key in the metadata (for example, referred to as register_number_in_one_key), so that even if adjustments are made later for performance reasons, the compatibility of kvrocks data can be maintained.

Currently, a fixed value for register_number_in_one_key can be hardcoded in the code.

cc @tutububug

[mapleFU]

Based on the current design, typically one Redis key will introduce a maximum of 16384 RocksDB keys (registers). Each value corresponding to a RocksDB key contains only one integer. This may be inefficient; merging multiple registers onto one key could reduce the number of keys introduced. WDYT?

There're two parts of things.

1. Bitmap type can be used to do the dense logic, which would reducing the keycount
2. sparse hll type can be introduced to save some overhead here.

[PragmaTwice]

Based on the current design, typically one Redis key will introduce a maximum of 16384 RocksDB keys (registers). Each value corresponding to a RocksDB key contains only one integer. This may be inefficient; merging multiple registers onto one key could reduce the number of keys introduced. WDYT?

There're two parts of things.

1. Bitmap type can be used to do the dense logic, which would reducing the keycount
2. sparse hll type can be introduced to save some overhead here.

So I think the question is, should we also introduce two mode of hll encoding (sparse and dense layout) and an auto switching policy between these two layout?

[mapleFU]

So I think the question is, should we also introduce two mode of hll encoding (sparse and dense layout) and an auto switching policy between these two layout?

I prefer to do this :-) But we can regard it as a further optimization

[mapleFU]

can you change it to a function?

[mapleFU]

Other Looks ok to me

[tutububug]

Regarding your design, I have some questions:

1. Based on the current design, typically one Redis key will introduce a maximum of 16384 RocksDB keys (registers). Each value corresponding to a RocksDB key contains only one integer. This may be inefficient; merging multiple registers onto one key could reduce the number of keys introduced. WDYT?

Not really. The register(subkey) only be stored which its count is not zero. This point is different from the memory implementation with static array as dense encode. On disk, I think its sparse encode naturally.

2. I noticed that integers are stored as string representations (`std::to_string`) rather than their binary form (e.g., subkey and register value). What is the reason for this approach?

The number of consecutive 0s is calculated from the last 50 digits of the hash value, so the maximum value is 50, and the maximum value stored in a string is 2 bytes. It should not waste a lot of space, and at the same time save the overhead of integer encoding and decoding. For keys, it may be more efficient, but the largest index is only 5 bytes (16383).

3. Having a constant `size` seems illogical since the number of subkeys linked to this Redis key varies. (However, if every write operation modifies the metadata, it may lead to a decrease in performance. I don't have a clear idea about this aspect.)

Currently, the ‘size’ variable has no practical purpose; the only requirement is that it be non-zero. Due to the implementation of kvrocks getmetadata, non-string type data structures with a size of 0 are judged to be expired, and the HLL add parameter that redis has implemented allows no parameters but the key will be stored. For compatibility, size is used as a constant to prevent expiration.

Concerning the code, although I haven't reviewed it thoroughly yet, there are some points worth mentioning:

1. The complete source code of MurmurHash can be placed in the `vendor` directory.

OK.

2. It appears that using `PFADD` in the code leads to an increasing number of RocksDB keys (registers). There seems to be no operation that reduces these keys until deleting this Redis key. How can we prevent an increase in RocksDB keys without a mechanism to decrease them?

For an HLL user key, the maximum register value is 16384, and it cannot be larger. In fact, I think this should be considered controllable compared to data structures such as hash, set, list, etc. whose size is determined by user input.

@PragmaTwice cc @git-hulk @mapleFU

[mapleFU]

The number of consecutive 0s is calculated from the last 50 digits of the hash value, so the maximum value is 50, and the maximum value stored in a string is 2 bytes. It should not waste a lot of space, and at the same time save the overhead of integer encoding and decoding. For keys, it may be more efficient, but the largest index is only 5 bytes (16383).

For rocksdb value, it's should be 1-2 bytes payload, the value size is also included. So, it introduce an extremly huge overhead. So I prefer the impl of bitmap/bitfield.

Get 2^14 times in rocksdb would also be heavy, and might break some statistics in rocksdb, which making it tent to compaction more or caching the unneccessary blocks.

[sonarcloud]

Quality Gate failed

Failed conditions
1 Security Hotspot

See analysis details on SonarCloud

[mapleFU]

This looks much better than previous version. The remaining are mostly code-style problem

[mapleFU]

Nit: you can add a todo, and optimizing the return value for getRegisters

[tutububug]

Do you mean the output argument 'registers' needs to be modified?

[tutububug]

This looks much better than previous version. The remaining are mostly code-style problem

fixed.

[PragmaTwice]

There is a lot of strange type casting in the code.

It is advisable to minimize the use of type casting, particularly const_cast and reinterpret_cast. It is recommended to consider modifying the function signature instead of relying solely on casts.

[tutububug]

There is a lot of strange type casting in the code.

It is advisable to minimize the use of type casting, particularly const_cast and reinterpret_cast. It is recommended to consider modifying the function signature instead of relying solely on casts.

fixed.

[tutububug]

Had fixed.

[mapleFU]

I just back from holidays and catch a cold. I'll take a carefully around this week. Would you mind me edit directly on this patch @tutububug

[tutububug]

I just back from holidays and catch a cold. I'll take a carefully around this week. Would you mind me edit directly on this patch @tutububug

OK, feel free to modify if needed. And also the description at https://github.com/apache/kvrocks-website/pull/207/files.