[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #7228: feature/GEODE-9834: SRANDMEMBER Command Support

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

nonbinaryprogrammer commented on a change in pull request #7228:
URL: https://github.com/apache/geode/pull/7228#discussion_r780458016



##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -202,48 +202,50 @@ static int setOpStoreResult(RegionProvider regionProvider, RedisKey destinationK
     return popped;
   }
 
-  public Collection<byte[]> srandmember(int count) {
-    int membersSize = members.size();
-    boolean duplicatesAllowed = count < 0;
-    if (duplicatesAllowed) {
-      count = -count;
-    }
+  public List<byte[]> srandmember(int count) {
+    boolean uniqueNumberList = count > 0;
+    count = uniqueNumberList ? count : -count;
+    int memberMapSize = members.getMemberMapSize();
 
-    if (!duplicatesAllowed && membersSize <= count && count != 1) {
-      return new ArrayList<>(members);
+    // TODO: Optomize algorithm
+    List<byte[]> result = new ArrayList<>(count);
+    if (uniqueNumberList) {
+      if (count >= members.size()) {
+        result.addAll(members);
+      } else {
+        srandomUniqueList(count, memberMapSize, result);
+      }
+    } else {
+      srandomDuplicateList(count, memberMapSize, result);
     }
+    return result;
+  }
 
-    Random rand = new Random();
+  private void srandomDuplicateList(int count, int memberMapSize, List<byte[]> result) {
+    while (result.size() != count) {
+      int randIndex = rand.nextInt(memberMapSize);
+      byte[] member = members.getKeyAtIndex(randIndex);
 
-    // TODO: this could be optimized to take advantage of MemberSet
-    // storing its data in an array. We probably don't need to copy it
-    // into another array here.
-    byte[][] entries = members.toArray(new byte[membersSize][]);
-
-    if (count == 1) {
-      byte[] randEntry = entries[rand.nextInt(entries.length)];
-      // TODO: Now that the result is no longer serialized this could use singleton.
-      // Note using ArrayList because Collections.singleton has serialization issues.
-      List<byte[]> result = new ArrayList<>(1);
-      result.add(randEntry);
-      return result;
-    }
-    if (duplicatesAllowed) {
-      List<byte[]> result = new ArrayList<>(count);
-      while (count > 0) {
-        result.add(entries[rand.nextInt(entries.length)]);
-        count--;
+      if (member != null) {
+        result.add(member);
       }
-      return result;
-    } else {
-      Set<byte[]> result = new MemberSet(count);
-      // Note that rand.nextInt can return duplicates when "count" is high
-      // so we need to use a Set to collect the results.
-      while (result.size() < count) {
-        byte[] s = entries[rand.nextInt(entries.length)];
-        result.add(s);
+    }
+  }
+
+  private void srandomUniqueList(int count, int memberMapSize, List<byte[]> result) {
+    List<Integer> allIndexes = new ArrayList<>(memberMapSize);
+    for (int i = 0; i < memberMapSize; i++) {
+      allIndexes.add(i);
+    }
+    Collections.shuffle(allIndexes);
+    int i = 0;
+    while (result.size() != count) {
+      byte[] member = members.getKeyAtIndex(i);

Review comment:
       why not just call `getKey(pos)` directly?

##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSetsIntegrationTest.java
##########
@@ -98,67 +97,6 @@ public void testSAdd_canStoreBinaryData() {
     assertThat(result).containsExactly(blob);
   }
 
-  @Test
-  public void srandmember_withStringFails() {
-    jedis.set("string", "value");
-    assertThatThrownBy(() -> jedis.srandmember("string")).hasMessageContaining("WRONGTYPE");
-  }
-
-  @Test
-  public void srandmember_withNonExistentKeyReturnsNull() {
-    assertThat(jedis.srandmember("non existent")).isNull();
-  }
-
-  @Test
-  public void srandmemberCount_withNonExistentKeyReturnsEmptyArray() {
-    assertThat(jedis.srandmember("non existent", 3)).isEmpty();
-  }
-
-  @Test
-  public void srandmember_returnsOneMember() {
-    jedis.sadd("key", "m1", "m2");
-    String result = jedis.srandmember("key");
-    assertThat(result).isIn("m1", "m2");
-  }
-
-  @Test
-  public void srandmemberCount_returnsTwoUniqueMembers() {
-    jedis.sadd("key", "m1", "m2", "m3");
-    List<String> results = jedis.srandmember("key", 2);
-    assertThat(results).hasSize(2);
-    assertThat(results).containsAnyOf("m1", "m2", "m3");
-    assertThat(results.get(0)).isNotEqualTo(results.get(1));
-  }
-
-  @Test
-  public void srandmemberNegativeCount_returnsThreeMembers() {
-    jedis.sadd("key", "m1", "m2", "m3");
-    List<String> results = jedis.srandmember("key", -3);
-    assertThat(results).hasSize(3);
-    assertThat(results).containsAnyOf("m1", "m2", "m3");
-  }
-
-  @Test
-  public void srandmemberNegativeCount_givenSmallSet_returnsThreeMembers() {
-    jedis.sadd("key", "m1");
-    List<String> results = jedis.srandmember("key", -3);
-    assertThat(results).hasSize(3);
-    assertThat(results).containsAnyOf("m1");
-  }
-
-  @Test
-  public void smembers_givenKeyNotProvided_returnsWrongNumberOfArgumentsError() {
-    assertThatThrownBy(() -> jedis.sendCommand("key", Protocol.Command.SMEMBERS))
-        .hasMessageContaining("ERR wrong number of arguments for 'smembers' command");
-  }
-
-  @Test
-  public void smembers_givenMoreThanTwoArguments_returnsWrongNumberOfArgumentsError() {
-    assertThatThrownBy(() -> jedis
-        .sendCommand("key", Protocol.Command.SMEMBERS, "key", "extraArg"))
-            .hasMessageContaining("ERR wrong number of arguments for 'smembers' command");
-  }

Review comment:
       did you mean to delete these smembers ones too?




-- 
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