DonalEvans commented on a change in pull request #7257:
URL: https://github.com/apache/geode/pull/7257#discussion_r784234078
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffIntegrationTest.java
##########
@@ -133,38 +133,56 @@ public void sdiffSetsNotModified_returnSetValues() {
String[] firstValues = createKeyValuesSet();
String[] secondValues = new String[] {"apple", "bottoms", "boots", "fur",
"peach"};
jedis.sadd("{tag1}setkey2", secondValues);
- jedis.sdiff(setKey, "{tag1}setkey2");
- assertThat(jedis.smembers(setKey)).containsExactlyInAnyOrder(firstValues);
+ jedis.sdiff(SET_KEY, "{tag1}setkey2");
+ assertThat(jedis.smembers(SET_KEY)).containsExactlyInAnyOrder(firstValues);
assertThat(jedis.smembers("{tag1}setkey2")).containsExactlyInAnyOrder(secondValues);
}
@Test
public void sdiffNonExistentSetsNotModified_returnEmptySet() {
- jedis.sdiff(nonExistentSetKey, "{tag1}nonExisistent2");
- assertThat(jedis.smembers(nonExistentSetKey)).isEmpty();
+ jedis.sdiff(NON_EXISTENT_SET_KEY, "{tag1}nonExisistent2");
+ assertThat(jedis.smembers(NON_EXISTENT_SET_KEY)).isEmpty();
assertThat(jedis.smembers("{tag1}nonExisistent2")).isEmpty();
}
@Test
public void
sdiffNonExistentSetAndSetNotModified_returnEmptySetAndSetValues() {
String[] firstValues = createKeyValuesSet();
- jedis.sdiff(nonExistentSetKey, setKey);
- assertThat(jedis.smembers(nonExistentSetKey).isEmpty());
- assertThat(jedis.smembers(setKey)).containsExactlyInAnyOrder(firstValues);
+ jedis.sdiff(NON_EXISTENT_SET_KEY, SET_KEY);
+ assertThat(jedis.smembers(NON_EXISTENT_SET_KEY).isEmpty());
+ assertThat(jedis.smembers(SET_KEY)).containsExactlyInAnyOrder(firstValues);
}
@Test
public void sdiffSetAndNonExistentSetNotModified_returnSetValueAndEmptySet()
{
String[] firstValues = createKeyValuesSet();
- jedis.sdiff(setKey, nonExistentSetKey);
- assertThat(jedis.smembers(setKey)).containsExactlyInAnyOrder(firstValues);
- assertThat(jedis.smembers(nonExistentSetKey).isEmpty());
+ jedis.sdiff(SET_KEY, NON_EXISTENT_SET_KEY);
+ assertThat(jedis.smembers(SET_KEY)).containsExactlyInAnyOrder(firstValues);
+ assertThat(jedis.smembers(NON_EXISTENT_SET_KEY).isEmpty());
}
@Test
- public void sdiffWithDifferentyKeyType_returnsWrongTypeError() {
- jedis.set("ding", "dong");
- assertThatThrownBy(() ->
jedis.sdiff("ding")).hasMessageContaining(ERROR_WRONG_TYPE);
+ public void sdiff_withNonSetKeyAsFirstKey_returnsWrongTypeError() {
+ String stringKey = "{tag1}ding";
+ jedis.set(stringKey, "dong");
+
+ String[] members = createKeyValuesSet();
+ String secondSetKey = "{tag1}secondKey";
+ jedis.sadd(secondSetKey, members);
+ assertThatThrownBy(() -> jedis.sdiff(stringKey, SET_KEY, secondSetKey))
+ .hasMessageContaining(ERROR_WRONG_TYPE);
+ }
+
+ @Test
+ public void sdiff_withNonSetKeyAsThirdKey_returnsWrongTypeError() {
Review comment:
Could we also get tests of the behaviour when there are three keys, the
first key does not exist, and the third key is not a set? This is a distinct
error case that this PR fixes the behaviour for, but doesn't currently have
tests for. An example for the SDIFF case would be:
```
@Test
public void
sdiff_withNonSetKeyAsThirdKeyAndNonExistentSetAsFirstKey_returnsWrongTypeError()
{
String stringKey = "{tag1}ding";
jedis.set(stringKey, "dong");
String secondSetKey = "{tag1}secondKey";
jedis.sadd(secondSetKey, "member");
assertThatThrownBy(() -> jedis.sdiff(NON_EXISTENT_SET_KEY, secondSetKey,
stringKey))
.hasMessageContaining(ERROR_WRONG_TYPE);
}
```
but we should probably have a test like this for all the multi-key commands
that can return WRONGTYPE errors to make sure we're returning what native Redis
does.
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -287,22 +287,32 @@ public long zcount(SortedSetScoreRangeOptions
rangeOptions) {
return Coder.doubleToBytes(score);
}
- public static long zinterstore(RegionProvider regionProvider, RedisKey key,
- List<ZKeyWeight> keyWeights,
- ZAggregator aggregator) {
- List<RedisSortedSet> sets = new ArrayList<>(keyWeights.size());
+ public static long zinterstore(RegionProvider regionProvider, RedisKey
destinationKey,
+ List<ZKeyWeight> keyWeights, ZAggregator aggregator) {
+ boolean noIntersection = false;
+ List<RedisSortedSet> sets = new ArrayList<>(keyWeights.size());
for (ZKeyWeight keyWeight : keyWeights) {
RedisSortedSet set =
regionProvider.getTypedRedisData(REDIS_SORTED_SET,
keyWeight.getKey(), false);
+ // Need to loop through all keys to check for ERROR_WRONG_TYPE
+ if (noIntersection) {
+ continue;
+ }
+
if (set == NULL_REDIS_SORTED_SET) {
- return sortedSetOpStoreResult(regionProvider, key, new MemberMap(0),
new ScoreSet());
+ noIntersection = true;
} else {
sets.add(set);
}
}
+ if (noIntersection) {
+ return sortedSetOpStoreResult(regionProvider, destinationKey, new
MemberMap(0),
Review comment:
Technically unrelated to this PR, but the arguments for the
`sortedSetOpStoreResult()` method should probably be renamed. `MemberMap
interMembers` and `ScoreSet interScores` should probably just be `MemberMap
members` and `ScoreSet scores`. If it's not too much bother, would you mind
renaming them, since you're already modifying this class?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
