gsmiller commented on code in PR #914:
URL: https://github.com/apache/lucene/pull/914#discussion_r903092231
##########
lucene/facet/src/java/org/apache/lucene/facet/taxonomy/IntTaxonomyFacets.java:
##########
@@ -163,6 +164,76 @@ public Number getSpecificValue(String dim, String... path)
throws IOException {
return getValue(ord);
}
+ @Override
+ public FacetResult getAllChildren(String dim, String... path) throws
IOException {
+ DimConfig dimConfig = verifyDim(dim);
+ FacetLabel cp = new FacetLabel(dim, path);
+ int dimOrd = taxoReader.getOrdinal(cp);
+ if (dimOrd == -1) {
+ return null;
+ }
+
+ int aggregatedValue = 0;
+ int childCount = 0;
+
+ List<Integer> ordinals = new ArrayList<>();
+ List<Integer> ordValues = new ArrayList<>();
+
+ if (sparseValues != null) {
+ for (IntIntCursor c : sparseValues) {
+ int value = c.value;
+ int ord = c.key;
+ if (parents[ord] == dimOrd && value > 0) {
+ aggregatedValue = aggregationFunction.aggregate(aggregatedValue,
value);
+ childCount++;
+ ordinals.add(ord);
+ ordValues.add(value);
+ }
+ }
+ } else {
+ int[] children = getChildren();
+ int[] siblings = getSiblings();
+ int ord = children[dimOrd];
+ while (ord != TaxonomyReader.INVALID_ORDINAL) {
+ int value = values[ord];
+ if (value > 0) {
+ aggregatedValue = aggregationFunction.aggregate(aggregatedValue,
value);
+ childCount++;
+ ordinals.add(ord);
+ ordValues.add(value);
+ }
+ ord = siblings[ord];
+ }
+ }
+
+ if (aggregatedValue == 0) {
+ return null;
+ }
+
+ if (dimConfig.multiValued) {
+ if (dimConfig.requireDimCount) {
+ aggregatedValue = getValue(dimOrd);
+ } else {
+ // Our sum'd value is not correct, in general:
+ aggregatedValue = -1;
+ }
+ } else {
+ // Our sum'd dim value is accurate, so we keep it
+ }
+
+ int[] ordinalArray = new int[ordinals.size()];
+ for (int i = 0; i < ordinals.size(); i++) {
+ ordinalArray[i] = ordinals.get(i);
+ }
Review Comment:
Ah, I see. Shoot. It bugs me that we need to copy these ordinals from a list
to an array just to do this bulk path lookup, but I see what you're saying. It
would be nice if `TaxonomyReader` could directly support `List` in addition to
an array, but I don't think this use-case justifies trying to add that right
now. Would you mind adding a `TODO` comment here to mention that it would be
nice if we didn't need to do this copy just to look up bulk paths? We can leave
it at that for now and optimize later if/as necessary. Thanks for pointing this
out!
##########
lucene/facet/src/java/org/apache/lucene/facet/sortedset/AbstractSortedSetDocValueFacetCounts.java:
##########
@@ -72,6 +72,40 @@ public FacetResult getTopChildren(int topN, String dim,
String... path) throws I
return createFacetResult(topChildrenForPath, dim, path);
}
+ @Override
+ public FacetResult getAllChildren(String dim, String... path) throws
IOException {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+
+ if (dimConfig.hierarchical) {
+ int pathOrd = (int) dv.lookupTerm(new
BytesRef(FacetsConfig.pathToString(dim, path)));
+ if (pathOrd < 0) {
+ // path was never indexed
+ return null;
+ }
+ SortedSetDocValuesReaderState.DimTree dimTree = state.getDimTree(dim);
+ return getPathResult(dimConfig, dim, path, pathOrd,
dimTree.iterator(pathOrd));
+ } else {
+ if (path.length > 0) {
+ throw new IllegalArgumentException(
+ "Field is not configured as hierarchical, path should be 0
length");
+ }
+ OrdRange ordRange = state.getOrdRange(dim);
+ if (ordRange == null) {
+ // means dimension was never indexed
+ return null;
+ }
+ 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 getPathResult(dimConfig, dim, null, dimOrd, childIt);
+ }
+ }
Review Comment:
Of course! There was a lot of change happening while you were working on
this, so I'm sure you were working against an earlier version and just didn't
notice some of the change to getTopChildren. Happy to point them out.
##########
lucene/facet/src/java/org/apache/lucene/facet/Facets.java:
##########
@@ -29,6 +29,12 @@ public abstract class Facets {
/** Default constructor. */
public Facets() {}
+ /**
+ * Returns all the children labels with non-zero counts under the specified
path in the unsorted
+ * order. Returns null if the specified path doesn't exist or if this
dimension was never seen.
+ */
Review Comment:
Sorry to be picky about this, but since it's part of the externally facing
API/javadoc, I think it's important to scrutinize a little bit. What do you
think of:
```suggestion
/**
* Returns all child labels with non-zero counts under the specified path.
Users should make no
* assumptions about ordering of the children. Returns null if the
specified path doesn't exist or
* if this dimension was never seen.
*/
```
--
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
