[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6726: GEODE-9185: Add ZPOPMAX Radish command

[GitBox <gi...@apache.org>]

dschneider-pivotal commented on a change in pull request #6726:
URL: https://github.com/apache/geode/pull/6726#discussion_r680063808



##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -395,6 +395,31 @@ long zrevrank(byte[] member) {
     return null;
   }
 
+  List<byte[]> zpopmax(Region<RedisKey, RedisData> region, RedisKey key, int count) {
+    Iterator<AbstractOrderedSetEntry> scoresIterator =
+        scoreSet.getIndexRange(scoreSet.size() - 1, count, true);
+    List<byte[]> result = new ArrayList<>();
+
+    if (!scoresIterator.hasNext()) {
+      return result;

Review comment:
       could the canonical empty list be returned here?

##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -395,6 +395,31 @@ long zrevrank(byte[] member) {
     return null;
   }
 
+  List<byte[]> zpopmax(Region<RedisKey, RedisData> region, RedisKey key, int count) {
+    Iterator<AbstractOrderedSetEntry> scoresIterator =
+        scoreSet.getIndexRange(scoreSet.size() - 1, count, true);
+    List<byte[]> result = new ArrayList<>();

Review comment:
       if you return the canonical empty list then you could delay the ArrayList allocation until after the empty check. Can you use the arguments passed to getIndexRange to compute a better initial size of this ArrayList?




-- 
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: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org