Yuti-G commented on code in PR #974:
URL: https://github.com/apache/lucene/pull/974#discussion_r909227446


##########
lucene/facet/src/java/org/apache/lucene/facet/range/RangeFacetCounts.java:
##########
@@ -232,20 +233,43 @@ public FacetResult getAllChildren(String dim, String... 
path) throws IOException
     return new FacetResult(dim, path, totCount, labelValues, 
labelValues.length);
   }
 
-  // The current getTopChildren method is not returning "top" ranges. Instead, 
it returns all
-  // user-provided ranges in
-  // the order the user specified them when instantiating. This concept is 
being introduced and
-  // supported in the
-  // getAllChildren functionality in LUCENE-10550. getTopChildren is 
temporarily calling
-  // getAllChildren to maintain its
-  // current behavior, and the current implementation will be replaced by an 
actual "top children"
-  // implementation
-  // in LUCENE-10614
-  // TODO: fix getTopChildren in LUCENE-10614
   @Override
   public FacetResult getTopChildren(int topN, String dim, String... path) 
throws IOException {
     validateTopN(topN);
-    return getAllChildren(dim, path);
+    validateDimAndPathForGetChildren(dim, path);
+
+    int resultSize = Math.min(topN, counts.length);
+    PriorityQueue<LabelAndValue> pq =
+        new PriorityQueue<>(resultSize) {
+          @Override
+          protected boolean lessThan(LabelAndValue a, LabelAndValue b) {
+            int cmp = Integer.compare(a.value.intValue(), b.value.intValue());
+            if (cmp == 0) {
+              cmp = b.label.compareTo(a.label);
+            }
+            return cmp < 0;
+          }
+        };
+
+    for (int i = 0; i < counts.length; i++) {
+      if (pq.size() < resultSize) {
+        pq.add(new LabelAndValue(ranges[i].label, counts[i]));

Review Comment:
   True, I think this can actually make a good use case for users who want to 
get all results regardless of the count. If users want to get FacetResults with 
count > 0, they can call `getTopChildren` instead. I will open a spin-off issue 
to change `getAllChildren` to also return children with a zero-count in all 
Facets implementation, and add more to the `getAllChildren` javadoc in Facets 
to explain this.



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

Reply via email to