-
Notifications
You must be signed in to change notification settings - Fork 71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Python: adds ZRANGE command #906
Conversation
f008fcb
to
299a092
Compare
please align tests with #910 |
299a092
to
cf5cff3
Compare
- For range queries by index (rank), use RangeByIndex. | ||
- For range queries by lexicographical order, use RangeByLex. | ||
- For range queries by score, use RangeByScore. | ||
reverse (bool): If True, reverses the sorted set, with index 0 as the element with the highest score |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: missing a period at the end
|
||
Returns: | ||
List[str]: A list of elements within the specified range. | ||
For idex queries, If `start` is greater than either the end index of the sorted set or `stop`, an empty list is returned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we decided on the last meeting - lets not document in the client all of the edge cases that the server is responsible for, so we won't need to change our documentation if they change their implementation. Users can go to the shared zrange link to find more info.
- If `stop` is greater than either the end index of the sorted set or `start` and `reverse` is set to True, | ||
an empty list is returned. | ||
If `key` does not exist, it is treated as an empty sorted set, and the command returns an empty array. | ||
If `key` holds a value that is not a sorted set, an error is returned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an error is raised.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to remove it anyway
Examples: | ||
>>> await zrange("my_sorted_set", RangeByIndex(0, -1)) | ||
['member1', 'member2', 'member3'] # Returns all members in ascending order. | ||
>>> await zrange("non_existing_key", RangeByScore(ScoreBoundary(0), ScoreBoundary(-1))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets skip the non existing key example and instead have a more complex example that gives more information, for example RangeByScore with InfBound.Negative, ScoreBoundary(3), limit=2
args = [key, str(range_query.start), str(range_query.stop)] | ||
if reverse: | ||
args.append("REV") | ||
if not isinstance(range_query, RangeByIndex): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be more readable if you do:
args = [key, str(range_query.start), str(range_query.stop)]
if reverse:
args.append("REV")
if isinstance(range_query, RangeByScore):
args.append("BYSCORE")
elif isinstance(range_query, RangeByLex):
args.append("BYLEX")
if hasattr(range_query, 'limit') and range_query.limit is not None:
args.extend(
[
"LIMIT",
str(range_query.limit.offset),
str(range_query.limit.count),
]
)
Examples: | ||
>>> await zrange_withscores("my_sorted_set", RangeByScore(ScoreBoundary(10), ScoreBoundary(20))) | ||
{'member1': 10.5, 'member2': 15.2} # Returns members with scores between 10 and 20 with their scores. | ||
>>> await zrange_withscores("non_existing_key", RangeByScore(ScoreBoundary(0), ScoreBoundary(-1))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
32da251
to
da1d895
Compare
@barshaul ready |
Enumeration representing numeric and lexicographic positive and negative infinity bounds for sorted set scores. | ||
""" | ||
|
||
POS_INF = {"default_arg": "+inf", "lex_arg": "+"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If +inf is only used with score, change default_arg to score_arg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you checked if this API is possible in nodeJS, Java? (having one enum with two potential arg values)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it isn't, we might better choosing your prev solution
""" | ||
score_min = ( | ||
min_score.value | ||
if not type(min_score) == InfBound |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets not use a negative if, e.g.:
score_min = min_score["score_arg"] if type(min_score) == InfBound else min_score.value
limit: Optional[Limit] = None, | ||
): | ||
self.start = ( | ||
start.value if not type(start) == InfBound else start.value["default_arg"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
>>> await client.zrange("my_sorted_set", RangeByIndex(0, -1)) | ||
['member1', 'member2', 'member3'] # Returns all members in ascending order. | ||
>>> await client.zrange("my_sorted_set", RangeByScore(start=InfBound.NEG_INF, stop= ScoreBoundary(3), limit= Limit(0 , 2))) | ||
['member2', 'member3'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we don't have a comment explaining this example output? if it's too complicated, simplify the example
) -> Mapping[str, float]: | ||
""" | ||
Returns the specified range of elements with their scores in the sorted set stored at `key`. | ||
Similar to ZRANGE but with a WTHISCORE flag. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WTHISCORE -> WITHSCORE
reverse (bool): If True, reverses the sorted set, with index 0 as the element with the highest score. | ||
|
||
Returns: | ||
Map[str , float]: A map of elements and their scores within the specified range. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Map -> Mapping
>>> await client.zrange_withscores("my_sorted_set", RangeByScore(ScoreBoundary(10), ScoreBoundary(20))) | ||
{'member1': 10.5, 'member2': 15.2} # Returns members with scores between 10 and 20 with their scores. | ||
>>> await client.zrange("my_sorted_set", RangeByScore(start=InfBound.NEG_INF, stop= ScoreBoundary(3), limit= Limit(0 , 2))) | ||
{'member4': -2.0, 'member7': 1.5} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
Args: | ||
start (Union[InfBound, ScoreBoundary]): The start score boundary. | ||
stop (Union[InfBound, ScoreBoundary]): The stop score boundary. | ||
limit (Optional[Limit]): The limit argument for a range query. Defaults to None. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets refer to the Limit class for more info, try to find python docstring way of doing it (maybe https://stackoverflow.com/questions/21289806/link-to-class-method-in-python-docstring)
Args: | ||
start (Union[InfBound, LexBoundary]): The start lexicographic boundary. | ||
stop (Union[InfBound, LexBoundary]): The stop lexicographic boundary. | ||
limit (Optional[Limit]): The limit argument for a range query. Defaults to None. See `Limit`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see Limit
class for more information.
675b72c
to
67691e4
Compare
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.