dschneider-pivotal commented on a change in pull request #6715:
URL: https://github.com/apache/geode/pull/6715#discussion_r686200170
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -272,12 +272,12 @@ long zcard() {
}
long zcount(SortedSetScoreRangeOptions rangeOptions) {
- AbstractOrderedSetEntry minEntry = new
ScoreDummyOrderedSetEntry(rangeOptions.getMinimum(),
- rangeOptions.isMinExclusive(), true);
+ AbstractOrderedSetEntry minEntry = new
ScoreDummyOrderedSetEntry(rangeOptions.getStartRange(),
Review 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
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -381,6 +381,55 @@ long zrem(Region<RedisKey, RedisData> region, RedisKey
key, List<byte[]> members
return getRange(min, max, withScores, true);
}
+ List<byte[]> zrevrangebyscore(SortedSetScoreRangeOptions rangeOptions,
boolean withScores) {
+ AbstractOrderedSetEntry maxEntry =
+ new ScoreDummyOrderedSetEntry(rangeOptions.getStartRange(),
rangeOptions.isStartExclusive(),
+ false);
+ int maxIndex = scoreSet.indexOf(maxEntry);
+
+ AbstractOrderedSetEntry minEntry =
+ new ScoreDummyOrderedSetEntry(rangeOptions.getEndRange(),
rangeOptions.isEndExclusive(),
+ true);
+ int minIndex = scoreSet.indexOf(minEntry);
+ if (minIndex > getSortedSetSize()) {
+ return Collections.emptyList();
+ }
+
+ if (minIndex == maxIndex) {
+ return Collections.emptyList();
+ }
+
+ // Okay, if we make it this far there's a potential range of things to
return.
+ int count = Integer.MAX_VALUE;
+ if (rangeOptions.hasLimit()) {
+ count = rangeOptions.getCount();
+ maxIndex -= rangeOptions.getOffset();
+ if (maxIndex < 0) {
+ return Collections.emptyList();
+ }
+ }
+
+ int startIndex = maxIndex - 1;
+ int endIndex = Math.min(count, maxIndex - minIndex);
+ int initialCapacity = startIndex - endIndex;
+ if (withScores) {
+ initialCapacity *= 2;
+ }
+
+ Iterator<AbstractOrderedSetEntry> entryIterator =
+ scoreSet.getIndexRange(startIndex, endIndex, true);
+ List<byte[]> result = new ArrayList<>(initialCapacity > 0 ?
initialCapacity : 0);
Review 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)
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/AbstractSortedSetRangeOptions.java
##########
@@ -66,26 +66,31 @@ void parseLimitArguments(List<byte[]> commandElements, int
commandIndex) {
// If limit specified but count is zero, or min > max, or min == max and
either are exclusive, the
// range cannot contain any elements
- boolean isEmptyRange() {
- int minVsMax = compareMinToMax();
- return (hasLimit && (count == 0 || offset < 0)) || minVsMax == 1
- || (minVsMax == 0 && (isMinExclusive || isMaxExclusive));
+ boolean isEmptyRange(boolean isRev) {
+ int startVsEnd = compareStartToEnd();
Review 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;
}
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
