[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6680: GEODE-9182: Implement Radish ZCOUNT command

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

jdeppe-pivotal commented on a change in pull request #6680:
URL: https://github.com/apache/geode/pull/6680#discussion_r666603660



##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -259,6 +262,18 @@ long zcard() {
     return members.size();
   }
 
+  long zcount(SortedSetRangeOptions rangeOptions) {
+    OrderedSetEntry minEntry =
+        new OrderedSetEntry(rangeOptions.getMinDouble(), rangeOptions.isMinExclusive(), true);
+    long minRank = scoreSet.indexOf(minEntry);
+
+    OrderedSetEntry maxEntry =
+        new OrderedSetEntry(rangeOptions.getMaxDouble(), rangeOptions.isMaxExclusive(), false);
+    long maxRank = scoreSet.indexOf(maxEntry);
+
+    return maxRank - minRank;

Review comment:
       Very minor nit - please would you rename `[min|max]Rank` to `[min|max]Index`

##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -414,6 +429,28 @@ private static double processByteArrayAsDouble(byte[] value) {
     }
   }
 
+  // Comparison to allow the use of bLEAST_MEMBER_NAME and bGREATEST_MEMBER_NAME to always be less
+  // than or greater than respectively any other byte array representing a member name. The use of
+  // "==" is important as it allows us to differentiate between the constant byte arrays defined
+  // in StringBytesGlossary and user-supplied member names which may be equal in content but have
+  // a different memory address.
+  private static int checkDummyMemberNames(byte[] array1, byte[] array2) {

Review comment:
       I'm a little concerned that this is on a hot path and that the primary code path will require traversing all the conditionals. One alternative I can think of is to have these special sentinels try and actually be min/max values - `byte[] {-128, -128, -128, ...}` and `byte[] {127, 127, 127, ...}` for some largish sized byte array. That way the comparisons can just happen naturally. I realize though that in an extreme corner case a user may actually try and add such a value. I'm OK leaving it like this for now, but at some point I'd like to satisfy myself on the performance aspects.

##########
File path: geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/collections/OrderStatisticsTreeTest.java
##########
@@ -193,16 +194,19 @@ public void testSize() {
 
   @Test
   public void testIndexOf() {
-    for (int i = 100; i < 200; ++i) {
-      assertThat(tree.add(i)).isTrue();
+    int stepSize = 10;
+    int entries = 20;
+    // All entries are multiple of 10, from 0 to 190
+    for (int i = 0; i < entries; ++i) {
+      assertThat(tree.add(i * stepSize)).isTrue();
     }
 
-    for (int i = 100; i < 200; ++i) {
-      assertThat(tree.indexOf(i)).isEqualTo(i - 100);
-    }
+    // Test values smaller than the lowest entry and greater than the largest entry
+    assertThat(tree.indexOf(-1)).isEqualTo(0);
+    assertThat(tree.indexOf(1 + entries * stepSize)).isEqualTo(tree.size());

Review comment:
       The `1 +` is unnecessary here.




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