[GitHub] [geode] DonalEvans commented on a change in pull request #7389: GEODE-9950: Add LRANGE command

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

DonalEvans commented on a change in pull request #7389:
URL: https://github.com/apache/geode/pull/7389#discussion_r816201891



##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/list/LRangeExecutor.java
##########
@@ -0,0 +1,48 @@
+/*
+ * 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.commands.executor.list;
+
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_NOT_INTEGER;
+import static org.apache.geode.redis.internal.netty.Coder.bytesToLong;
+import static org.apache.geode.redis.internal.netty.Coder.narrowLongToInt;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.commands.Command;
+import org.apache.geode.redis.internal.commands.executor.CommandExecutor;
+import org.apache.geode.redis.internal.commands.executor.RedisResponse;
+import org.apache.geode.redis.internal.data.RedisKey;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class LRangeExecutor implements CommandExecutor {
+  @Override
+  public RedisResponse executeCommand(Command command, ExecutionHandlerContext context) {
+    List<byte[]> commandElems = command.getProcessedCommand();
+    RedisKey key = command.getKey();

Review comment:
       Very small nitpick, but it's a tiny bit more efficient to move this line to after the start and stop indexes are retrieved, as it's possible that we error out before ever needing to use the key. Definitely not a required change though.

##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLRangeIntegrationTest.java
##########
@@ -0,0 +1,274 @@
+/*
+ * 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.commands.executor.list;
+
+import static org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertExactNumberOfArgs;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_NOT_INTEGER;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_WRONG_TYPE;
+import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.BIND_ADDRESS;
+import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.REDIS_CLIENT_TIMEOUT;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicReference;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.Protocol;
+
+import org.apache.geode.redis.ConcurrentLoopingThreads;
+import org.apache.geode.redis.RedisIntegrationTest;
+
+public abstract class AbstractLRangeIntegrationTest implements RedisIntegrationTest {
+  private static final String NON_EXISTENT_LIST_KEY = "{tag1}nonExistentKey";
+  private static final String LIST_KEY = "{tag1}listKey";
+  private static final String[] LIST_ELEMENTS =
+      {"aardvark", "bats", "chameleon", "deer", "elephant", "flamingo", "goat"};
+  private static final String[] LIST_ELEMENTS_REVERSE =
+      {"goat", "flamingo", "elephant", "deer", "chameleon", "bats", "aardvark"};
+  private static final int LAST_INDEX = LIST_ELEMENTS.length;
+
+  private JedisCluster jedis;
+
+  @Before
+  public void setUp() {
+    jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, getPort()), REDIS_CLIENT_TIMEOUT);
+  }
+
+  @After
+  public void tearDown() {
+    flushAll();
+    jedis.close();
+  }
+
+  @Test
+  public void lrange_wrongNumberOfArgs_returnsError() {
+    assertExactNumberOfArgs(jedis, Protocol.Command.LRANGE, 3);
+  }
+
+  @Test
+  public void lrange_withNonExistentList_withStartIndexLessThanStopIndex_returnsEmptyList() {
+    assertThat(jedis.lrange(NON_EXISTENT_LIST_KEY, -10, 10)).isEmpty();
+  }
+
+  @Test
+  public void lrange_withNonExistentList_withStartIndexGreaterThanStopIndex_returnsEmptyList() {
+    assertThat(jedis.lrange(NON_EXISTENT_LIST_KEY, 10, -10)).isEmpty();
+  }
+
+  @Test
+  public void lrange_withPositiveStartIndex_withPositiveStopIndex_returnsEmptyList() {

Review comment:
       A lot of the tests in this class are very similar, differing only in the specific values of start and stop they use. As a result, the names of the tests can get a bit imprecise and possibly misleading, such as this one, since we don't expect that LRANGE will return an empty list in ALL cases when start is positive and stop is positive.
   
   A better approach (that would reduce the amount of test code significantly) might be to follow the pattern in `AbstractZRevRangeIntegrationTest.shouldReturnEmptyCollection_givenInvalidRange()` and `AbstractZRevRangeIntegrationTest.zrevrange_withValidRanges()`, using "valid" and "invalid" ranges and parameterizing the tests. This would allow the test names to specify the range they're using exactly, rather than having to use imprecise descriptions like "positive" or "negative" which may not capture the behaviour we're actually interested in testing.

##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java
##########
@@ -38,28 +42,88 @@
   protected static final int REDIS_LIST_OVERHEAD = memoryOverhead(RedisList.class);
   private final SizeableByteArrayList elementList;
 
+  private static final int INVALID_INDEX = -1;
+
   public RedisList() {
     this.elementList = new SizeableByteArrayList();
   }
 
+  /**
+   * @param start start index of desired elments
+   * @param stop stop index of desired elments

Review comment:
       Typos here, should be "elements"

##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java
##########
@@ -38,28 +42,88 @@
   protected static final int REDIS_LIST_OVERHEAD = memoryOverhead(RedisList.class);
   private final SizeableByteArrayList elementList;
 
+  private static final int INVALID_INDEX = -1;
+
   public RedisList() {
     this.elementList = new SizeableByteArrayList();
   }
 
+  /**
+   * @param start start index of desired elments
+   * @param stop stop index of desired elments
+   * @return list of elements in the range (inclusive).
+   */
+  public List<byte[]> lrange(int start, int stop) {
+    start = normalizeStartIndex(start);
+    stop = normalizeStopIndex(stop);
+
+    int elementSize = elementList.size();
+    if (start > stop || elementSize <= start) {
+      return Collections.emptyList();
+    }
+
+    int resultLength = stop - start + 1;
+
+    // Finds the shortest distance to access nodes in range
+    if (start <= elementSize - stop - 1) {
+      // Starts at head to access nodes at start index then iterates forwards
+      List<byte[]> result = new ArrayList<>(resultLength);
+      ListIterator<byte[]> iterator = elementList.listIterator(start);
+
+      for (int i = start; i <= stop; i++) {
+        byte[] element = iterator.next();
+        result.add(element);
+      }
+      return result;
+
+    } else {
+      // Starts at tail to access nodes at stop index then iterates backwards
+      byte[][] result = new byte[resultLength][];
+      ListIterator<byte[]> iterator = elementList.listIterator(stop + 1);
+
+      for (int i = resultLength - 1; i >= 0; i--) {
+        byte[] element = iterator.previous();
+        result[i] = element;
+      }
+      return Arrays.asList(result);
+    }
+  }
+
   /**
    * @param index index of desired element. Positive index starts at the head. Negative index starts
    *        at the tail.
    * @return element at index. Null if index is out of range.
    */
   public byte[] lindex(int index) {
-    if (index < 0) {
-      // Changes negative index to corresponding positive index to utilize get(int index)
-      index = elementList.size() + index;
-    }
+    index = getArrayIndex(index);
 
-    if (index < 0 || elementList.size() <= index) {
+    if (index == INVALID_INDEX || elementList.size() <= index) {
       return null;
     } else {
       return elementList.get(index);
     }
   }
 
+  private int normalizeStartIndex(int startIndex) {
+    return Math.max(0, getArrayIndex(startIndex));
+  }
+
+  private int normalizeStopIndex(int stopIndex) {
+    return Math.min(elementList.size() - 1, getArrayIndex(stopIndex));
+  }
+
+  /**
+   * Changes negative index to corresponding positive index.
+   * The index will still be negative if there is no corresponding positive index.
+   */
+  private int getArrayIndex(int listIndex) {
+    if (listIndex < 0) {
+      listIndex = elementList.size() + listIndex;
+      listIndex = Math.max(listIndex, INVALID_INDEX);
+    }
+    return listIndex;

Review comment:
       This might be slightly clearer as:
   ```
       if (listIndex < 0) {
         listIndex = elementList.size() + listIndex;
         if (listIndex < 0) {
           return INVALID_INDEX;
         }
       }
       return listIndex;
   ```
   Also, the comment on the method could be changed to say something like "Returns INVALID_INDEX if there is no corresponding positive index" to make it more specific.




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