[vector sets] VRANGE implementation #14235
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
|
New Issues (7)Checkmarx found the following issues in this Pull Request
Fixed Issues (4)Great job! The following issues were fixed in this Pull Request
|
GuyAv46
reviewed
Aug 5, 2025
Comment on lines
+1876
to
+1877
| void *data; | ||
| while ((key_data = RedisModule_DictNextC(iter, &key_len, &data)) != NULL) { |
There was a problem hiding this comment.
Not using data, and it's an optional parameter of the API. Consider removing
Suggested change
| void *data; | |
| while ((key_data = RedisModule_DictNextC(iter, &key_len, &data)) != NULL) { | |
| while ((key_data = RedisModule_DictNextC(iter, &key_len, NULL)) != NULL) { |
| result = self.redis.execute_command('VRANGE', self.test_key, '(apple', '[cherry', '10') | ||
| result = [r.decode() for r in result] | ||
| assert 'apple' not in result, "Exclusive boundary should not include 'apple'" | ||
| assert 'apricot' in result and 'banana' in result and 'cherry' in result, "Should include elements after apple up to cherry" |
There was a problem hiding this comment.
Consider asserting the full reply
| result = self.redis.execute_command('VRANGE', self.test_key, '[banana', '(cherry', '10') | ||
| result = [r.decode() for r in result] | ||
| assert 'banana' in result, "Should include banana" | ||
| assert 'cherry' not in result, "Exclusive end should not include cherry" |
There was a problem hiding this comment.
Consider asserting the full reply.
The same comment applies to the cases listed below.
|
|
||
| # Test 8: Count of 0 returns empty array | ||
| result = self.redis.execute_command('VRANGE', self.test_key, '-', '+', '0') | ||
| assert len(result) == 0, "Count of 0 should return empty array" |
There was a problem hiding this comment.
Consider asserting the returned value, and not its size. If something breaks, we won't know what we have got.
Suggested change
| assert len(result) == 0, "Count of 0 should return empty array" | |
| assert result == [], "Count of 0 should return empty array" |
Same comment for the other cases below
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is basically the Vector Set iteration primitive.
It exploits the underlying radix tree implementation.
The usage pattern is strongly reminiscent of other Redis commands doing similar things.
The command usage is straightforward:
The above command returns 10 (or less, if less are available in the specified range) elements from "Redis" (inclusive) to the maximum possible element. The comparison is performed byte by byte, as
memcmp()would do, in this way the elements have a total order. The start and end range can be either a string, prefixedby
[or((the prefix is mandatory) to tell the command if the range is inclusive or exclusive, or can be the special symbols-and+that means the maximum and minimum element.More info can be found in the implementation itself and in the README file change.