Python: add BZPOPMIN and BZPOPMAX commands by aaron-congo · Pull Request #1399 · valkey-io/valkey-glide

aaron-congo · 2024-05-09T23:08:41Z

Issue #, if available:
N/A

Description of changes:

redis docs:
- https://redis.io/docs/latest/commands/bzpopmin/
- https://redis.io/docs/latest/commands/bzpopmax/
includes value conversion:
- BZPOPMIN/BZPOPMAX returns an array containing a key, member, and score. RESP2 returns the score as a string, but RESP3 returns the score as a double. The value conversion added in this PR ensures the score is always returned as a double

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

GumpacG · 2024-05-10T19:01:58Z

glide-core/src/client/value_conversion.rs

    Lolwut,
    ArrayOfArraysOfDoubleOrNull,
    ArrayOfKeyValuePairs,
+    KeyWithMemberAndScore,


How come this wasn't included in the Java implementation?

ig it was forgotten, good catch:)

Yeah its because the conversion is needed for RESP2 responses. The Java wrapper doesn't support RESP2 yet but Python does

GumpacG · 2024-05-10T20:36:14Z

glide-core/src/client/value_conversion.rs

+                if array.len() == 3
+                    && matches!(array[2], Value::BulkString(_) | Value::SimpleString(_)) =>
+            {
+                array[2] =


Does this only convert the third argument to double? Can there be more arguments with more keys where the timeout is greater than the third argument?

redis> ZADD zset1 0 a 1 b 2 c (integer) 3 redis> BZPOPMIN zset1 zset2 0 1) "zset1" 2) "a" 3) "0"

replay can be one of the following:

Nil reply: when no element could be popped and the timeout expired.
Array reply: the keyname, popped member, and its score.

so when its an array replay, which means we popped an element, we want to convert just the score from string to double

Gotcha, thanks!

shohamazon · 2024-05-13T12:17:05Z

glide-core/src/client/value_conversion.rs

    Lolwut,
    ArrayOfArraysOfDoubleOrNull,
    ArrayOfKeyValuePairs,
+    KeyWithMemberAndScore,


ig it was forgotten, good catch:)

shohamazon · 2024-05-13T12:19:57Z

glide-core/src/client/value_conversion.rs

+                if array.len() == 3
+                    && matches!(array[2], Value::BulkString(_) | Value::SimpleString(_)) =>
+            {
+                array[2] =


redis> ZADD zset1 0 a 1 b 2 c (integer) 3 redis> BZPOPMIN zset1 zset2 0 1) "zset1" 2) "a" 3) "0"

replay can be one of the following:

Nil reply: when no element could be popped and the timeout expired.
Array reply: the keyname, popped member, and its score.

so when its an array replay, which means we popped an element, we want to convert just the score from string to double

shohamazon · 2024-05-13T12:22:53Z

python/python/glide/async_commands/core.py

+        BZPOPMAX is the blocking variant of ZPOPMAX.
+
+        BZPOPMAX is a client blocking command, see https://github.com/aws/glide-for-redis/wiki/General-Concepts#blocking-commands for more details and best practices.


Suggested change

BZPOPMAX is the blocking variant of ZPOPMAX.

BZPOPMAX is a client blocking command, see https://github.com/aws/glide-for-redis/wiki/General-Concepts#blocking-commands for more details and best practices.

`BZPOPMAX` is the blocking variant of `ZPOPMAX`.

`BZPOPMAX` is a client blocking command, see https://github.com/aws/glide-for-redis/wiki/General-Concepts#blocking-commands for more details and best practices.

maybe we should do:

BBZPOPMAX is a client blocking command, see [Blocking commands](https://github.com/aws/glide-for-redis/wiki/General-Concepts#blocking-commands) for more details and best practices.

I don't think hyperlinks like these are supported by default, according to this stack overflow post they aren't, but some tools and/or documentation generators may support them. Looks like VScode does, but I'm using pycharm and it doesn't. Let me know if you still want me to change this to a hyperlink though

python/python/glide/async_commands/core.py

python/python/glide/async_commands/transaction.py

python/python/tests/test_async_client.py

aaron-congo · 2024-05-13T21:49:51Z

@shohamazon Thanks for the review, all comments are addressed now

Yury-Fridlyand · 2024-05-13T23:48:27Z

@aaron-congo please, resolve conflicts and merge

* Python: add BZPOPMIN and BZPOPMAX commands (#266) * Update PR link * PR suggestions * Fix rust

aaron-congo requested a review from a team as a code owner May 9, 2024 23:08

aaron-congo added the python 🐍 Python wrapper label May 9, 2024

aaron-congo requested review from GumpacG, Yury-Fridlyand, acarbonetto, barshaul, jonathanl-bq, shohamazon and tjzhang-BQ and removed request for Yury-Fridlyand May 9, 2024 23:09

Yury-Fridlyand approved these changes May 10, 2024

View reviewed changes

GumpacG approved these changes May 10, 2024

View reviewed changes

shohamazon reviewed May 13, 2024

View reviewed changes

aaron-congo force-pushed the python/integ_acongo_bzpopmax branch from a67c7ba to 3f1e3c4 Compare May 13, 2024 21:12

acarbonetto approved these changes May 13, 2024

View reviewed changes

Elen-Ghulam approved these changes May 13, 2024

View reviewed changes

aaron-congo and others added 4 commits May 13, 2024 16:54

Python: add BZPOPMIN and BZPOPMAX commands (#266)

63256d3

Update PR link

93bc0fd

PR suggestions

abefd9d

Fix rust

6a23610

aaron-congo force-pushed the python/integ_acongo_bzpopmax branch from 9c3e15f to 6a23610 Compare May 13, 2024 23:54

acarbonetto merged commit 8c590a7 into valkey-io:main May 14, 2024

acarbonetto deleted the python/integ_acongo_bzpopmax branch May 14, 2024 05:25

avifenesh pushed a commit that referenced this pull request May 14, 2024

Python: add BZPOPMIN and BZPOPMAX commands (#1399)

5deb969

* Python: add BZPOPMIN and BZPOPMAX commands (#266) * Update PR link * PR suggestions * Fix rust

shohamazon mentioned this pull request May 15, 2024

Python: add BZMPOP command #1412

Merged

cyip10 pushed a commit to Bit-Quill/valkey-glide that referenced this pull request Jun 24, 2024

Python: add BZPOPMIN and BZPOPMAX commands (valkey-io#1399)

8ee2173

* Python: add BZPOPMIN and BZPOPMAX commands (#266) * Update PR link * PR suggestions * Fix rust

		BZPOPMAX is the blocking variant of ZPOPMAX.

		BZPOPMAX is a client blocking command, see https://github.com/aws/glide-for-redis/wiki/General-Concepts#blocking-commands for more details and best practices.

-        BZPOPMAX is the blocking variant of ZPOPMAX.
-        BZPOPMAX is a client blocking command, see https://github.com/aws/glide-for-redis/wiki/General-Concepts#blocking-commands for more details and best practices.
+        `BZPOPMAX` is the blocking variant of `ZPOPMAX`.
+        `BZPOPMAX` is a client blocking command, see https://github.com/aws/glide-for-redis/wiki/General-Concepts#blocking-commands for more details and best practices.

Conversation

aaron-congo commented May 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaron-congo May 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aaron-congo commented May 13, 2024

Uh oh!

Yury-Fridlyand commented May 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

aaron-congo May 13, 2024 •

edited

Loading