Why are two types of return values used here? The meaning of the data they output seems to be the same(array member+score).

127.0.0.1:6379> hello 3
1# "server" => "redis"
2# "version" => "6.2.1"
3# "proto" => (integer) 3
4# "id" => (integer) 2266
5# "mode" => "standalone"
6# "role" => "master"
7# "modules" => (empty array)

127.0.0.1:6379> zadd set 1 one 2 two
(integer) 2

127.0.0.1:6379> zrange set 0 -1 withscores
1) 1) "one"
   2) (double) 1
2) 1) "two"
   2) (double) 2

127.0.0.1:6379> zpopmax set 2
1) "two"
2) (double) 2
3) "one"
4) (double) 1

Comment From: oranagra

@redis/core-team are you aware of any reason for that, or was it just overlooked in 6.0? note that we did fix the RESP3 response type of SRANDMEMBER in 6.2 (from set to a 2d array, in RC stage), maybe we can fix this in 7.0?

Comment From: itamarhaber

I believe it was simply overlooked. It should be made consistent, and since full RESP3 is still "beta-ish" we may be able to reason that it's valid to include it in the next patch/minor release.

Comment From: jhelbaum

Hi - I'm new to the project. Any objection to me taking this issue? It looks like a straightforward way to start getting involved.

Comment From: oranagra

@jhelbaum welcome. Go ahead..

Comment From: oranagra

I now realize that the blocking variant (e.g. BZPOPMAX), also returns the key name, so we have a choice between several odd behaviors: 1. have BZPOPMAX behave differently than ZPOPMAX (i.e. in RESP3, the blocking variant will keep returning a flat array, and the non-blocking variant a nested one) - this is what the PR currently implements. 2. have the the blocking variant return a nested array (and jagged), where the first entry is just the key name (single 2nd level record), and the rest of the entries have two records (similarly to the non-blocking). 3. keep the current behavior, in which both blocking and non-blocking variants return a flat array, which is inconsistent with ZRANGE.

I feel that making ZPOPMAX inconsistent with BZPOPMAX is more severe than having it inconsistent with ZRANGE. @redis/core-team i would like to hear your opinion.

Comment From: jhelbaum

I may well be missing something, but from what i can see the blocking variants only return a single value per call. In which case there's no use for a nested array. Just a triple of return values.

Comment From: jhelbaum

Any followup to this?

Comment From: oranagra

No response.. looking at it again myself. We do have plans for a COUNT option for BLPOP (#766), so i guess it applies for BZPOP too.

Another interesting consideration is that we recently added a COUNT option to LPOP (existed in ZPOP since day one). In LPOP, when it is used without COUNT, it responds with a simple string reply, and when used with COUNT it responds with an array. So considering that, BLPOP today responds with a key + element pair, so when BLPOP will be added a COUNT option, semantically it could / should respond with an array key + element pairs. Note that the important difference between these is not just the blocking attribute, but more importantly that the blocking variant takes multiple keys, while the non-blocking variant takes just one key. However despite the fact it takes multiple keys, it eventually operates on just one key, and if / when the COUNT argument will be added, that fact will remain (it'll still work on just one key). So although semantically it could return an array of key + element pairs, it would be more efficient to return the key-name just once, and an array of elements.

Now back to ZPOP / BZPOP, the difference between sorted-set and a list is that each element is accompanied by score, so where LPOP without COUNT was returning just one element, ZPOP without COUNT returns a pair (aka an array), and ZPOP with COUNT returns an array of pairs (in RESP2, that's a simple flat array too). So in RESP2, it appears as if the response type doesn't change when COUNT is added.

Keeping the blocking variant for later, ZPOP with RESP3 could still behave like LPOP with RESP3, i.e. when COUNT is not provided, return a single pair (no nested array), and when COUNT is provided return a nested array of pairs. (this is slightly different than the current PR, but that's what i now think we should do).

Then going back to the blocking variant, when we some day add the COUNT option, if we go with the semantic approach, it would be valid to return a flat list of triplets in RESP2, and a nested array of triplets in RESP3. Or we can go with the efficient approach, and return the key name just once. Either way, that dilemma will be true also for BLPOP, not just BZPOP, and either way, we don't have that dilemma at present.

I.e. by deciding that when COUNT is not provided we don't create a nested array, we are able to postpone the BZPOP problem for another day.

@redis/core-team please approve.

Comment From: jhelbaum

I updated the pull request with the changed behavior (no nested array unless COUNT is provided).

Comment From: yossigo

@oranagra I agree with the reasoning in the last comment and the current solution.