Skip to content

Conversation

@oranagra
Copy link
Member

It was confusing as to why these don't return a map type.
the reason is that order matters, so we need to make sure the client
library knows to respect it.
Added comments in the implementation and tests to cover it.

It was confusing as to why these don't return a map type.
the reason is that order matters, so we need to make sure the client
library knows to respect it.
Added comments in the implementation and tests to cover it.
@oranagra oranagra added the state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten label Jan 24, 2021
@oranagra
Copy link
Member Author

apparently the spec says:

Map: an ordered collection of key-value pairs. Keys and values can be any other RESP3 type.

but that may be a controversial decision, see antirez/RESP3#30
so i think we may want to stay away from that and never rely on map being ordered.

regardless, for backwards compatibility with redis 6.0, we can't probably change the response type of ZRANGE in RESP3.
if we act fast, maybe we can change the response of ZINTER etc (new commands in 6.2).

@redis/core-team please share your thoughts.

@guybe7
Copy link
Collaborator

guybe7 commented Jan 25, 2021

my vote to keeping the response as an array of tuples, it'll be weird if two commands that reply with elements from a zset would reply differently

@yossigo
Copy link
Collaborator

yossigo commented Jan 25, 2021

Agree with @guybe7

@oranagra oranagra merged commit 9e56d39 into redis:unstable Jan 26, 2021
JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Mar 2, 2021
It was confusing as to why these don't return a map type.
the reason is that order matters, so we need to make sure the client
library knows to respect it.
Added comments in the implementation and tests to cover it.
@oranagra oranagra deleted the zrange_resp3_tests branch August 8, 2022 14:05
oranagra added a commit to redis/redis-specifications that referenced this pull request Jan 5, 2023
see:
* redis/redis#8391
* redis/redis#8504

Co-authored-by: Oran Agra <oran@redislabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants