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/11 20:51:43 UTC
[GitHub] [geode] Kris-10-0 opened a new pull request #7257: GEODE-9936: Modified multi-key commands for wrong type key
Kris-10-0 opened a new pull request #7257:
URL: https://github.com/apache/geode/pull/7257
<!-- Thank you for submitting a contribution to Apache Geode. -->
<!-- In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
-->
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [x] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [x] Is your initial contribution a single, squashed commit?
- [x] Does `gradlew build` run cleanly?
- [x] Have you written or updated unit tests to verify your changes?
- [NA] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
<!-- Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
-->
--
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
[GitHub] [geode] Kris-10-0 commented on a change in pull request #7257: GEODE-9936: Modified multi-key commands for wrong type key
Posted by GitBox <gi...@apache.org>.
Kris-10-0 commented on a change in pull request #7257:
URL: https://github.com/apache/geode/pull/7257#discussion_r783515972
##########
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:
Should I rename the other local variables in the class to follow that format.
--
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
[GitHub] [geode] DonalEvans merged pull request #7257: GEODE-9936: Modified multi-key commands for wrong type key
Posted by GitBox <gi...@apache.org>.
DonalEvans merged pull request #7257:
URL: https://github.com/apache/geode/pull/7257
--
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
[GitHub] [geode] Kris-10-0 commented on pull request #7257: GEODE-9936: Modified multi-key commands for wrong type key
Posted by GitBox <gi...@apache.org>.
Kris-10-0 commented on pull request #7257:
URL: https://github.com/apache/geode/pull/7257#issuecomment-1012388596
Forced pushed to rebase
--
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
[GitHub] [geode] DonalEvans commented on a change in pull request #7257: GEODE-9936: Modified multi-key commands for wrong type key
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7257:
URL: https://github.com/apache/geode/pull/7257#discussion_r783523650
##########
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:
That would be awesome, thanks!
--
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
[GitHub] [geode] DonalEvans commented on a change in pull request #7257: GEODE-9936: Modified multi-key commands for wrong type key
Posted by GitBox <gi...@apache.org>.
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #7257: GEODE-9936: Modified multi-key commands for wrong type key
Posted by GitBox <gi...@apache.org>.
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #7257: GEODE-9936: Modified multi-key commands for wrong type key
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7257:
URL: https://github.com/apache/geode/pull/7257#discussion_r782637671
##########
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) {
Review comment:
I think that if there is no value associated with the key then we return a `NULL_REDIS_SET` from `getTypedRedisData()`, which returns 0 when `scard()` is called on it, so this check is actually doing something.
--
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
[GitHub] [geode] Kris-10-0 commented on a change in pull request #7257: GEODE-9936: Modified multi-key commands for wrong type key
Posted by GitBox <gi...@apache.org>.
Kris-10-0 commented on a change in pull request #7257:
URL: https://github.com/apache/geode/pull/7257#discussion_r783513887
##########
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) {
Review comment:
I changed it to NULL_REDIS_SET, because I thought it would make it easier to understand. Let me know if I should change it back.
--
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
[GitHub] [geode] DonalEvans commented on a change in pull request #7257: GEODE-9936: Modified multi-key commands for wrong type key
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7257:
URL: https://github.com/apache/geode/pull/7257#discussion_r782654454
##########
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("{tag1}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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #7257: GEODE-9936: Modified multi-key commands for wrong type key
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7257:
URL: https://github.com/apache/geode/pull/7257#discussion_r782637671
##########
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) {
Review comment:
I think that if there is no value associated with the key then we return a `NULL_REDIS_SET` here, which returns 0 when `scard()` is called on it, so this check is actually doing something.
--
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
[GitHub] [geode] DonalEvans commented on pull request #7257: GEODE-9936: Modified multi-key commands for wrong type key
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on pull request #7257:
URL: https://github.com/apache/geode/pull/7257#issuecomment-1010561019
In addition to the comments I left, this test if added to `AbstractZInterStoreIntegrationTest` also passes with native Redis but fails with geode-for-redis:
```
@Test
public void shouldReturnWrongTypeError_withNonSortedSetKeyAsThirdSourceKeyAndNonExistentSortedSetAsFirstSourceKey() {
jedis.zadd(KEY2, 1, "value2");
final String STRING_KEY = "{tag1}stringKey";
jedis.set(STRING_KEY, "value");
assertThatThrownBy(() -> jedis.zinterstore(NEW_SET, "{tag1}nonExistentKey", KEY2,
STRING_KEY)).hasMessageContaining(RedisConstants.ERROR_WRONG_TYPE);
}
```
This can be fixed by changing the for loop in the `zinterstore()` method to the following:
```
for (ZKeyWeight keyWeight : keyWeights) {
RedisSortedSet set =
regionProvider.getTypedRedisData(REDIS_SORTED_SET, keyWeight.getKey(), false);
if (set != NULL_REDIS_SORTED_SET) {
sets.add(set);
}
}
```
--
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
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7257: GEODE-9936: Modified multi-key commands for wrong type key
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #7257:
URL: https://github.com/apache/geode/pull/7257#discussion_r782633334
##########
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++) {
RedisSet curSet = regionProvider.getTypedRedisData(REDIS_SET, keys.get(i), updateStats);
if (curSet.scard() == 0) {
Review comment:
Since we never store a empty RedisSet (if it is empty it just gets removed) scard will never return 0. You should delete this if block.
##########
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) {
Review comment:
Since we never store a empty RedisSet (if it is empty it just gets removed) scard will never return 0. You should delete this if block.
--
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