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: [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]
