Python: add XINFO STREAM command #1816

aaron-congo · 2024-07-04T20:08:18Z

Issue #, if available:
N/A

Description of changes:
https://redis.io/docs/latest/commands/xinfo-stream/

Note 1: The value conversion is a work-in-progress, so please do not focus reviews on that section. You can however review the value conversion tests. The Python integration tests are passing for RESP3 but failing on RESP2 because the value conversion still needs some work.
Note 2: I will need to rebase onto main, however I will hold off on doing this because we have two people working on this branch.

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

jonathanl-bq · 2024-07-04T23:13:05Z

glide-core/src/client/value_conversion.rs

+                    return Err((ErrorKind::TypeError, "Groups key not found").into());
+                };
+
+                if array.get(groups_index + 1).is_none() {


Not sure if we need it here, but you might want to add an overflow check here, in case groups_index is usize::MAX

Definitely a good callout, but I don't think it is needed. usize::MAX is 18_446_744_073_709_551_615, but groups_index + 1 is always 18 with the current version of redis. For this to overflow, valkey would have to add enough fields in this array to boost that number over usize::MAX, which seems very unlikely.

glide-core/src/client/value_conversion.rs

jonathanl-bq · 2024-07-04T23:16:07Z

glide-core/src/client/value_conversion.rs

+        */
+        ExpectedReturnType::XInfoStreamFullReturnType => match value {
+            Value::Map(_) => Ok(value),  // Response is already in RESP3 format - no conversion needed
+            Value::Array(mut array) => {


Could probably refactor some of this code, since it's repetitive in some areas. We're essentially using the array like an association list, so we probably could write a (zero-cost) abstraction around this behaviour to make things a bit easier to reason about. Not a blocker, but something to think about.

Yes, not sure if we will get around to it in this PR but we'll see. I agree that we can probably do some refactoring here to improve things

glide-core/src/client/value_conversion.rs

Yury-Fridlyand · 2024-07-04T23:44:35Z

glide-core/src/client/value_conversion.rs

+
+                if groups.is_empty() {
+                    let converted_response = convert_to_expected_type(Value::Array(array), Some(ExpectedReturnType::Map {
+                        key_type: &Some(ExpectedReturnType::BulkString),


Suggested change

key_type: &Some(ExpectedReturnType::BulkString),

key_type: &Some(ExpectedReturnType::SimpleString),

I don't understand how it causes nested map[] to convert

This line is covering the scenario where "groups" is empty, i.e. there are no groups for the stream. Usually the nested maps are inside of groups (each group needs to be converted to a map, and each consumer inside that group map needs to be converted to a map). But since groups is empty in this block there are no nested maps that we need to convert

Oh I see
So you just flat the recursion into a nested loops.
Maybe we need to rework it in the CompletableFuture

python/python/glide/async_commands/core.py

python/python/tests/test_async_client.py

python/python/tests/test_transaction.py

glide-core/src/client/value_conversion.rs

acarbonetto · 2024-07-05T00:18:47Z

glide-core/src/request_type.rs

@@ -214,6 +214,7 @@ pub enum RequestType {
    XAutoClaim = 203,
    XInfoGroups = 204,
    XInfoConsumers = 205,
+    XInfoStream = 207,
    Scan = 206,


this ordering is unacceptable!

Lol 206 was stolen from me, it was either group the XInfo commands together but have weird numerical ordering or group them apart but have consistently increasing number values. I didn't want to change the scan numerical value to 207 because that could mess with other people's local code if they forget to rebuild after pulling this change

you could've kept the numbers while swapping the ordering 😆 - welp, too late.

Yury-Fridlyand

I disagree with having bulk strings as map keys in the response, but this probably should be updated for all related commands together.
LGTM

acarbonetto · 2024-07-05T00:42:11Z

I disagree with having bulk strings as map keys in the response, but this probably should be updated for all related commands together.

Yes. A cleanup task for both teams to make a conscious effort to refactor and cleanup typing. We have way too much code and quite a bit of hand-waving going on in this file. Something to address in the coming month(s).

* Grab value conversion from Java draft PR * wip * wip * refactor value conversion wip * Make value_conversion build * Add value conversion test * Verified test values * Fix errors in test * RESP3 tests passing * Tests passing for RESP3, mypy passing * Update CHANGELOG, minor doc fix * Fix failing value conversion test * Clean up value_conversion.rs a bit * minor cleanup * Modify test to cover failing scenario * Python tests passing * Fix test function signatures * Fix failing pytest, cleanup value conversion comments * PR suggestions --------- Co-authored-by: Jonathan Louie <[email protected]>

Yury-Fridlyand added the python label Jul 4, 2024

aaron-congo and others added 17 commits July 4, 2024 15:57

Grab value conversion from Java draft PR

8b2fd6c

wip

dae7f2e

wip

048b862

refactor value conversion wip

e53564d

Make value_conversion build

df5a672

Add value conversion test

636abe4

Verified test values

4de16eb

Fix errors in test

c7f64bf

RESP3 tests passing

20f616f

Tests passing for RESP3, mypy passing

67d1fbd

Update CHANGELOG, minor doc fix

e28886d

Fix failing value conversion test

4320076

Clean up value_conversion.rs a bit

dd6211e

minor cleanup

d524644

Modify test to cover failing scenario

29e1190

Python tests passing

b958d39

Fix test function signatures

3b70d43

aaron-congo force-pushed the python/integ_acongo_xinfo_stream branch from 37542ef to 3b70d43 Compare July 4, 2024 23:11

aaron-congo marked this pull request as ready for review July 4, 2024 23:11

aaron-congo requested a review from a team as a code owner July 4, 2024 23:11

jonathanl-bq reviewed Jul 4, 2024

View reviewed changes

aaron-congo added 2 commits July 4, 2024 16:20

Fix failing pytest, cleanup value conversion comments

613d291

PR suggestions

e3b4f4b

Yury-Fridlyand reviewed Jul 5, 2024

View reviewed changes

acarbonetto approved these changes Jul 5, 2024

View reviewed changes

Yury-Fridlyand approved these changes Jul 5, 2024

View reviewed changes

Elen-Ghulam approved these changes Jul 5, 2024

View reviewed changes

acarbonetto merged commit a939df4 into valkey-io:main Jul 5, 2024
79 checks passed

acarbonetto mentioned this pull request Jul 5, 2024

Glide-Core: Refactor and cleanup value_conversion.rs #1819

Open

2 tasks

acarbonetto deleted the python/integ_acongo_xinfo_stream branch July 5, 2024 00:56

Yury-Fridlyand mentioned this pull request Jul 5, 2024

Python: fix mypy #1829

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python: add XINFO STREAM command #1816

Python: add XINFO STREAM command #1816

aaron-congo commented Jul 4, 2024 •

edited

Loading

jonathanl-bq Jul 4, 2024

aaron-congo Jul 4, 2024

jonathanl-bq Jul 4, 2024

aaron-congo Jul 4, 2024

Yury-Fridlyand Jul 4, 2024

Yury-Fridlyand Jul 4, 2024

aaron-congo Jul 5, 2024 •

edited

Loading

Yury-Fridlyand Jul 5, 2024

acarbonetto Jul 5, 2024

aaron-congo Jul 5, 2024 •

edited

Loading

acarbonetto Jul 5, 2024

Yury-Fridlyand left a comment

acarbonetto commented Jul 5, 2024

	key_type: &Some(ExpectedReturnType::BulkString),
	key_type: &Some(ExpectedReturnType::SimpleString),

Python: add XINFO STREAM command #1816

Python: add XINFO STREAM command #1816

Conversation

aaron-congo commented Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaron-congo Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaron-congo Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yury-Fridlyand left a comment

Choose a reason for hiding this comment

acarbonetto commented Jul 5, 2024

aaron-congo commented Jul 4, 2024 •

edited

Loading

aaron-congo Jul 5, 2024 •

edited

Loading

aaron-congo Jul 5, 2024 •

edited

Loading