[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6716: GEODE-9388: Implement Radish ZRANGEBYLEX Command

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

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



##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -636,4 +663,48 @@ public int getSizeInBytes() {
       return 0;
     }
   }
+
+  // Dummy entry used to find the rank of an element with the given member name for lexically
+  // ordered sets
+  static class MemberDummyOrderedSetEntry extends AbstractOrderedSetEntry {
+    boolean isExclusive;

Review comment:
       should these two boolean fields be final? It looks like they are never modfied

##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/AbstractSortedSetRangeOptions.java
##########
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.redis.internal.executor.sortedset;
+
+import static org.apache.geode.redis.internal.netty.Coder.bytesToLong;
+import static org.apache.geode.redis.internal.netty.Coder.equalsIgnoreCaseBytes;
+import static org.apache.geode.redis.internal.netty.Coder.narrowLongToInt;
+import static org.apache.geode.redis.internal.netty.StringBytesGlossary.bLIMIT;
+import static org.apache.geode.redis.internal.netty.StringBytesGlossary.bMINUS;
+
+import java.util.List;
+
+public abstract class AbstractSortedSetRangeOptions<T> {
+  boolean isMinExclusive;
+  T minimum;
+  boolean isMaxExclusive;
+  T maximum;
+  boolean hasLimit;
+  int offset;
+  int count;
+
+  AbstractSortedSetRangeOptions(byte[] minimumBytes, byte[] maximumBytes) {
+    parseMinimum(minimumBytes);
+    parseMaximum(maximumBytes);
+  }
+
+  void parseLimitArguments(List<byte[]> commandElements, int commandIndex) {
+    if (!equalsIgnoreCaseBytes(commandElements.get(commandIndex - 2), bLIMIT)) {
+      throw new IllegalArgumentException();
+    } else {
+      byte[] offsetBytes = commandElements.get(commandIndex - 1);
+      if (offsetBytes[0] == bMINUS) {
+        throw new NumberFormatException();
+      }
+      // Only set the values for the first limit we parse
+
+      int parsedOffset = narrowLongToInt(bytesToLong(offsetBytes));
+      int parsedCount = narrowLongToInt(bytesToLong(commandElements.get(commandIndex)));
+      if (!hasLimit) {
+        hasLimit = true;
+        this.offset = parsedOffset;
+        this.count = parsedCount;
+        if (this.count < 0) {

Review comment:
       it seems a little better to only set "this.count" once. So do the < 0 check against parsedCount

##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -443,6 +445,31 @@ long zrevrank(byte[] member) {
     return result;
   }
 
+  private List<byte[]> addLimitToRange(AbstractSortedSetRangeOptions<?> rangeOptions,
+      boolean withScores, int minIndex, int maxIndex) {
+    int count = Integer.MAX_VALUE;
+    if (rangeOptions.hasLimit()) {
+      count = rangeOptions.getCount();
+      minIndex += rangeOptions.getOffset();
+      if (minIndex > getSortedSetSize()) {
+        return Collections.emptyList();
+      }
+    }
+    Iterator<AbstractOrderedSetEntry> entryIterator =
+        scoreSet.getIndexRange(minIndex, Math.min(count, maxIndex - minIndex), false);
+
+    List<byte[]> result = new ArrayList<>();

Review comment:
       Can we give this ArrayList an initialize size to make this code perform better? It seems like it would be Math.min(count, maxIndex - minIndex) and *2 if withScores.

##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/StringBytesGlossary.java
##########
@@ -175,12 +175,13 @@
   @MakeImmutable
   public static final byte[] bABSTTL = stringToBytes("ABSTTL");
 
-  // AbstractZRangeExecutor
+  // Various ZRangeExecutors
   @MakeImmutable
   public static final byte[] bWITHSCORES = stringToBytes("WITHSCORES");
-
   @MakeImmutable
   public static final byte[] bLIMIT = stringToBytes("LIMIT");
+  public static final byte bPLUS = SIMPLE_STRING_ID; // +

Review comment:
       what is the deal with the "b" prefix? At one point I thought it meant the constant was a byte array but here it is just a byte. When should the prefix be used? Also why does it exist? I have not seen this pattern before in java.

##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZRangeByScoreExecutor.java
##########
@@ -33,35 +30,35 @@
 public class ZRangeByScoreExecutor extends AbstractExecutor {
   @Override
   public RedisResponse executeCommand(Command command, ExecutionHandlerContext context) {
-    RedisSortedSetCommands redisSortedSetCommands = context.getSortedSetCommands();
-
     List<byte[]> commandElements = command.getProcessedCommand();
 
-    SortedSetRangeOptions rangeOptions;
-    boolean withScores = false;
+    SortedSetScoreRangeOptions rangeOptions;
 
     try {
       byte[] minBytes = commandElements.get(2);
       byte[] maxBytes = commandElements.get(3);
-      rangeOptions = new SortedSetRangeOptions(minBytes, maxBytes);
+      rangeOptions = new SortedSetScoreRangeOptions(minBytes, maxBytes);
     } catch (NumberFormatException ex) {
       return RedisResponse.error(ERROR_MIN_MAX_NOT_A_FLOAT);
     }
 
     // Native redis allows multiple "withscores" and "limit ? ?" clauses; the last "limit"
     // clause overrides any previous ones
-    if (commandElements.size() >= 5) {
-      int currentCommandElement = 4;
+    // In order to match the error reporting behaviour of native Redis, the argument list must be
+    // parsed in reverse order. Stop parsing at index = 4, since 0 is the command name, 1 is the
+    // key, 2 is the min and 3 is the max
+    boolean withScores = false;
 
-      while (currentCommandElement < commandElements.size()) {
+    if (commandElements.size() >= 5) {

Review comment:
       This whole block of code is much the same as the one in ZRangeByLexExecutor. I think all you need is somehow to tell rangeOptions.parseLimitArguments that it should also parse the withScores flag. You could then ask rangeOptions if isWithScores is true.
   You could have the method that does this whole for loop in rangeOptions in a method that returns a RedisResponse. If it returns one then this code would return that response. Otherwise it would continue 

##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -636,4 +663,48 @@ public int getSizeInBytes() {
       return 0;
     }
   }
+
+  // Dummy entry used to find the rank of an element with the given member name for lexically
+  // ordered sets
+  static class MemberDummyOrderedSetEntry extends AbstractOrderedSetEntry {
+    boolean isExclusive;
+    boolean isMinimum;
+
+    MemberDummyOrderedSetEntry(byte[] member, double score, boolean isExclusive,
+        boolean isMinimum) {
+      this.member = member;
+      this.scoreBytes = null;
+      this.score = score;
+      this.isExclusive = isExclusive;
+      this.isMinimum = isMinimum;
+    }
+
+    @Override
+    public int compareMembers(byte[] array1, byte[] array2) {
+      int dummyNameComparison = checkDummyMemberNames(array1, array2);
+      if (dummyNameComparison != 0) {
+        return dummyNameComparison;
+      } else {
+        // If not using dummy member names, move on to actual lexical comparison
+        int stringComparison = javaImplementationOfAnsiCMemCmp(array1, array2);
+        if (stringComparison == 0) {
+          if (isMinimum && isExclusive || !isMinimum && !isExclusive) {

Review comment:
       you could use the XOR operator to simplify this to "if (isMinimum ^ isExclusive)". Note that if they are both true or both false then XOR returns false so you need to flip your return value (if (A^B) return -1 else return 1).
   I think I saw some other code recently in another PR of yours that could have used XOR.




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