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.

Sign up for GitHub

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

Jump to bottom

Python: add SORT_RO command. #1528

Merged
merged 15 commits into from
Jul 4, 2024
Merged

Python: add SORT_RO command. #1528

merged 15 commits into from
Jul 4, 2024

Conversation

GilboaAWS
Copy link
Collaborator

No description provided.

@GilboaAWS GilboaAWS added the python Python wrapper label Jun 5, 2024
@GilboaAWS GilboaAWS requested a review from a team as a code owner June 5, 2024 14:54
@Yury-Fridlyand Yury-Fridlyand mentioned this pull request Jun 5, 2024
Merged
Yury-Fridlyand
Yury-Fridlyand reviewed Jun 7, 2024
View reviewed changes
Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

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

Please add changelog entry

glide-core/src/protobuf/redis_request.proto Outdated Show resolved Hide resolved
python/python/glide/async_commands/cluster_commands.py Outdated Show resolved Hide resolved
python/python/glide/async_commands/cluster_commands.py Show resolved Hide resolved
python/python/glide/async_commands/standalone_commands.py Outdated Show resolved Hide resolved
python/python/glide/async_commands/standalone_commands.py Outdated Show resolved Hide resolved
python/python/glide/async_commands/transaction.py Show resolved Hide resolved
python/python/glide/async_commands/transaction.py Outdated Show resolved Hide resolved
python/python/glide/async_commands/transaction.py Outdated Show resolved Hide resolved
python/python/glide/async_commands/transaction.py Show resolved Hide resolved
python/python/tests/test_transaction.py Outdated Show resolved Hide resolved
@Yury-Fridlyand
Copy link
Collaborator

Please add changelog entry

Yury-Fridlyand
Yury-Fridlyand reviewed Jun 14, 2024
View reviewed changes
Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

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

Please double check that docs are aligned

python/python/glide/async_commands/transaction.py Show resolved Hide resolved
python/python/glide/async_commands/transaction.py
alpha: Optional[bool] = None,
) -> TTransaction:
"""
Sorts the elements in the list, set, or sorted set at `key` and returns the result.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Sorts the elements in the list, set, or sorted set at `key` and returns the result.
Sorts the elements in the list, set, or sorted set at `key` and returns the result.
The `sort_ro` command can be used to sort elements based on different criteria and apply transformations on sorted elements.

python/python/glide/async_commands/cluster_commands.py Show resolved Hide resolved
python/python/glide/async_commands/cluster_commands.py Show resolved Hide resolved
This was referenced Jun 15, 2024
Merged
Merged
adarovadya
adarovadya reviewed Jul 3, 2024
View reviewed changes
python/python/glide/async_commands/cluster_commands.py Outdated

By default, sorting is numeric, and elements are compared by their value interpreted as double precision floating point numbers.

See https://valkey.io/commands/sort_ro for more details.
Copy link
Collaborator

@adarovadya adarovadya Jul 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this link is not exits. there is a mention about this command in - https://valkey.io/commands/sort/ . but not describe how to use in sort_ro.
I asked Medy if is going to be added or to refer to sort command

adarovadya
adarovadya reviewed Jul 3, 2024
View reviewed changes
python/python/glide/async_commands/cluster_commands.py Outdated
"""
args = _build_sort_args(key, None, limit, None, order, alpha)
result = await self._execute_command(RequestType.SortReadOnly, args)
return cast(List[str], result)
Copy link
Collaborator

Choose a reason for hiding this comment

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

change the str

adarovadya
adarovadya reviewed Jul 3, 2024
View reviewed changes
python/python/glide/async_commands/transaction.py Outdated
"""
Sorts the elements in the list, set, or sorted set at `key` and returns the result.

See https://valkey.io/commands/sort_ro for more details.
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

adarovadya
adarovadya reviewed Jul 3, 2024
View reviewed changes
python/python/tests/test_async_client.py Outdated
@@ -4749,6 +4765,15 @@ async def test_sort_and_sort_store_with_get_or_by_args(
["Dave", "Bob", "Alice", "Charlie", "Eve"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

please add a test which check list of bytes and mixed list of bytes and strings

Yury-Fridlyand
Yury-Fridlyand reviewed Jul 3, 2024
View reviewed changes
Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

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

Please double check that doc is aligned with java client - we had there few rounds of reviews and already merged it.
You can update docs on both clients if you find it is neccesary.

python/python/glide/async_commands/transaction.py Outdated
@@ -4611,6 +4611,7 @@ def sort(
"""
Sorts the elements in the list, set, or sorted set at `key` and returns the result.
The `sort` command can be used to sort elements based on different criteria and apply transformations on sorted elements.
This command is routed to primary nodes only.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess you don't need to add it here - the command isn't routed there, an entire transaction is routed ... according to the first command it has.
If you want to have such notice in transaction implementation, please reword it to reflect the actual behavior.

@GilboaAWS GilboaAWS merged commit d250458 into valkey-io:main Jul 4, 2024
20 checks passed
cyip10 pushed a commit to Bit-Quill/valkey-glide that referenced this pull request Jul 4, 2024
@GilboaAWS @cyip10
Python: add SORT_RO command. (valkey-io#1528)
70d3516 
* Python: add SORT_RO command.
@GilboaAWS GilboaAWS deleted the sort_ro_python branch July 7, 2024 11:23
cyip10 pushed a commit to Bit-Quill/valkey-glide that referenced this pull request Jul 16, 2024
@GilboaAWS @cyip10
Python: add SORT_RO command. (valkey-io#1528)
bd4f7da 
* Python: add SORT_RO command.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yury-Fridlyand Yury-Fridlyand Yury-Fridlyand approved these changes

@GumpacG GumpacG GumpacG left review comments

@yipin-chen yipin-chen yipin-chen approved these changes

@acarbonetto acarbonetto acarbonetto approved these changes

@adarovadya adarovadya adarovadya approved these changes

@aaron-congo aaron-congo Awaiting requested review from aaron-congo

Assignees
No one assigned
Labels
python Python wrapper
Projects
Valkey-GLIDE - internal
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@GilboaAWS @Yury-Fridlyand @adarovadya @GumpacG @acarbonetto @yipin-chen