mikemccand commented on a change in pull request #288:
URL: https://github.com/apache/lucene/pull/288#discussion_r705520199



##########
File path: 
lucene/facet/src/java/org/apache/lucene/facet/taxonomy/IntTaxonomyFacets.java
##########
@@ -253,4 +257,78 @@ public FacetResult getTopChildren(int topN, String dim, 
String... path) throws I
 
     return new FacetResult(dim, path, totValue, labelValues, childCount);
   }
+
+  /**
+   * Class that uses FixedBitSet to store counts for all ordinals with 1 count 
and IntIntHashMap for
+   * all other counts
+   */
+  private static class IntIntHashMapWithFixedBitSet implements 
Iterable<IntIntCursor> {
+    // if the key exists, fixedBitSet[key] will be true, if fixedBitSet[key] 
is true but the key in
+    // intIntHashMap
+    // does not exist, then the value is 1
+    private final FixedBitSet fixedBitSet;
+    private final IntIntHashMap intIntHashMap;
+
+    IntIntHashMapWithFixedBitSet(int numCategories) {
+      fixedBitSet = new FixedBitSet(numCategories);
+      intIntHashMap = new IntIntHashMap();
+    }
+
+    public int addTo(int key, int incrementValue) {
+      if (!fixedBitSet.getAndSet(key) && incrementValue == 1) {
+        return 1;
+      }
+      int currentValue = intIntHashMap.addTo(key, incrementValue);
+      if (currentValue == 1) {
+        intIntHashMap.remove(key);

Review comment:
       Eeeek, how would this happen?  I think `incrementValue` must always be 
`> 0`?  If it is `1` we short-circuit in the above `if`.

##########
File path: 
lucene/facet/src/java/org/apache/lucene/facet/taxonomy/IntTaxonomyFacets.java
##########
@@ -253,4 +257,78 @@ public FacetResult getTopChildren(int topN, String dim, 
String... path) throws I
 
     return new FacetResult(dim, path, totValue, labelValues, childCount);
   }
+
+  /**
+   * Class that uses FixedBitSet to store counts for all ordinals with 1 count 
and IntIntHashMap for
+   * all other counts
+   */
+  private static class IntIntHashMapWithFixedBitSet implements 
Iterable<IntIntCursor> {
+    // if the key exists, fixedBitSet[key] will be true, if fixedBitSet[key] 
is true but the key in
+    // intIntHashMap
+    // does not exist, then the value is 1
+    private final FixedBitSet fixedBitSet;
+    private final IntIntHashMap intIntHashMap;
+
+    IntIntHashMapWithFixedBitSet(int numCategories) {
+      fixedBitSet = new FixedBitSet(numCategories);
+      intIntHashMap = new IntIntHashMap();
+    }
+
+    public int addTo(int key, int incrementValue) {
+      if (!fixedBitSet.getAndSet(key) && incrementValue == 1) {

Review comment:
       Can you use `fixedBitSet.getAndSet(key) == false` instead, to reduce 
chance of future refactoring bugs?

##########
File path: 
lucene/facet/src/java/org/apache/lucene/facet/taxonomy/IntTaxonomyFacets.java
##########
@@ -253,4 +257,78 @@ public FacetResult getTopChildren(int topN, String dim, 
String... path) throws I
 
     return new FacetResult(dim, path, totValue, labelValues, childCount);
   }
+
+  /**
+   * Class that uses FixedBitSet to store counts for all ordinals with 1 count 
and IntIntHashMap for
+   * all other counts
+   */
+  private static class IntIntHashMapWithFixedBitSet implements 
Iterable<IntIntCursor> {
+    // if the key exists, fixedBitSet[key] will be true, if fixedBitSet[key] 
is true but the key in
+    // intIntHashMap
+    // does not exist, then the value is 1
+    private final FixedBitSet fixedBitSet;
+    private final IntIntHashMap intIntHashMap;
+
+    IntIntHashMapWithFixedBitSet(int numCategories) {
+      fixedBitSet = new FixedBitSet(numCategories);
+      intIntHashMap = new IntIntHashMap();
+    }
+
+    public int addTo(int key, int incrementValue) {
+      if (!fixedBitSet.getAndSet(key) && incrementValue == 1) {
+        return 1;
+      }
+      int currentValue = intIntHashMap.addTo(key, incrementValue);
+      if (currentValue == 1) {
+        intIntHashMap.remove(key);
+      }
+      return currentValue;
+    }
+
+    public int get(int key) {
+      if (fixedBitSet.get(key)) {
+        return intIntHashMap.getOrDefault(key, 1);

Review comment:
       Maybe it would be better if the bitset was used only for the "precisely 
== 1" case?  Then we could avoid checking hash map if we see the bit is set.  
Hmm, although then iteration is hairy since you'd have to do a merge sort of 
the bits and the hash map keys .. nevermind!




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