Is that the proper way to test if a client is using resp3? Should we use HELLO command instead?

try:
    hello_info = r.execute_command('HELLO')
    # Check the 'proto' field in the response
    if isinstance(hello_info, dict):  # RESP3 returns a dict
        print(f"Current protocol version: {hello_info.get('proto')}")
        print("Connection is using RESP3")
    else:  # RESP2 returns an array
        # Find the 'proto' value in the array
        for i in range(0, len(hello_info), 2):
            if hello_info[i] == b'proto':
                print(f"Current protocol version: {hello_info[i+1]}")
                print("Connection is using RESP2")
                break
except Exception as e:
    print(f"Error checking protocol: {e}")

Yes, what you suggested is better. But perhaps the Python Redis object has an explicit resp3 field actually, I need to check if it is exported.

Even better: I believe the test should create both RESP2 and RESP3 connections at startup, so that the tests can decide what to use and test the behavior in both sides. No test will be needed, we just use what we want based on the protocol to test, in the case when (like this one) there are divergences in the behavior.

Test updated creating a RESP3 connection and testing the behavior separately for RESP2 / RESP3.

Should these tests be part of CI? Or only here to show the expected behavior?

The whole Python test of Vector Sets should be part of the CI, but its integration is pending right now. Somebody will do it in the next future apparently.

Isn't it supposed to be a map rather than an array, based on the comment above? Also, why do it only for resp3 and not for resp2 as well?

For RESP3, we create a map that is: map -> attribute_or_score if just one was selected, or map -> [score,attrib] if multiple will be selected. Basically the behavior will be that if just one extra information other than the element is requested, we return a simple map, otherwise we return a map mapping to an array. This is backward compatible, produces less protocol and so forth, but changes the reply format in case multiple additional items are requested. It is a tradeoff. The alternative would be that the case case would return item -> [score], a single item array, which is a bit odd.

Got it, thanks

Why testing if resp3 here? The outer map was opened either way, no?

Because for RESP3, we always return a map, and for RESP3 we always return a flat array of item, score, attrib, ..., so the two code paths have different logic. In Redis you are not used to see this explicit check because we don't have any other data type that has multiple attributes attached to the same set-alike element. For instance with sorted sets we just have the score.

I see. In that case, the response should start with the same logic, right? Currently we have

    /* Return results */
    if (withscores)
        RedisModule_ReplyWithMap(ctx, REDISMODULE_POSTPONED_LEN);
    else
        RedisModule_ReplyWithArray(ctx, REDISMODULE_POSTPONED_LEN);

but I believe it should be:

    /* Return results */
    if (resp3 && (withscores || withattribs))
        RedisModule_ReplyWithMap(ctx, REDISMODULE_POSTPONED_LEN);
    else
        RedisModule_ReplyWithArray(ctx, REDISMODULE_POSTPONED_LEN);

@alonre24 I think you are right, thanks, probably it was impossible to catch this with tests, since I believe that the functions were refactored at some point into moduleReplyWithCollection(), so now basically if the reply is postponed, calling one or the other is exactly the same, and later the calls that set the length are the correct ones, so it works regardless. Verifying and pushing the fix.

@alonre24 fix pushed. Indeed right now RedisModule_ReplyWithMap() or RedisModule_ReplyWithArray() are equivalent if the reply is postponed, so everything worked but was conceptually wrong anyway. Fixed and refactored to make it more explicit.

I see, it makes sense.
I Approved the PR

hi @antirez does it belong to VSIM not VLINKS?