[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6701: GEODE-9429: Allow Radish HSCAN to handle all valid values for COUNT

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

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



##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -237,35 +237,32 @@ public int hstrlen(byte[] field) {
     return new ArrayList<>(hash.keySet());
   }
 
-  public ImmutablePair<Integer, List<byte[]>> hscan(Pattern matchPattern,
-      int count,
-      int cursor) {
-
-    ArrayList<byte[]> resultList = new ArrayList<>(count + 2);
+  public ImmutablePair<Integer, List<ImmutablePair<byte[], byte[]>>> hscan(Pattern matchPattern,
+      int count, int cursor) {
+    // No need to allocate more space than it's possible to use given the size of the hash
+    int initialCapacity = Math.min(count, hash.size());
+    List<ImmutablePair<byte[], byte[]>> resultList = new ArrayList<>(initialCapacity);
     do {
       cursor = hash.scan(cursor, 1,
           (list, key, value) -> addIfMatching(matchPattern, list, key, value), resultList);
-    } while (cursor != 0 && resultList.size() < (count * 2));
+    } while (cursor != 0 && resultList.size() < count);
 
     return new ImmutablePair<>(cursor, resultList);
   }
 
-  private void addIfMatching(Pattern matchPattern, List<byte[]> resultList, byte[] key,
-      byte[] value) {
+  private void addIfMatching(Pattern matchPattern, List<ImmutablePair<byte[], byte[]>> resultList,
+      byte[] key, byte[] value) {
     if (matchPattern != null) {
       if (matchPattern.matcher(bytesToString(key)).matches()) {
-        resultList.add(key);
-        resultList.add(value);
+        resultList.add(new ImmutablePair<>(key, value));

Review comment:
       Conceptually I like pairing the key and value here, but it is an additional memory allocation that doesn't add any functional value AFAICT.

##########
File path: geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/hash/AbstractHScanIntegrationTest.java
##########
@@ -577,16 +569,44 @@ private void multipleHScanAndAssertOnSizeOfResultSet(Jedis jedis,
         .isEqualTo(initialHashData.size());
   }
 
-  private Map<String, String> makeEntrySet(int sizeOfDataSet) {
+  private Map<String, String> initializeThreeFieldHash() {
+    Map<String, String> entryMap = new HashMap<>();
+    entryMap.put(FIELD_ONE, VALUE_ONE);
+    entryMap.put(FIELD_TWO, VALUE_TWO);
+    entryMap.put(FIELD_THREE, VALUE_THREE);
+    jedis.hmset(HASH_KEY, entryMap);
+    return entryMap;
+  }
+
+  Map<byte[], byte[]> initializeThreeFieldHashBytes() {
+    Map<byte[], byte[]> entryMap = new HashMap<>();
+    entryMap.put(FIELD_ONE.getBytes(), VALUE_ONE.getBytes());
+    entryMap.put(FIELD_TWO.getBytes(), VALUE_TWO.getBytes());
+    entryMap.put(FIELD_THREE_BYTES, VALUE_THREE.getBytes());
+    jedis.hmset(HASH_KEY.getBytes(), entryMap);
+    return entryMap;
+  }
+
+  private Map<String, String> makeEntryMap() {
     Map<String, String> dataSet = new HashMap<>();
-    for (int i = 0; i < sizeOfDataSet; i++) {
+    for (int i = 0; i < SIZE_OF_ENTRY_MAP; i++) {
       dataSet.put(BASE_FIELD + i, "value_" + i);
     }
     return dataSet;
   }
 
-  private int getSlotForKey(String key) {
-    int crc = CRC16.calculate(key);
+  private int getSlotForKey() {

Review comment:
       The slot could just be a static field now.




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