You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2022/03/14 20:16:14 UTC
[GitHub] [lucene] Yuti-G opened a new pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Yuti-G opened a new pull request #747:
URL: https://github.com/apache/lucene/pull/747
# Description
This change
* adds a new API - getTopDims in Facets to support users specify the number of dims and children they want to get, and returns only these dims and children.
* override getTopDims in SortedSetDocValuesFacetCounts to optimize the current method of getting dimCount, return FacetResult and resolve child paths for only the requested dims.
# Solution
* Implement a default getTopDims function in the Facets class.
* Override getTopDims and refactor the getPathResult function in SortedSetDocValuesFacetCounts to get dimCount (aggregated dim values) more efficiently by checking if dimCount has been populated in indexing time (setRequireDimCount == true) before accumulating dimCount using a while loop.
* Use priority queue to store the requested top n dims and populate labels and returns FacetResult for those dims.
# Tests
Added new testing for both default and overridden implementations of getTopDims
# Checklist
Please review the following and check all that apply:
- [X] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/lucene/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [X] I have created a Jira issue and added the issue ID to my pull request title.
- [X] I have given Lucene maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [X] I have developed this patch against the `main` branch.
- [X] I have run `./gradlew check`.
- [X] I have added tests for my changes.
--
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
[GitHub] [lucene] Yuti-G commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r830539828
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -411,7 +484,109 @@ public int compare(FacetResult a, FacetResult b) {
}
}
});
-
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<DimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(DimValueResult a, DimValueResult b) {
+ if (a.value > b.value) {
+ return false;
+ } else if (a.value < b.value) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, ChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
+ int dimCount;
+
+ for (String dim : state.getDims()) {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ // get dim value
+ dimCount =
+ getDimValue(
+ dimConfig, dim, dimOrd, dimTree.iterator(), topNChildren, cacheChildOrdsResult);
+ } else {
+ OrdRange ordRange = state.getOrdRange(dim);
+ 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();
+ }
+ dimCount = getDimValue(dimConfig, dim, dimOrd, childIt, topNChildren, cacheChildOrdsResult);
+ }
+
+ if (dimCount != 0) {
+ // use priority queue to store DimValueResult for topNDims
+ if (pq.size() < topNDims) {
+ pq.insertWithOverflow(new DimValueResult(dim, dimCount));
+ } else {
+ if (dimCount > pq.top().value
+ || (dimCount == pq.top().value && dim.compareTo(pq.top().dim) < 0)) {
+ DimValueResult bottomDim = pq.pop();
+ bottomDim.dim = dim;
+ bottomDim.value = dimCount;
+ pq.insertWithOverflow(bottomDim);
+ }
+ }
+ }
+ }
+
+ // get FacetResult for topNDims
+ int resultSize = pq.size();
+ FacetResult[] results = new FacetResult[resultSize];
+
+ while (pq.size() > 0) {
+ DimValueResult dimValueResult = pq.pop();
+ FacetResult facetResult =
+ getFacetResultForDim(
+ dimValueResult.dim, topNChildren, cacheChildOrdsResult.get(dimValueResult.dim));
+ results[--resultSize] = facetResult;
Review comment:
Thanks! I should've paid more attention to the good code style :)
--
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
[GitHub] [lucene] gsmiller merged pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
gsmiller merged pull request #747:
URL: https://github.com/apache/lucene/pull/747
--
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
[GitHub] [lucene] Yuti-G commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r829346533
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
+
+ for (String dim : state.getDims()) {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ // get dim value
+ int dimCount =
+ getDimValue(
+ dimConfig, dim, dimOrd, dimTree.iterator(), topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ // use priority queue to store SortedSetDocValuesDimValueResult for topNDims
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ } else {
+ OrdRange ordRange = state.getOrdRange(dim);
+ 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();
+ }
+ int dimCount =
+ getDimValue(dimConfig, dim, dimOrd, childIt, topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ }
+ }
+
+ // get FacetResult for topNDims
+ List<FacetResult> results = new LinkedList<>();
+ while (pq.size() > 0) {
+ SortedSetDocValuesDimValueResult dimValueResult = pq.pop();
+ if (dimValueResult != null) {
+ FacetResult factResult =
+ getFacetResultForDim(dimValueResult.dim, topNChildren, cacheChildOrdsResult);
+ if (factResult != null) {
Review comment:
Ah.. sorry, I forgot to remove the null checks after changing the return type from `Number` to `int` in getDimValue. I will address this issue in the new commit. Thank you!
--
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
[GitHub] [lucene] Yuti-G commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r831558514
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -178,10 +229,23 @@ private FacetResult getPathResult(
}
}
- if (q == null) {
- return null;
+ if (dimConfig.hierarchical == false) {
Review comment:
Thanks @gsmiller! I added the code below in getDimValue to exit the function earlier if these conditions have been met.
`
if (dimConfig.hierarchical == true || (dimConfig.multiValued && dimConfig.requireDimCount)) {
return counts[dimOrd];
}
`
But if I add the same conditions to `getChildOrdsResult`, it would be a duplicate check when calling getChildOrdsResult from `getDimValue`.
`GetPathResult` is another method that calls `getChildOrdsResult`, and I think the first condition in the return statement plus the existing check in `getChildOrdsResult` handles all the conditions. Please correct me if I am wrong. Thanks again! :)
<img width="851" alt="Screen Shot 2022-03-21 at 2 17 52 PM" src="https://user-images.githubusercontent.com/44444710/159365382-52d86138-4092-4aec-a811-f41c579c64fb.png">
--
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
[GitHub] [lucene] Yuti-G commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r830539916
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -411,7 +484,109 @@ public int compare(FacetResult a, FacetResult b) {
}
}
});
-
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<DimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(DimValueResult a, DimValueResult b) {
+ if (a.value > b.value) {
+ return false;
+ } else if (a.value < b.value) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, ChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
+ int dimCount;
+
+ for (String dim : state.getDims()) {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ // get dim value
+ dimCount =
+ getDimValue(
+ dimConfig, dim, dimOrd, dimTree.iterator(), topNChildren, cacheChildOrdsResult);
+ } else {
+ OrdRange ordRange = state.getOrdRange(dim);
+ 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();
+ }
+ dimCount = getDimValue(dimConfig, dim, dimOrd, childIt, topNChildren, cacheChildOrdsResult);
+ }
+
+ if (dimCount != 0) {
+ // use priority queue to store DimValueResult for topNDims
+ if (pq.size() < topNDims) {
+ pq.insertWithOverflow(new DimValueResult(dim, dimCount));
+ } else {
+ if (dimCount > pq.top().value
+ || (dimCount == pq.top().value && dim.compareTo(pq.top().dim) < 0)) {
+ DimValueResult bottomDim = pq.pop();
+ bottomDim.dim = dim;
+ bottomDim.value = dimCount;
+ pq.insertWithOverflow(bottomDim);
+ }
+ }
+ }
+ }
+
+ // get FacetResult for topNDims
+ int resultSize = pq.size();
+ FacetResult[] results = new FacetResult[resultSize];
+
+ while (pq.size() > 0) {
+ DimValueResult dimValueResult = pq.pop();
+ FacetResult facetResult =
+ getFacetResultForDim(
+ dimValueResult.dim, topNChildren, cacheChildOrdsResult.get(dimValueResult.dim));
+ results[--resultSize] = facetResult;
+ }
+ return Arrays.asList(results);
+ }
+
+ /**
+ * Creates ChildOrdsResult to store dimCount, childCount, and TopOrdAndIntQueue q for
+ * getPathResult.
Review comment:
Thanks! Added it to all javadoc.
--
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
[GitHub] [lucene] gsmiller commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
gsmiller commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r826448307
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +493,107 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // get topNDims by their values
+ SortedSetDocValueDimValuePriorityQueue pq =
+ new SortedSetDocValueDimValuePriorityQueue(topNDims);
+ cacheChildOrdsResult = new HashMap<>();
+ for (String dim : state.getDims()) {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ // get dim value
+ Number value = getDimValue(dimConfig, dim, dimOrd, dimTree.iterator(), topNChildren);
+ if (value != null) {
+ // use priority queue to store SortedSetDocValuesDimValueResult for topNDims
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, value));
+ }
+ } else {
+ OrdRange ordRange = state.getOrdRange(dim);
+ 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();
+ }
+ Number value = getDimValue(dimConfig, dim, dimOrd, childIt, topNChildren);
+ if (value != null) {
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, value));
+ }
+ }
+ }
+
+ // get FacetResult for topNDims
+ List<FacetResult> results = new LinkedList<>();
+ while (pq.size() > 0) {
+ SortedSetDocValuesDimValueResult dimValueResult = pq.pop();
+ if (dimValueResult != null) {
+ FacetResult factResult = getFacetResultForDim(dimValueResult.dim, topNChildren);
+ if (factResult != null) {
+ results.add(0, factResult);
+ }
+ }
+ }
+ // free cacheChildOrdsResult after getting results
+ cacheChildOrdsResult = null;
+ return results;
+ }
+
+ /**
+ * Creates SortedSetDocValuesChildOrdsResult to store dimCount, childCount, and TopOrdAndIntQueue
+ * q for getPathResult.
+ */
+ private class SortedSetDocValuesChildOrdsResult {
+ final int dimCount;
+ final int childCount;
+ final TopOrdAndIntQueue q;
+
+ SortedSetDocValuesChildOrdsResult(int dimCount, int childCount, TopOrdAndIntQueue q) {
+ this.dimCount = dimCount;
+ this.childCount = childCount;
+ this.q = q;
+ }
+ }
+
+ /**
+ * Creates SortedSetDocValuesDimValueResult to store the label and count of dim in order to sort
+ * by these two fields.
+ */
+ private class SortedSetDocValuesDimValueResult {
+ final String dim;
+ final Number value;
+
+ SortedSetDocValuesDimValueResult(String dim, Number value) {
+ this.dim = dim;
+ this.value = value;
+ }
+ }
+
+ /**
+ * Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ * string values.
+ */
+ private class SortedSetDocValueDimValuePriorityQueue
Review comment:
minor: you could also consider inlining this as an anonymous class since you're only using it from one location currently. There's a bit of an existing convention for doing that, but if you think this is more readable, I don't have a strong opinion.
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -190,20 +235,45 @@ private FacetResult getPathResult(
String[] parts = FacetsConfig.stringToPath(term.utf8ToString());
labelValues[i] = new LabelAndValue(parts[parts.length - 1], ordAndValue.value);
}
+ return labelValues;
+ }
- if (dimConfig.hierarchical == false) {
+ /** Returns value/count of a dimension. */
+ private Number getDimValue(
+ FacetsConfig.DimConfig dimConfig,
+ String dim,
+ int pathOrd,
+ PrimitiveIterator.OfInt childOrds,
+ int topN)
+ throws IOException {
+
+ // if dimConfig.hierarchical == true, return dimCount directly
Review comment:
Ah I see. This is because we _always_ index dim counts directly when hierarchical. Nice.
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -190,20 +235,45 @@ private FacetResult getPathResult(
String[] parts = FacetsConfig.stringToPath(term.utf8ToString());
labelValues[i] = new LabelAndValue(parts[parts.length - 1], ordAndValue.value);
}
+ return labelValues;
+ }
- if (dimConfig.hierarchical == false) {
+ /** Returns value/count of a dimension. */
+ private Number getDimValue(
Review comment:
I'm not sure abstracting to a `Number` here gets us much right? We know that we're counting as integers, so I think we can just return `int` and simplify things (and avoid object creation and unnecessary indirection). Is there something I'm maybe missing though where `Number` is helpful?
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -76,6 +79,7 @@
final String field;
final int[] counts;
+ private HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult;
Review comment:
I like the idea you've got here of memorizing partial results in the `getTopDims` case so they don't need to be recomputed (e.g., the dim counts, the child PQ, etc.). I think that's smart! I might suggest an approach though where you avoid using a member field to maintain this and instead pass the map around to the various helper methods that care about it. Providing a `null` map on input to the helper methods can indicate you don't need it populated, or that partial results aren't available. The advantage is that you don't need to remember to clear the map, etc., and I think it will be a little easier to reason about (avoiding mutable state can be a nice thing).
I've got some other comments on the diff for this class that I can follow up with, but this is sort of a main structural bit that would be nice to bottom out on before I follow up with additional feedback.
--
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
[GitHub] [lucene] Yuti-G commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r826461610
##########
File path: lucene/facet/src/test/org/apache/lucene/facet/sortedset/TestSortedSetDocValuesFacets.java
##########
@@ -104,6 +104,65 @@ public void testBasic() throws Exception {
"dim=b path=[] value=2 childCount=2\n buzz (2)\n baz (1)\n",
facets.getTopChildren(10, "b").toString());
+ // test getAllDims
+ List<FacetResult> results = facets.getAllDims(10);
+ assertEquals(2, results.size());
+ assertEquals(
+ "dim=b path=[] value=2 childCount=2\n buzz (2)\n baz (1)\n",
+ results.get(0).toString());
+ assertEquals(
+ "dim=a path=[] value=-1 childCount=3\n foo (2)\n bar (1)\n zoo (1)\n",
+ results.get(1).toString());
+
+ // test getAllDims(1, 0) with topN = 0
+ expectThrows(
Review comment:
Sure! I will create a Jira and resolve it. getAllDims(0) does throw `IllegalArgumentException` in TaxonomyFacetCounts because it calls`getTopChildren(0, dim)` and getTopChildren throws it, but the overridden implementation in SSDV does not specify it. I was not sure if the two implementations should behave the same on this exception. Thank you so much for confirming this!
The javadoc of this test has a typo, should be `// test getAllDims(0) with topN = 0`, will fix this as well.
--
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
[GitHub] [lucene] gsmiller commented on pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
gsmiller commented on pull request #747:
URL: https://github.com/apache/lucene/pull/747#issuecomment-1080958647
Thanks for running the benchmarks. The regression to taxo-faceting seems weird indeed since you didn't touch that code at all (as you point out). Maybe double-check that the only difference between your candidate and baseline repos is in fact this one change? The taxo faceting stuff can be a little noisy, but still...
I went ahead and ran benchmarks with your change on my box as well since it was easy to do and don't see any regressions jumping out at me. So I'm confident this change is ready to go. I'll work on merging and backporting here in a little bit. Thanks!
```
TaskQPS baseline StdDevQPS candidate StdDev Pct diff p-value
Prefix3 112.09 (8.0%) 106.11 (8.6%) -5.3% ( -20% - 12%) 0.041
BrowseDayOfYearSSDVFacets 13.10 (20.8%) 12.61 (19.0%) -3.7% ( -36% - 45%) 0.555
IntNRQ 119.06 (12.9%) 115.58 (18.0%) -2.9% ( -29% - 32%) 0.554
HighTermMonthSort 136.28 (15.2%) 133.62 (13.5%) -2.0% ( -26% - 31%) 0.667
HighTermDayOfYearSort 76.04 (21.2%) 74.87 (11.7%) -1.5% ( -28% - 39%) 0.775
MedSloppyPhrase 135.42 (3.0%) 133.54 (4.8%) -1.4% ( -8% - 6%) 0.275
LowSloppyPhrase 96.57 (2.8%) 95.24 (3.5%) -1.4% ( -7% - 5%) 0.170
OrNotHighLow 1002.65 (2.6%) 989.38 (2.4%) -1.3% ( -6% - 3%) 0.094
AndHighMed 217.28 (3.2%) 214.70 (2.9%) -1.2% ( -7% - 5%) 0.225
OrHighMed 151.51 (3.8%) 149.74 (2.7%) -1.2% ( -7% - 5%) 0.265
MedPhrase 76.05 (3.2%) 75.25 (2.8%) -1.1% ( -6% - 5%) 0.266
MedTerm 1667.21 (4.6%) 1652.30 (4.9%) -0.9% ( -9% - 8%) 0.550
HighSloppyPhrase 26.50 (2.8%) 26.26 (4.3%) -0.9% ( -7% - 6%) 0.442
OrHighHigh 30.91 (4.2%) 30.64 (3.0%) -0.9% ( -7% - 6%) 0.448
LowPhrase 139.61 (3.4%) 138.39 (2.9%) -0.9% ( -6% - 5%) 0.379
AndHighHigh 66.53 (4.7%) 66.12 (4.1%) -0.6% ( -8% - 8%) 0.656
HighPhrase 219.08 (4.2%) 217.79 (3.7%) -0.6% ( -8% - 7%) 0.637
Wildcard 63.45 (2.6%) 63.14 (3.1%) -0.5% ( -5% - 5%) 0.585
BrowseDateSSDVFacets 2.33 (9.8%) 2.32 (9.7%) -0.5% ( -18% - 21%) 0.879
AndHighLow 709.83 (4.9%) 706.66 (4.4%) -0.4% ( -9% - 9%) 0.762
HighTerm 1717.46 (3.7%) 1710.17 (5.5%) -0.4% ( -9% - 9%) 0.774
OrNotHighHigh 1573.17 (2.4%) 1566.71 (3.3%) -0.4% ( -5% - 5%) 0.654
LowTerm 2058.72 (3.5%) 2050.32 (3.8%) -0.4% ( -7% - 7%) 0.723
Fuzzy2 40.65 (1.5%) 40.48 (1.6%) -0.4% ( -3% - 2%) 0.417
Respell 63.85 (1.3%) 63.68 (1.7%) -0.3% ( -3% - 2%) 0.561
Fuzzy1 96.68 (1.5%) 96.52 (1.7%) -0.2% ( -3% - 3%) 0.742
OrNotHighMed 1134.72 (2.6%) 1133.02 (3.0%) -0.2% ( -5% - 5%) 0.866
OrHighLow 524.60 (3.5%) 524.69 (2.6%) 0.0% ( -5% - 6%) 0.986
LowSpanNear 14.45 (2.4%) 14.47 (2.5%) 0.1% ( -4% - 5%) 0.891
MedSpanNear 79.05 (3.0%) 79.16 (3.1%) 0.1% ( -5% - 6%) 0.889
HighSpanNear 6.91 (4.0%) 6.93 (4.1%) 0.3% ( -7% - 8%) 0.821
LowIntervalsOrdered 19.99 (4.4%) 20.06 (3.6%) 0.3% ( -7% - 8%) 0.786
MedIntervalsOrdered 26.31 (4.7%) 26.41 (3.8%) 0.4% ( -7% - 9%) 0.796
OrHighNotHigh 1077.33 (3.2%) 1082.44 (3.8%) 0.5% ( -6% - 7%) 0.669
OrHighNotLow 1396.37 (3.8%) 1404.14 (4.2%) 0.6% ( -7% - 8%) 0.664
PKLookup 169.17 (2.7%) 170.37 (3.1%) 0.7% ( -4% - 6%) 0.440
AndHighHighDayTaxoFacets 16.20 (1.9%) 16.32 (1.9%) 0.7% ( -3% - 4%) 0.229
OrHighMedDayTaxoFacets 12.05 (3.7%) 12.15 (4.0%) 0.8% ( -6% - 8%) 0.504
AndHighMedDayTaxoFacets 29.61 (2.2%) 29.91 (1.5%) 1.0% ( -2% - 4%) 0.085
HighIntervalsOrdered 11.20 (10.3%) 11.31 (9.3%) 1.1% ( -16% - 23%) 0.735
OrHighNotMed 1138.85 (3.5%) 1151.60 (3.8%) 1.1% ( -5% - 8%) 0.331
TermDTSort 144.73 (16.2%) 146.39 (11.7%) 1.1% ( -23% - 34%) 0.798
BrowseRandomLabelSSDVFacets 9.22 (7.5%) 9.38 (9.1%) 1.7% ( -13% - 19%) 0.529
BrowseRandomLabelTaxoFacets 18.62 (14.1%) 18.94 (11.5%) 1.7% ( -20% - 31%) 0.670
MedTermDayTaxoFacets 47.08 (4.3%) 47.94 (3.7%) 1.8% ( -5% - 10%) 0.150
BrowseDayOfYearTaxoFacets 22.69 (18.3%) 23.13 (15.9%) 2.0% ( -27% - 44%) 0.717
BrowseDateTaxoFacets 22.60 (18.0%) 23.07 (15.6%) 2.1% ( -26% - 43%) 0.696
BrowseMonthTaxoFacets 28.93 (19.2%) 29.54 (15.1%) 2.1% ( -27% - 45%) 0.699
BrowseMonthSSDVFacets 13.68 (21.2%) 14.25 (24.9%) 4.2% ( -34% - 63%) 0.565
HighTermTitleBDVSort 36.73 (20.6%) 39.65 (32.0%) 8.0% ( -36% - 76%) 0.349
```
--
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
[GitHub] [lucene] Yuti-G commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r827269196
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -190,20 +235,45 @@ private FacetResult getPathResult(
String[] parts = FacetsConfig.stringToPath(term.utf8ToString());
labelValues[i] = new LabelAndValue(parts[parts.length - 1], ordAndValue.value);
}
+ return labelValues;
+ }
- if (dimConfig.hierarchical == false) {
+ /** Returns value/count of a dimension. */
+ private Number getDimValue(
Review comment:
Thanks @gsmiller! The reason I wanted to return `Number` is that FacetResult takes dimCount in Number as a parameter, but it can take `int` as well.
> /** Sole constructor. */
public FacetResult(
String dim, String[] path, Number value, LabelAndValue[] labelValues, int childCount) {
this.dim = dim;
this.path = path;
this.value = value;
this.labelValues = labelValues;
this.childCount = childCount;
}
I can change the return type from `Number` to `int`. The only concern is that it would change the following behavior when `q == null` since I was trying to be consistent with the original `getPathResult` function. However, I just tested changing the return type from `Number` to `int` in my local and still passed all the tests.
<img width="421" alt="Screen Shot 2022-03-15 at 10 56 33 AM" src="https://user-images.githubusercontent.com/44444710/158441533-ecf1bb90-eb47-4fef-850f-75a934441040.png">
<img width="711" alt="Screen Shot 2022-03-15 at 10 55 33 AM" src="https://user-images.githubusercontent.com/44444710/158441360-10cb8869-a1df-4787-a11b-22a225478bc2.png">
--
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
[GitHub] [lucene] Yuti-G commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r829320811
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/Facets.java
##########
@@ -48,4 +48,13 @@ public abstract FacetResult getTopChildren(int topN, String dim, String... path)
* indexed, for example depending on the type of document.
*/
public abstract List<FacetResult> getAllDims(int topN) throws IOException;
+
+ /**
+ * Returns labels for topN dimensions and their topNChildren sorted by the number of hits that
+ * dimension matched
+ */
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
Review comment:
Thank you so much!
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -366,33 +435,55 @@ public Number getSpecificValue(String dim, String... path) throws IOException {
return counts[ord];
}
+ /** Returns FacetResult for a dimension. */
+ private FacetResult getFacetResultForDim(
+ String dim,
+ int topNChildren,
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult)
+ throws IOException {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ FacetResult fr =
+ getPathResult(
+ dimConfig,
+ dim,
+ emptyPath,
+ dimOrd,
+ dimTree.iterator(),
+ topNChildren,
+ cacheChildOrdsResult);
+ if (fr != null) {
+ return fr;
+ }
+ } else {
+ OrdRange ordRange = state.getOrdRange(dim);
+ 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();
+ }
+ FacetResult fr =
+ getPathResult(
+ dimConfig, dim, emptyPath, dimOrd, childIt, topNChildren, cacheChildOrdsResult);
+ if (fr != null) {
+ return fr;
+ }
+ }
+ return null;
+ }
Review comment:
Thanks for cleaning up code!
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -143,9 +146,49 @@ private FacetResult getPathResult(
String[] path,
int pathOrd,
PrimitiveIterator.OfInt childOrds,
- int topN)
+ int topN,
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult)
Review comment:
Thanks for the great suggestion! This is definitely more readable.
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -190,20 +234,45 @@ private FacetResult getPathResult(
String[] parts = FacetsConfig.stringToPath(term.utf8ToString());
labelValues[i] = new LabelAndValue(parts[parts.length - 1], ordAndValue.value);
}
+ return labelValues;
+ }
- if (dimConfig.hierarchical == false) {
+ /** Returns value/count of a dimension. */
+ private int getDimValue(
+ FacetsConfig.DimConfig dimConfig,
+ String dim,
+ int pathOrd,
+ PrimitiveIterator.OfInt childOrds,
+ int topN,
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult) {
+
+ // if dimConfig.hierarchical == true, return dimCount directly
+ if (dimConfig.hierarchical == true && pathOrd >= 0) {
+ return counts[pathOrd];
+ }
+
+ // if dimConfig.hierarchical == false
+ if (dimConfig.multiValued) {
// see if dimCount is actually reliable or needs to be reset
- if (dimConfig.multiValued) {
- if (dimConfig.requireDimCount) {
- dimCount = counts[pathOrd];
- } else {
- dimCount = -1; // dimCount is in accurate at this point, so set it to -1
- }
+ if (dimConfig.requireDimCount && pathOrd >= 0) {
+ return counts[pathOrd];
+ } else {
+ return -1; // dimCount is inaccurate at this point, so set it to -1
}
- return new FacetResult(dim, emptyPath, dimCount, labelValues, childCount);
- } else {
- return new FacetResult(dim, path, counts[pathOrd], labelValues, childCount);
}
+
+ // if dimCount was not aggregated at indexing time, iterate over childOrds to get dimCount
+ SortedSetDocValuesChildOrdsResult childOrdsResult = getChildOrdsResult(childOrds, topN);
+ if (childOrdsResult.q == null) {
+ return 0;
+ }
Review comment:
Yes, sorry for the negligence. Thanks!
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -190,20 +234,45 @@ private FacetResult getPathResult(
String[] parts = FacetsConfig.stringToPath(term.utf8ToString());
labelValues[i] = new LabelAndValue(parts[parts.length - 1], ordAndValue.value);
}
+ return labelValues;
+ }
- if (dimConfig.hierarchical == false) {
+ /** Returns value/count of a dimension. */
+ private int getDimValue(
+ FacetsConfig.DimConfig dimConfig,
+ String dim,
+ int pathOrd,
+ PrimitiveIterator.OfInt childOrds,
+ int topN,
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult) {
+
+ // if dimConfig.hierarchical == true, return dimCount directly
+ if (dimConfig.hierarchical == true && pathOrd >= 0) {
Review comment:
I came across a `pathOrd` check under `getTopChildren` and thought it would be safer to ensure `pathOrd` >= 0 when getting `counts[pathOrd]`, but could be redundant. I just removed it in `getDimValues` and still passed all the tests. I will update it in my next PR :) Thanks!
<img width="815" alt="Screen Shot 2022-03-17 at 10 39 54 AM" src="https://user-images.githubusercontent.com/44444710/158862245-eda0f0a3-a1fc-427d-8924-ba076976f1c8.png">
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
+
+ for (String dim : state.getDims()) {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ // get dim value
+ int dimCount =
+ getDimValue(
+ dimConfig, dim, dimOrd, dimTree.iterator(), topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ // use priority queue to store SortedSetDocValuesDimValueResult for topNDims
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ } else {
+ OrdRange ordRange = state.getOrdRange(dim);
+ 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();
+ }
+ int dimCount =
+ getDimValue(dimConfig, dim, dimOrd, childIt, topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ }
+ }
+
+ // get FacetResult for topNDims
+ List<FacetResult> results = new LinkedList<>();
+ while (pq.size() > 0) {
+ SortedSetDocValuesDimValueResult dimValueResult = pq.pop();
+ if (dimValueResult != null) {
+ FacetResult factResult =
+ getFacetResultForDim(dimValueResult.dim, topNChildren, cacheChildOrdsResult);
+ if (factResult != null) {
+ results.add(0, factResult);
+ }
+ }
+ }
+ return results;
+ }
+
+ /**
+ * Creates SortedSetDocValuesChildOrdsResult to store dimCount, childCount, and TopOrdAndIntQueue
+ * q for getPathResult.
+ */
+ private class SortedSetDocValuesChildOrdsResult {
Review comment:
Sure! Great suggestion. Thanks!
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
Review comment:
Thanks :)
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -143,9 +146,49 @@ private FacetResult getPathResult(
String[] path,
int pathOrd,
PrimitiveIterator.OfInt childOrds,
- int topN)
+ int topN,
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult)
throws IOException {
+ SortedSetDocValuesChildOrdsResult childOrdsResult;
+
+ // if getTopDims is called, get results from cacheChildOrdsResult, otherwise call
+ // getChildOrdsResult to get dimCount, childCount and TopOrdAndIntQueue q
+ if (cacheChildOrdsResult != null && cacheChildOrdsResult.containsKey(dim)) {
+ childOrdsResult = cacheChildOrdsResult.get(dim);
+ } else {
+ childOrdsResult = getChildOrdsResult(childOrds, topN);
+ }
+
+ if (childOrdsResult.q == null) {
+ return null;
+ }
+
+ LabelAndValue[] labelValues = getLabelValuesFromTopOrdAndIntQueue(childOrdsResult.q);
+
+ int dimCount = childOrdsResult.dimCount;
Review comment:
Will do. Thanks!
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
+
+ for (String dim : state.getDims()) {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ // get dim value
+ int dimCount =
+ getDimValue(
+ dimConfig, dim, dimOrd, dimTree.iterator(), topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ // use priority queue to store SortedSetDocValuesDimValueResult for topNDims
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ } else {
+ OrdRange ordRange = state.getOrdRange(dim);
+ 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();
+ }
+ int dimCount =
+ getDimValue(dimConfig, dim, dimOrd, childIt, topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ }
+ }
+
+ // get FacetResult for topNDims
+ List<FacetResult> results = new LinkedList<>();
Review comment:
Sure, fixed size array is definitely less costly. Thanks!
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
+
+ for (String dim : state.getDims()) {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ // get dim value
+ int dimCount =
+ getDimValue(
+ dimConfig, dim, dimOrd, dimTree.iterator(), topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ // use priority queue to store SortedSetDocValuesDimValueResult for topNDims
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ } else {
+ OrdRange ordRange = state.getOrdRange(dim);
+ 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();
+ }
+ int dimCount =
+ getDimValue(dimConfig, dim, dimOrd, childIt, topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ }
+ }
+
+ // get FacetResult for topNDims
+ List<FacetResult> results = new LinkedList<>();
+ while (pq.size() > 0) {
+ SortedSetDocValuesDimValueResult dimValueResult = pq.pop();
+ if (dimValueResult != null) {
+ FacetResult factResult =
+ getFacetResultForDim(dimValueResult.dim, topNChildren, cacheChildOrdsResult);
+ if (factResult != null) {
Review comment:
Ah.. sorry, I forgot to remove the null checks after changing the return type from `Number` to `int` in getDimValue. I will address this issue in the new PR. Thank you!
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
+
+ for (String dim : state.getDims()) {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ // get dim value
+ int dimCount =
+ getDimValue(
+ dimConfig, dim, dimOrd, dimTree.iterator(), topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ // use priority queue to store SortedSetDocValuesDimValueResult for topNDims
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
Review comment:
I will look into that. Thanks!
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
Review comment:
I haven't spent enough time on ordinal, and will look into this. 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
[GitHub] [lucene] Yuti-G commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r830539760
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -114,7 +116,7 @@ public FacetResult getTopChildren(int topN, String dim, String... path) throws I
return null;
}
DimTree dimTree = state.getDimTree(dim);
- return getPathResult(dimConfig, dim, path, pathOrd, dimTree.iterator(pathOrd), topN);
+ return getPathResult(dimConfig, dim, path, pathOrd, dimTree.iterator(pathOrd), topN, null);
Review comment:
Great idea, 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
[GitHub] [lucene] gsmiller commented on pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
gsmiller commented on pull request #747:
URL: https://github.com/apache/lucene/pull/747#issuecomment-1078517172
This looks good to me at this point. Since this changes the code path for existing faceting functionality, it would be a good idea to run `luceneutil` benchmarks and make sure there are no obvious regressions. I wouldn't suspect there to be, but it would be nice to catch them now if there are. @Yuti-G would you mind posting those results here?
--
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
[GitHub] [lucene] Yuti-G commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r827351556
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -76,6 +79,7 @@
final String field;
final int[] counts;
+ private HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult;
Review comment:
Thank you so much! That's a great idea. I will try this approach :)
--
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
[GitHub] [lucene] Yuti-G commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r834674496
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -178,10 +229,23 @@ private FacetResult getPathResult(
}
}
- if (q == null) {
- return null;
+ if (dimConfig.hierarchical == false) {
Review comment:
Got it. It is certainly more clear to return `childOrdsResult.dimCount` for both conditions in `getPathResult`. I just added a condition check in `getChildOrdsResult`. Please let me know if there is any question. 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
[GitHub] [lucene] Yuti-G commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r829320811
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/Facets.java
##########
@@ -48,4 +48,13 @@ public abstract FacetResult getTopChildren(int topN, String dim, String... path)
* indexed, for example depending on the type of document.
*/
public abstract List<FacetResult> getAllDims(int topN) throws IOException;
+
+ /**
+ * Returns labels for topN dimensions and their topNChildren sorted by the number of hits that
+ * dimension matched
+ */
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
Review comment:
Thank you so much!
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -366,33 +435,55 @@ public Number getSpecificValue(String dim, String... path) throws IOException {
return counts[ord];
}
+ /** Returns FacetResult for a dimension. */
+ private FacetResult getFacetResultForDim(
+ String dim,
+ int topNChildren,
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult)
+ throws IOException {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ FacetResult fr =
+ getPathResult(
+ dimConfig,
+ dim,
+ emptyPath,
+ dimOrd,
+ dimTree.iterator(),
+ topNChildren,
+ cacheChildOrdsResult);
+ if (fr != null) {
+ return fr;
+ }
+ } else {
+ OrdRange ordRange = state.getOrdRange(dim);
+ 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();
+ }
+ FacetResult fr =
+ getPathResult(
+ dimConfig, dim, emptyPath, dimOrd, childIt, topNChildren, cacheChildOrdsResult);
+ if (fr != null) {
+ return fr;
+ }
+ }
+ return null;
+ }
Review comment:
Thanks for cleaning up code!
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -143,9 +146,49 @@ private FacetResult getPathResult(
String[] path,
int pathOrd,
PrimitiveIterator.OfInt childOrds,
- int topN)
+ int topN,
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult)
Review comment:
Thanks for the great suggestion! This is definitely more readable.
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -190,20 +234,45 @@ private FacetResult getPathResult(
String[] parts = FacetsConfig.stringToPath(term.utf8ToString());
labelValues[i] = new LabelAndValue(parts[parts.length - 1], ordAndValue.value);
}
+ return labelValues;
+ }
- if (dimConfig.hierarchical == false) {
+ /** Returns value/count of a dimension. */
+ private int getDimValue(
+ FacetsConfig.DimConfig dimConfig,
+ String dim,
+ int pathOrd,
+ PrimitiveIterator.OfInt childOrds,
+ int topN,
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult) {
+
+ // if dimConfig.hierarchical == true, return dimCount directly
+ if (dimConfig.hierarchical == true && pathOrd >= 0) {
+ return counts[pathOrd];
+ }
+
+ // if dimConfig.hierarchical == false
+ if (dimConfig.multiValued) {
// see if dimCount is actually reliable or needs to be reset
- if (dimConfig.multiValued) {
- if (dimConfig.requireDimCount) {
- dimCount = counts[pathOrd];
- } else {
- dimCount = -1; // dimCount is in accurate at this point, so set it to -1
- }
+ if (dimConfig.requireDimCount && pathOrd >= 0) {
+ return counts[pathOrd];
+ } else {
+ return -1; // dimCount is inaccurate at this point, so set it to -1
}
- return new FacetResult(dim, emptyPath, dimCount, labelValues, childCount);
- } else {
- return new FacetResult(dim, path, counts[pathOrd], labelValues, childCount);
}
+
+ // if dimCount was not aggregated at indexing time, iterate over childOrds to get dimCount
+ SortedSetDocValuesChildOrdsResult childOrdsResult = getChildOrdsResult(childOrds, topN);
+ if (childOrdsResult.q == null) {
+ return 0;
+ }
Review comment:
Yes, sorry for the negligence. Thanks!
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -190,20 +234,45 @@ private FacetResult getPathResult(
String[] parts = FacetsConfig.stringToPath(term.utf8ToString());
labelValues[i] = new LabelAndValue(parts[parts.length - 1], ordAndValue.value);
}
+ return labelValues;
+ }
- if (dimConfig.hierarchical == false) {
+ /** Returns value/count of a dimension. */
+ private int getDimValue(
+ FacetsConfig.DimConfig dimConfig,
+ String dim,
+ int pathOrd,
+ PrimitiveIterator.OfInt childOrds,
+ int topN,
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult) {
+
+ // if dimConfig.hierarchical == true, return dimCount directly
+ if (dimConfig.hierarchical == true && pathOrd >= 0) {
Review comment:
I came across a `pathOrd` check under `getTopChildren` and thought it would be safer to ensure `pathOrd` >= 0 when getting `counts[pathOrd]`, but could be redundant. I just removed it in `getDimValues` and still passed all the tests. I will update it in my next PR :) Thanks!
<img width="815" alt="Screen Shot 2022-03-17 at 10 39 54 AM" src="https://user-images.githubusercontent.com/44444710/158862245-eda0f0a3-a1fc-427d-8924-ba076976f1c8.png">
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
+
+ for (String dim : state.getDims()) {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ // get dim value
+ int dimCount =
+ getDimValue(
+ dimConfig, dim, dimOrd, dimTree.iterator(), topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ // use priority queue to store SortedSetDocValuesDimValueResult for topNDims
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ } else {
+ OrdRange ordRange = state.getOrdRange(dim);
+ 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();
+ }
+ int dimCount =
+ getDimValue(dimConfig, dim, dimOrd, childIt, topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ }
+ }
+
+ // get FacetResult for topNDims
+ List<FacetResult> results = new LinkedList<>();
+ while (pq.size() > 0) {
+ SortedSetDocValuesDimValueResult dimValueResult = pq.pop();
+ if (dimValueResult != null) {
+ FacetResult factResult =
+ getFacetResultForDim(dimValueResult.dim, topNChildren, cacheChildOrdsResult);
+ if (factResult != null) {
+ results.add(0, factResult);
+ }
+ }
+ }
+ return results;
+ }
+
+ /**
+ * Creates SortedSetDocValuesChildOrdsResult to store dimCount, childCount, and TopOrdAndIntQueue
+ * q for getPathResult.
+ */
+ private class SortedSetDocValuesChildOrdsResult {
Review comment:
Sure! Great suggestion. Thanks!
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
Review comment:
Thanks :)
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -143,9 +146,49 @@ private FacetResult getPathResult(
String[] path,
int pathOrd,
PrimitiveIterator.OfInt childOrds,
- int topN)
+ int topN,
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult)
throws IOException {
+ SortedSetDocValuesChildOrdsResult childOrdsResult;
+
+ // if getTopDims is called, get results from cacheChildOrdsResult, otherwise call
+ // getChildOrdsResult to get dimCount, childCount and TopOrdAndIntQueue q
+ if (cacheChildOrdsResult != null && cacheChildOrdsResult.containsKey(dim)) {
+ childOrdsResult = cacheChildOrdsResult.get(dim);
+ } else {
+ childOrdsResult = getChildOrdsResult(childOrds, topN);
+ }
+
+ if (childOrdsResult.q == null) {
+ return null;
+ }
+
+ LabelAndValue[] labelValues = getLabelValuesFromTopOrdAndIntQueue(childOrdsResult.q);
+
+ int dimCount = childOrdsResult.dimCount;
Review comment:
Will do. Thanks!
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
+
+ for (String dim : state.getDims()) {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ // get dim value
+ int dimCount =
+ getDimValue(
+ dimConfig, dim, dimOrd, dimTree.iterator(), topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ // use priority queue to store SortedSetDocValuesDimValueResult for topNDims
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ } else {
+ OrdRange ordRange = state.getOrdRange(dim);
+ 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();
+ }
+ int dimCount =
+ getDimValue(dimConfig, dim, dimOrd, childIt, topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ }
+ }
+
+ // get FacetResult for topNDims
+ List<FacetResult> results = new LinkedList<>();
Review comment:
Sure, fixed size array is definitely less costly. Thanks!
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
+
+ for (String dim : state.getDims()) {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ // get dim value
+ int dimCount =
+ getDimValue(
+ dimConfig, dim, dimOrd, dimTree.iterator(), topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ // use priority queue to store SortedSetDocValuesDimValueResult for topNDims
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ } else {
+ OrdRange ordRange = state.getOrdRange(dim);
+ 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();
+ }
+ int dimCount =
+ getDimValue(dimConfig, dim, dimOrd, childIt, topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ }
+ }
+
+ // get FacetResult for topNDims
+ List<FacetResult> results = new LinkedList<>();
+ while (pq.size() > 0) {
+ SortedSetDocValuesDimValueResult dimValueResult = pq.pop();
+ if (dimValueResult != null) {
+ FacetResult factResult =
+ getFacetResultForDim(dimValueResult.dim, topNChildren, cacheChildOrdsResult);
+ if (factResult != null) {
Review comment:
Ah.. sorry, I forgot to remove the null checks after changing the return type from `Number` to `int` in getDimValue. I will address this issue in the new PR. Thank you!
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
+
+ for (String dim : state.getDims()) {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ // get dim value
+ int dimCount =
+ getDimValue(
+ dimConfig, dim, dimOrd, dimTree.iterator(), topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ // use priority queue to store SortedSetDocValuesDimValueResult for topNDims
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
Review comment:
I will look into that. Thanks!
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
Review comment:
I haven't spent enough time on ordinal, and will look into this. Thanks!
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
+
+ for (String dim : state.getDims()) {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ // get dim value
+ int dimCount =
+ getDimValue(
+ dimConfig, dim, dimOrd, dimTree.iterator(), topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ // use priority queue to store SortedSetDocValuesDimValueResult for topNDims
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ } else {
+ OrdRange ordRange = state.getOrdRange(dim);
+ 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();
+ }
+ int dimCount =
+ getDimValue(dimConfig, dim, dimOrd, childIt, topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ }
+ }
+
+ // get FacetResult for topNDims
+ List<FacetResult> results = new LinkedList<>();
+ while (pq.size() > 0) {
+ SortedSetDocValuesDimValueResult dimValueResult = pq.pop();
+ if (dimValueResult != null) {
+ FacetResult factResult =
+ getFacetResultForDim(dimValueResult.dim, topNChildren, cacheChildOrdsResult);
+ if (factResult != null) {
Review comment:
Ah.. sorry, I forgot to remove the null checks after changing the return type from `Number` to `int` in getDimValue. I will address this issue in the new commit. Thank you!
--
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
[GitHub] [lucene] Yuti-G commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r830539688
##########
File path: lucene/facet/src/test/org/apache/lucene/facet/sortedset/TestSortedSetDocValuesFacets.java
##########
@@ -1221,6 +1326,12 @@ public void testRandomHierarchicalFlatMix() throws Exception {
assertEquals(expectedAllDims, actualAllDims);
+ // test getTopDims(1, 10)
+ if (actualAllDims.size() > 0) {
+ List<FacetResult> topDimsResults1 = facets.getTopDims(1, 10);
Review comment:
> I didn't really have a lot of real feedback, just minor nits here and there. I did want to say that I have a PR out for a benchmark that compares the faceting API performance for SSDV vs taxonomy for high cardinality hierarchical faceting. Here is the link: [mikemccand/luceneutil#166](https://github.com/mikemccand/luceneutil/pull/166). We should add `getTopDims` to this benchmark once it is done :)
Thanks @mdmarshmallow for all the great feedback! This is my first official PR and really appreciate all the suggestions. I'd like to add `getTopDims`to benchmark once it is done and can I ask you questions if I come across any? Thanks again :)
--
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
[GitHub] [lucene] Yuti-G edited a comment on pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G edited a comment on pull request #747:
URL: https://github.com/apache/lucene/pull/747#issuecomment-1067392546
> Thanks for picking this up! I've looked at everything except for your overridden implementation in SSDV faceting, but since I may run out of time to look at that today, I'll go ahead and publish my feedback on your default implementation and testing. I'll follow up with more feedback soon. Thanks again!
Thanks @gsmiller for reviewing my PR and leaving the detailed feedback! I will address them in my next PR, appreciated :)
--
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
[GitHub] [lucene] Yuti-G commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r826461610
##########
File path: lucene/facet/src/test/org/apache/lucene/facet/sortedset/TestSortedSetDocValuesFacets.java
##########
@@ -104,6 +104,65 @@ public void testBasic() throws Exception {
"dim=b path=[] value=2 childCount=2\n buzz (2)\n baz (1)\n",
facets.getTopChildren(10, "b").toString());
+ // test getAllDims
+ List<FacetResult> results = facets.getAllDims(10);
+ assertEquals(2, results.size());
+ assertEquals(
+ "dim=b path=[] value=2 childCount=2\n buzz (2)\n baz (1)\n",
+ results.get(0).toString());
+ assertEquals(
+ "dim=a path=[] value=-1 childCount=3\n foo (2)\n bar (1)\n zoo (1)\n",
+ results.get(1).toString());
+
+ // test getAllDims(1, 0) with topN = 0
+ expectThrows(
Review comment:
Sure! I will create a Jira and resolve it. getAllDims(0) does throw `IllegalArgumentException` in TaxonomyFacetCounts because `getTopChildren(0, dim)` throws it, but the overridden implementation in SSDV does not specify it. I was not sure if the two implementations should behave the same on this exception. Thank you so much for confirming this!
The javadoc of this test has a typo, should be `// test getAllDims(0) with topN = 0`, will fix this as well.
--
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
[GitHub] [lucene] gsmiller commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
gsmiller commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r829161071
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -143,9 +146,49 @@ private FacetResult getPathResult(
String[] path,
int pathOrd,
PrimitiveIterator.OfInt childOrds,
- int topN)
+ int topN,
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult)
Review comment:
If I'm reading the code correctly, this map is used in a "read only" way here, meaning you never put an entry in the cache—you only look to see if something else previously stored results there. Also, this method is specific to a single dimension. Because of this, I would suggest just passing in a single `SortedSetDocValuesChildOrdsResult` instance instead of the map. That instance could be `null` if no results have been memorized. I think that would make the code a little more readable. My first thought was, "why isn't this cache getting populated on a 'miss' here?"
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -190,20 +234,45 @@ private FacetResult getPathResult(
String[] parts = FacetsConfig.stringToPath(term.utf8ToString());
labelValues[i] = new LabelAndValue(parts[parts.length - 1], ordAndValue.value);
}
+ return labelValues;
+ }
- if (dimConfig.hierarchical == false) {
+ /** Returns value/count of a dimension. */
+ private int getDimValue(
+ FacetsConfig.DimConfig dimConfig,
+ String dim,
+ int pathOrd,
+ PrimitiveIterator.OfInt childOrds,
+ int topN,
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult) {
+
+ // if dimConfig.hierarchical == true, return dimCount directly
+ if (dimConfig.hierarchical == true && pathOrd >= 0) {
+ return counts[pathOrd];
+ }
+
+ // if dimConfig.hierarchical == false
+ if (dimConfig.multiValued) {
// see if dimCount is actually reliable or needs to be reset
- if (dimConfig.multiValued) {
- if (dimConfig.requireDimCount) {
- dimCount = counts[pathOrd];
- } else {
- dimCount = -1; // dimCount is in accurate at this point, so set it to -1
- }
+ if (dimConfig.requireDimCount && pathOrd >= 0) {
+ return counts[pathOrd];
+ } else {
+ return -1; // dimCount is inaccurate at this point, so set it to -1
}
- return new FacetResult(dim, emptyPath, dimCount, labelValues, childCount);
- } else {
- return new FacetResult(dim, path, counts[pathOrd], labelValues, childCount);
}
+
+ // if dimCount was not aggregated at indexing time, iterate over childOrds to get dimCount
+ SortedSetDocValuesChildOrdsResult childOrdsResult = getChildOrdsResult(childOrds, topN);
+ if (childOrdsResult.q == null) {
+ return 0;
+ }
Review comment:
This can just be removed right? The `return childOrdsResult.dimCount` path below will return `0` in this case won't it?
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
Review comment:
I wonder if there's a way to use the dimension's ordinal as the key instead of the string. I haven't looked closely enough to see how difficult that would be, but if it's possible, it would be more efficient to use the hppc `IntIntHashMap` here instead of Java's HashMap.
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
Review comment:
minor: Another option for doing this that some people prefer is below. I'm happy with either so don't feel you need to change this, but thought I'd suggest it in case you hadn't seen this approach:
```suggestion
int cmp = Integer.compare(a.value.intValue(), b.value.intValue());
if (cmp == 0) {
cmp = b.dim.compareTo(a.dim);
}
return cmp < 0;
```
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
+
+ for (String dim : state.getDims()) {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ // get dim value
+ int dimCount =
+ getDimValue(
+ dimConfig, dim, dimOrd, dimTree.iterator(), topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ // use priority queue to store SortedSetDocValuesDimValueResult for topNDims
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ } else {
+ OrdRange ordRange = state.getOrdRange(dim);
+ 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();
+ }
+ int dimCount =
+ getDimValue(dimConfig, dim, dimOrd, childIt, topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ }
+ }
+
+ // get FacetResult for topNDims
+ List<FacetResult> results = new LinkedList<>();
+ while (pq.size() > 0) {
+ SortedSetDocValuesDimValueResult dimValueResult = pq.pop();
+ if (dimValueResult != null) {
Review comment:
Do we ever put `null` into the queue? We shouldn't right?
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
+
+ for (String dim : state.getDims()) {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ // get dim value
+ int dimCount =
+ getDimValue(
+ dimConfig, dim, dimOrd, dimTree.iterator(), topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ // use priority queue to store SortedSetDocValuesDimValueResult for topNDims
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ } else {
+ OrdRange ordRange = state.getOrdRange(dim);
+ 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();
+ }
+ int dimCount =
+ getDimValue(dimConfig, dim, dimOrd, childIt, topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ }
+ }
+
+ // get FacetResult for topNDims
+ List<FacetResult> results = new LinkedList<>();
+ while (pq.size() > 0) {
+ SortedSetDocValuesDimValueResult dimValueResult = pq.pop();
+ if (dimValueResult != null) {
+ FacetResult factResult =
+ getFacetResultForDim(dimValueResult.dim, topNChildren, cacheChildOrdsResult);
+ if (factResult != null) {
Review comment:
Similar question: Can these ever be `null`?
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
+
+ for (String dim : state.getDims()) {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ // get dim value
+ int dimCount =
+ getDimValue(
+ dimConfig, dim, dimOrd, dimTree.iterator(), topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ // use priority queue to store SortedSetDocValuesDimValueResult for topNDims
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
Review comment:
Could you have a look at how PQ insertion works elsewhere in this file and try to follow the same paradigm of reusing instances and inline updating the bottom value instead of always instantiating new instances of `SortedSetDocValuesDimValueResult`? We can save some object creation here.
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -366,33 +435,55 @@ public Number getSpecificValue(String dim, String... path) throws IOException {
return counts[ord];
}
+ /** Returns FacetResult for a dimension. */
+ private FacetResult getFacetResultForDim(
+ String dim,
+ int topNChildren,
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult)
+ throws IOException {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ FacetResult fr =
+ getPathResult(
+ dimConfig,
+ dim,
+ emptyPath,
+ dimOrd,
+ dimTree.iterator(),
+ topNChildren,
+ cacheChildOrdsResult);
+ if (fr != null) {
+ return fr;
+ }
+ } else {
+ OrdRange ordRange = state.getOrdRange(dim);
+ 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();
+ }
+ FacetResult fr =
+ getPathResult(
+ dimConfig, dim, emptyPath, dimOrd, childIt, topNChildren, cacheChildOrdsResult);
+ if (fr != null) {
+ return fr;
+ }
+ }
+ return null;
+ }
Review comment:
minor: You can simplify the return branches here a little bit:
```suggestion
FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
if (dimConfig.hierarchical) {
DimTree dimTree = state.getDimTree(dim);
int dimOrd = dimTree.dimStartOrd;
return getPathResult(
dimConfig,
dim,
emptyPath,
dimOrd,
dimTree.iterator(),
topNChildren,
cacheChildOrdsResult);
} else {
OrdRange ordRange = state.getOrdRange(dim);
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, emptyPath, dimOrd, childIt, topNChildren, cacheChildOrdsResult);
}
```
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -190,20 +234,45 @@ private FacetResult getPathResult(
String[] parts = FacetsConfig.stringToPath(term.utf8ToString());
labelValues[i] = new LabelAndValue(parts[parts.length - 1], ordAndValue.value);
}
+ return labelValues;
+ }
- if (dimConfig.hierarchical == false) {
+ /** Returns value/count of a dimension. */
+ private int getDimValue(
+ FacetsConfig.DimConfig dimConfig,
+ String dim,
+ int pathOrd,
+ PrimitiveIterator.OfInt childOrds,
+ int topN,
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult) {
+
+ // if dimConfig.hierarchical == true, return dimCount directly
+ if (dimConfig.hierarchical == true && pathOrd >= 0) {
Review comment:
Did you run into an issue where `pathOrd` was negative here? Curious how this check came about. I would assume lots of other things would break if we got in a situation where `pathOrd` was negative, but maybe you found some interesting case?
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
+
+ for (String dim : state.getDims()) {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ // get dim value
+ int dimCount =
+ getDimValue(
+ dimConfig, dim, dimOrd, dimTree.iterator(), topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ // use priority queue to store SortedSetDocValuesDimValueResult for topNDims
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ } else {
+ OrdRange ordRange = state.getOrdRange(dim);
+ 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();
+ }
+ int dimCount =
+ getDimValue(dimConfig, dim, dimOrd, childIt, topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ }
+ }
+
+ // get FacetResult for topNDims
+ List<FacetResult> results = new LinkedList<>();
Review comment:
Could we just use a `FacetResult[]` here instead since we know the exact size it should be? Then you can add to it in reverse like you need, and just return `Arrays.asList(results)` at the end.
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
+
+ for (String dim : state.getDims()) {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ // get dim value
+ int dimCount =
+ getDimValue(
+ dimConfig, dim, dimOrd, dimTree.iterator(), topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ // use priority queue to store SortedSetDocValuesDimValueResult for topNDims
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ } else {
+ OrdRange ordRange = state.getOrdRange(dim);
+ 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();
+ }
+ int dimCount =
+ getDimValue(dimConfig, dim, dimOrd, childIt, topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ }
+ }
+
+ // get FacetResult for topNDims
+ List<FacetResult> results = new LinkedList<>();
+ while (pq.size() > 0) {
+ SortedSetDocValuesDimValueResult dimValueResult = pq.pop();
+ if (dimValueResult != null) {
+ FacetResult factResult =
+ getFacetResultForDim(dimValueResult.dim, topNChildren, cacheChildOrdsResult);
+ if (factResult != null) {
+ results.add(0, factResult);
+ }
+ }
+ }
+ return results;
+ }
+
+ /**
+ * Creates SortedSetDocValuesChildOrdsResult to store dimCount, childCount, and TopOrdAndIntQueue
+ * q for getPathResult.
+ */
+ private class SortedSetDocValuesChildOrdsResult {
+ final int dimCount;
+ final int childCount;
+ final TopOrdAndIntQueue q;
+
+ SortedSetDocValuesChildOrdsResult(int dimCount, int childCount, TopOrdAndIntQueue q) {
+ this.dimCount = dimCount;
+ this.childCount = childCount;
+ this.q = q;
+ }
+ }
+
+ /**
+ * Creates SortedSetDocValuesDimValueResult to store the label and count of dim in order to sort
+ * by these two fields.
+ */
+ private class SortedSetDocValuesDimValueResult {
Review comment:
Please make this `static`. Also, similar minor comment about considering a less verbose name.
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -143,9 +146,49 @@ private FacetResult getPathResult(
String[] path,
int pathOrd,
PrimitiveIterator.OfInt childOrds,
- int topN)
+ int topN,
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult)
throws IOException {
+ SortedSetDocValuesChildOrdsResult childOrdsResult;
+
+ // if getTopDims is called, get results from cacheChildOrdsResult, otherwise call
+ // getChildOrdsResult to get dimCount, childCount and TopOrdAndIntQueue q
+ if (cacheChildOrdsResult != null && cacheChildOrdsResult.containsKey(dim)) {
+ childOrdsResult = cacheChildOrdsResult.get(dim);
+ } else {
+ childOrdsResult = getChildOrdsResult(childOrds, topN);
+ }
+
+ if (childOrdsResult.q == null) {
+ return null;
+ }
+
+ LabelAndValue[] labelValues = getLabelValuesFromTopOrdAndIntQueue(childOrdsResult.q);
+
+ int dimCount = childOrdsResult.dimCount;
Review comment:
I wonder if this logic would be better placed in `getChildOrdsResults`? That way `SortedSetDocValuesChildOrdsResult` would always have an accurate `dimCount` (or `-1` if an accurate value can't be computed). What do you think?
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/Facets.java
##########
@@ -48,4 +48,13 @@ public abstract FacetResult getTopChildren(int topN, String dim, String... path)
* indexed, for example depending on the type of document.
*/
public abstract List<FacetResult> getAllDims(int topN) throws IOException;
+
+ /**
+ * Returns labels for topN dimensions and their topNChildren sorted by the number of hits that
+ * dimension matched
+ */
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
Review comment:
Thanks for the update! It might be nice to also state that results should be the same as calling `getAllDims` and then only using the first `topNDims`, but may be more efficient.
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
+
+ for (String dim : state.getDims()) {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ // get dim value
+ int dimCount =
+ getDimValue(
+ dimConfig, dim, dimOrd, dimTree.iterator(), topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ // use priority queue to store SortedSetDocValuesDimValueResult for topNDims
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ } else {
+ OrdRange ordRange = state.getOrdRange(dim);
+ 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();
+ }
+ int dimCount =
+ getDimValue(dimConfig, dim, dimOrd, childIt, topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ }
+ }
+
+ // get FacetResult for topNDims
+ List<FacetResult> results = new LinkedList<>();
+ while (pq.size() > 0) {
+ SortedSetDocValuesDimValueResult dimValueResult = pq.pop();
+ if (dimValueResult != null) {
+ FacetResult factResult =
+ getFacetResultForDim(dimValueResult.dim, topNChildren, cacheChildOrdsResult);
+ if (factResult != null) {
+ results.add(0, factResult);
+ }
+ }
+ }
+ return results;
+ }
+
+ /**
+ * Creates SortedSetDocValuesChildOrdsResult to store dimCount, childCount, and TopOrdAndIntQueue
+ * q for getPathResult.
+ */
+ private class SortedSetDocValuesChildOrdsResult {
Review comment:
Please make this `static`. You could also make the name a little shorter since it's just privately scoped and it's a little redundant to say "SortedSetDocValues". Maybe just `ChildOrdsResult`?
--
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
[GitHub] [lucene] gsmiller commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
gsmiller commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r829161071
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -143,9 +146,49 @@ private FacetResult getPathResult(
String[] path,
int pathOrd,
PrimitiveIterator.OfInt childOrds,
- int topN)
+ int topN,
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult)
Review comment:
If I'm reading the code correctly, this map is used in a "read only" way here, meaning you never put an entry in the cache—you only look to see if something else previously stored results there. Also, this method is specific to a single dimension. Because of this, I would suggest just passing in a single `SortedSetDocValuesChildOrdsResult` instance instead of the map. That instance could be `null` if no results have been memorized. I think that would make the code a little more readable. My first thought was, "why isn't this cache getting populated on a 'miss' here?"
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -190,20 +234,45 @@ private FacetResult getPathResult(
String[] parts = FacetsConfig.stringToPath(term.utf8ToString());
labelValues[i] = new LabelAndValue(parts[parts.length - 1], ordAndValue.value);
}
+ return labelValues;
+ }
- if (dimConfig.hierarchical == false) {
+ /** Returns value/count of a dimension. */
+ private int getDimValue(
+ FacetsConfig.DimConfig dimConfig,
+ String dim,
+ int pathOrd,
+ PrimitiveIterator.OfInt childOrds,
+ int topN,
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult) {
+
+ // if dimConfig.hierarchical == true, return dimCount directly
+ if (dimConfig.hierarchical == true && pathOrd >= 0) {
+ return counts[pathOrd];
+ }
+
+ // if dimConfig.hierarchical == false
+ if (dimConfig.multiValued) {
// see if dimCount is actually reliable or needs to be reset
- if (dimConfig.multiValued) {
- if (dimConfig.requireDimCount) {
- dimCount = counts[pathOrd];
- } else {
- dimCount = -1; // dimCount is in accurate at this point, so set it to -1
- }
+ if (dimConfig.requireDimCount && pathOrd >= 0) {
+ return counts[pathOrd];
+ } else {
+ return -1; // dimCount is inaccurate at this point, so set it to -1
}
- return new FacetResult(dim, emptyPath, dimCount, labelValues, childCount);
- } else {
- return new FacetResult(dim, path, counts[pathOrd], labelValues, childCount);
}
+
+ // if dimCount was not aggregated at indexing time, iterate over childOrds to get dimCount
+ SortedSetDocValuesChildOrdsResult childOrdsResult = getChildOrdsResult(childOrds, topN);
+ if (childOrdsResult.q == null) {
+ return 0;
+ }
Review comment:
This can just be removed right? The `return childOrdsResult.dimCount` path below will return `0` in this case won't it?
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
Review comment:
I wonder if there's a way to use the dimension's ordinal as the key instead of the string. I haven't looked closely enough to see how difficult that would be, but if it's possible, it would be more efficient to use the hppc `IntIntHashMap` here instead of Java's HashMap.
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
Review comment:
minor: Another option for doing this that some people prefer is below. I'm happy with either so don't feel you need to change this, but thought I'd suggest it in case you hadn't seen this approach:
```suggestion
int cmp = Integer.compare(a.value.intValue(), b.value.intValue());
if (cmp == 0) {
cmp = b.dim.compareTo(a.dim);
}
return cmp < 0;
```
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
+
+ for (String dim : state.getDims()) {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ // get dim value
+ int dimCount =
+ getDimValue(
+ dimConfig, dim, dimOrd, dimTree.iterator(), topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ // use priority queue to store SortedSetDocValuesDimValueResult for topNDims
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ } else {
+ OrdRange ordRange = state.getOrdRange(dim);
+ 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();
+ }
+ int dimCount =
+ getDimValue(dimConfig, dim, dimOrd, childIt, topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ }
+ }
+
+ // get FacetResult for topNDims
+ List<FacetResult> results = new LinkedList<>();
+ while (pq.size() > 0) {
+ SortedSetDocValuesDimValueResult dimValueResult = pq.pop();
+ if (dimValueResult != null) {
Review comment:
Do we ever put `null` into the queue? We shouldn't right?
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
+
+ for (String dim : state.getDims()) {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ // get dim value
+ int dimCount =
+ getDimValue(
+ dimConfig, dim, dimOrd, dimTree.iterator(), topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ // use priority queue to store SortedSetDocValuesDimValueResult for topNDims
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ } else {
+ OrdRange ordRange = state.getOrdRange(dim);
+ 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();
+ }
+ int dimCount =
+ getDimValue(dimConfig, dim, dimOrd, childIt, topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ }
+ }
+
+ // get FacetResult for topNDims
+ List<FacetResult> results = new LinkedList<>();
+ while (pq.size() > 0) {
+ SortedSetDocValuesDimValueResult dimValueResult = pq.pop();
+ if (dimValueResult != null) {
+ FacetResult factResult =
+ getFacetResultForDim(dimValueResult.dim, topNChildren, cacheChildOrdsResult);
+ if (factResult != null) {
Review comment:
Similar question: Can these ever be `null`?
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
+
+ for (String dim : state.getDims()) {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ // get dim value
+ int dimCount =
+ getDimValue(
+ dimConfig, dim, dimOrd, dimTree.iterator(), topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ // use priority queue to store SortedSetDocValuesDimValueResult for topNDims
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
Review comment:
Could you have a look at how PQ insertion works elsewhere in this file and try to follow the same paradigm of reusing instances and inline updating the bottom value instead of always instantiating new instances of `SortedSetDocValuesDimValueResult`? We can save some object creation here.
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -366,33 +435,55 @@ public Number getSpecificValue(String dim, String... path) throws IOException {
return counts[ord];
}
+ /** Returns FacetResult for a dimension. */
+ private FacetResult getFacetResultForDim(
+ String dim,
+ int topNChildren,
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult)
+ throws IOException {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ FacetResult fr =
+ getPathResult(
+ dimConfig,
+ dim,
+ emptyPath,
+ dimOrd,
+ dimTree.iterator(),
+ topNChildren,
+ cacheChildOrdsResult);
+ if (fr != null) {
+ return fr;
+ }
+ } else {
+ OrdRange ordRange = state.getOrdRange(dim);
+ 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();
+ }
+ FacetResult fr =
+ getPathResult(
+ dimConfig, dim, emptyPath, dimOrd, childIt, topNChildren, cacheChildOrdsResult);
+ if (fr != null) {
+ return fr;
+ }
+ }
+ return null;
+ }
Review comment:
minor: You can simplify the return branches here a little bit:
```suggestion
FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
if (dimConfig.hierarchical) {
DimTree dimTree = state.getDimTree(dim);
int dimOrd = dimTree.dimStartOrd;
return getPathResult(
dimConfig,
dim,
emptyPath,
dimOrd,
dimTree.iterator(),
topNChildren,
cacheChildOrdsResult);
} else {
OrdRange ordRange = state.getOrdRange(dim);
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, emptyPath, dimOrd, childIt, topNChildren, cacheChildOrdsResult);
}
```
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -190,20 +234,45 @@ private FacetResult getPathResult(
String[] parts = FacetsConfig.stringToPath(term.utf8ToString());
labelValues[i] = new LabelAndValue(parts[parts.length - 1], ordAndValue.value);
}
+ return labelValues;
+ }
- if (dimConfig.hierarchical == false) {
+ /** Returns value/count of a dimension. */
+ private int getDimValue(
+ FacetsConfig.DimConfig dimConfig,
+ String dim,
+ int pathOrd,
+ PrimitiveIterator.OfInt childOrds,
+ int topN,
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult) {
+
+ // if dimConfig.hierarchical == true, return dimCount directly
+ if (dimConfig.hierarchical == true && pathOrd >= 0) {
Review comment:
Did you run into an issue where `pathOrd` was negative here? Curious how this check came about. I would assume lots of other things would break if we got in a situation where `pathOrd` was negative, but maybe you found some interesting case?
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
+
+ for (String dim : state.getDims()) {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ // get dim value
+ int dimCount =
+ getDimValue(
+ dimConfig, dim, dimOrd, dimTree.iterator(), topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ // use priority queue to store SortedSetDocValuesDimValueResult for topNDims
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ } else {
+ OrdRange ordRange = state.getOrdRange(dim);
+ 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();
+ }
+ int dimCount =
+ getDimValue(dimConfig, dim, dimOrd, childIt, topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ }
+ }
+
+ // get FacetResult for topNDims
+ List<FacetResult> results = new LinkedList<>();
Review comment:
Could we just use a `FacetResult[]` here instead since we know the exact size it should be? Then you can add to it in reverse like you need, and just return `Arrays.asList(results)` at the end.
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
+
+ for (String dim : state.getDims()) {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ // get dim value
+ int dimCount =
+ getDimValue(
+ dimConfig, dim, dimOrd, dimTree.iterator(), topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ // use priority queue to store SortedSetDocValuesDimValueResult for topNDims
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ } else {
+ OrdRange ordRange = state.getOrdRange(dim);
+ 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();
+ }
+ int dimCount =
+ getDimValue(dimConfig, dim, dimOrd, childIt, topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ }
+ }
+
+ // get FacetResult for topNDims
+ List<FacetResult> results = new LinkedList<>();
+ while (pq.size() > 0) {
+ SortedSetDocValuesDimValueResult dimValueResult = pq.pop();
+ if (dimValueResult != null) {
+ FacetResult factResult =
+ getFacetResultForDim(dimValueResult.dim, topNChildren, cacheChildOrdsResult);
+ if (factResult != null) {
+ results.add(0, factResult);
+ }
+ }
+ }
+ return results;
+ }
+
+ /**
+ * Creates SortedSetDocValuesChildOrdsResult to store dimCount, childCount, and TopOrdAndIntQueue
+ * q for getPathResult.
+ */
+ private class SortedSetDocValuesChildOrdsResult {
+ final int dimCount;
+ final int childCount;
+ final TopOrdAndIntQueue q;
+
+ SortedSetDocValuesChildOrdsResult(int dimCount, int childCount, TopOrdAndIntQueue q) {
+ this.dimCount = dimCount;
+ this.childCount = childCount;
+ this.q = q;
+ }
+ }
+
+ /**
+ * Creates SortedSetDocValuesDimValueResult to store the label and count of dim in order to sort
+ * by these two fields.
+ */
+ private class SortedSetDocValuesDimValueResult {
Review comment:
Please make this `static`. Also, similar minor comment about considering a less verbose name.
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -143,9 +146,49 @@ private FacetResult getPathResult(
String[] path,
int pathOrd,
PrimitiveIterator.OfInt childOrds,
- int topN)
+ int topN,
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult)
throws IOException {
+ SortedSetDocValuesChildOrdsResult childOrdsResult;
+
+ // if getTopDims is called, get results from cacheChildOrdsResult, otherwise call
+ // getChildOrdsResult to get dimCount, childCount and TopOrdAndIntQueue q
+ if (cacheChildOrdsResult != null && cacheChildOrdsResult.containsKey(dim)) {
+ childOrdsResult = cacheChildOrdsResult.get(dim);
+ } else {
+ childOrdsResult = getChildOrdsResult(childOrds, topN);
+ }
+
+ if (childOrdsResult.q == null) {
+ return null;
+ }
+
+ LabelAndValue[] labelValues = getLabelValuesFromTopOrdAndIntQueue(childOrdsResult.q);
+
+ int dimCount = childOrdsResult.dimCount;
Review comment:
I wonder if this logic would be better placed in `getChildOrdsResults`? That way `SortedSetDocValuesChildOrdsResult` would always have an accurate `dimCount` (or `-1` if an accurate value can't be computed). What do you think?
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/Facets.java
##########
@@ -48,4 +48,13 @@ public abstract FacetResult getTopChildren(int topN, String dim, String... path)
* indexed, for example depending on the type of document.
*/
public abstract List<FacetResult> getAllDims(int topN) throws IOException;
+
+ /**
+ * Returns labels for topN dimensions and their topNChildren sorted by the number of hits that
+ * dimension matched
+ */
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
Review comment:
Thanks for the update! It might be nice to also state that results should be the same as calling `getAllDims` and then only using the first `topNDims`, but may be more efficient.
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
+
+ for (String dim : state.getDims()) {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ // get dim value
+ int dimCount =
+ getDimValue(
+ dimConfig, dim, dimOrd, dimTree.iterator(), topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ // use priority queue to store SortedSetDocValuesDimValueResult for topNDims
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ } else {
+ OrdRange ordRange = state.getOrdRange(dim);
+ 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();
+ }
+ int dimCount =
+ getDimValue(dimConfig, dim, dimOrd, childIt, topNChildren, cacheChildOrdsResult);
+ if (dimCount != 0) {
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, dimCount));
+ }
+ }
+ }
+
+ // get FacetResult for topNDims
+ List<FacetResult> results = new LinkedList<>();
+ while (pq.size() > 0) {
+ SortedSetDocValuesDimValueResult dimValueResult = pq.pop();
+ if (dimValueResult != null) {
+ FacetResult factResult =
+ getFacetResultForDim(dimValueResult.dim, topNChildren, cacheChildOrdsResult);
+ if (factResult != null) {
+ results.add(0, factResult);
+ }
+ }
+ }
+ return results;
+ }
+
+ /**
+ * Creates SortedSetDocValuesChildOrdsResult to store dimCount, childCount, and TopOrdAndIntQueue
+ * q for getPathResult.
+ */
+ private class SortedSetDocValuesChildOrdsResult {
Review comment:
Please make this `static`. You could also make the name a little shorter since it's just privately scoped and it's a little redundant to say "SortedSetDocValues". Maybe just `ChildOrdsResult`?
--
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
[GitHub] [lucene] Yuti-G commented on pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on pull request #747:
URL: https://github.com/apache/lucene/pull/747#issuecomment-1073132559
> I didn't really have a lot of real feedback, just minor nits here and there. I did want to say that I have a PR out for a benchmark that compares the faceting API performance for SSDV vs taxonomy for high cardinality hierarchical faceting. Here is the link: [mikemccand/luceneutil#166](https://github.com/mikemccand/luceneutil/pull/166). We should add `getTopDims` to this benchmark once it is done :)
Thanks @mdmarshmallow for all the great feedback! This is my first official PR and really appreciate all the suggestions. I'd like to add `getTopDims`to benchmark once it is done and can I ask you questions if I come across any? Thanks again :)
--
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
[GitHub] [lucene] gsmiller commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
gsmiller commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r832488053
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -178,10 +229,23 @@ private FacetResult getPathResult(
}
}
- if (q == null) {
- return null;
+ if (dimConfig.hierarchical == false) {
Review comment:
I'm not sure all the cases are covered. What about `getTopChildren`? It looks like it could call `getPathResult` for a hierarchical + multivalued case. Maybe I'm just missing something?
But either way, I'd prefer `getChildOrdsResult` not rely on the specific ways it gets called today. If it's providing correct counts, it should do so in all cases. Or alternatively, you could state that it's invalid to use that method for the hierarchical + multivalued case and put an assert in to ensure that's true (but it seems like it will need to be called for that case). Does this make sense?
--
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
[GitHub] [lucene] Yuti-G commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r827269196
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -190,20 +235,45 @@ private FacetResult getPathResult(
String[] parts = FacetsConfig.stringToPath(term.utf8ToString());
labelValues[i] = new LabelAndValue(parts[parts.length - 1], ordAndValue.value);
}
+ return labelValues;
+ }
- if (dimConfig.hierarchical == false) {
+ /** Returns value/count of a dimension. */
+ private Number getDimValue(
Review comment:
Thanks @gsmiller! The reason I wanted to return `Number` is that FacetResult takes dimCount in Number as a parameter, but it can take `int` as well.
> /** Sole constructor. */
public FacetResult(
String dim, String[] path, Number value, LabelAndValue[] labelValues, int childCount) {
this.dim = dim;
this.path = path;
this.value = value;
this.labelValues = labelValues;
this.childCount = childCount;
}
I can change the return type from `Number` to `int`. The only concern is that it would change the following behavior when `q == null` since I was trying to be consistent of the original `getPathResult` function. However, I just tested changing the return type from `Number` to `int` in my local and passed all the tests.
<img width="421" alt="Screen Shot 2022-03-15 at 10 56 33 AM" src="https://user-images.githubusercontent.com/44444710/158441533-ecf1bb90-eb47-4fef-850f-75a934441040.png">
<img width="711" alt="Screen Shot 2022-03-15 at 10 55 33 AM" src="https://user-images.githubusercontent.com/44444710/158441360-10cb8869-a1df-4787-a11b-22a225478bc2.png">
--
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
[GitHub] [lucene] gsmiller commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
gsmiller commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r834419587
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -178,10 +229,23 @@ private FacetResult getPathResult(
}
}
- if (q == null) {
- return null;
+ if (dimConfig.hierarchical == false) {
Review comment:
Oh I see. That's tricky. I was actually going to recommend collapsing that branching condition at the bottom of `getPathResult` to always leverage `childOrdsResult.dimCount`, but I didn't want to nitpick too much. Even though it's an internal, private method, it feels super trappy that `getChildOrdsResult` can _sometimes_ still produce incorrect values for `dimCount`. I'd really prefer we address this so that `dimCount` is always correct and not worry about that level of optimization right now. Maybe we can tighten that up later? Maybe open a separate issue noting that we're double-checking and that we could probably optimize?
--
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
[GitHub] [lucene] Yuti-G commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r830539688
##########
File path: lucene/facet/src/test/org/apache/lucene/facet/sortedset/TestSortedSetDocValuesFacets.java
##########
@@ -1221,6 +1326,12 @@ public void testRandomHierarchicalFlatMix() throws Exception {
assertEquals(expectedAllDims, actualAllDims);
+ // test getTopDims(1, 10)
+ if (actualAllDims.size() > 0) {
+ List<FacetResult> topDimsResults1 = facets.getTopDims(1, 10);
Review comment:
> I didn't really have a lot of real feedback, just minor nits here and there. I did want to say that I have a PR out for a benchmark that compares the faceting API performance for SSDV vs taxonomy for high cardinality hierarchical faceting. Here is the link: [mikemccand/luceneutil#166](https://github.com/mikemccand/luceneutil/pull/166). We should add `getTopDims` to this benchmark once it is done :)
Thanks @mdmarshmallow for all the great feedback! This is my first official PR and really appreciate all the suggestions. I'd like to add `getTopDims`to benchmark once it is done and can I ask you questions if I come across any? Thanks again :)
--
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
[GitHub] [lucene] Yuti-G commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r831558514
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -178,10 +229,23 @@ private FacetResult getPathResult(
}
}
- if (q == null) {
- return null;
+ if (dimConfig.hierarchical == false) {
Review comment:
Thanks @gsmiller! I added the code below in getDimValue to exit the function earlier if these conditions have been met.
`
if (dimConfig.hierarchical == true || (dimConfig.multiValued && dimConfig.requireDimCount)) {
return counts[dimOrd];
}
`
But if I add the same conditions to `getChildOrd`, then it would be a duplicate check when calling getChildOrdsResult from `getDimValue`.
GetPathResult is another method that calls `getChildOrdsResult`, and I think the first condition in the return statement plus the existing check in `getChildOrdsResult` handles the conditions. Please correct me if I am wrong. Thanks again! :)
<img width="851" alt="Screen Shot 2022-03-21 at 2 17 52 PM" src="https://user-images.githubusercontent.com/44444710/159365382-52d86138-4092-4aec-a811-f41c579c64fb.png">
--
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
[GitHub] [lucene] Yuti-G commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r826456302
##########
File path: lucene/facet/src/test/org/apache/lucene/facet/range/TestRangeFacetCounts.java
##########
@@ -243,6 +243,24 @@ public void testLongGetAllDims() throws Exception {
"dim=field path=[] value=22 childCount=5\n less than 10 (10)\n less than or equal to 10 (11)\n over 90 (9)\n 90 or above (10)\n over 1000 (1)\n",
result.get(0).toString());
+ // test getAllDims(1)
+ List<FacetResult> test1Child = facets.getAllDims(1);
+ assertEquals(1, test1Child.size());
+ assertEquals(
+ "dim=field path=[] value=22 childCount=5\n less than 10 (10)\n less than or equal to 10 (11)\n over 90 (9)\n 90 or above (10)\n over 1000 (1)\n",
+ test1Child.get(0).toString());
+
+ // test default implementation of getTopDims
+ List<FacetResult> topNDimsResult = facets.getTopDims(1, 1);
+ assertEquals(1, topNDimsResult.size());
+ assertEquals(
Review comment:
Thanks for catching 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-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
[GitHub] [lucene] gsmiller commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
gsmiller commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r828050128
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -190,20 +235,45 @@ private FacetResult getPathResult(
String[] parts = FacetsConfig.stringToPath(term.utf8ToString());
labelValues[i] = new LabelAndValue(parts[parts.length - 1], ordAndValue.value);
}
+ return labelValues;
+ }
- if (dimConfig.hierarchical == false) {
+ /** Returns value/count of a dimension. */
+ private Number getDimValue(
Review comment:
I see what you're saying here. It seems like there are three cases here:
1. We've explicitly indexed a count for the dimension (because it's hierarchical or multivalued and we've signaled that we require dim counts in FacetsConfig). In this case, `getDimValue` can simply return the indexed count.
2. We have not indexed a count for the dimension and we need to compute it by aggregating over the children. If the refinement is configured as multivalued and we _didn't_ configure it as needing dim counts, we can't do that accurately and need to communicate back to the caller that we cannot compute an accurate count for the dim. Right now your code is doing this by returning `-1`, which I think makes sense.
3. We have not indexed a count and need to compute it, and the dimension is _not_ configured as multivalued. So we can accurately compute a count by aggregating over the children. In this case, we should just always return that count. It looks like your implementation right now is effectively special-casing the situation where there are no children (i.e., the PQ is null), but I think you should just return `0` there (i.e., just return `childOrdsResult.dimCount`). Returning `0` is the correct behavior since the value is actually `0` when there are no children. So instead of returning `null` or `-1`, I would suggest always returning `childOrdsResult.dimCount`.
--
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
[GitHub] [lucene] gsmiller commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
gsmiller commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r826440044
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/Facets.java
##########
@@ -48,4 +48,13 @@ public abstract FacetResult getTopChildren(int topN, String dim, String... path)
* indexed, for example depending on the type of document.
*/
public abstract List<FacetResult> getAllDims(int topN) throws IOException;
+
+ /**
+ * Returns labels for topN dimensions and their topNChildren sorted by the number of hits that
+ * dimension matched
+ */
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
Review comment:
I like the approach of providing a default implementation here so existing sub-classes will be fully backwards-compatible (and they don't need to worry about providing an implementation if this suits their needs). It might be nice to mention explicitly in the javadoc that sub-classes may _want_ to override this implementation though with a more efficient one if they're able.
##########
File path: lucene/facet/src/test/org/apache/lucene/facet/sortedset/TestSortedSetDocValuesFacets.java
##########
@@ -104,6 +104,65 @@ public void testBasic() throws Exception {
"dim=b path=[] value=2 childCount=2\n buzz (2)\n baz (1)\n",
facets.getTopChildren(10, "b").toString());
+ // test getAllDims
Review comment:
Thank you for adding so much testing, including coverage for existing `getAllDims` functionality!
##########
File path: lucene/facet/src/test/org/apache/lucene/facet/sortedset/TestSortedSetDocValuesFacets.java
##########
@@ -104,6 +104,65 @@ public void testBasic() throws Exception {
"dim=b path=[] value=2 childCount=2\n buzz (2)\n baz (1)\n",
facets.getTopChildren(10, "b").toString());
+ // test getAllDims
+ List<FacetResult> results = facets.getAllDims(10);
+ assertEquals(2, results.size());
+ assertEquals(
+ "dim=b path=[] value=2 childCount=2\n buzz (2)\n baz (1)\n",
+ results.get(0).toString());
+ assertEquals(
+ "dim=a path=[] value=-1 childCount=3\n foo (2)\n bar (1)\n zoo (1)\n",
+ results.get(1).toString());
+
+ // test getAllDims(1, 0) with topN = 0
+ expectThrows(
Review comment:
Ouch! This seems like a poor (existing) experience for users. Would you mind creating a Jira to track this? We should probably change this behavior to throw an `IllegalArgumentException` at least instead of just an NPE. Thanks for uncovering this!
##########
File path: lucene/facet/src/test/org/apache/lucene/facet/range/TestRangeFacetCounts.java
##########
@@ -243,6 +243,24 @@ public void testLongGetAllDims() throws Exception {
"dim=field path=[] value=22 childCount=5\n less than 10 (10)\n less than or equal to 10 (11)\n over 90 (9)\n 90 or above (10)\n over 1000 (1)\n",
result.get(0).toString());
+ // test getAllDims(1)
+ List<FacetResult> test1Child = facets.getAllDims(1);
+ assertEquals(1, test1Child.size());
+ assertEquals(
+ "dim=field path=[] value=22 childCount=5\n less than 10 (10)\n less than or equal to 10 (11)\n over 90 (9)\n 90 or above (10)\n over 1000 (1)\n",
+ test1Child.get(0).toString());
+
+ // test default implementation of getTopDims
+ List<FacetResult> topNDimsResult = facets.getTopDims(1, 1);
+ assertEquals(1, topNDimsResult.size());
+ assertEquals(
Review comment:
minor: Since `FacetResult` properly implements `equals`, you could just do `assertEquals(test1Child, topNDimsResult)`. This makes it slightly more obvious to the reader that you expect the exact same behavior as `getAllDims`
##########
File path: lucene/facet/src/test/org/apache/lucene/facet/sortedset/TestSortedSetDocValuesFacets.java
##########
@@ -104,6 +104,65 @@ public void testBasic() throws Exception {
"dim=b path=[] value=2 childCount=2\n buzz (2)\n baz (1)\n",
facets.getTopChildren(10, "b").toString());
+ // test getAllDims
+ List<FacetResult> results = facets.getAllDims(10);
+ assertEquals(2, results.size());
+ assertEquals(
+ "dim=b path=[] value=2 childCount=2\n buzz (2)\n baz (1)\n",
+ results.get(0).toString());
+ assertEquals(
+ "dim=a path=[] value=-1 childCount=3\n foo (2)\n bar (1)\n zoo (1)\n",
+ results.get(1).toString());
+
+ // test getAllDims(1, 0) with topN = 0
+ expectThrows(
+ NullPointerException.class,
+ () -> {
+ facets.getAllDims(0);
+ });
+
+ // test getTopDims(10, 10) and expect same results from getAllDims(10)
+ List<FacetResult> allDimsResults = facets.getTopDims(10, 10);
+ assertEquals(2, results.size());
+ assertEquals(
+ "dim=b path=[] value=2 childCount=2\n buzz (2)\n baz (1)\n",
+ allDimsResults.get(0).toString());
+ assertEquals(
+ "dim=a path=[] value=-1 childCount=3\n foo (2)\n bar (1)\n zoo (1)\n",
+ allDimsResults.get(1).toString());
+
+ // test getTopDims(2, 1)
+ List<FacetResult> topDimsResults = facets.getTopDims(2, 1);
Review comment:
minor: similar comment about relying on `FacetResult` equality here instead
--
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
[GitHub] [lucene] Yuti-G commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r831558514
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -178,10 +229,23 @@ private FacetResult getPathResult(
}
}
- if (q == null) {
- return null;
+ if (dimConfig.hierarchical == false) {
Review comment:
Thanks @gsmiller! I added the code below in getDimValue to exit the function earlier if these conditions have been met.
`
if (dimConfig.hierarchical == true || (dimConfig.multiValued && dimConfig.requireDimCount)) {
return counts[dimOrd];
}
`
But if I add the same conditions to `getChildOrdsResult`, it would be a duplicate check when calling getChildOrdsResult from `getDimValue`.
GetPathResult is another method that calls `getChildOrdsResult`, and I think the first condition in the return statement plus the existing check in `getChildOrdsResult` handles all the conditions. Please correct me if I am wrong. Thanks again! :)
<img width="851" alt="Screen Shot 2022-03-21 at 2 17 52 PM" src="https://user-images.githubusercontent.com/44444710/159365382-52d86138-4092-4aec-a811-f41c579c64fb.png">
--
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
[GitHub] [lucene] Yuti-G commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r831558514
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -178,10 +229,23 @@ private FacetResult getPathResult(
}
}
- if (q == null) {
- return null;
+ if (dimConfig.hierarchical == false) {
Review comment:
Thanks @gsmiller! I added the code below in getDimValue to exit the function earlier if these conditions have been met.
`
if (dimConfig.hierarchical == true || (dimConfig.multiValued && dimConfig.requireDimCount)) {
return counts[dimOrd];
}
`
But if I add the same conditions to `getChildOrdsResult`, it would be a duplicate check when calling `getChildOrdsResult` from `getDimValue`.
`GetPathResult` is another method that calls `getChildOrdsResult`, and I think the first condition in the return statement plus the existing check in `getChildOrdsResult` handles all the conditions. Please correct me if I am wrong. Thanks again! :)
<img width="851" alt="Screen Shot 2022-03-21 at 2 17 52 PM" src="https://user-images.githubusercontent.com/44444710/159365382-52d86138-4092-4aec-a811-f41c579c64fb.png">
--
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
[GitHub] [lucene] Yuti-G commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r832745109
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -178,10 +229,23 @@ private FacetResult getPathResult(
}
}
- if (q == null) {
- return null;
+ if (dimConfig.hierarchical == false) {
Review comment:
Thanks @gsmiller! Sorry that I didn't explain my code clear.
> What about getTopChildren? It looks like it could call getPathResult for a hierarchical + multivalued case. Maybe I'm just missing something?
For the hierarchical + multivalued case, the return statement in `getPathResult`will set dimCount = counts[pathOrd]. The logic here is that when a dim is hierarchical, dimCount will always equal to counts[pathOrd] regardless of the boolean value of multivalued and requireDimCount, and therefore, we only need to be concerned about the case when hierarchical == false, which is checked in `getChildOrdsResult`.
I added tests for `getTopChildren`, `getAllDims`, and `getTopChildren` to test different boolean combinations of `hierarchical`, `multivalued`, and `requireDimCount` to ensure the current design does not change the existing logic for other functionalities but only add support to `getTopDims`, and `getChildOrdsResult` does not rely on `getDimValue` to provide/reset dimCount. Please let me know if I am missing anything here. Thanks again!
--
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
[GitHub] [lucene] gsmiller commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
gsmiller commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r828063663
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -190,20 +235,45 @@ private FacetResult getPathResult(
String[] parts = FacetsConfig.stringToPath(term.utf8ToString());
labelValues[i] = new LabelAndValue(parts[parts.length - 1], ordAndValue.value);
}
+ return labelValues;
+ }
- if (dimConfig.hierarchical == false) {
+ /** Returns value/count of a dimension. */
+ private Number getDimValue(
Review comment:
Ah, also I see what you're saying about maintaining the existing behavior, which is that a `FacetResult` is not returned when there are no children seen (i.e., `getTopChildren` returns `null` and `getAllDims` doesn't include a result for the dim in this case). That's a little odd in my opinion since we _could_ return meaningful information that it had a `0` count and no children, but let's try to keep consistent behavior for now.
So I would still recommend what I wrote above, but then in `getTopDims` I would check for a value of `0` and then skip over that dim (i.e., don't add it to your PQ). That way `getDimValue` can behave in a logical way and only `getTopDims` needs to know that it shouldn't include results with a zero count.
--
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
[GitHub] [lucene] Yuti-G commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r826456758
##########
File path: lucene/facet/src/test/org/apache/lucene/facet/sortedset/TestSortedSetDocValuesFacets.java
##########
@@ -104,6 +104,65 @@ public void testBasic() throws Exception {
"dim=b path=[] value=2 childCount=2\n buzz (2)\n baz (1)\n",
facets.getTopChildren(10, "b").toString());
+ // test getAllDims
Review comment:
Thank you so much! I added more testing for getAllDims in order to compare the behavior of getTopDims :)
--
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
[GitHub] [lucene] Yuti-G commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r830444104
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
Review comment:
Hmmm, I am still having trouble understanding the benefits of using IntIntHashMap since it requires both keys and values to be int. I will spend more time during the weekend :)
--
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
[GitHub] [lucene] mdmarshmallow commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
mdmarshmallow commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r830444294
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -114,7 +116,7 @@ public FacetResult getTopChildren(int topN, String dim, String... path) throws I
return null;
}
DimTree dimTree = state.getDimTree(dim);
- return getPathResult(dimConfig, dim, path, pathOrd, dimTree.iterator(pathOrd), topN);
+ return getPathResult(dimConfig, dim, path, pathOrd, dimTree.iterator(pathOrd), topN, null);
Review comment:
Small nit, but could we have an overloaded method signature here? I think it will make the code a bit cleaner than just passing `null`.
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -133,7 +135,7 @@ public FacetResult getTopChildren(int topN, String dim, String... path) throws I
// child:
childIt.next();
}
- return getPathResult(dimConfig, dim, null, dimOrd, childIt, topN);
+ return getPathResult(dimConfig, dim, null, dimOrd, childIt, topN, null);
Review comment:
Same thing here. Actually there is a few places where `null` is just passed into various methods. I think new signatures excluding the `cacheChildOrdsResult` with comments explaining what they are would make the code clearer.
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -411,7 +484,109 @@ public int compare(FacetResult a, FacetResult b) {
}
}
});
-
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<DimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(DimValueResult a, DimValueResult b) {
+ if (a.value > b.value) {
+ return false;
+ } else if (a.value < b.value) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, ChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
+ int dimCount;
+
+ for (String dim : state.getDims()) {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ // get dim value
+ dimCount =
+ getDimValue(
+ dimConfig, dim, dimOrd, dimTree.iterator(), topNChildren, cacheChildOrdsResult);
+ } else {
+ OrdRange ordRange = state.getOrdRange(dim);
+ 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();
+ }
+ dimCount = getDimValue(dimConfig, dim, dimOrd, childIt, topNChildren, cacheChildOrdsResult);
+ }
+
+ if (dimCount != 0) {
+ // use priority queue to store DimValueResult for topNDims
+ if (pq.size() < topNDims) {
+ pq.insertWithOverflow(new DimValueResult(dim, dimCount));
+ } else {
+ if (dimCount > pq.top().value
+ || (dimCount == pq.top().value && dim.compareTo(pq.top().dim) < 0)) {
+ DimValueResult bottomDim = pq.pop();
+ bottomDim.dim = dim;
+ bottomDim.value = dimCount;
+ pq.insertWithOverflow(bottomDim);
+ }
+ }
+ }
+ }
+
+ // get FacetResult for topNDims
+ int resultSize = pq.size();
+ FacetResult[] results = new FacetResult[resultSize];
+
+ while (pq.size() > 0) {
+ DimValueResult dimValueResult = pq.pop();
+ FacetResult facetResult =
+ getFacetResultForDim(
+ dimValueResult.dim, topNChildren, cacheChildOrdsResult.get(dimValueResult.dim));
+ results[--resultSize] = facetResult;
+ }
+ return Arrays.asList(results);
+ }
+
+ /**
+ * Creates ChildOrdsResult to store dimCount, childCount, and TopOrdAndIntQueue q for
+ * getPathResult.
Review comment:
The `TopOrdAndIntQueue` is a queue for the dimension's top children right? Maybe just include that in the comments to make it a bit more clear about what it is.
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -411,7 +484,109 @@ public int compare(FacetResult a, FacetResult b) {
}
}
});
-
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<DimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(DimValueResult a, DimValueResult b) {
+ if (a.value > b.value) {
+ return false;
+ } else if (a.value < b.value) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, ChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
+ int dimCount;
+
+ for (String dim : state.getDims()) {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ // get dim value
+ dimCount =
+ getDimValue(
+ dimConfig, dim, dimOrd, dimTree.iterator(), topNChildren, cacheChildOrdsResult);
+ } else {
+ OrdRange ordRange = state.getOrdRange(dim);
+ 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();
+ }
+ dimCount = getDimValue(dimConfig, dim, dimOrd, childIt, topNChildren, cacheChildOrdsResult);
+ }
+
+ if (dimCount != 0) {
+ // use priority queue to store DimValueResult for topNDims
+ if (pq.size() < topNDims) {
+ pq.insertWithOverflow(new DimValueResult(dim, dimCount));
+ } else {
+ if (dimCount > pq.top().value
+ || (dimCount == pq.top().value && dim.compareTo(pq.top().dim) < 0)) {
+ DimValueResult bottomDim = pq.pop();
+ bottomDim.dim = dim;
+ bottomDim.value = dimCount;
+ pq.insertWithOverflow(bottomDim);
+ }
+ }
+ }
+ }
+
+ // get FacetResult for topNDims
+ int resultSize = pq.size();
+ FacetResult[] results = new FacetResult[resultSize];
+
+ while (pq.size() > 0) {
+ DimValueResult dimValueResult = pq.pop();
+ FacetResult facetResult =
+ getFacetResultForDim(
+ dimValueResult.dim, topNChildren, cacheChildOrdsResult.get(dimValueResult.dim));
+ results[--resultSize] = facetResult;
Review comment:
Can we put `resultSize--` in the previous line and then just pass in `resultSize` here. I've gotten some comments before about inline pre-increments like this not being the most readable, so figured I'd just let you know.
##########
File path: lucene/facet/src/test/org/apache/lucene/facet/sortedset/TestSortedSetDocValuesFacets.java
##########
@@ -1221,6 +1326,12 @@ public void testRandomHierarchicalFlatMix() throws Exception {
assertEquals(expectedAllDims, actualAllDims);
+ // test getTopDims(1, 10)
+ if (actualAllDims.size() > 0) {
+ List<FacetResult> topDimsResults1 = facets.getTopDims(1, 10);
Review comment:
Since this unit test is pretty thorough with what it checks, I think you should just `facets.getTopDims(n, 10)` where `n` goes from 1 to `actualAllDims.size()`.
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -411,7 +484,109 @@ public int compare(FacetResult a, FacetResult b) {
}
}
});
-
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<DimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(DimValueResult a, DimValueResult b) {
+ if (a.value > b.value) {
+ return false;
+ } else if (a.value < b.value) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, ChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
Review comment:
Another nit: Maybe change this name to `dimToChildOrdsResult` or something like that? Calling it a cache was a bit confusing to me. Ignore if you think the name makes sense though.
--
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
[GitHub] [lucene] gsmiller commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
gsmiller commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r831491304
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -190,20 +254,33 @@ private FacetResult getPathResult(
String[] parts = FacetsConfig.stringToPath(term.utf8ToString());
labelValues[i] = new LabelAndValue(parts[parts.length - 1], ordAndValue.value);
}
+ return labelValues;
+ }
- if (dimConfig.hierarchical == false) {
- // see if dimCount is actually reliable or needs to be reset
- if (dimConfig.multiValued) {
- if (dimConfig.requireDimCount) {
- dimCount = counts[pathOrd];
- } else {
- dimCount = -1; // dimCount is in accurate at this point, so set it to -1
- }
- }
- return new FacetResult(dim, emptyPath, dimCount, labelValues, childCount);
- } else {
- return new FacetResult(dim, path, counts[pathOrd], labelValues, childCount);
+ /** Returns value/count of a dimension. */
+ private int getDimValue(
+ FacetsConfig.DimConfig dimConfig,
+ String dim,
+ int dimOrd,
+ PrimitiveIterator.OfInt childOrds,
+ int topN,
+ HashMap<String, ChildOrdsResult> dimToChildOrdsResult) {
+
+ // if dimConfig.hierarchical == true || dim is multiValued and dim count has been aggregated at
+ // indexing time, return dimCount directly
+ if (dimConfig.hierarchical == true || (dimConfig.multiValued && dimConfig.requireDimCount)) {
+ return counts[dimOrd];
}
+
+ // if dimCount was not aggregated at indexing time, iterate over childOrds to get dimCount
+ ChildOrdsResult childOrdsResult = getChildOrdsResult(childOrds, topN, dimConfig, dimOrd);
+
+ // if no early termination, store dim and childOrdsResult into hashmap
+ if (dimToChildOrdsResult != null) {
Review comment:
minor: Can `dimToChildOrdsResult` ever be `null` here?
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -178,10 +229,23 @@ private FacetResult getPathResult(
}
}
- if (q == null) {
- return null;
+ if (dimConfig.hierarchical == false) {
Review comment:
The dimCount might need to be reset if hierarchical too right? What if the dim is configured as multi-valued and hierarchical? You kind of have code for this already in `getDimValue`. Maybe this is something like:
```
if (dimConfig.hierarchical || (dimConfig.multiValued && dimConfig.requireDimCount)) {
dimCount = counts[pathOrd];
} else if (dimConfig.multiValued) {
dimCount = -1;
}
```
--
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
[GitHub] [lucene] Yuti-G commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r832745109
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -178,10 +229,23 @@ private FacetResult getPathResult(
}
}
- if (q == null) {
- return null;
+ if (dimConfig.hierarchical == false) {
Review comment:
Thanks @gsmiller! Sorry that I didn't explain my code clear.
> What about getTopChildren? It looks like it could call getPathResult for a hierarchical + multivalued case. Maybe I'm just missing something?
For the hierarchical + multivalued case, the return statement in `getPathResult`will set dimCount = counts[pathOrd]. The logic here is that when a dim is hierarchical, dimCount will always equal to counts[pathOrd] regardless of the boolean value multivalued, and therefore, we only need to be concerned about the case when hierarchical == false, which is checked in `getChildOrdsResult`.
I added tests for `getTopChildren`, `getAllDims`, and `getTopChildren` to test different boolean combinations of `hierarchical`, `multivalued`, and `requireDimCount` to ensure the current design does not change the existing logic for other functionalities but only add support to `getTopDims`, and `getChildOrdsResult` does not rely on `getDimValue` to provide/reset dimCount. Please let me know if I am missing anything here. Thanks again!
--
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
[GitHub] [lucene] Yuti-G commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r831568451
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
Review comment:
No problem at all! It's a great opportunity to learn IntIntHashMap. 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
[GitHub] [lucene] gsmiller commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
gsmiller commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r834841171
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -178,10 +229,23 @@ private FacetResult getPathResult(
}
}
- if (q == null) {
- return null;
+ if (dimConfig.hierarchical == false) {
Review comment:
Thanks for making that change!
--
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
[GitHub] [lucene] Yuti-G commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r828260482
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +493,107 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // get topNDims by their values
+ SortedSetDocValueDimValuePriorityQueue pq =
+ new SortedSetDocValueDimValuePriorityQueue(topNDims);
+ cacheChildOrdsResult = new HashMap<>();
+ for (String dim : state.getDims()) {
+ FacetsConfig.DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ if (dimConfig.hierarchical) {
+ DimTree dimTree = state.getDimTree(dim);
+ int dimOrd = dimTree.dimStartOrd;
+ // get dim value
+ Number value = getDimValue(dimConfig, dim, dimOrd, dimTree.iterator(), topNChildren);
+ if (value != null) {
+ // use priority queue to store SortedSetDocValuesDimValueResult for topNDims
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, value));
+ }
+ } else {
+ OrdRange ordRange = state.getOrdRange(dim);
+ 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();
+ }
+ Number value = getDimValue(dimConfig, dim, dimOrd, childIt, topNChildren);
+ if (value != null) {
+ pq.insertWithOverflow(new SortedSetDocValuesDimValueResult(dim, value));
+ }
+ }
+ }
+
+ // get FacetResult for topNDims
+ List<FacetResult> results = new LinkedList<>();
+ while (pq.size() > 0) {
+ SortedSetDocValuesDimValueResult dimValueResult = pq.pop();
+ if (dimValueResult != null) {
+ FacetResult factResult = getFacetResultForDim(dimValueResult.dim, topNChildren);
+ if (factResult != null) {
+ results.add(0, factResult);
+ }
+ }
+ }
+ // free cacheChildOrdsResult after getting results
+ cacheChildOrdsResult = null;
+ return results;
+ }
+
+ /**
+ * Creates SortedSetDocValuesChildOrdsResult to store dimCount, childCount, and TopOrdAndIntQueue
+ * q for getPathResult.
+ */
+ private class SortedSetDocValuesChildOrdsResult {
+ final int dimCount;
+ final int childCount;
+ final TopOrdAndIntQueue q;
+
+ SortedSetDocValuesChildOrdsResult(int dimCount, int childCount, TopOrdAndIntQueue q) {
+ this.dimCount = dimCount;
+ this.childCount = childCount;
+ this.q = q;
+ }
+ }
+
+ /**
+ * Creates SortedSetDocValuesDimValueResult to store the label and count of dim in order to sort
+ * by these two fields.
+ */
+ private class SortedSetDocValuesDimValueResult {
+ final String dim;
+ final Number value;
+
+ SortedSetDocValuesDimValueResult(String dim, Number value) {
+ this.dim = dim;
+ this.value = value;
+ }
+ }
+
+ /**
+ * Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ * string values.
+ */
+ private class SortedSetDocValueDimValuePriorityQueue
Review comment:
Thanks for the suggestion! I was a bit worried about making the `getTopDims` function too long, and thanks for confirming this is a convention.
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -190,20 +235,45 @@ private FacetResult getPathResult(
String[] parts = FacetsConfig.stringToPath(term.utf8ToString());
labelValues[i] = new LabelAndValue(parts[parts.length - 1], ordAndValue.value);
}
+ return labelValues;
+ }
- if (dimConfig.hierarchical == false) {
+ /** Returns value/count of a dimension. */
+ private Number getDimValue(
Review comment:
Thank you so much for the detailed explanation!
--
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
[GitHub] [lucene] Yuti-G commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r832745109
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -178,10 +229,23 @@ private FacetResult getPathResult(
}
}
- if (q == null) {
- return null;
+ if (dimConfig.hierarchical == false) {
Review comment:
Thanks @gsmiller! Sorry that I didn't explain my code clear.
> What about getTopChildren? It looks like it could call getPathResult for a hierarchical + multivalued case. Maybe I'm just missing something?
For the hierarchical + multivalued case, the return statement in `getPathResult`will set dimCount = counts[pathOrd]. The logic here is that when a dim is hierarchical, dimCount will always equal to counts[pathOrd] regardless of the boolean value of multivalued and requireDimCount, and therefore, we only need to be concerned about the case when hierarchical == false, which is checked in `getChildOrdsResult`. Any function that calls `getPathResult` will call `getChildOrdsResult` to set the `dimCount` when hierarchical == false, and re-evaluate `dimCount` when hierarchical == true in the return state of `getPathResult`.
I added tests for `getTopChildren`, `getAllDims`, and `getTopDims` to test different boolean combinations of `hierarchical`, `multivalued`, and `requireDimCount` to ensure the current design does not change the existing logic for other functionalities but only add support to `getTopDims`, and `getChildOrdsResult` does not rely on `getDimValue` to provide/reset `dimCount`. Please let me know if I am missing anything here. Thanks again!
--
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
[GitHub] [lucene] Yuti-G commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r832745109
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -178,10 +229,23 @@ private FacetResult getPathResult(
}
}
- if (q == null) {
- return null;
+ if (dimConfig.hierarchical == false) {
Review comment:
Thanks @gsmiller! Sorry that I didn't explain my code clear.
> What about getTopChildren? It looks like it could call getPathResult for a hierarchical + multivalued case. Maybe I'm just missing something?
For the hierarchical + multivalued case, the return statement in `getPathResult`will set dimCount = counts[pathOrd]. The logic here is that when a dim is hierarchical, dimCount will always equal to counts[pathOrd] regardless of the boolean value of multivalued and requireDimCount, and therefore, we only need to be concerned about the case when hierarchical == false, which is checked in `getChildOrdsResult`.
I added tests for `getTopChildren`, `getAllDims`, and `getTopDims` to test different boolean combinations of `hierarchical`, `multivalued`, and `requireDimCount` to ensure the current design does not change the existing logic for other functionalities but only add support to `getTopDims`, and `getChildOrdsResult` does not rely on `getDimValue` to provide/reset `dimCount`. Please let me know if I am missing anything here. Thanks again!
--
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
[GitHub] [lucene] Yuti-G commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r826462222
##########
File path: lucene/facet/src/test/org/apache/lucene/facet/sortedset/TestSortedSetDocValuesFacets.java
##########
@@ -104,6 +104,65 @@ public void testBasic() throws Exception {
"dim=b path=[] value=2 childCount=2\n buzz (2)\n baz (1)\n",
facets.getTopChildren(10, "b").toString());
+ // test getAllDims
+ List<FacetResult> results = facets.getAllDims(10);
+ assertEquals(2, results.size());
+ assertEquals(
+ "dim=b path=[] value=2 childCount=2\n buzz (2)\n baz (1)\n",
+ results.get(0).toString());
+ assertEquals(
+ "dim=a path=[] value=-1 childCount=3\n foo (2)\n bar (1)\n zoo (1)\n",
+ results.get(1).toString());
+
+ // test getAllDims(1, 0) with topN = 0
+ expectThrows(
+ NullPointerException.class,
+ () -> {
+ facets.getAllDims(0);
+ });
+
+ // test getTopDims(10, 10) and expect same results from getAllDims(10)
+ List<FacetResult> allDimsResults = facets.getTopDims(10, 10);
+ assertEquals(2, results.size());
+ assertEquals(
+ "dim=b path=[] value=2 childCount=2\n buzz (2)\n baz (1)\n",
+ allDimsResults.get(0).toString());
+ assertEquals(
+ "dim=a path=[] value=-1 childCount=3\n foo (2)\n bar (1)\n zoo (1)\n",
+ allDimsResults.get(1).toString());
+
+ // test getTopDims(2, 1)
+ List<FacetResult> topDimsResults = facets.getTopDims(2, 1);
Review comment:
Thank you so much! I will check and fix other tests that have this issue.
--
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
[GitHub] [lucene] Yuti-G commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r829347283
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
Review comment:
Thanks for showing me an alternative method :)
--
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
[GitHub] [lucene] Yuti-G commented on pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on pull request #747:
URL: https://github.com/apache/lucene/pull/747#issuecomment-1072947749
> I like the progress. Thanks! Left some more detailed comments this time around on the `SortedSetDocValuesFacetCounts` implementation. Thanks again!
Thanks @gsmiller so much for all the detailed comments and great suggestions! My code looks much cleaner now :)
--
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
[GitHub] [lucene] Yuti-G commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r826455347
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/Facets.java
##########
@@ -48,4 +48,13 @@ public abstract FacetResult getTopChildren(int topN, String dim, String... path)
* indexed, for example depending on the type of document.
*/
public abstract List<FacetResult> getAllDims(int topN) throws IOException;
+
+ /**
+ * Returns labels for topN dimensions and their topNChildren sorted by the number of hits that
+ * dimension matched
+ */
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
Review comment:
My current javadoc does not well describe this new functionality, and I will add more to it. 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
[GitHub] [lucene] Yuti-G commented on pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on pull request #747:
URL: https://github.com/apache/lucene/pull/747#issuecomment-1067392546
> Thanks for picking this up! I've looked at everything except for your overridden implementation in SSDV faceting, but since I may run out of time to look at that today, I'll go ahead and publish my feedback on your default implementation and testing. I'll follow up with more feedback soon. Thanks again!
Thanks @gsmiller for reviewing my PR and leaving the detailed feedback! I will address them in my next PR. Appreciated it :)
--
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
[GitHub] [lucene] gsmiller commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
gsmiller commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r831477355
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/sortedset/SortedSetDocValuesFacetCounts.java
##########
@@ -414,4 +505,101 @@ public int compare(FacetResult a, FacetResult b) {
return results;
}
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<SortedSetDocValuesDimValueResult> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(
+ SortedSetDocValuesDimValueResult a, SortedSetDocValuesDimValueResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return false;
+ } else if (a.value.intValue() < b.value.intValue()) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ HashMap<String, SortedSetDocValuesChildOrdsResult> cacheChildOrdsResult = new HashMap<>();
Review comment:
Ugh, sorry I meant `IntObjectHashMap`. The benefit I was hoping for would be not needing to resolve the dimension strings but after I took a closer look, we've already got the strings in `state.getDims()`. So feel free to ignore this. I think what you're doing here is fine. Thanks and sorry if I caused confusion!
--
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
[GitHub] [lucene] gsmiller commented on a change in pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
gsmiller commented on a change in pull request #747:
URL: https://github.com/apache/lucene/pull/747#discussion_r827230655
##########
File path: lucene/facet/src/test/org/apache/lucene/facet/sortedset/TestSortedSetDocValuesFacets.java
##########
@@ -104,6 +104,65 @@ public void testBasic() throws Exception {
"dim=b path=[] value=2 childCount=2\n buzz (2)\n baz (1)\n",
facets.getTopChildren(10, "b").toString());
+ // test getAllDims
+ List<FacetResult> results = facets.getAllDims(10);
+ assertEquals(2, results.size());
+ assertEquals(
+ "dim=b path=[] value=2 childCount=2\n buzz (2)\n baz (1)\n",
+ results.get(0).toString());
+ assertEquals(
+ "dim=a path=[] value=-1 childCount=3\n foo (2)\n bar (1)\n zoo (1)\n",
+ results.get(1).toString());
+
+ // test getAllDims(1, 0) with topN = 0
+ expectThrows(
Review comment:
Thank you!
--
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
[GitHub] [lucene] Yuti-G commented on pull request #747: LUCENE-10325: Add getTopDims functionality to Facets
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on pull request #747:
URL: https://github.com/apache/lucene/pull/747#issuecomment-1079608718
> This looks good to me at this point. Since this changes the code path for existing faceting functionality, it would be a good idea to run `luceneutil` benchmarks and make sure there are no obvious regressions. I wouldn't suspect there to be, but it would be nice to catch them now if there are. @Yuti-G would you mind posting those results here?
Thanks @gsmiller! I ran benchmarks several times but the first 3 taxoFacets tasks are still showing statistically significant, but my code didn’t touch taxonomy faceting. Not sure if this will cause issues.
<img width="842" alt="Screen Shot 2022-03-25 at 10 17 26 PM" src="https://user-images.githubusercontent.com/44444710/160225861-fae93347-9b2e-4427-bcc2-f71af224f54f.png">
--
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