gsmiller commented on code in PR #974:
URL: https://github.com/apache/lucene/pull/974#discussion_r912001915
##########
lucene/demo/src/test/org/apache/lucene/demo/facet/TestRangeFacetsExample.java:
##########
@@ -27,7 +27,7 @@ public class TestRangeFacetsExample extends LuceneTestCase {
public void testSimple() throws Exception {
RangeFacetsExample example = new RangeFacetsExample();
example.index();
- FacetResult result = example.search();
+ FacetResult result = example.searchChildren();
Review Comment:
Let's not rename these methods without good reason. Best to touch as few
places as possible if we're not actually changing anything in this file and
there's no reason we need to rename.
##########
lucene/demo/src/java/org/apache/lucene/demo/facet/RangeFacetsExample.java:
##########
@@ -83,7 +83,7 @@ private FacetsConfig getConfig() {
}
/** User runs a query and counts facets. */
- public FacetResult search() throws IOException {
+ public FacetResult searchChildren() throws IOException {
Review Comment:
Let's not rename this method please.
##########
lucene/facet/src/java/org/apache/lucene/facet/range/RangeFacetCounts.java:
##########
@@ -232,20 +233,69 @@ public FacetResult getAllChildren(String dim, String...
path) throws IOException
return new FacetResult(dim, path, totCount, labelValues,
labelValues.length);
}
- // The current getTopChildren method is not returning "top" ranges. Instead,
it returns all
- // user-provided ranges in
- // the order the user specified them when instantiating. This concept is
being introduced and
- // supported in the
- // getAllChildren functionality in LUCENE-10550. getTopChildren is
temporarily calling
- // getAllChildren to maintain its
- // current behavior, and the current implementation will be replaced by an
actual "top children"
- // implementation
- // in LUCENE-10614
- // TODO: fix getTopChildren in LUCENE-10614
+ /** Reusable entry to hold range label and int count. */
+ private static class Entry {
Review Comment:
Please mark the class as `final`. Also would be nice to move it down towards
the bottom of the file for better organization.
##########
lucene/demo/src/test/org/apache/lucene/demo/facet/TestDistanceFacetsExample.java:
##########
@@ -25,7 +25,7 @@ public class TestDistanceFacetsExample extends LuceneTestCase
{
public void testSimple() throws Exception {
DistanceFacetsExample example = new DistanceFacetsExample();
example.index();
- FacetResult result = example.search();
+ FacetResult result = example.searchChildren();
Review Comment:
Let's not rename this method.
##########
lucene/facet/src/java/org/apache/lucene/facet/range/RangeFacetCounts.java:
##########
@@ -232,20 +233,69 @@ public FacetResult getAllChildren(String dim, String...
path) throws IOException
return new FacetResult(dim, path, totCount, labelValues,
labelValues.length);
}
- // The current getTopChildren method is not returning "top" ranges. Instead,
it returns all
- // user-provided ranges in
- // the order the user specified them when instantiating. This concept is
being introduced and
- // supported in the
- // getAllChildren functionality in LUCENE-10550. getTopChildren is
temporarily calling
- // getAllChildren to maintain its
- // current behavior, and the current implementation will be replaced by an
actual "top children"
- // implementation
- // in LUCENE-10614
- // TODO: fix getTopChildren in LUCENE-10614
+ /** Reusable entry to hold range label and int count. */
+ private static class Entry {
+ int count;
+ String label;
+ }
+
@Override
public FacetResult getTopChildren(int topN, String dim, String... path)
throws IOException {
validateTopN(topN);
- return getAllChildren(dim, path);
+ validateDimAndPathForGetChildren(dim, path);
+
+ int resultSize = Math.min(topN, counts.length);
+ PriorityQueue<Entry> pq =
+ new PriorityQueue<>(resultSize) {
+ @Override
+ protected boolean lessThan(Entry a, Entry b) {
+ int cmp = Integer.compare(a.count, b.count);
+ if (cmp == 0) {
+ cmp = b.label.compareTo(a.label);
+ }
+ return cmp < 0;
+ }
+ };
+
+ Entry e = null;
+ for (int i = 0; i < counts.length; i++) {
+ if (pq.size() < resultSize) {
+ if (counts[i] != 0) {
Review Comment:
I'd suggest moving this non-zero check to be the first thing inside the
loop. So something like:
```
if (counts[i] == 0) {
continue;
}
```
##########
lucene/facet/src/java/org/apache/lucene/facet/range/RangeFacetCounts.java:
##########
@@ -232,20 +233,69 @@ public FacetResult getAllChildren(String dim, String...
path) throws IOException
return new FacetResult(dim, path, totCount, labelValues,
labelValues.length);
}
- // The current getTopChildren method is not returning "top" ranges. Instead,
it returns all
- // user-provided ranges in
- // the order the user specified them when instantiating. This concept is
being introduced and
- // supported in the
- // getAllChildren functionality in LUCENE-10550. getTopChildren is
temporarily calling
- // getAllChildren to maintain its
- // current behavior, and the current implementation will be replaced by an
actual "top children"
- // implementation
- // in LUCENE-10614
- // TODO: fix getTopChildren in LUCENE-10614
+ /** Reusable entry to hold range label and int count. */
+ private static class Entry {
+ int count;
+ String label;
+ }
+
@Override
public FacetResult getTopChildren(int topN, String dim, String... path)
throws IOException {
validateTopN(topN);
- return getAllChildren(dim, path);
+ validateDimAndPathForGetChildren(dim, path);
+
+ int resultSize = Math.min(topN, counts.length);
+ PriorityQueue<Entry> pq =
+ new PriorityQueue<>(resultSize) {
+ @Override
+ protected boolean lessThan(Entry a, Entry b) {
+ int cmp = Integer.compare(a.count, b.count);
+ if (cmp == 0) {
+ cmp = b.label.compareTo(a.label);
+ }
+ return cmp < 0;
+ }
+ };
+
+ Entry e = null;
+ for (int i = 0; i < counts.length; i++) {
+ if (pq.size() < resultSize) {
Review Comment:
You don't need this extra check on the pq size or the corresponding `else`
branch. `e` will be non-null as soon as the pq is full because of the
`insertWithOverlow` call. Have a look at something like
`LongValueFacetCounts#getTopChildren` as an example please.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
