gsmiller commented on a change in pull request #718: URL: https://github.com/apache/lucene/pull/718#discussion_r826085655
########## File path: lucene/facet/src/java/org/apache/lucene/facet/taxonomy/IntTaxonomyFacets.java ########## @@ -173,17 +185,17 @@ public FacetResult getTopChildren(int topN, String dim, String... path) throws I if (sparseValues != null) { for (IntIntCursor c : sparseValues) { - int count = c.value; + int value = c.value; int ord = c.key; - if (parents[ord] == dimOrd && count > 0) { - totValue += count; + if (parents[ord] == dimOrd && value > 0) { + aggregatedValue = aggregationFunction.aggregate(aggregatedValue, value); childCount++; - if (count > bottomValue) { + if (value > bottomValue) { Review comment: That's right. There are a number of things actually preventing us from cleanly adding something like `min`. I had it originally but as I started looking at all the changes it would require, I backed off for the time being (especially since I don't have a concrete use-case in mind). One interesting challenge is that these facets implementations all assume the weights are positive values. There are a lot of `> 0` checks floating around the various implementations to check whether-or-not a value had any "weight" associated with it. This makes sense when using counts, but it's weird when generally associated weights with the values. So `min` started to feel a little weird and I just left it out for now. -- 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: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org