[GitHub] [lucene] mdmarshmallow commented on a change in pull request #611: LUCENE-9952: Fix dim count inaccuracies in SSDV faceting when a dim is multi-valued

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

mdmarshmallow commented on a change in pull request #611:
URL: https://github.com/apache/lucene/pull/611#discussion_r788936337



##########
File path: lucene/facet/src/java/org/apache/lucene/facet/FacetsConfig.java
##########
@@ -494,10 +498,16 @@ private void processSSDVFacetFields(
                     + facetField.path.length
                     + " components");
           }
+          if (dimConfig.multiValued && dimConfig.requireDimCount) {
+            // If non-hierarchical but multi-valued and requiring dim count, make sure to
+            // explicitly index the dimension so we can get accurate counts for it:
+            String dimString = pathToString(facetLabel.components, 1);

Review comment:
       I believe you can just use `facetField.dim` here instead of reconstructing the label.

##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/ConcurrentSortedSetDocValuesFacetCounts.java
##########
@@ -121,12 +124,26 @@ public FacetResult getTopChildren(int topN, String dim, String... path) throws I
         // means dimension was never indexed
         return null;
       }
-      return getDim(dim, null, -1, ordRange.iterator(), topN);
+      int dimOrd = ordRange.start;
+      PrimitiveIterator.OfInt childIt = ordRange.iterator();
+      if (dimConfig.multiValued && dimConfig.requireDimCount) {
+        // If the dim is multi-valued and requires dim counts, we know we've explicitly indexed
+        // the dimension and we need to skip past it so the iterator is positioned on the first
+        // child:
+        childIt.next();
+      }
+      return getDim(dimConfig, dimOrd, dim, null, -1, childIt, topN);
     }
   }
 
   private FacetResult getDim(
-      String dim, String[] path, int pathOrd, PrimitiveIterator.OfInt childOrds, int topN)
+      FacetsConfig.DimConfig dimConfig,
+      int dimOrd,

Review comment:
       I feel like passing in a `dimOrd` and a `pathOrd` here is redundant. All `pathOrd` really does is specify the path ord that we want the counts of, and it seems like `dimOrd` does the same thing. Since you are passing in `dimConfig` now, instead of the `if (pathOrd == -1)` check, you can just do `if (dimConfig.hierarchical == false)`. And then just use `pathOrd` in both cases instead of `dimOrd` in the non-hierarchical case and `pathOrd` in the hierarchical case. Also, maybe `getDim` should be renamed now (probably should have done that in LUCENE-10250), maybe to something like `getPath`? Since it isn't only limited to dims anymore.




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