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/06 00:44:37 UTC
[GitHub] [geode] DonalEvans commented on a change in pull request #7240: feature/GEODE-9836: SUNION Command Support
DonalEvans commented on a change in pull request #7240:
URL: https://github.com/apache/geode/pull/7240#discussion_r779223113
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -74,12 +74,26 @@ public RedisSet(int expectedSize) {
*/
public RedisSet() {}
+ public static Set<byte[]> sunion(RegionProvider regionProvider, List<RedisKey> keys) {
+ MemberSet result = calculateUnion(regionProvider, keys, true);
+ return getSetOpResult(result);
+ }
+
+ private static MemberSet calculateUnion(RegionProvider regionProvider, List<RedisKey> keys,
+ boolean updateStats) {
+ MemberSet union = new MemberSet();
+ for (RedisKey key : keys) {
+ RedisSet curSet = regionProvider.getTypedRedisData(REDIS_SET, key, updateStats);
+ if (curSet.scard() != 0) {
+ union.addAll(curSet.members);
+ }
Review comment:
I think it should be fine to not check the cardinality of the current set here, since if it's empty, calling `addAll()` with it won't actually result in any work being done.
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSUnionIntegrationTest.java
##########
@@ -56,31 +63,122 @@ public void sunionErrors_givenTooFewArguments() {
}
@Test
- public void sunionstroreErrors_givenTooFewArgumentst() {
- assertAtLeastNArgs(jedis, Protocol.Command.SUNIONSTORE, 2);
+ public void sunion_returnsAllValuesInSet_setNotModified() {
+ jedis.sadd(setKey1, setMembers1);
+ assertThat(jedis.sunion(setKey1)).containsExactlyInAnyOrder(setMembers1);
+ assertThat(jedis.smembers(setKey1)).containsExactlyInAnyOrder(setMembers1);
}
@Test
- public void testSUnion() {
- String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
- String[] secondSet = new String[] {"apple", "microsoft", "linux", "peach"};
- String[] thirdSet = new String[] {"luigi", "bowser", "peach", "mario"};
- jedis.sadd("{user1}set1", firstSet);
- jedis.sadd("{user1}set2", secondSet);
- jedis.sadd("{user1}set3", thirdSet);
+ public void sunionWithNonExistentSet_returnsEmptySet_nonExistentKeyDoesNotExist() {
+ assertThat(jedis.sunion(nonExistentSetKey)).isEmpty();
+ assertThat(jedis.exists(nonExistentSetKey)).isFalse();
+ }
- Set<String> resultSet = jedis.sunion("{user1}set1", "{user1}set2");
- String[] expected =
- new String[] {"pear", "apple", "plum", "orange", "peach", "microsoft", "linux"};
- assertThat(resultSet).containsExactlyInAnyOrder(expected);
+ @Test
+ public void sunionWithNonExistentSetAndSet_returnsAllValuesInSet() {
+ jedis.sadd(setKey1, setMembers1);
+ assertThat(jedis.sunion(nonExistentSetKey, setKey1)).containsExactlyInAnyOrder(setMembers1);
+ }
- Set<String> otherResultSet = jedis.sunion("{user1}nonexistent", "{user1}set1");
- assertThat(otherResultSet).containsExactlyInAnyOrder(firstSet);
+ @Test
+ public void sunionWithSetAndNonExistentSet_returnsAllValuesInSet() {
+ jedis.sadd(setKey1, setMembers1);
+ assertThat(jedis.sunion(setKey1, nonExistentSetKey))
+ .containsExactlyInAnyOrder(setMembers1);
+ }
- jedis.sadd("{user1}newEmpty", "born2die");
- jedis.srem("{user1}newEmpty", "born2die");
- Set<String> yetAnotherResultSet = jedis.sunion("{user1}set2", "{user1}newEmpty");
- assertThat(yetAnotherResultSet).containsExactlyInAnyOrder(secondSet);
+ @Test
+ public void sunionWithMultipleNonExistentSets_returnsEmptySet() {
+ assertThat(jedis.sunion("{user1}nonExistentSet1", "{user1}nonExistentSet2")).isEmpty();
+ }
+
+ @Test
+ public void sunionWithNonOverlappingSets_returnsUnionOfSets() {
+ String[] secondSetMembers = new String[] {"apple", "microsoft", "linux", "peach"};
+ jedis.sadd(setKey1, setMembers1);
+ jedis.sadd(setKey2, secondSetMembers);
+
+ String[] result =
+ {"one", "two", "three", "four", "five", "apple", "microsoft", "linux", "peach"};
+ assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(result);
+ }
+
+ @Test
+ public void sunionWithSetsWithSomeSharedValues_returnsUnionOfSets() {
+ String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+ jedis.sadd(setKey1, setMembers1);
+ jedis.sadd(setKey2, secondSetMembers);
+
+ String[] result = {"one", "two", "three", "four", "five", "linux", "peach"};
+ assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(result);
+ }
+
+ @Test
+ public void sunionWithSetsWithAllSharedValues_returnsUnionOfSets() {
+ jedis.sadd(setKey1, setMembers1);
+ jedis.sadd(setKey2, setMembers1);
+
+ assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(setMembers1);
+ }
+
+ @Test
+ public void sunionWithMultipleSets_returnsUnionOfSets() {
+ String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+ String thirdSetKey = "{user1}setKey3";
+ String[] thirdSetMembers = new String[] {"fire", "dance", "floor", "five"};
+ jedis.sadd(setKey1, setMembers1);
+ jedis.sadd(setKey2, secondSetMembers);
+ jedis.sadd(thirdSetKey, thirdSetMembers);
+
+
+ String[] result =
+ {"one", "two", "three", "four", "five", "linux", "peach", "fire", "dance", "floor"};
+ assertThat(jedis.sunion(setKey1, setKey2, thirdSetKey))
+ .containsExactlyInAnyOrder(result);
+ }
+
+ @Test
+ public void sunionWithSetsFromDifferentSlots_returnsCrossSlotError() {
+ String setKeyDifferentSlot = "{user2}setKey2";
+ String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+ jedis.sadd(setKey1, setMembers1);
+ jedis.sadd(setKeyDifferentSlot, secondSetMembers);
+
+ assertThatThrownBy(() -> jedis.sunion(setKey1, setKeyDifferentSlot))
+ .hasMessageContaining(ERROR_DIFFERENT_SLOTS);
+ }
+
+
+ @Test
+ public void sunionWithDifferentKeyType_returnsWrongTypeError() {
+ jedis.set("ding", "dong");
+ assertThatThrownBy(() -> jedis.sunion("ding")).hasMessageContaining(ERROR_WRONG_TYPE);
+ }
+
+ @Test
+ public void ensureSetConsistency_whenRunningConcurrently() {
+ String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+ jedis.sadd(setKey1, setMembers1);
+ jedis.sadd(setKey2, secondSetMembers);
+
+ String[] unionMembers = {"one", "two", "three", "four", "five", "linux", "peach"};
+ Set<String> unionList = new HashSet<>(Arrays.asList(unionMembers));
+ Set<String> firstSetMembersList = new HashSet<>(Arrays.asList(setMembers1));
+
+
+ final AtomicReference<Set<String>> sunionResultReference = new AtomicReference<>();
+ new ConcurrentLoopingThreads(1000,
+ i -> jedis.srem(setKey2, secondSetMembers),
+ i -> sunionResultReference.set(jedis.sunion(setKey1, setKey2)))
+ .runWithAction(() -> {
+ assertThat(sunionResultReference).satisfiesAnyOf(
+ sunionResult -> assertThat(sunionResult.get())
+ .containsExactlyInAnyOrderElementsOf(firstSetMembersList),
+ sunionResult -> assertThat(sunionResult.get())
+ .containsExactlyInAnyOrderElementsOf(unionList));
Review comment:
These assertions can be simplified to:
```
assertThat(sunionResultReference).satisfiesAnyOf(
sunionResult -> assertThat(sunionResult.get())
.containsExactlyInAnyOrder(setMembers1),
sunionResult -> assertThat(sunionResult.get())
.containsExactlyInAnyOrder(unionMembers));
```
which means we no longer need the `unionList` and `firstSetMembersList` variables.
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSUnionIntegrationTest.java
##########
@@ -32,16 +38,17 @@
import org.apache.geode.redis.ConcurrentLoopingThreads;
import org.apache.geode.redis.RedisIntegrationTest;
-import org.apache.geode.test.awaitility.GeodeAwaitility;
public abstract class AbstractSUnionIntegrationTest implements RedisIntegrationTest {
private JedisCluster jedis;
- private static final int REDIS_CLIENT_TIMEOUT =
- Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());
+ private static final String setKey1 = "{user1}setKey1";
+ private static final String[] setMembers1 = {"one", "two", "three", "four", "five"};
+ private static final String nonExistentSetKey = "{user1}nonExistentSet";
+ private static final String setKey2 = "{user1}setKey2";
Review comment:
Just to avoid potential confusion, could we use a different hash tag on the keys here? The tag has nothing to do with users, just to which slot keys map to, so there's a potential for confusion with this naming. Maybe just "{tag}setKey1" etc. to keep it simple?
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSUnionIntegrationTest.java
##########
@@ -56,31 +63,122 @@ public void sunionErrors_givenTooFewArguments() {
}
@Test
- public void sunionstroreErrors_givenTooFewArgumentst() {
- assertAtLeastNArgs(jedis, Protocol.Command.SUNIONSTORE, 2);
+ public void sunion_returnsAllValuesInSet_setNotModified() {
+ jedis.sadd(setKey1, setMembers1);
+ assertThat(jedis.sunion(setKey1)).containsExactlyInAnyOrder(setMembers1);
+ assertThat(jedis.smembers(setKey1)).containsExactlyInAnyOrder(setMembers1);
}
@Test
- public void testSUnion() {
- String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
- String[] secondSet = new String[] {"apple", "microsoft", "linux", "peach"};
- String[] thirdSet = new String[] {"luigi", "bowser", "peach", "mario"};
- jedis.sadd("{user1}set1", firstSet);
- jedis.sadd("{user1}set2", secondSet);
- jedis.sadd("{user1}set3", thirdSet);
+ public void sunionWithNonExistentSet_returnsEmptySet_nonExistentKeyDoesNotExist() {
+ assertThat(jedis.sunion(nonExistentSetKey)).isEmpty();
+ assertThat(jedis.exists(nonExistentSetKey)).isFalse();
+ }
- Set<String> resultSet = jedis.sunion("{user1}set1", "{user1}set2");
- String[] expected =
- new String[] {"pear", "apple", "plum", "orange", "peach", "microsoft", "linux"};
- assertThat(resultSet).containsExactlyInAnyOrder(expected);
+ @Test
+ public void sunionWithNonExistentSetAndSet_returnsAllValuesInSet() {
+ jedis.sadd(setKey1, setMembers1);
+ assertThat(jedis.sunion(nonExistentSetKey, setKey1)).containsExactlyInAnyOrder(setMembers1);
+ }
- Set<String> otherResultSet = jedis.sunion("{user1}nonexistent", "{user1}set1");
- assertThat(otherResultSet).containsExactlyInAnyOrder(firstSet);
+ @Test
+ public void sunionWithSetAndNonExistentSet_returnsAllValuesInSet() {
+ jedis.sadd(setKey1, setMembers1);
+ assertThat(jedis.sunion(setKey1, nonExistentSetKey))
+ .containsExactlyInAnyOrder(setMembers1);
+ }
- jedis.sadd("{user1}newEmpty", "born2die");
- jedis.srem("{user1}newEmpty", "born2die");
- Set<String> yetAnotherResultSet = jedis.sunion("{user1}set2", "{user1}newEmpty");
- assertThat(yetAnotherResultSet).containsExactlyInAnyOrder(secondSet);
+ @Test
+ public void sunionWithMultipleNonExistentSets_returnsEmptySet() {
+ assertThat(jedis.sunion("{user1}nonExistentSet1", "{user1}nonExistentSet2")).isEmpty();
+ }
+
+ @Test
+ public void sunionWithNonOverlappingSets_returnsUnionOfSets() {
+ String[] secondSetMembers = new String[] {"apple", "microsoft", "linux", "peach"};
+ jedis.sadd(setKey1, setMembers1);
+ jedis.sadd(setKey2, secondSetMembers);
+
+ String[] result =
+ {"one", "two", "three", "four", "five", "apple", "microsoft", "linux", "peach"};
+ assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(result);
+ }
+
+ @Test
+ public void sunionWithSetsWithSomeSharedValues_returnsUnionOfSets() {
+ String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+ jedis.sadd(setKey1, setMembers1);
+ jedis.sadd(setKey2, secondSetMembers);
+
+ String[] result = {"one", "two", "three", "four", "five", "linux", "peach"};
+ assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(result);
+ }
+
+ @Test
+ public void sunionWithSetsWithAllSharedValues_returnsUnionOfSets() {
+ jedis.sadd(setKey1, setMembers1);
+ jedis.sadd(setKey2, setMembers1);
+
+ assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(setMembers1);
+ }
+
+ @Test
+ public void sunionWithMultipleSets_returnsUnionOfSets() {
+ String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+ String thirdSetKey = "{user1}setKey3";
+ String[] thirdSetMembers = new String[] {"fire", "dance", "floor", "five"};
+ jedis.sadd(setKey1, setMembers1);
+ jedis.sadd(setKey2, secondSetMembers);
+ jedis.sadd(thirdSetKey, thirdSetMembers);
+
+
+ String[] result =
+ {"one", "two", "three", "four", "five", "linux", "peach", "fire", "dance", "floor"};
+ assertThat(jedis.sunion(setKey1, setKey2, thirdSetKey))
+ .containsExactlyInAnyOrder(result);
+ }
Review comment:
This test doesn't seem to be testing anything not already covered in the tests with two sets. Are both necessary?
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -74,12 +74,26 @@ public RedisSet(int expectedSize) {
*/
public RedisSet() {}
+ public static Set<byte[]> sunion(RegionProvider regionProvider, List<RedisKey> keys) {
+ MemberSet result = calculateUnion(regionProvider, keys, true);
+ return getSetOpResult(result);
Review comment:
I think it should be fine to just use
```
return calculateUnion(regionProvider, keys, true);
```
here, since that never returns null.
Ideally, none of the calculate* methods should return null, but rather an empty MemberSet which can then safely be returned from the calling method, removing the need for the `getSetOpResult()` method.
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSUnionIntegrationTest.java
##########
@@ -56,31 +63,122 @@ public void sunionErrors_givenTooFewArguments() {
}
@Test
- public void sunionstroreErrors_givenTooFewArgumentst() {
- assertAtLeastNArgs(jedis, Protocol.Command.SUNIONSTORE, 2);
+ public void sunion_returnsAllValuesInSet_setNotModified() {
+ jedis.sadd(setKey1, setMembers1);
+ assertThat(jedis.sunion(setKey1)).containsExactlyInAnyOrder(setMembers1);
+ assertThat(jedis.smembers(setKey1)).containsExactlyInAnyOrder(setMembers1);
}
@Test
- public void testSUnion() {
- String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
- String[] secondSet = new String[] {"apple", "microsoft", "linux", "peach"};
- String[] thirdSet = new String[] {"luigi", "bowser", "peach", "mario"};
- jedis.sadd("{user1}set1", firstSet);
- jedis.sadd("{user1}set2", secondSet);
- jedis.sadd("{user1}set3", thirdSet);
+ public void sunionWithNonExistentSet_returnsEmptySet_nonExistentKeyDoesNotExist() {
+ assertThat(jedis.sunion(nonExistentSetKey)).isEmpty();
+ assertThat(jedis.exists(nonExistentSetKey)).isFalse();
+ }
- Set<String> resultSet = jedis.sunion("{user1}set1", "{user1}set2");
- String[] expected =
- new String[] {"pear", "apple", "plum", "orange", "peach", "microsoft", "linux"};
- assertThat(resultSet).containsExactlyInAnyOrder(expected);
+ @Test
+ public void sunionWithNonExistentSetAndSet_returnsAllValuesInSet() {
+ jedis.sadd(setKey1, setMembers1);
+ assertThat(jedis.sunion(nonExistentSetKey, setKey1)).containsExactlyInAnyOrder(setMembers1);
+ }
- Set<String> otherResultSet = jedis.sunion("{user1}nonexistent", "{user1}set1");
- assertThat(otherResultSet).containsExactlyInAnyOrder(firstSet);
+ @Test
+ public void sunionWithSetAndNonExistentSet_returnsAllValuesInSet() {
+ jedis.sadd(setKey1, setMembers1);
+ assertThat(jedis.sunion(setKey1, nonExistentSetKey))
+ .containsExactlyInAnyOrder(setMembers1);
+ }
- jedis.sadd("{user1}newEmpty", "born2die");
- jedis.srem("{user1}newEmpty", "born2die");
- Set<String> yetAnotherResultSet = jedis.sunion("{user1}set2", "{user1}newEmpty");
- assertThat(yetAnotherResultSet).containsExactlyInAnyOrder(secondSet);
+ @Test
+ public void sunionWithMultipleNonExistentSets_returnsEmptySet() {
+ assertThat(jedis.sunion("{user1}nonExistentSet1", "{user1}nonExistentSet2")).isEmpty();
+ }
+
+ @Test
+ public void sunionWithNonOverlappingSets_returnsUnionOfSets() {
+ String[] secondSetMembers = new String[] {"apple", "microsoft", "linux", "peach"};
+ jedis.sadd(setKey1, setMembers1);
+ jedis.sadd(setKey2, secondSetMembers);
+
+ String[] result =
+ {"one", "two", "three", "four", "five", "apple", "microsoft", "linux", "peach"};
+ assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(result);
+ }
+
+ @Test
+ public void sunionWithSetsWithSomeSharedValues_returnsUnionOfSets() {
+ String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+ jedis.sadd(setKey1, setMembers1);
+ jedis.sadd(setKey2, secondSetMembers);
+
+ String[] result = {"one", "two", "three", "four", "five", "linux", "peach"};
+ assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(result);
+ }
+
+ @Test
+ public void sunionWithSetsWithAllSharedValues_returnsUnionOfSets() {
+ jedis.sadd(setKey1, setMembers1);
+ jedis.sadd(setKey2, setMembers1);
+
+ assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(setMembers1);
+ }
+
+ @Test
+ public void sunionWithMultipleSets_returnsUnionOfSets() {
+ String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+ String thirdSetKey = "{user1}setKey3";
+ String[] thirdSetMembers = new String[] {"fire", "dance", "floor", "five"};
+ jedis.sadd(setKey1, setMembers1);
+ jedis.sadd(setKey2, secondSetMembers);
+ jedis.sadd(thirdSetKey, thirdSetMembers);
+
+
+ String[] result =
+ {"one", "two", "three", "four", "five", "linux", "peach", "fire", "dance", "floor"};
+ assertThat(jedis.sunion(setKey1, setKey2, thirdSetKey))
+ .containsExactlyInAnyOrder(result);
+ }
+
+ @Test
+ public void sunionWithSetsFromDifferentSlots_returnsCrossSlotError() {
+ String setKeyDifferentSlot = "{user2}setKey2";
+ String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+ jedis.sadd(setKey1, setMembers1);
+ jedis.sadd(setKeyDifferentSlot, secondSetMembers);
+
+ assertThatThrownBy(() -> jedis.sunion(setKey1, setKeyDifferentSlot))
+ .hasMessageContaining(ERROR_DIFFERENT_SLOTS);
+ }
+
+
+ @Test
+ public void sunionWithDifferentKeyType_returnsWrongTypeError() {
Review comment:
Could we also get a test where the first key passed to SUNION is a set, but the second key is some other type 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