Skip to content

remove COMMAND GETKEYS call from ZUNION and ZUNIONSTORE #4

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

Merged
merged 2 commits into from
Jul 4, 2023
Merged

remove COMMAND GETKEYS call from ZUNION and ZUNIONSTORE #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b011b6b
make EVALSHA and EVALSHA_RO to avoid command_keys as well
zach-iee Jun 30, 2023
4b228cf
make ZUNION, ZUNIONSTORE to avoid command_keys as well
zach-iee Jun 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion redis/cluster.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -957,7 +957,7 @@ def determine_slot(self, *args):
# redis server to parse the keys. Besides, there is a bug in redis<7.0
# where `self._get_command_keys()` fails anyway. So, we special case
# EVAL/EVALSHA.
if command in ("EVAL", "EVALSHA"):
if command in ("EVAL", "EVALSHA", "EVAL_RO", "EVALSHA_RO"):
# command syntax: EVAL "script body" num_keys ...
if len(args) <= 2:
raise RedisClusterException(f"Invalid args in command: {args}")
Expand All @@ -968,6 +968,14 @@ def determine_slot(self, *args):
if len(eval_keys) == 0:
return random.randrange(0, REDIS_CLUSTER_HASH_SLOTS)
keys = eval_keys
elif command == "ZUNIONSTORE":
Copy link

Choose a reason for hiding this comment

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

georadius
georadiusbymember
migrate
sort
sort_ro
zdiffstore
zinterstore

shouldn't we raise an exception for above commands?

Copy link

Choose a reason for hiding this comment

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

I assume that we are not using those commands, but just in case.

Copy link
Author

Choose a reason for hiding this comment

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

yeap, i'm trying to block those commands in soda code, let me write the PR link once it's ready

Copy link

Choose a reason for hiding this comment

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

got it, please be careful that we should block direct call like execute_command(sort)

Copy link
Author

Choose a reason for hiding this comment

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

https://github.com/sendbird/soda/pull/16246

# https://github.com/redis/redis/pull/12380
# command syntax: ZUNIONSTORE destination numkeys key [key ...]
# [WEIGHTS weight [weight ...]] [AGGREGATE <SUM | MIN | MAX>]
if len(args) <= 3:
raise RedisClusterException(f"Invalid args in ZUNIONSTORE: {args}")
num_actual_keys = args[2]
keys = (args[1],) + args[3 : 3 + num_actual_keys]
else:
keys = self._get_command_keys(*args)
if keys is None or len(keys) == 0:
Expand Down
20 changes: 20 additions & 0 deletions tests/test_cluster.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2006,10 +2006,30 @@ def test_cluster_zunion(self, r):
]

def test_cluster_zunionstore_sum(self, r):
assert r.zunionstore("{foo}d", ["{foo}" + str(i) for i in range(0, 256)]) == 0

r.zadd("{foo}a", {"a1": 1, "a2": 1, "a3": 1})
r.zadd("{foo}b", {"a1": 2, "a2": 2, "a3": 2})
r.zadd("{foo}c", {"a1": 6, "a3": 5, "a4": 4})

result_key = "{foo}d"
failed_keys = ["{foo1}a", "{foo}b", "{foo}c"]
with pytest.raises(
RedisClusterException,
match="ZUNIONSTORE - all keys must map to the same key slot",
):
r.zunionstore(result_key, failed_keys)

result_key = "{foo1}d"
failed_keys = ["{foo}a", "{foo}b"]
with pytest.raises(
RedisClusterException,
match="ZUNIONSTORE - all keys must map to the same key slot",
):
r.zunionstore(result_key, failed_keys)

assert r.zunionstore("{foo}d", ["{foo}a", "{foo}b", "{foo}c"]) == 4
assert r.zunionstore("{foo}e", ["{foo}a"]) == 3
assert r.zrange("{foo}d", 0, -1, withscores=True) == [
(b"a2", 3),
(b"a4", 4),
Expand Down