zrange with scores gives confusing results when duplicate scores are present

[david-macmahon]

When all the elements of a zset have unique scores, the OrderedCollections.OrderedSet returned by zrange(conn, zsetname, 0, n, :withscores) makes sense:

julia> zadd(r, "zsettest", 1, "one")
1

julia> zadd(r, "zsettest", 2, "two")
1

julia> zrange(r, "zsettest", 0, 1, :withscores)
OrderedCollections.OrderedSet{AbstractString} with 4 elements:
  "one"
  "1"
  "two"
  "2"

But when there are multiple entries with the same score, the duplicate scores are not duplicated in the returned OrderedCollections.OrderedSet (because it is a set after all):

julia> zadd(r, "zsettest", 1, "three")
1

julia> zrange(r, "zsettest", 0, 2, :withscores)
OrderedCollections.OrderedSet{AbstractString} with 5 elements:
  "one"
  "1"
  "three"
  "two"
  "2"

One way to fix this would be for the elements of the OrderedSet to be value => score or pairs or (value, score) tuples. I think that would allow the return type still to be OrderedSet while not creating ambiguity about which is value and which is score (e.g. when values and scores look alike), but it would definitely be a breaking change compared to the current (arguably broken) behavior. FWIW, this is what redis-cli returns:

$ redis-cli
127.0.0.1:6379> ZRANGE zsettest 0 2 withscores
1) "one"
2) "1"
3) "three"
4) "1"
5) "two"
6) "2"

[jkaye2012]

Yeah, withscores definitely makes OrderedSet{String} not the correct structure. I think it should probably be AbstractDict{String, Float64} or something. Should just be able to add the applicable overload to commands.jl and it should work it out. We already have a major version bump pending right now, so this is something we could get in there.

I haven't bumped it yet because I haven't really been using Julia for years now other than to maintain this lib. Would be happy to do so, though a PR would be ideal since I'm not so familiar with the ecosystem these days.

OrderedSet{String} I think is correct for all operations that don't use withscores, so this would only affect uses with that option.

[david-macmahon]

I guess maybe OrderedDict would be the concrete type, but that would change the return type based on input options. Maybe an OrderedSet{Pair{AbstractString, Float64}} would be preferable since it's still an OrderedSet, but could be used to easily create a Dict or OrderedDict.

[jkaye2012]

Unfortunately, any change to fix this issue is going to have to change the return type based on input options. All things equal, I think I would prefer using the "most correct" type since users will have to handle the difference in any event.

…

On Mon, Nov 1, 2021, 6:18 PM david-macmahon ***@***.***> wrote: I guess maybe OrderedDict would be the concrete type, but that would change the return type based on input options. Maybe an OrderedSet{Pair{AbstractString, Float64}} would be preferable since it's still an OrderedSet, but could be used to easily create a Dict or OrderedDict. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#85 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA7FL7BPPTP3FUWQILUQGXDUJ4U6RANCNFSM5HFFWHJA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[david-macmahon]

Hmm, I'm not sure whether a return type of Union{OrderedSet{AbstractString}, OrderedDict{AbstractString,Float64}} is better/worse/same as OrderedSet{Union{AbstractString, Pair{AbstractString,Float64}} in terms of type stability. I guess the second way would more correctly be Union{OrderedSet{AbstractString}, OrderedSet{Pair{AbstractString,Float64}}} so yeah, I think your idea is better.

[xgdgsc]

Any plan to address this? Python seems use a list of tuples https://github.com/redis/redis-py/blob/0c242404d790fbcca9252be21eb63182f2834e7d/redis/commands/core.py#L4689

[jkaye2012]

I'd be happy to accept a PR for it, I am not working on it myself though. It should be a simple enough change.

The big decision here is whether to break the API and bump the major version, or deprecate the existing method and add a new one that has the better implementation/return type. I'd probably lean towards the latter personally.