DonalEvans commented on a change in pull request #7219:
URL: https://github.com/apache/geode/pull/7219#discussion_r778402027
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -323,18 +339,48 @@ public long zinterstore(RegionProvider regionProvider,
RedisKey key, List<ZKeyWe
}
}
}
+
if (addToSet) {
- memberAdd(member, newScore);
+ OrderedSetEntry entry = interMembers.get(member);
+ if (entry == null) {
+ entry = new OrderedSetEntry(member, newScore);
+ interMembers.put(member, entry);
+ interScores.add(entry);
+ } else if (entry.getScore() != newScore) {
+ interScores.remove(entry);
+ entry.updateScore(newScore);
+ interScores.add(entry);
+ }
}
}
- if (removeFromRegion()) {
- regionProvider.getDataRegion().remove(key);
- } else {
- regionProvider.getLocalDataRegion().put(key, this);
+ return sortedSetOpStoreResult(regionProvider, key, interMembers,
interScores);
+ }
+
+ @VisibleForTesting
+ static long sortedSetOpStoreResult(RegionProvider regionProvider, RedisKey
destinationKey,
+ MemberMap interMembers, ScoreSet interScores) {
+ RedisSortedSet destinationSet =
+ regionProvider.getTypedRedisDataElseRemove(REDIS_SORTED_SET,
destinationKey, false);
Review comment:
The `getTypedRedisDataElseRemove()` method has a `TODO` on it saying it
should be removed once SDIFFSTORE is merged. With its use here, I assume that
is no longer correct. Could the comment be removed?
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -323,18 +339,48 @@ public long zinterstore(RegionProvider regionProvider,
RedisKey key, List<ZKeyWe
}
}
}
+
if (addToSet) {
- memberAdd(member, newScore);
+ OrderedSetEntry entry = interMembers.get(member);
+ if (entry == null) {
+ entry = new OrderedSetEntry(member, newScore);
+ interMembers.put(member, entry);
+ interScores.add(entry);
+ } else if (entry.getScore() != newScore) {
+ interScores.remove(entry);
+ entry.updateScore(newScore);
+ interScores.add(entry);
+ }
}
}
- if (removeFromRegion()) {
- regionProvider.getDataRegion().remove(key);
- } else {
- regionProvider.getLocalDataRegion().put(key, this);
+ return sortedSetOpStoreResult(regionProvider, key, interMembers,
interScores);
+ }
+
+ @VisibleForTesting
+ static long sortedSetOpStoreResult(RegionProvider regionProvider, RedisKey
destinationKey,
+ MemberMap interMembers, ScoreSet interScores) {
+ RedisSortedSet destinationSet =
+ regionProvider.getTypedRedisDataElseRemove(REDIS_SORTED_SET,
destinationKey, false);
+
+ if (interMembers == null || interScores.isEmpty()) {
Review comment:
The LGTM warning here can be fixed either by making this:
```
if (interMembers == null || interScores == null || interScores.isEmpty()) {
```
or by changing it to:
```
if (interMembers.isEmpty() || interScores.isEmpty()) {
```
and changing the call to `sortedSetOpStoreResult()` on line 299 to pass an
empty `MemberMap` and empty `ScoreSet` rather than `null`. I would tend to
prefer the second approach, as avoiding passing null around is generally better.
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -463,18 +509,24 @@ public long zrevrank(byte[] member) {
return null;
}
- public long zunionstore(RegionProvider regionProvider, RedisKey key,
List<ZKeyWeight> keyWeights,
+ public static long zunionstore(RegionProvider regionProvider, RedisKey key,
+ List<ZKeyWeight> keyWeights,
ZAggregator aggregator) {
+ MemberMap unionMembers = null;
+ ScoreSet unionScores = null;
for (ZKeyWeight keyWeight : keyWeights) {
RedisSortedSet set =
regionProvider.getTypedRedisData(REDIS_SORTED_SET,
keyWeight.getKey(), false);
if (set == NULL_REDIS_SORTED_SET) {
continue;
+ } else if (unionMembers == null) {
+ unionMembers = new MemberMap((int) set.zcard());
+ unionScores = new ScoreSet();
}
double weight = keyWeight.getWeight();
for (AbstractOrderedSetEntry entry : set.members.values()) {
Review comment:
Minor thing, but the code in this for loop could be made a little more
readable by changing the if to and if/else, removing the `continue` and having
the if condition be `if (existingValue != null)`. This puts the smaller block
of code at the top of the if statement, which improves readability, and makes
it clear that there are two possible cases with two different behaviours rather
than an if followed by other code, which could imply that the code outside the
if is expected to execute regardless of how the if condition evaluates.
##########
File path:
geode-for-redis/src/test/java/org/apache/geode/redis/internal/data/RedisSortedSetTest.java
##########
@@ -153,6 +157,58 @@ public void zadd_stores_delta_that_is_stable() {
assertThat(sortedSet1.hasDelta()).isFalse();
}
+ @Test
+ public void zstoreoperations_stores_delta_that_is_stable() {
Review comment:
This test and the one below it should probably be renamed
"sortedSetOpStoreResult_*" as there is no method named "zstoreoperations".
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -463,18 +509,24 @@ public long zrevrank(byte[] member) {
return null;
}
- public long zunionstore(RegionProvider regionProvider, RedisKey key,
List<ZKeyWeight> keyWeights,
+ public static long zunionstore(RegionProvider regionProvider, RedisKey key,
+ List<ZKeyWeight> keyWeights,
ZAggregator aggregator) {
+ MemberMap unionMembers = null;
+ ScoreSet unionScores = null;
Review comment:
Rather than declaring these as null and only assigning them if we find a
set, I think it might be better to create them up front and avoid them ever
being null when passed into `sortedSetOpStoreResult()`. While this doesn't
allow an initial size to be set for `unionMembers`, that's not much of a loss
of optimization, since even if we do use the first set we find to set an
initial size, it's entirely possible that the initial size is far smaller than
the final size and so we end up doing a bunch of resizes anyway.
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -323,18 +339,48 @@ public long zinterstore(RegionProvider regionProvider,
RedisKey key, List<ZKeyWe
}
}
}
+
if (addToSet) {
- memberAdd(member, newScore);
+ OrderedSetEntry entry = interMembers.get(member);
+ if (entry == null) {
Review comment:
This check will always return true, because we are iterating the
contents of `smallestSet` (which we know doesn't contain any duplicate entries)
and only adding to `interMembers` in one place, so it's not possible that it
will already contain the member.
--
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]
