[GitHub] [lucene] Yuti-G commented on a diff in pull request #779: LUCENE-10488: Optimize Facets#getTopDims in IntTaxonomyFacets

[GitBox <gi...@apache.org>]

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


##########
lucene/facet/src/java/org/apache/lucene/facet/taxonomy/IntTaxonomyFacets.java:
##########
@@ -169,18 +176,54 @@ public FacetResult getTopChildren(int topN, String dim, String... path) throws I
       return null;
     }
 
-    TopOrdAndIntQueue q = new TopOrdAndIntQueue(Math.min(taxoReader.getSize(), topN));
+    ChildOrdsResult childOrdsResult = getChildOrdsResult(dimConfig, dimOrd, topN);
+
+    if (childOrdsResult.q == null || childOrdsResult.aggregatedValue == 0) {
+      return null;
+    }
 
+    LabelAndValue[] labelValues = getLabelValues(childOrdsResult.q, cp.length);
+    return new FacetResult(
+        dim, path, childOrdsResult.aggregatedValue, labelValues, childOrdsResult.childCount);
+  }
+
+  /**
+   * Returns label values for dims This portion of code is moved from getTopChildren because
+   * getTopDims needs to reuse it
+   */
+  private LabelAndValue[] getLabelValues(TopOrdAndIntQueue q, int facetLabelLength)

Review Comment:
   Sorry for the confusion. I passed in the "parent" `FacetLabel` in the latest commit. Thanks!



##########
lucene/facet/src/java/org/apache/lucene/facet/taxonomy/TaxonomyFacets.java:
##########
@@ -109,7 +109,23 @@ public boolean childrenLoaded() {
    * @lucene.experimental
    */
   public boolean siblingsLoaded() {
-    return children != null;
+    return siblings != null;
+  }
+
+  /**
+   * Returns existing int[] mapping each ordinal to its next sibling to avoid re-creating int[] for
+   * siblings in subclass
+   */
+  public int[] getExistingSiblings() throws IOException {
+    return childrenLoaded() ? this.siblings : getSiblings();
+  }
+
+  /**
+   * Returns existing int[] mapping each ordinal to its first child to avoid re-creating int[] for
+   * children in subclass
+   */
+  public int[] getExistingChildren() throws IOException {
+    return childrenLoaded() ? this.children : getChildren();

Review Comment:
   I am not sure what I was thinking at that time, probably was trying to utilize and test the siblingsLoaded() and childrenLoaded() methods and emphasize that getTopDims will not re-initialize the costly int[] for children and siblings. Had removed these two methods in the latest commit. Thanks!



-- 
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-unsubscribe@lucene.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org