GEODE-9379: Implement ZREVRANGEBYSCORE #6715
Conversation
ringles
requested review from
DonalEvans,
jdeppe-pivotal,
nonbinaryprogrammer and
upthewaterspout
as code owners
...compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
Outdated
Show resolved
Hide resolved
...n/java/org/apache/geode/redis/internal/executor/sortedset/AbstractZRangeByScoreExecutor.java
Outdated
Show resolved
Hide resolved
DonalEvans
left a comment
DonalEvans
left a comment
There was a problem hiding this comment.
This PR needs to add a test in AbstractHitsMissesIntegrationTest to confirm that stats are correctly updated. Also, I think it would be beneficial to wait for this PR to be merged and then rebase, since there's some code added there that would be useful to this PR.
.../apache/geode/redis/internal/executor/sortedset/AbstractZRevRangeByScoreIntegrationTest.java
Outdated
Show resolved
Hide resolved
.../apache/geode/redis/internal/executor/sortedset/AbstractZRevRangeByScoreIntegrationTest.java
Outdated
Show resolved
Hide resolved
.../apache/geode/redis/internal/executor/sortedset/AbstractZRevRangeByScoreIntegrationTest.java
Outdated
Show resolved
Hide resolved
.../apache/geode/redis/internal/executor/sortedset/AbstractZRevRangeByScoreIntegrationTest.java
Show resolved
Hide resolved
.../apache/geode/redis/internal/executor/sortedset/AbstractZRevRangeByScoreIntegrationTest.java
Outdated
Show resolved
Hide resolved
| return getRange(min, max, withScores, false); | ||
| } | ||
|
|
||
| List<byte[]> zrevrangebyscore(SortedSetRangeOptions rangeOptions, boolean withScores) { |
There was a problem hiding this comment.
This method should also be overridden in NullRedisSortedSet to return an empty list.
There was a problem hiding this comment.
Yup, caught that earlier today!
There was a problem hiding this comment.
Could this method also be moved down in the class to maintain the alphabetical ordering of the command methods? It should be between zrevrange() and zrevrank(). This same feedback applies to the methods in RedisSortedSetCommands.
...compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
Outdated
Show resolved
Hide resolved
...compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
Outdated
Show resolved
Hide resolved
...n/java/org/apache/geode/redis/internal/executor/sortedset/AbstractZRangeByScoreExecutor.java
Outdated
Show resolved
Hide resolved
...n/java/org/apache/geode/redis/internal/executor/sortedset/AbstractZRangeByScoreExecutor.java
Outdated
Show resolved
Hide resolved
DonalEvans
left a comment
DonalEvans
left a comment
There was a problem hiding this comment.
Looks good, with the understanding that it'll be rebased and modified to make use of new methods once the ZRANGEBYLEX PR has been merged. I'll give it another look over once that's done for final approval.
| return getRange(min, max, withScores, false); | ||
| } | ||
|
|
||
| List<byte[]> zrevrangebyscore(SortedSetRangeOptions rangeOptions, boolean withScores) { |
There was a problem hiding this comment.
Could this method also be moved down in the class to maintain the alphabetical ordering of the command methods? It should be between zrevrange() and zrevrank(). This same feedback applies to the methods in RedisSortedSetCommands.
| } | ||
|
|
||
| int startIndex = maxIndex - 1; | ||
| int endIndex = Math.min(count, maxIndex - minIndex); |
There was a problem hiding this comment.
This variable is actually the expected number of elements to be returned, not the index of the maximum element. The code found in the addLimitToRange() method added as part of the ZRANGEBYLEX PR could be used here to simplify things and reduce code reuse.
There was a problem hiding this comment.
Using addLimitToRange() now (with some tweaks to handle reverse ranges).
| long zcount(SortedSetScoreRangeOptions rangeOptions) { | ||
| AbstractOrderedSetEntry minEntry = new ScoreDummyOrderedSetEntry(rangeOptions.getMinimum(), | ||
| rangeOptions.isMinExclusive(), true); | ||
| AbstractOrderedSetEntry minEntry = new ScoreDummyOrderedSetEntry(rangeOptions.getStartRange(), |
There was a problem hiding this comment.
consider refactoring this code so it looks like:
long minIndex = getMinIndex(rangeOptions);
long maxIndex = getMaxIndex(rangeOptions);
I think we could use getMinIndex and getMaxIndex in a couple of places
There was a problem hiding this comment.
Didn't do it quite that way, but extracted a couple methods that indeed can be used in several places. Good catch; hope this is along the lines of your thinking...
|
|
||
| Iterator<AbstractOrderedSetEntry> entryIterator = | ||
| scoreSet.getIndexRange(startIndex, endIndex, true); | ||
| List<byte[]> result = new ArrayList<>(initialCapacity > 0 ? initialCapacity : 0); |
There was a problem hiding this comment.
if initialCapacity is <= 0 does that mean we will return an empty list? If so should we just have checked (up at line 414) if (initialCapacity <= 0) {return Collections.emptyList();}? Then this could just be new ArrayList<>(initialCapacity)
There was a problem hiding this comment.
Yeah, per Donal we're using the addLimitToRange (with some tweaks now).
| return (hasLimit && (count == 0 || offset < 0)) || minVsMax == 1 | ||
| || (minVsMax == 0 && (isMinExclusive || isMaxExclusive)); | ||
| boolean isEmptyRange(boolean isRev) { | ||
| int startVsEnd = compareStartToEnd(); |
There was a problem hiding this comment.
Much is now repeated between the if and then else.
I think this code would be clearer if you didn't try to do everything in one boolean expression. For example:
if (hasLimit && (count == 0 || offset < 0)) {
return true;
}
int startVsEnd = compareStartToEnd();
if (startVsEnd == 0 && (isStartExclusive || isEndExclusive)) {
return true;
}
if (isRev) {
return startVsEnd == -1;
} else {
return startVsEnd == 1;
}
There was a problem hiding this comment.
Adjusted the code, and the leading comment to match the test order.
ringles
dismissed
DonalEvans’s stale review
Changes addressed but not available to validate.
5 participants
Add support for the ZREVRANGEBYSCORE Redis comand.