Add noscores option to command ZSCAN.

[CharlesChen888]

Command syntax is now:

ZSCAN key cursor [MATCH pattern] [COUNT count] [NOSCORES]

Return format:

127.0.0.1:6379> zadd z 1 a 2 b 3 c
(integer) 3
127.0.0.1:6379> zscan z 0
1) "0"
2) 1) "a"
   2) "1"
   3) "b"
   4) "2"
   5) "c"
   6) "3"
127.0.0.1:6379> zscan z 0 noscores
1) "0"
2) 1) "a"
   2) "b"
   3) "c"

when NOSCORES is on, the command will only return members in the zset, without scores.

For client side parsing the command return, I believe it is fine as long as the command is backwards compatible. The return structure are still lists, what has changed is the content. And clients can tell the difference by the subcommand they use.

Since novalues option of HSCAN is already accepted (redis/redis#12765), I think similar thing can be done to ZSCAN.

[zuiderkwast]

I suggested this command. Nobody asked for it, but it seems like a logical thing to add together with HSCAN NOVALUES.

@valkey-io/core-team Let's have a vote on this new argument. Give your :+1: or :-1: please.

[madolson]

I was pretty strongly against this command because I don't want to add commands that aren't solving a problem. HSCAN without values made more sense, because values can be large, but here the values are just decimals which can never be that expensive.

[CharlesChen888]

Whether values are expensive to return to client is a relative thing. If the sds length of scores and members are in the same order of magnitude (in which case, names of members are short), we can certainly save considerable traffic by using noscores. Sometimes in situations with heavy network load, we try to save as much as we can.

[enjoy-binbin]

I see it was rejected in the previous redis core-team. i agree with madolson that we should not add commands that aren't solving a problem. But on the other hands, although it may not solve many problems, it seem it does will slove some problems (like CharlesChen888's case). Another thing is that it is symmetrical will HSCAN novalues. (i am a guy prefer symmetrical)

[soloestoy]

IMHO, ZSCAN and HSCAN have no much differences, and I prefer keeping symmetry : )

[zuiderkwast]

@CharlesChen888 please don't vote when it's a core team vote (AKA technical steering committee). We're trying to be transparent and do voting in the open.

Feel free to do other emojis and to thumbs up or down on other comments.

[madolson]

it seem it does will slove some problems (like CharlesChen888's case).

I would just like to clarify this isn't solving any known problem, it's a solution (reducing usage) to an imagined problem. I was asking for a use case where you would want to incrementally scan a semi-large sorted set and only care about the field names (and not care about order I suppose). Ideally we would be able to articulate "why" a user would want to use this command.

Another reason the old team was against is that this can already be solved by ZRANGE (which by default doesn't include scores). A valid counter point here is that ZSCAN is more efficient, since we are skipping the O(Log(N)) seek at the start of each ZRANGE, but we haven't seen folks materially complain about that.

IMHO, ZSCAN and HSCAN have no much differences, and I prefer keeping symmetry : )

I still don't really think symmetry is a needed or useful property for Valkey. We can make the same argument about a lot of other commands, like why doesn't INCR support XX (Increment only if the key is there). I don't want this to get bogged down into whether that is useful, because the suggestion was that it was important to have symmetry. Ideally we would come up with some tenets to make these types of decisions easier.

I don't specifically care about this command that strongly, I just don't want this to be the start of adding a bunch of more commands for "symmetry".

[soloestoy]

Perhaps "symmetry" wasn't the most precise term, using the "same type" might be a more fitting description.

The topic of whether user demand should precede the implementation of commands, or vice versa, is quite interesting. In this case, I lean towards implementing the command first. My intuition tells me that upon seeing hscan novalue, users of zscan or zrange would naturally think of zscan noscore.

[PingXie]

What is the end to end use case here? This proposal seems to suggest that there is a need to retrieve just the members of the sorted set, unordered. What would an application do with this information?

As far as general guidelines are concerned, I agree that we should not get ahead of ourselves too much and "predict" use cases, especially in the user facing interface areas. It is a slippery slope.

[CharlesChen888]

What is the end to end use case here? This proposal seems to suggest that there is a need to retrieve just the members of the sorted set, unordered. What would an application do with this information?

Perhaps sending messages to all players on a ranking list (which is one of the most common use cases for zset), and the list is too long for a single ZRANGE.

[madolson]

Perhaps sending messages to all players on a ranking list (which is one of the most common use cases for zset), and the list is too long for a single ZRANGE.

You can incrementally scan through a sorted set with ZRANGE though. It has different properties of the SCAN though, since it wouldn't guarantee at least once delivery I suppose, since the scores could change.

[hwware]

I have no objecttion to add this option. Becuase according to the SCAN, SSCAN, HSCAN and ZSCAN doc (https://redis.io/docs/latest/commands/scan/) , the return value is a two element multi-bulk reply, where the first element is a string representing an unsigned 64 bit number (the cursor), and the second element is a multi-bulk with an array of elements.

For the ZSCAN, if option NOSCORES is added, only the member value as an array is returned, without their scores. It could make the client codes much easiter.

[madolson]

@hwware and @PingXie Your two votes are missing btw. Do you want to be explicit and add it to viktors comment.

[PingXie]

@hwware and @PingXie Your two votes are missing btw. Do you want to be explicit and add it to viktors comment

Voted.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 87.50000% with 2 lines in your changes are missing coverage. Please review.

:exclamation: No coverage uploaded for pull request base (unstable@2ec8f63). Click here to learn what that means.

Additional details and impacted files

@@             Coverage Diff             @@
##             unstable     #324   +/-   ##
===========================================
  Coverage            ?   68.43%           
===========================================
  Files               ?      109           
  Lines               ?    61689           
  Branches            ?        0           
===========================================
  Hits                ?    42215           
  Misses              ?    19474           
  Partials            ?        0           

Files	Coverage Δ
src/commands.def	100.00% <ø> (ø)
src/db.c	87.39% <87.50%> (ø)

[madolson]

Still missing a doc PR, but other than that also LGTM.

[hwware]

@hwware and @PingXie Your two votes are missing btw. Do you want to be explicit and add it to viktors comment.

Voted, no objection for this.

[zuiderkwast]

OK, this is ready to merge. @CharlesChen888 Do you have a doc PR for this?

[CharlesChen888]

@zuiderkwast doc PR is ready.