Skip to content
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

Merged
merged 3 commits into from
Mar 3, 2024
Merged

Conversation

shohamazon
Copy link
Member

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.

@shohamazon shohamazon requested a review from a team as a code owner February 6, 2024 15:56
@shohamazon shohamazon marked this pull request as draft February 6, 2024 16:08
@shachlanAmazon
Copy link
Contributor

please align tests with #910

- 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
Copy link
Contributor

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.
Copy link
Contributor

@barshaul barshaul Feb 18, 2024

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

an error is raised.

Copy link
Member Author

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)))
Copy link
Contributor

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):
Copy link
Contributor

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)))
Copy link
Contributor

Choose a reason for hiding this comment

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

same

@shohamazon shohamazon force-pushed the python/zrange branch 3 times, most recently from 32da251 to da1d895 Compare February 22, 2024 18:56
@shohamazon
Copy link
Member Author

@barshaul ready

Enumeration representing numeric and lexicographic positive and negative infinity bounds for sorted set scores.
"""

POS_INF = {"default_arg": "+inf", "lex_arg": "+"}
Copy link
Contributor

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

Copy link
Contributor

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)

Copy link
Contributor

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
Copy link
Contributor

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"]
Copy link
Contributor

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']
Copy link
Contributor

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.
Copy link
Contributor

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.
Copy link
Contributor

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}
Copy link
Contributor

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.
Copy link
Contributor

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)

CHANGELOG.md Outdated Show resolved Hide resolved
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`.
Copy link
Contributor

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.

@shohamazon shohamazon merged commit ceefa07 into aws:main Mar 3, 2024
5 checks passed
@shohamazon shohamazon deleted the python/zrange branch March 3, 2024 14:07
cyip10 pushed a commit to Bit-Quill/glide-for-redis that referenced this pull request Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants