Skip to content

Make zscore transparently varargs#6230

Closed
josiahcarlson wants to merge 2 commits intoredis:unstablefrom
josiahcarlson:patch-1
Closed

Make zscore transparently varargs#6230
josiahcarlson wants to merge 2 commits intoredis:unstablefrom
josiahcarlson:patch-1

Conversation

@josiahcarlson
Copy link

Hashes have hmget, maybe just make hget varargs too? Probably a very similar change.

Hashes have hmget, maybe just make hget varargs too?
Copy link
Member

@itamarhaber itamarhaber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A useful extension, thanks @josiahcarlson :)

@antirez
Copy link
Contributor

antirez commented Jul 12, 2019

Hello, this was discussed many times, but I'll try to explain it again: in Redis we never want a single command to return a different value type (plain VS array) just because the number of arguments changed. Because otherwise when you do:

foo = redis.zscore(*myarray_of_keys);

You don't want foo to be at random an array or a single value, otherwise you have to make extra checks later. This gets even more harder with languages like Java that have often hard-coded signatures for methods.

So the solution would be in theory to add ZMSCORE, that always returns an array (even an empty one if there are no arguments at all, this makes even more sense and is used in some Redis APIs). However is it worth it? Depends on use cases, but I've the feeling that it could be a good idea. What is the common use case for fetching multiple scores at the same time?

@antirez
Copy link
Contributor

antirez commented Jul 12, 2019

P.S. note that in write commands instead, we often converted them to vararg, because they returned 0 or 1, so we extended the return value to be the number of affected elements, which is completely backward compatibile and does not change the reply type with the number of arguments. This is one of the few rules of the Redis API.

@josiahcarlson
Copy link
Author

My use case is simply that I have scores in a sorted set that I want to know, because that's the primary place where we put those values. And being able to get more than 1 with ~half as many hash lookups would be nice.

But I closed this PR because I think Redis has too many root-level verbs already (more than 200 now), and adding another as zmscore is not what I intended when I opened it.

@josiahcarlson josiahcarlson deleted the patch-1 branch July 15, 2019 15:26
@antirez
Copy link
Contributor

antirez commented Jul 15, 2019

@josiahcarlson I was asking what is the application reason to fetch multiple scores by value. I ask that for pure curiosity because I don't implement most applications with Redis itself nowadays and this is not a common thing I hear from the community, maybe there is a very legitimate pattern. However I think that even if you are right that Redis has many root verbs, ZMSCORE does not make the situation much worse, because most Redis users understand that ZSCORE and ZMSCORE are kinda the same thing. Sometimes we avoided adding a new command by exploiting the fact that the first argument would be a number, so one could do ZSCORE MANY keya keyb keyc, however here the first argument is a string so it's more complex to find a way to make it backward compatible, and at the same time having a form that is clearly different when used in "multiple keys" mode. So I'm not completely against adding ZMSCORE, I wonder what other users think. Reopening in the hope to see some further comment from other folks.

@antirez
Copy link
Contributor

antirez commented Jul 15, 2019

Oh no I can't reopen since the branch was removed.

@josiahcarlson josiahcarlson restored the patch-1 branch July 17, 2019 03:12
@josiahcarlson josiahcarlson reopened this Jul 17, 2019
@josiahcarlson
Copy link
Author

josiahcarlson commented Jul 17, 2019

Reopening because you requested.

My use case is simply that I've got indexes whose contents I'm verifying and using at the same time. I have already done a few things to make sure that I'm using Lua to minimize round-trips, but if I'm doing 50 calls to ZSCORE with the same key as part of a larger operation, it seems like maybe avoiding the main hash lookup the 49 times it isn't necessary makes sense.

If this is something you actually like, I might also suggest doing something similar for ZRANK, ZREVRANK, SISMEMBER, GETBIT, HEXISTS, and any others where there doesn't already exist a multiple-read / write version. I've done applications with Redis + Lua using those operations, and I've used all of the above in a loop for data fetching, index construction, etc.

@antirez
Copy link
Contributor

antirez commented Jul 18, 2019

@josiahcarlson good point: if we decide to do that, does not make much sense to stop at this instance, but to address similar things as well. There are already open issues for MEXISTS and alike indeed. Another more radical choice could be to have the next releases of Redis to return an array for such commands so that we can make the old command vararg, but in that case we would break the API, which was historically almost never done in Redis. But it is useful to outline all the possibilities we have I guess. Note that certain commands are vararg "friendly" in the list you proposed. For instance GETBIT, since it gets the bit as a number, can be made vararg without problems:

GETBIT mykey 2134 -> Normal form

GETBIT mykey BITS 1 2 3 4 100 200 300 -> New form returning an array

Those are low hanging fruits.

@TysonAndre
Copy link
Contributor

So the solution would be in theory to add ZMSCORE, that always returns an array (even an empty one if there are no arguments at all, this makes even more sense and is used in some Redis APIs).

Redis server unstable(6.2.0) will have a ZMSCORE command added with the described behavior - #7593 was merged . I'm mentioning this because it may reduce the need to make zscore support varargs

@itamarhaber
Copy link
Member

Thanks @TysonAndre for bumping this - adding to a to-be-closed tag as ZMSCORE seems to fit the bill.

@itamarhaber itamarhaber added the state:to-be-closed requesting the core team to close the issue label Aug 5, 2020
@oranagra
Copy link
Member

oranagra commented Aug 6, 2020

since #7593 was already merged, we can close this one right away..
thanks everyone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

state:to-be-closed requesting the core team to close the issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants