dschneider-pivotal commented on a change in pull request #7236:
URL: https://github.com/apache/geode/pull/7236#discussion_r783283487



##########
File path: 
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -127,6 +128,50 @@ private static MemberSet calculateDiff(RegionProvider 
regionProvider, List<Redis
     return diff;
   }
 
+  public static Set<byte[]> sinter(RegionProvider regionProvider, 
List<RedisKey> keys) {
+    List<RedisSet> sets = createRedisSetList(keys, regionProvider);
+    RedisSet smallestSet = findSmallest(sets);
+    if (smallestSet.scard() == 0) {
+      return Collections.emptySet();
+    }
+
+    MemberSet result = new MemberSet(smallestSet.scard());
+    for (byte[] member : smallestSet.members) {
+      boolean addToSet = true;
+      for (RedisSet otherSet : sets)

Review comment:
       add a curly bracket to this "for" loop for its body. Our coding standard 
requires we always add brackets even if a body has one statement

##########
File path: 
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -127,6 +128,50 @@ private static MemberSet calculateDiff(RegionProvider 
regionProvider, List<Redis
     return diff;
   }
 
+  public static Set<byte[]> sinter(RegionProvider regionProvider, 
List<RedisKey> keys) {
+    List<RedisSet> sets = createRedisSetList(keys, regionProvider);
+    RedisSet smallestSet = findSmallest(sets);
+    if (smallestSet.scard() == 0) {
+      return Collections.emptySet();
+    }
+
+    MemberSet result = new MemberSet(smallestSet.scard());
+    for (byte[] member : smallestSet.members) {
+      boolean addToSet = true;
+      for (RedisSet otherSet : sets)
+        if (!otherSet.equals(smallestSet) && 
!otherSet.members.contains(member)) {

Review comment:
       don't use "equals()" here. It is too expensive. I think `if (otherSet == 
smallestSet) {continue;}` is what you want. This is just an optimization (the 
code would work without this check) but it is waste of time. But by using 
"equals()" we even waste more time.

##########
File path: 
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -127,6 +128,50 @@ private static MemberSet calculateDiff(RegionProvider 
regionProvider, List<Redis
     return diff;
   }
 
+  public static Set<byte[]> sinter(RegionProvider regionProvider, 
List<RedisKey> keys) {
+    List<RedisSet> sets = createRedisSetList(keys, regionProvider);
+    RedisSet smallestSet = findSmallest(sets);

Review comment:
       add "final" just to make clear "smallestSet" will not be changing




-- 
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]


Reply via email to