feat (hset): Support arguments (count, withvalues) in HRANDFIELD

[theyueli]

Fixes: #858

It might possibly fix: #1707

The algorithms that support both encodings (string map and listpack) have been implemented and tested. To use string map requires a larger hset (my tests used 500+ entries)

The random selection algorithms implemented for string map class are the reimplementation of the same algorithms used by Redis' listpack. (therefore same time complexities)

Without this patch:

127.0.0.1:6379> HSET coin heads obverse tails reverse edge null
(integer) 3
127.0.0.1:6379> HRANDFIELD coin 3 WITHVALUES
(error) ERR wrong number of arguments for 'hrandfield' command
127.0.0.1:6379> HRANDFIELD coin -1
(error) ERR wrong number of arguments for 'hrandfield' command
127.0.0.1:6379> 

after this patch:

127.0.0.1:6379> HSET coin heads obverse tails reverse edge null
(integer) 3
127.0.0.1:6379> HRANDFIELD coin -5 WITHVALUES
 1) "tails"
 2) "reverse"
 3) "tails"
 4) "reverse"
 5) "heads"
 6) "obverse"
 7) "tails"
 8) "reverse"
 9) "edge"
10) "null"
127.0.0.1:6379> HRANDFIELD coin 3
1) "heads"
2) "tails"
3) "edge"
127.0.0.1:6379> HRANDFIELD coin 3 WITHVALUES
1) "heads"
2) "obverse"
3) "tails"
4) "reverse"
5) "edge"
6) "null"

[royjacobson]

redis claim complexity of O(N) with N being the amount of items returned. AFAICT RandomPairsUnique and RandomPairs in this PR are O(M) with M=size(map). Am I missing something?

[dranikpg]

@royjacobson

I might be wrong, but it seems like listpacks implementation is also O(M) 😮 They use the same algorithms as here

Actually we can make our implementation O(N) by harnessing stringmaps internals. But I don't think its mandatory as I already answered in the first PR on this issue

[kostasrim]

Good work 👨‍🍳 , some minor comments

[kostasrim]

you can use a std::map<uint32_t, uint32_t>. The key represents the index, and value represents the number of times you encountered it (we need map, since it's an ordered container). Each time you loop, you check if the index rand() % total_size already exists. If it does you increment it's occurrence (the value) by 1. Otherwise (if the index is not already inserted) you insert it with an associated value of 1 (since it's the first occurrence).

That way you will:

1. Get rid of std::sort.
2. Simplify the code below, since now you don't need two while loops. You only need a for loop, and for each element, you output occurrence number of times

[dranikpg]

Good suggestion, but I don't think it matters that much. Sort only gets slow for a really big number of elements and for it the i/o time is much larger

[kostasrim]

No no, I do not care about the performance here, I don't expect that std::sort will have any measurable impact since it the count will always be relatively small. But by using a map it will simplify the implemetnaion of this function and make it more readable

[theyueli]

I prefer to keep as is. sort() is more implicit when std::map is used. Personally, the current version has better readabliy by making sort obvious.

[kostasrim]

So a) I think stl associative ordered containers are used exactly for that, so I would argue that vector + sort is an antipattern. b) how is what you wrote simpler than this 3 lines? No pair accesses, no nested while loops no nothing:

for(auto it = begin(); it < end() && picks_sz < count ; ++it) {
  auto [key, frequency] = *it;
  keys.insert(keys.end(), frequency, key);
  picks_sz += frequency;
}

I might a have a mistake on the above code but that's the gist of it

[theyueli]

@dranikpg @kostasrim addressed all the comments, please take another look, thanks!

[dranikpg]

1. But I still encourage you to use a unique_ptr instead of raw arrays
2. about RandomPairs(), we usually take output arguments by pointer (and returning values is even better) but I don't wanna be picky

[theyueli]

1. But I still encourage you to use a unique_ptr instead of raw arrays

thanks @dranikpg, I just changed to use unique_ptr, please take a second look.

[dranikpg]

👍🏻