You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2022/01/05 01:15:37 UTC
[GitHub] [geode] DonalEvans commented on a change in pull request #7236: GEODE-9829: Add SINTER command to Redis supported commands.
DonalEvans commented on a change in pull request #7236:
URL: https://github.com/apache/geode/pull/7236#discussion_r778479474
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -110,6 +111,47 @@ private static MemberSet calculateDiff(RegionProvider regionProvider, List<Redis
return diff;
}
+ public static Set<byte[]> sinter(RegionProvider regionProvider, List<RedisKey> keys) {
+ MemberSet result = calculateInter(regionProvider, keys);
+ if (result == null) {
+ return Collections.emptySet();
+ }
+ return result;
+ }
+
+ private static MemberSet calculateInter(RegionProvider regionProvider, List<RedisKey> keys) {
+ List<RedisSet> sets = new ArrayList<>(keys.size());
+ RedisSet smallestSet = null;
+
+ for (RedisKey key : keys) {
+ RedisSet redisSet = regionProvider.getTypedRedisData(REDIS_SET, key, true);
+ if (redisSet == NULL_REDIS_SET || redisSet.scard() == 0) {
+ return null;
Review comment:
Rather than returning null here, it would be better to return `Collections.emptySet()`. This would then make the `sinter()` method just
```
return calculateInter(regionProvider, keys);
```
making the `calculateInter()` method redundant and allowing it to be inlined.
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -110,6 +111,47 @@ private static MemberSet calculateDiff(RegionProvider regionProvider, List<Redis
return diff;
}
+ public static Set<byte[]> sinter(RegionProvider regionProvider, List<RedisKey> keys) {
+ MemberSet result = calculateInter(regionProvider, keys);
+ if (result == null) {
+ return Collections.emptySet();
+ }
+ return result;
+ }
+
+ private static MemberSet calculateInter(RegionProvider regionProvider, List<RedisKey> keys) {
+ List<RedisSet> sets = new ArrayList<>(keys.size());
+ RedisSet smallestSet = null;
+
+ for (RedisKey key : keys) {
+ RedisSet redisSet = regionProvider.getTypedRedisData(REDIS_SET, key, true);
+ if (redisSet == NULL_REDIS_SET || redisSet.scard() == 0) {
+ return null;
+ } else {
+ if (smallestSet == null || smallestSet.scard() > redisSet.scard()) {
+ smallestSet = redisSet;
+ }
+ sets.add(redisSet);
+ }
+ }
+
+ MemberSet result = new MemberSet(smallestSet.scard());
Review comment:
The LGTM warning introduced here can be fixed by not initializing `smallestSet` as null but rather as `NULL_REDIS_SET`. The null check on line 131 would then also need to be changed to compare to `NULL_REDIS_SET` rather than null.
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSInterIntegrationTest.java
##########
@@ -72,14 +76,78 @@ public void testSInter() {
String[] expected = new String[] {"peach"};
assertThat(resultSet).containsExactlyInAnyOrder(expected);
+ }
+
+ @Test
+ public void testSInter_givenNonSet_returnsErrorWrongType() {
+ String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
+ String nonSet = "apple";
+ jedis.sadd("{user1}set1", firstSet);
+ jedis.set("{user1}nonSet", nonSet);
+
+ assertThatThrownBy(() -> jedis.sinter("{user1}set1", "{user1}nonSet")).hasMessageContaining(
+ ERROR_WRONG_TYPE);
+ }
- Set<String> emptyResultSet = jedis.sinter("{user1}nonexistent", "{user1}set2", "{user1}set3");
+ @Test
+ public void testSInter_givenNoIntersection_returnsEmptySet() {
+ String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
+ String[] secondSet = new String[] {"ubuntu", "microsoft", "linux", "solaris"};
+ jedis.sadd("{user1}set1", firstSet);
+ jedis.sadd("{user1}set2", secondSet);
+
+ Set<String> emptyResultSet = jedis.sinter("{user1}set1", "{user1}set2");
assertThat(emptyResultSet).isEmpty();
+ }
- jedis.sadd("{user1}newEmpty", "born2die");
- jedis.srem("{user1}newEmpty", "born2die");
- Set<String> otherEmptyResultSet = jedis.sinter("{user1}set2", "{user1}newEmpty");
- assertThat(otherEmptyResultSet).isEmpty();
+ @Test
+ public void testSInter_givenSingleSet_returnsAllMembers() {
+ String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
+ jedis.sadd("{user1}set1", firstSet);
+
+ Set<String> resultSet = jedis.sinter("{user1}set1");
+ assertThat(resultSet).containsExactlyInAnyOrder(firstSet);
+ }
+
+ @Test
+ public void testSInter_givenFullyMatchingSet_returnsAllMembers() {
+ String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
+ String[] secondSet = new String[] {"apple", "pear", "plum", "peach", "orange",};
+ jedis.sadd("{user1}set1", firstSet);
+ jedis.sadd("{user1}set2", secondSet);
+
+ Set<String> resultSet = jedis.sinter("{user1}set1", "{user1}set2");
+ assertThat(resultSet).containsExactlyInAnyOrder(firstSet);
+ }
+
+ @Test
+ public void testSInter_givenNonExistentSingleSet_returnsEmptySet() {
+ Set<String> emptyResultSet = jedis.sinter("{user1}set1");
+ assertThat(emptyResultSet).isEmpty();
+ }
+
+ @Test
+ public void ensureSetConsistency_whenRunningConcurrently() {
+ String[] values = new String[] {"pear", "apple", "plum", "orange", "peach"};
+ Set<String> valuesList = new HashSet<>(Arrays.asList(values));
+ String[] newValues = new String[] {"ubuntu", "orange", "peach", "linux"};
+ jedis.sadd("{user1}firstset", values);
+ jedis.sadd("{user1}secondset", values);
+
+ final AtomicReference<Set<String>> sinterResultReference = new AtomicReference<>();
+ new ConcurrentLoopingThreads(1000,
+ i -> jedis.sadd("{user1}thirdset", newValues),
+ i -> sinterResultReference.set(
+ jedis.sinter("{user1}firstset", "{user1}secondset", "{user1}thirdset")))
+ .runWithAction(() -> {
+ String[] result = new String[] {"orange", "peach"};
+ Set<String> expectedResultSet = new HashSet<>(Arrays.asList(result));
Review comment:
These lines can be moved to outside of the `ConcurrentLoopingThreads`, as they're constant and don't need to be recreated every iteration. Also, it's not necessary to create a HashSet here, you can instead just assert:
```
sInterResult -> assertThat(sInterResult.get()).containsExactlyInAnyOrder(result));
```
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -110,6 +111,47 @@ private static MemberSet calculateDiff(RegionProvider regionProvider, List<Redis
return diff;
}
+ public static Set<byte[]> sinter(RegionProvider regionProvider, List<RedisKey> keys) {
+ MemberSet result = calculateInter(regionProvider, keys);
+ if (result == null) {
+ return Collections.emptySet();
+ }
+ return result;
+ }
+
+ private static MemberSet calculateInter(RegionProvider regionProvider, List<RedisKey> keys) {
+ List<RedisSet> sets = new ArrayList<>(keys.size());
+ RedisSet smallestSet = null;
+
+ for (RedisKey key : keys) {
+ RedisSet redisSet = regionProvider.getTypedRedisData(REDIS_SET, key, true);
+ if (redisSet == NULL_REDIS_SET || redisSet.scard() == 0) {
Review comment:
There is a redundant check here, as it's not possible for a RedisSet to be empty without also being a `NULL_REDIS_SET`.
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSInterIntegrationTest.java
##########
@@ -72,14 +76,78 @@ public void testSInter() {
String[] expected = new String[] {"peach"};
assertThat(resultSet).containsExactlyInAnyOrder(expected);
+ }
+
+ @Test
+ public void testSInter_givenNonSet_returnsErrorWrongType() {
+ String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
+ String nonSet = "apple";
+ jedis.sadd("{user1}set1", firstSet);
+ jedis.set("{user1}nonSet", nonSet);
+
+ assertThatThrownBy(() -> jedis.sinter("{user1}set1", "{user1}nonSet")).hasMessageContaining(
+ ERROR_WRONG_TYPE);
+ }
- Set<String> emptyResultSet = jedis.sinter("{user1}nonexistent", "{user1}set2", "{user1}set3");
+ @Test
+ public void testSInter_givenNoIntersection_returnsEmptySet() {
+ String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
+ String[] secondSet = new String[] {"ubuntu", "microsoft", "linux", "solaris"};
+ jedis.sadd("{user1}set1", firstSet);
+ jedis.sadd("{user1}set2", secondSet);
+
+ Set<String> emptyResultSet = jedis.sinter("{user1}set1", "{user1}set2");
assertThat(emptyResultSet).isEmpty();
+ }
- jedis.sadd("{user1}newEmpty", "born2die");
- jedis.srem("{user1}newEmpty", "born2die");
- Set<String> otherEmptyResultSet = jedis.sinter("{user1}set2", "{user1}newEmpty");
- assertThat(otherEmptyResultSet).isEmpty();
+ @Test
+ public void testSInter_givenSingleSet_returnsAllMembers() {
+ String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
+ jedis.sadd("{user1}set1", firstSet);
+
+ Set<String> resultSet = jedis.sinter("{user1}set1");
+ assertThat(resultSet).containsExactlyInAnyOrder(firstSet);
+ }
+
+ @Test
+ public void testSInter_givenFullyMatchingSet_returnsAllMembers() {
+ String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
+ String[] secondSet = new String[] {"apple", "pear", "plum", "peach", "orange",};
+ jedis.sadd("{user1}set1", firstSet);
+ jedis.sadd("{user1}set2", secondSet);
+
+ Set<String> resultSet = jedis.sinter("{user1}set1", "{user1}set2");
+ assertThat(resultSet).containsExactlyInAnyOrder(firstSet);
+ }
+
+ @Test
+ public void testSInter_givenNonExistentSingleSet_returnsEmptySet() {
+ Set<String> emptyResultSet = jedis.sinter("{user1}set1");
+ assertThat(emptyResultSet).isEmpty();
+ }
+
+ @Test
+ public void ensureSetConsistency_whenRunningConcurrently() {
+ String[] values = new String[] {"pear", "apple", "plum", "orange", "peach"};
+ Set<String> valuesList = new HashSet<>(Arrays.asList(values));
Review comment:
This variable is never used.
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSInterIntegrationTest.java
##########
@@ -60,9 +64,9 @@ public void sinterstoreErrors_givenTooFewArguments() {
}
@Test
- public void testSInter() {
- String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
- String[] secondSet = new String[] {"apple", "microsoft", "linux", "peach"};
+ public void testSInter_givenIntersection_returnsIntersectedMembers() {
+ String[] firstSet = new String[] {"peach"};
+ String[] secondSet = new String[] {"linux", "peach"};
String[] thirdSet = new String[] {"luigi", "bowser", "peach", "mario"};
jedis.sadd("{user1}set1", firstSet);
Review comment:
The strings `"{user1}set1"` and `"{user1}set2" are used a lot in this class. Could they be extracted to constants please?
--
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