DonalEvans commented on a change in pull request #7257:
URL: https://github.com/apache/geode/pull/7257#discussion_r782638870
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffIntegrationTest.java
##########
@@ -167,6 +167,30 @@ public void
sdiffWithDifferentyKeyType_returnsWrongTypeError() {
assertThatThrownBy(() ->
jedis.sdiff("ding")).hasMessageContaining(ERROR_WRONG_TYPE);
}
+ @Test
+ public void sdiff_withDifferentKeyTypeAndTwoSetKeys_returnsWrongTypeError() {
+ String diffTypeKey = "{tag1}ding";
+ jedis.set(diffTypeKey, "dong");
+
+ String[] members = createKeyValuesSet();
+ String secondSetKey = "{tag1}secondKey";
+ jedis.sadd(secondSetKey, members);
+ assertThatThrownBy(() -> jedis.sdiff(diffTypeKey, setKey, secondSetKey))
+ .hasMessageContaining(ERROR_WRONG_TYPE);
+ }
+
+ @Test
+ public void sdiff_withTwoSetKeysAndDifferentKeyType_returnsWrongTypeError() {
Review comment:
This test name might be a little clearer as something like
"sdiff_withNonSetKeyAsThirdKey_returnsWrongTypeError"
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffIntegrationTest.java
##########
@@ -167,6 +167,30 @@ public void
sdiffWithDifferentyKeyType_returnsWrongTypeError() {
assertThatThrownBy(() ->
jedis.sdiff("ding")).hasMessageContaining(ERROR_WRONG_TYPE);
}
+ @Test
+ public void sdiff_withDifferentKeyTypeAndTwoSetKeys_returnsWrongTypeError() {
+ String diffTypeKey = "{tag1}ding";
Review comment:
Could this (and the other variables with the same name added in this PR)
be renamed to `stringKey` to avoid confusion around "diff" meaning different vs
the DIFF operation?
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/sortedset/AbstractZInterStoreIntegrationTest.java
##########
@@ -78,6 +78,32 @@ public void shouldError_givenWrongKeyType() {
STRING_KEY, KEY1)).hasMessage("WRONGTYPE " +
RedisConstants.ERROR_WRONG_TYPE);
}
+ @Test
+ public void shouldError_givenTwoSortedSetKeysAndWrongKeyType() {
+ Map<String, Double> scores = buildMapOfMembersAndScores(1, 5);
+ Map<String, Double> nonIntersectionScores = buildMapOfMembersAndScores(6,
10);
+ jedis.zadd(KEY1, scores);
+ jedis.zadd(KEY2, nonIntersectionScores);
+
+ final String STRING_KEY = "{tag1}stringKey";
+ jedis.set(STRING_KEY, "value");
+
+ assertThatThrownBy(() -> jedis.zinterstore(NEW_SET, KEY1, KEY2,
+ STRING_KEY)).hasMessageContaining(RedisConstants.ERROR_WRONG_TYPE);
+ }
+
+ @Test
+ public void shouldError_givenWrongKeyTypeAndTwoSortedSetKeys() {
Review comment:
This test name might be a little clearer as something like
"shouldReturnWrongTypeError_givenNonSortedSetKeyAsFirstSourceKey". Also, this
test case duplicates the test "shouldError_givenWrongKeyType" but is clearer
than that one, so that one should probably be removed.
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/sortedset/AbstractZUnionStoreIntegrationTest.java
##########
@@ -70,6 +70,44 @@ public void shouldError_givenWrongKeyType() {
.hasMessage("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
}
+ @Test
+ public void shouldError_givenTwoSortedSetKeysAndWrongKeyType() {
+ Map<String, Double> scores1 = new LinkedHashMap<>();
+ scores1.put("player1", Double.NEGATIVE_INFINITY);
+ scores1.put("player2", 0D);
+ scores1.put("player3", 1D);
+ scores1.put("player4", Double.POSITIVE_INFINITY);
+ jedis.zadd(KEY1, scores1);
+
+ Map<String, Double> scores2 = makeScoreMap(10, x -> (double) (9 - x));
+ jedis.zadd(KEY2, scores2);
+
+ final String DIFF_TYPE_KEY = "{tag1}stringKey";
+ jedis.set(DIFF_TYPE_KEY, "value");
+
+ assertThatThrownBy(() -> jedis.zunionstore(NEW_SET, KEY1, KEY2,
DIFF_TYPE_KEY))
+ .hasMessageContaining(RedisConstants.ERROR_WRONG_TYPE);
+ }
+
+ @Test
+ public void shouldError_givenWrongKeyTypeAndTwoSortedSetKeys() {
Review comment:
This test name might be a little clearer as something like
"shouldReturnWrongTypeError_givenNonSortedSetKeyAsFirstSourceKey". Also, this
test case duplicates the test "shouldError_givenWrongKeyType" but is clearer
than that one, so that one should probably be removed.
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffStoreIntegrationTest.java
##########
@@ -66,6 +66,30 @@ public void
sdiffstore_DifferentKeyType_returnsWrongTypeError() {
.hasMessageContaining(ERROR_WRONG_TYPE);
}
+ @Test
+ public void
sdiffstore_withDifferentKeyTypeAndTwoSetKeys_returnsWrongTypeError() {
Review comment:
This test name might be a little clearer as something like
"sdiffstore_withNonSetKeyAsFirstSourceKey_returnsWrongTypeError"
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffIntegrationTest.java
##########
@@ -167,6 +167,30 @@ public void
sdiffWithDifferentyKeyType_returnsWrongTypeError() {
assertThatThrownBy(() ->
jedis.sdiff("ding")).hasMessageContaining(ERROR_WRONG_TYPE);
}
+ @Test
+ public void sdiff_withDifferentKeyTypeAndTwoSetKeys_returnsWrongTypeError() {
Review comment:
This test name might be a little clearer as something like
"sdiff_withNonSetKeyAsFirstKey_returnsWrongTypeError"
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/sortedset/AbstractZUnionStoreIntegrationTest.java
##########
@@ -70,6 +70,44 @@ public void shouldError_givenWrongKeyType() {
.hasMessage("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
}
+ @Test
+ public void shouldError_givenTwoSortedSetKeysAndWrongKeyType() {
+ Map<String, Double> scores1 = new LinkedHashMap<>();
+ scores1.put("player1", Double.NEGATIVE_INFINITY);
+ scores1.put("player2", 0D);
+ scores1.put("player3", 1D);
+ scores1.put("player4", Double.POSITIVE_INFINITY);
+ jedis.zadd(KEY1, scores1);
+
+ Map<String, Double> scores2 = makeScoreMap(10, x -> (double) (9 - x));
+ jedis.zadd(KEY2, scores2);
+
+ final String DIFF_TYPE_KEY = "{tag1}stringKey";
+ jedis.set(DIFF_TYPE_KEY, "value");
+
+ assertThatThrownBy(() -> jedis.zunionstore(NEW_SET, KEY1, KEY2,
DIFF_TYPE_KEY))
+ .hasMessageContaining(RedisConstants.ERROR_WRONG_TYPE);
+ }
+
+ @Test
+ public void shouldError_givenWrongKeyTypeAndTwoSortedSetKeys() {
+ Map<String, Double> scores1 = new LinkedHashMap<>();
+ scores1.put("player1", Double.NEGATIVE_INFINITY);
+ scores1.put("player2", 0D);
+ scores1.put("player3", 1D);
+ scores1.put("player4", Double.POSITIVE_INFINITY);
+ jedis.zadd(KEY1, scores1);
+
+ Map<String, Double> scores2 = makeScoreMap(10, x -> (double) (9 - x));
+ jedis.zadd(KEY2, scores2);
+
+ final String DIFF_TYPE_KEY = "{tag1}stringKey";
Review comment:
Could this be renamed to "stringKey" please?
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/sortedset/AbstractZUnionStoreIntegrationTest.java
##########
@@ -70,6 +70,44 @@ public void shouldError_givenWrongKeyType() {
.hasMessage("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
}
+ @Test
+ public void shouldError_givenTwoSortedSetKeysAndWrongKeyType() {
+ Map<String, Double> scores1 = new LinkedHashMap<>();
+ scores1.put("player1", Double.NEGATIVE_INFINITY);
+ scores1.put("player2", 0D);
+ scores1.put("player3", 1D);
+ scores1.put("player4", Double.POSITIVE_INFINITY);
+ jedis.zadd(KEY1, scores1);
+
+ Map<String, Double> scores2 = makeScoreMap(10, x -> (double) (9 - x));
+ jedis.zadd(KEY2, scores2);
+
+ final String DIFF_TYPE_KEY = "{tag1}stringKey";
Review comment:
Could this be renamed to "stringKey" please?
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffStoreIntegrationTest.java
##########
@@ -66,6 +66,30 @@ public void
sdiffstore_DifferentKeyType_returnsWrongTypeError() {
.hasMessageContaining(ERROR_WRONG_TYPE);
}
+ @Test
+ public void
sdiffstore_withDifferentKeyTypeAndTwoSetKeys_returnsWrongTypeError() {
+ String diffTypeKey = "{tag1}ding";
+ jedis.set(diffTypeKey, "dong");
+
+ String secondSetKey = "{tag1}secondKey";
+ jedis.sadd(setKey, setMembers);
+ jedis.sadd(secondSetKey, setMembers);
+ assertThatThrownBy(() -> jedis.sdiffstore(destinationKey, diffTypeKey,
setKey, secondSetKey))
+ .hasMessageContaining(ERROR_WRONG_TYPE);
+ }
+
+ @Test
+ public void
sdiffstore_withTwoSetKeysAndDifferentKeyType_returnsWrongTypeError() {
Review comment:
This test name might be a little clearer as something like
"sdiffstore_withNonSetKeyAsThirdSourceKey_returnsWrongTypeError"
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/sortedset/AbstractZUnionStoreIntegrationTest.java
##########
@@ -70,6 +70,44 @@ public void shouldError_givenWrongKeyType() {
.hasMessage("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
}
+ @Test
+ public void shouldError_givenTwoSortedSetKeysAndWrongKeyType() {
Review comment:
This test name might be a little clearer as something like
"shouldReturnWrongTypeError_givenNonSortedSetKeyAsThirdSourceKey"
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/sortedset/AbstractZUnionStoreIntegrationTest.java
##########
@@ -70,6 +70,44 @@ public void shouldError_givenWrongKeyType() {
.hasMessage("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
}
+ @Test
+ public void shouldError_givenTwoSortedSetKeysAndWrongKeyType() {
+ Map<String, Double> scores1 = new LinkedHashMap<>();
+ scores1.put("player1", Double.NEGATIVE_INFINITY);
+ scores1.put("player2", 0D);
+ scores1.put("player3", 1D);
+ scores1.put("player4", Double.POSITIVE_INFINITY);
+ jedis.zadd(KEY1, scores1);
+
+ Map<String, Double> scores2 = makeScoreMap(10, x -> (double) (9 - x));
+ jedis.zadd(KEY2, scores2);
+
+ final String DIFF_TYPE_KEY = "{tag1}stringKey";
+ jedis.set(DIFF_TYPE_KEY, "value");
+
+ assertThatThrownBy(() -> jedis.zunionstore(NEW_SET, KEY1, KEY2,
DIFF_TYPE_KEY))
+ .hasMessageContaining(RedisConstants.ERROR_WRONG_TYPE);
+ }
+
+ @Test
+ public void shouldError_givenWrongKeyTypeAndTwoSortedSetKeys() {
+ Map<String, Double> scores1 = new LinkedHashMap<>();
+ scores1.put("player1", Double.NEGATIVE_INFINITY);
+ scores1.put("player2", 0D);
+ scores1.put("player3", 1D);
+ scores1.put("player4", Double.POSITIVE_INFINITY);
+ jedis.zadd(KEY1, scores1);
+
+ Map<String, Double> scores2 = makeScoreMap(10, x -> (double) (9 - x));
+ jedis.zadd(KEY2, scores2);
Review comment:
See above comments about simplifying the set-up for these tests.
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/sortedset/AbstractZInterStoreIntegrationTest.java
##########
@@ -78,6 +78,32 @@ public void shouldError_givenWrongKeyType() {
STRING_KEY, KEY1)).hasMessage("WRONGTYPE " +
RedisConstants.ERROR_WRONG_TYPE);
}
+ @Test
+ public void shouldError_givenTwoSortedSetKeysAndWrongKeyType() {
Review comment:
This test name might be a little clearer as something like
"shouldReturnWrongTypeError_givenNonSortedSetKeyAsThirdSourceKey"
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/sortedset/AbstractZUnionStoreIntegrationTest.java
##########
@@ -70,6 +70,44 @@ public void shouldError_givenWrongKeyType() {
.hasMessage("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
}
+ @Test
+ public void shouldError_givenTwoSortedSetKeysAndWrongKeyType() {
+ Map<String, Double> scores1 = new LinkedHashMap<>();
+ scores1.put("player1", Double.NEGATIVE_INFINITY);
+ scores1.put("player2", 0D);
+ scores1.put("player3", 1D);
+ scores1.put("player4", Double.POSITIVE_INFINITY);
+ jedis.zadd(KEY1, scores1);
+
+ Map<String, Double> scores2 = makeScoreMap(10, x -> (double) (9 - x));
+ jedis.zadd(KEY2, scores2);
Review comment:
See above comments about simplifying the set-up for these tests.
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/sortedset/AbstractZInterStoreIntegrationTest.java
##########
@@ -78,6 +78,32 @@ public void shouldError_givenWrongKeyType() {
STRING_KEY, KEY1)).hasMessage("WRONGTYPE " +
RedisConstants.ERROR_WRONG_TYPE);
}
+ @Test
+ public void shouldError_givenTwoSortedSetKeysAndWrongKeyType() {
+ Map<String, Double> scores = buildMapOfMembersAndScores(1, 5);
+ Map<String, Double> nonIntersectionScores = buildMapOfMembersAndScores(6,
10);
+ jedis.zadd(KEY1, scores);
+ jedis.zadd(KEY2, nonIntersectionScores);
+
+ final String STRING_KEY = "{tag1}stringKey";
Review comment:
Minor point, but all-caps variable names should be reserved for static
constants rather than local variables. (See the [Google Java Style
guide](https://google.github.io/styleguide/javaguide.html#s5.2-specific-identifier-names)
that Geode follows). I think the reason there are some local variables with
the wrong format in this file already is due to previous refactoring when they
were turned from constants into local variables but not renamed.
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/sortedset/AbstractZInterStoreIntegrationTest.java
##########
@@ -78,6 +78,32 @@ public void shouldError_givenWrongKeyType() {
STRING_KEY, KEY1)).hasMessage("WRONGTYPE " +
RedisConstants.ERROR_WRONG_TYPE);
}
+ @Test
+ public void shouldError_givenTwoSortedSetKeysAndWrongKeyType() {
+ Map<String, Double> scores = buildMapOfMembersAndScores(1, 5);
+ Map<String, Double> nonIntersectionScores = buildMapOfMembersAndScores(6,
10);
+ jedis.zadd(KEY1, scores);
+ jedis.zadd(KEY2, nonIntersectionScores);
Review comment:
The set-up in this test can be simplified, since we don't really care
what's in the sorted sets, just that they ARE sorted sets. Something like this
maybe:
```
jedis.zadd(KEY1, 1, "value1");
jedis.zadd(KEY2, 1, "value2");
```
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/sortedset/AbstractZInterStoreIntegrationTest.java
##########
@@ -78,6 +78,32 @@ public void shouldError_givenWrongKeyType() {
STRING_KEY, KEY1)).hasMessage("WRONGTYPE " +
RedisConstants.ERROR_WRONG_TYPE);
}
+ @Test
+ public void shouldError_givenTwoSortedSetKeysAndWrongKeyType() {
+ Map<String, Double> scores = buildMapOfMembersAndScores(1, 5);
+ Map<String, Double> nonIntersectionScores = buildMapOfMembersAndScores(6,
10);
+ jedis.zadd(KEY1, scores);
+ jedis.zadd(KEY2, nonIntersectionScores);
+
+ final String STRING_KEY = "{tag1}stringKey";
+ jedis.set(STRING_KEY, "value");
+
+ assertThatThrownBy(() -> jedis.zinterstore(NEW_SET, KEY1, KEY2,
+ STRING_KEY)).hasMessageContaining(RedisConstants.ERROR_WRONG_TYPE);
+ }
+
+ @Test
+ public void shouldError_givenWrongKeyTypeAndTwoSortedSetKeys() {
+ Map<String, Double> scores = buildMapOfMembersAndScores();
+ jedis.zadd(KEY1, scores);
Review comment:
This set-up can also be simplified:
```
jedis.zadd(KEY1, 1, "value1");
jedis.zadd(KEY2, 1, "value2");
```
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -108,21 +104,18 @@ public static int sdiffstore(RegionProvider
regionProvider, RedisKey destination
private static MemberSet calculateDiff(RegionProvider regionProvider,
List<RedisKey> keys,
boolean updateStats) {
RedisSet firstSet = regionProvider.getTypedRedisData(REDIS_SET,
keys.get(0), updateStats);
+ MemberSet diff = new MemberSet();
if (firstSet.scard() == 0) {
- return null;
+ return diff;
}
- MemberSet diff = new MemberSet(firstSet.members);
+ diff = new MemberSet(firstSet.members);
for (int i = 1; i < keys.size(); i++) {
Review comment:
This early return causes incorrect behaviour if the first set key does
not exist. The following test passes with native Redis but fails with
geode-for-redis:
```
@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("emptyKey", secondSetKey,
stringKey))
.hasMessageContaining(ERROR_WRONG_TYPE);
}
```
To fix the issue, this block should be:
```
RedisSet firstSet = regionProvider.getTypedRedisData(REDIS_SET,
keys.get(0), updateStats);
MemberSet diff = new MemberSet(firstSet.members);
for (int i = 1; i < keys.size(); i++) {
```
--
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]
