dcapwell commented on code in PR #6:
URL: https://github.com/apache/cassandra-accord/pull/6#discussion_r943025600


##########
accord-core/src/main/java/accord/primitives/Keys.java:
##########
@@ -0,0 +1,449 @@
+package accord.primitives;
+
+import java.util.*;
+import java.util.function.IntFunction;
+import java.util.function.Predicate;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import accord.api.Key;
+import accord.utils.IndexedFold;
+import accord.utils.IndexedFoldIntersectToLong;
+import accord.utils.IndexedFoldToLong;
+import accord.utils.IndexedPredicate;
+import accord.utils.IndexedRangeFoldToLong;
+import accord.utils.SortedArrays;
+import org.apache.cassandra.utils.concurrent.Inline;
+
+@SuppressWarnings("rawtypes")
+// TODO: this should probably be a BTree
+// TODO: check that foldl call-sites are inlined and optimised by HotSpot
+public class Keys implements Iterable<Key>
+{
+    public static final Keys EMPTY = new Keys(new Key[0]);
+
+    final Key[] keys;
+
+    public Keys(SortedSet<? extends Key> keys)
+    {
+        this.keys = keys.toArray(Key[]::new);
+    }
+
+    public Keys(Collection<? extends Key> keys)
+    {
+        this.keys = keys.toArray(Key[]::new);
+        Arrays.sort(this.keys);
+    }
+
+    public Keys(Key[] keys)
+    {
+        this.keys = keys;
+        Arrays.sort(keys);
+    }
+
+    @Override
+    public boolean equals(Object o)
+    {
+        if (this == o) return true;
+        if (o == null || getClass() != o.getClass()) return false;
+        Keys keys1 = (Keys) o;
+        return Arrays.equals(keys, keys1.keys);
+    }
+
+    @Override
+    public int hashCode()
+    {
+        return Arrays.hashCode(keys);
+    }
+
+    public int indexOf(Key key)
+    {
+        return Arrays.binarySearch(keys, key);
+    }
+
+    public boolean contains(Key key)
+    {
+        return indexOf(key) >= 0;
+    }
+
+    public Key get(int indexOf)
+    {
+        return keys[indexOf];
+    }
+
+    public boolean isEmpty()
+    {
+        return keys.length == 0;
+    }
+
+    public int size()
+    {
+        return keys.length;
+    }
+
+    public Keys select(int[] indexes)
+    {
+        Key[] selection = new Key[indexes.length];
+        for (int i = 0 ; i < indexes.length ; ++i)
+            selection[i] = keys[indexes[i]];
+        return new Keys(selection);
+    }
+
+    /**
+     * return true if this keys collection contains all keys found in the 
given keys
+     */
+    public boolean containsAll(Keys that)
+    {
+        if (that.isEmpty())
+            return true;
+
+        return foldlIntersect(that, (li, ri, k, p, v) -> v + 1, 0, 0, 0) == 
that.size();
+    }
+
+    public Keys union(Keys that)
+    {
+        return wrap(SortedArrays.linearUnion(keys, that.keys, Key[]::new), 
that);
+    }
+
+    public Keys intersect(Keys that)
+    {
+        return wrap(SortedArrays.linearIntersection(keys, that.keys, 
Key[]::new), that);
+    }
+
+    public Keys slice(KeyRanges ranges)
+    {
+        return wrap(SortedArrays.sliceWithOverlaps(keys, ranges.ranges, 
Key[]::new, (k, r) -> -r.compareTo(k), KeyRange::compareTo));

Review Comment:
   in trying to review `SortedArrays.sliceWithOverlaps` I figured it would be 
easier to test this method rather than add tests for 
`SortedArrays.sliceWithOverlaps` directly, but got bitten by the fact that 
nothing forces `KeyRanges` to be sorted, and overlapping ranges don't have a 
clear sort order.



##########
accord-core/src/main/java/accord/primitives/KeyRanges.java:
##########
@@ -0,0 +1,395 @@
+package accord.primitives;
+
+import accord.api.Key;
+import accord.api.RoutingKey;
+import accord.utils.SortedArrays;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.Iterators;
+
+import java.util.*;
+
+public class KeyRanges implements Iterable<KeyRange>
+{
+    public static final KeyRanges EMPTY = new KeyRanges(new KeyRange[0]);
+
+    // TODO: fix raw parameterized use
+    final KeyRange[] ranges;
+
+    public KeyRanges(KeyRange[] ranges)
+    {
+        Preconditions.checkNotNull(ranges);
+        this.ranges = ranges;

Review Comment:
   `Keys.slice` has a requirement that these are sorted, yet I don't see any 
code that enforces this... the only caller I see that actually builds ranges is 
`accord.topology.Topology#Topology(long, accord.topology.Shard...)` but that is 
the user... so should this sort the ranges? And if so how? 
   
   Just pushed tests for `Keys.slice` and hitting the fact that overlapping 
ranges tricks it, so it starts to filter out even though the ranges include all 
Ints



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


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to