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/06/22 21:24:13 UTC
[GitHub] [lucene] Yuti-G opened a new pull request, #974: LUCENE-10614: Properly support getTopChildren in RangeFacetCounts
Yuti-G opened a new pull request, #974:
URL: https://github.com/apache/lucene/pull/974
### Description
https://issues.apache.org/jira/browse/LUCENE-10614
<!--
If this is your first contribution to Lucene, please make sure you have reviewed the contribution guide.
https://github.com/apache/lucene/blob/main/CONTRIBUTING.md
-->
--
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 diff in pull request #974: LUCENE-10614: Properly support getTopChildren in RangeFacetCounts
Posted by GitBox <gi...@apache.org>.
gsmiller commented on code in PR #974:
URL: https://github.com/apache/lucene/pull/974#discussion_r908941811
##########
lucene/facet/src/java/org/apache/lucene/facet/range/RangeFacetCounts.java:
##########
@@ -232,20 +233,43 @@ 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
@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<LabelAndValue> pq =
+ new PriorityQueue<>(resultSize) {
+ @Override
+ protected boolean lessThan(LabelAndValue a, LabelAndValue b) {
+ int cmp = Integer.compare(a.value.intValue(), b.value.intValue());
+ if (cmp == 0) {
+ cmp = b.label.compareTo(a.label);
+ }
+ return cmp < 0;
+ }
+ };
+
+ for (int i = 0; i < counts.length; i++) {
+ if (pq.size() < resultSize) {
+ pq.add(new LabelAndValue(ranges[i].label, counts[i]));
Review Comment:
Hmm, good point. I don't think we should change the behavior of `getAllChildren`. I think users will expect to get back an entry for each range they provided, and since that's the existing behavior anyway, I'd prefer not to change it. So now I'm actually wondering if the `getAllChildren` API should actually provide _all_ children in all of the implementations, whether-or-not the count is zero. I think it's actually OK that `getAllChildren` is not identical behavior as `getTopChildren` with a huge top-n value. I think it's OK if we ignore entries with a zero-count in the "top children" case, but now I'm not so sure in the "all children" case. What do you think users would most likely expect here? Should we change `getAllChildren` in all `Facets` implementations to return children 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] gsmiller commented on a diff in pull request #974: LUCENE-10614: Properly support getTopChildren in RangeFacetCounts
Posted by GitBox <gi...@apache.org>.
gsmiller commented on code in PR #974:
URL: https://github.com/apache/lucene/pull/974#discussion_r914082183
##########
lucene/demo/src/java/org/apache/lucene/demo/facet/DistanceFacetsExample.java:
##########
@@ -212,7 +212,26 @@ public static Query getBoundingBoxQuery(
}
/** User runs a query and counts facets. */
- public FacetResult search() throws IOException {
+ public FacetResult searchAllChildren() throws IOException {
+
+ FacetsCollector fc = searcher.search(new MatchAllDocsQuery(), new FacetsCollectorManager());
+
+ Facets facets =
+ new DoubleRangeFacetCounts(
+ "field",
+ getDistanceValueSource(),
+ fc,
+ getBoundingBoxQuery(ORIGIN_LATITUDE, ORIGIN_LONGITUDE, 10.0),
+ ONE_KM,
+ TWO_KM,
+ FIVE_KM,
+ TEN_KM);
+
+ return facets.getAllChildren("field");
+ }
+
+ /** User runs a query and counts facets. */
+ public FacetResult searchTopChildren() throws IOException {
Review Comment:
I see. OK, a couple things confused me in your code. It looks like it's doing what I describe, but what threw me is that, 1) you're updating a variable named `currentTime` to represent the end of each range, and 2) the label says "Past ...", which made me thing it was a trailing window. As a couple suggestions, maybe rename your `then` and `currentTime` variables to something like `startTime` and `endTime`? And maybe rename the labels to just something like "Hour x - y"?
--
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 diff in pull request #974: LUCENE-10614: Properly support getTopChildren in RangeFacetCounts
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on code in PR #974:
URL: https://github.com/apache/lucene/pull/974#discussion_r918297532
##########
lucene/demo/src/java/org/apache/lucene/demo/facet/RangeFacetsExample.java:
##########
@@ -73,6 +76,35 @@ public void index() throws IOException {
indexWriter.addDocument(doc);
}
+ // Add documents with a fake timestamp, 3600 sec (1 hour) after "now", 7200 sec (2
+ // hours) after "now", ...:
+ long startTime = 0;
+ // Index error messages since a week (24 * 7 = 168 hours) ago
+ for (int i = 0; i < 168; i++) {
+ long endTime = startTime + (i + 1) * 3600;
+
+ // Choose a relatively larger number, e,g., "35", in order to create variation in count for
+ // the top-n children, so that getTopChildren(10) in the searchTopChildren functionality
+ // can return children with different counts
+ for (int j = 0; j < i % 35; j++) {
+ Document doc = new Document();
+ // index document at a different timestamp by using endTime - i * j
Review Comment:
I've created a spin off [issue](https://issues.apache.org/jira/browse/LUCENE-10652) to add a get top-n range faceting example to demo. Thanks again for reviewing my pr and left all the detailed and constructive 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 diff in pull request #974: LUCENE-10614: Properly support getTopChildren in RangeFacetCounts
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on code in PR #974:
URL: https://github.com/apache/lucene/pull/974#discussion_r908084991
##########
lucene/facet/src/java/org/apache/lucene/facet/range/RangeFacetCounts.java:
##########
@@ -232,20 +233,43 @@ 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
@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<LabelAndValue> pq =
+ new PriorityQueue<>(resultSize) {
+ @Override
+ protected boolean lessThan(LabelAndValue a, LabelAndValue b) {
+ int cmp = Integer.compare(a.value.intValue(), b.value.intValue());
+ if (cmp == 0) {
+ cmp = b.label.compareTo(a.label);
+ }
+ return cmp < 0;
+ }
+ };
+
+ for (int i = 0; i < counts.length; i++) {
+ if (pq.size() < resultSize) {
+ pq.add(new LabelAndValue(ranges[i].label, counts[i]));
Review Comment:
In this case, I propose we also change the `getAllChildren` functionality in RangeFacetCounts to populate LabelAndValue only when count is > 0 to be consistent with `getAllChildren` in other Facet implementations. Since if top-N is equal to all, we should return the same results from getAllChildren and getTopChildren. Please let me know what you think. 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 diff in pull request #974: LUCENE-10614: Properly support getTopChildren in RangeFacetCounts
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on code in PR #974:
URL: https://github.com/apache/lucene/pull/974#discussion_r908038451
##########
lucene/demo/src/java/org/apache/lucene/demo/facet/DistanceFacetsExample.java:
##########
@@ -212,7 +212,26 @@ public static Query getBoundingBoxQuery(
}
/** User runs a query and counts facets. */
- public FacetResult search() throws IOException {
+ public FacetResult searchAllChildren() throws IOException {
+
+ FacetsCollector fc = searcher.search(new MatchAllDocsQuery(), new FacetsCollectorManager());
+
+ Facets facets =
+ new DoubleRangeFacetCounts(
+ "field",
+ getDistanceValueSource(),
+ fc,
+ getBoundingBoxQuery(ORIGIN_LATITUDE, ORIGIN_LONGITUDE, 10.0),
+ ONE_KM,
+ TWO_KM,
+ FIVE_KM,
+ TEN_KM);
+
+ return facets.getAllChildren("field");
+ }
+
+ /** User runs a query and counts facets. */
+ public FacetResult searchTopChildren() throws IOException {
Review Comment:
I do not have a strong opinion about this. GetAllChildren does make more sense for range faceting. I will replace the getTopChildren with getAllChildren in the original demo. 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 diff in pull request #974: LUCENE-10614: Properly support getTopChildren in RangeFacetCounts
Posted by GitBox <gi...@apache.org>.
gsmiller commented on code in PR #974:
URL: https://github.com/apache/lucene/pull/974#discussion_r907916354
##########
lucene/facet/src/java/org/apache/lucene/facet/range/RangeFacetCounts.java:
##########
@@ -232,20 +233,43 @@ 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
@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<LabelAndValue> pq =
+ new PriorityQueue<>(resultSize) {
+ @Override
+ protected boolean lessThan(LabelAndValue a, LabelAndValue b) {
+ int cmp = Integer.compare(a.value.intValue(), b.value.intValue());
+ if (cmp == 0) {
+ cmp = b.label.compareTo(a.label);
+ }
+ return cmp < 0;
+ }
+ };
+
+ for (int i = 0; i < counts.length; i++) {
+ if (pq.size() < resultSize) {
+ pq.add(new LabelAndValue(ranges[i].label, counts[i]));
Review Comment:
I wonder if we should only add to the pq when the count is > 0 to be consistent with other Facet implementations. What do you think?
##########
lucene/demo/src/java/org/apache/lucene/demo/facet/DistanceFacetsExample.java:
##########
@@ -212,7 +212,26 @@ public static Query getBoundingBoxQuery(
}
/** User runs a query and counts facets. */
- public FacetResult search() throws IOException {
+ public FacetResult searchAllChildren() throws IOException {
+
+ FacetsCollector fc = searcher.search(new MatchAllDocsQuery(), new FacetsCollectorManager());
+
+ Facets facets =
+ new DoubleRangeFacetCounts(
+ "field",
+ getDistanceValueSource(),
+ fc,
+ getBoundingBoxQuery(ORIGIN_LATITUDE, ORIGIN_LONGITUDE, 10.0),
+ ONE_KM,
+ TWO_KM,
+ FIVE_KM,
+ TEN_KM);
+
+ return facets.getAllChildren("field");
+ }
+
+ /** User runs a query and counts facets. */
+ public FacetResult searchTopChildren() throws IOException {
Review Comment:
I'm not totally sold we need to demo the `getTopChildren` functionality. It feels like it will be a little obscure for range faceting to me. What do you think of just changing the existing example code in-place to use `getAllChildren` instead of `getTopChildren` since that's probably the more common use-case? Curious what you think though. Do you think we should demo `getTopChildren` as well?
##########
lucene/facet/src/java/org/apache/lucene/facet/range/RangeFacetCounts.java:
##########
@@ -232,20 +233,43 @@ 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
@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<LabelAndValue> pq =
+ new PriorityQueue<>(resultSize) {
+ @Override
+ protected boolean lessThan(LabelAndValue a, LabelAndValue b) {
+ int cmp = Integer.compare(a.value.intValue(), b.value.intValue());
+ if (cmp == 0) {
+ cmp = b.label.compareTo(a.label);
+ }
+ return cmp < 0;
+ }
+ };
+
+ for (int i = 0; i < counts.length; i++) {
+ if (pq.size() < resultSize) {
+ pq.add(new LabelAndValue(ranges[i].label, counts[i]));
+ } else {
+ int topValue = pq.top().value.intValue();
+ if (counts[i] > topValue
+ || (counts[i] == topValue && ranges[i].label.compareTo(pq.top().label) < 0)) {
+ pq.updateTop(new LabelAndValue(ranges[i].label, counts[i]));
+ }
+ }
Review Comment:
Ideally, you'd do something like this here to avoid creating unnecessary garbage:
```
} else {
LabelAndValue top = pq.top();
assert top != null;
int topValue = top.value.intValue();
if (counts[i] > topValue || (counts[i] == topValue && ranges[i].label.compareTo(top.label) < 0)) {
top.label = ranges[i].label;
top.value = counts[i];
pq.updateTop();
}
}
```
... but of course LabelAndValue is immutable. The way we do these elsewhere is to use some internal class to just store the pq entries, and then translate into `LabelAndValue` instances at the return. Have a look at `LongValueFacetCount#getTopChildren` as an example.
I would suggest following this similar approach for consistency here. Of course, this might actually create more garbage if top-n and the total number of ranges are very similar. So there's probably an optimization opportunity here where we implement this differently depending on the ratio of top-n to the total number of ranges (but I would save that for a separate issue/exploration).
--
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 diff in pull request #974: LUCENE-10614: Properly support getTopChildren in RangeFacetCounts
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on code in PR #974:
URL: https://github.com/apache/lucene/pull/974#discussion_r908084991
##########
lucene/facet/src/java/org/apache/lucene/facet/range/RangeFacetCounts.java:
##########
@@ -232,20 +233,43 @@ 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
@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<LabelAndValue> pq =
+ new PriorityQueue<>(resultSize) {
+ @Override
+ protected boolean lessThan(LabelAndValue a, LabelAndValue b) {
+ int cmp = Integer.compare(a.value.intValue(), b.value.intValue());
+ if (cmp == 0) {
+ cmp = b.label.compareTo(a.label);
+ }
+ return cmp < 0;
+ }
+ };
+
+ for (int i = 0; i < counts.length; i++) {
+ if (pq.size() < resultSize) {
+ pq.add(new LabelAndValue(ranges[i].label, counts[i]));
Review Comment:
In this case, I propose we also change the `getAllChildren` functionality in RangeFacetCounts to populate LabelAndValue only when count is > 0 to be consistent with `getAllChildren` in other Facet implementations. Please let me know what you think. 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 diff in pull request #974: LUCENE-10614: Properly support getTopChildren in RangeFacetCounts
Posted by GitBox <gi...@apache.org>.
gsmiller commented on code in PR #974:
URL: https://github.com/apache/lucene/pull/974#discussion_r910480915
##########
lucene/facet/src/java/org/apache/lucene/facet/range/RangeFacetCounts.java:
##########
@@ -232,20 +233,43 @@ 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
@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<LabelAndValue> pq =
+ new PriorityQueue<>(resultSize) {
+ @Override
+ protected boolean lessThan(LabelAndValue a, LabelAndValue b) {
+ int cmp = Integer.compare(a.value.intValue(), b.value.intValue());
+ if (cmp == 0) {
+ cmp = b.label.compareTo(a.label);
+ }
+ return cmp < 0;
+ }
+ };
+
+ for (int i = 0; i < counts.length; i++) {
+ if (pq.size() < resultSize) {
+ pq.add(new LabelAndValue(ranges[i].label, counts[i]));
Review Comment:
Perfect, 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 closed pull request #974: LUCENE-10614: Properly support getTopChildren in RangeFacetCounts
Posted by GitBox <gi...@apache.org>.
Yuti-G closed pull request #974: LUCENE-10614: Properly support getTopChildren in RangeFacetCounts
URL: https://github.com/apache/lucene/pull/974
--
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 diff in pull request #974: LUCENE-10614: Properly support getTopChildren in RangeFacetCounts
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on code in PR #974:
URL: https://github.com/apache/lucene/pull/974#discussion_r916342857
##########
lucene/demo/src/java/org/apache/lucene/demo/facet/RangeFacetsExample.java:
##########
@@ -73,6 +76,35 @@ public void index() throws IOException {
indexWriter.addDocument(doc);
}
+ // Add documents with a fake timestamp, 3600 sec (1 hour) after "now", 7200 sec (2
+ // hours) after "now", ...:
+ long startTime = 0;
+ // Index error messages since a week (24 * 7 = 168 hours) ago
+ for (int i = 0; i < 168; i++) {
+ long endTime = startTime + (i + 1) * 3600;
+
+ // Choose a relatively larger number, e,g., "35", in order to create variation in count for
+ // the top-n children, so that getTopChildren(10) in the searchTopChildren functionality
+ // can return children with different counts
+ for (int j = 0; j < i % 35; j++) {
+ Document doc = new Document();
+ // index document at a different timestamp by using endTime - i * j
Review Comment:
Using `Random` does add some complexity for testing, and I was trying to keep it as simple as the current example , but sorry that causes confusion. I will create a separate issue to add a top-n range faceting example after this pr is merged, and will try to use random and add clear comments to the example code. 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 diff in pull request #974: LUCENE-10614: Properly support getTopChildren in RangeFacetCounts
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on code in PR #974:
URL: https://github.com/apache/lucene/pull/974#discussion_r909238996
##########
lucene/demo/src/java/org/apache/lucene/demo/facet/DistanceFacetsExample.java:
##########
@@ -212,7 +212,26 @@ public static Query getBoundingBoxQuery(
}
/** User runs a query and counts facets. */
- public FacetResult search() throws IOException {
+ public FacetResult searchAllChildren() throws IOException {
+
+ FacetsCollector fc = searcher.search(new MatchAllDocsQuery(), new FacetsCollectorManager());
+
+ Facets facets =
+ new DoubleRangeFacetCounts(
+ "field",
+ getDistanceValueSource(),
+ fc,
+ getBoundingBoxQuery(ORIGIN_LATITUDE, ORIGIN_LONGITUDE, 10.0),
+ ONE_KM,
+ TWO_KM,
+ FIVE_KM,
+ TEN_KM);
+
+ return facets.getAllChildren("field");
+ }
+
+ /** User runs a query and counts facets. */
+ public FacetResult searchTopChildren() throws IOException {
Review Comment:
We have `getTopChildren` in other facet demo examples. I think that should be sufficient for now? However, it might be worth to make an example for children with 0 count and call `getTopChildren` and `getAllChildren` in parallel to demo the different results? I can add this example to `TestSimpleFacetsExample` if you think it is useful. 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 diff in pull request #974: LUCENE-10614: Properly support getTopChildren in RangeFacetCounts
Posted by GitBox <gi...@apache.org>.
gsmiller commented on code in PR #974:
URL: https://github.com/apache/lucene/pull/974#discussion_r918110247
##########
lucene/demo/src/java/org/apache/lucene/demo/facet/RangeFacetsExample.java:
##########
@@ -73,6 +76,35 @@ public void index() throws IOException {
indexWriter.addDocument(doc);
}
+ // Add documents with a fake timestamp, 3600 sec (1 hour) after "now", 7200 sec (2
+ // hours) after "now", ...:
+ long startTime = 0;
+ // Index error messages since a week (24 * 7 = 168 hours) ago
+ for (int i = 0; i < 168; i++) {
+ long endTime = startTime + (i + 1) * 3600;
+
+ // Choose a relatively larger number, e,g., "35", in order to create variation in count for
+ // the top-n children, so that getTopChildren(10) in the searchTopChildren functionality
+ // can return children with different counts
+ for (int j = 0; j < i % 35; j++) {
+ Document doc = new Document();
+ // index document at a different timestamp by using endTime - i * j
Review Comment:
OK, thank you! I like the idea and I think it's close, but I think we can come up with a simpler way. I think you could randomly distribute the "data points" within each hour when you index without impacting testing at all. The facet counts should remain the same regardless of how the data points are distributed, so testing should be stable I think? So maybe we hit a compromise that uses a stable number of data points per hour time period (which you could do with your modulus operation if you like) but then randomly jitter the data within each hour block?
But yeah, let's take it up as a follow on issue. Would you mind linking that here once you create it so the conversation is easier to follow for future readers? Thanks again for all the hard work!
--
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 #974: LUCENE-10614: Properly support getTopChildren in RangeFacetCounts
Posted by GitBox <gi...@apache.org>.
gsmiller merged PR #974:
URL: https://github.com/apache/lucene/pull/974
--
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 diff in pull request #974: LUCENE-10614: Properly support getTopChildren in RangeFacetCounts
Posted by GitBox <gi...@apache.org>.
gsmiller commented on code in PR #974:
URL: https://github.com/apache/lucene/pull/974#discussion_r914083105
##########
lucene/demo/src/java/org/apache/lucene/demo/facet/RangeFacetsExample.java:
##########
@@ -73,6 +76,30 @@ public void index() throws IOException {
indexWriter.addDocument(doc);
}
+ // Add documents with a fake timestamp, 3600 sec (1 hour) before "now", 7200 sec before (2
+ // hours) before "now", ...:
+ long currentTime = System.currentTimeMillis() / 1000L;
+ // Index error messages for the past week (24 * 7 = 168 hours)
+ for (int i = 0; i < 168; i++) {
+ long then = currentTime - (i + 1) * 3600;
+
+ // conditionally add different number of error messages to the past hour slot
+ for (int j = 0; j < i % 35; j++) {
Review Comment:
Why mod with a value of `35`? I'm not getting it. Is there a specific reason you're using the value `35`? If so, could you add a comment?
##########
lucene/demo/src/java/org/apache/lucene/demo/facet/RangeFacetsExample.java:
##########
@@ -73,6 +76,30 @@ public void index() throws IOException {
indexWriter.addDocument(doc);
}
+ // Add documents with a fake timestamp, 3600 sec (1 hour) before "now", 7200 sec before (2
+ // hours) before "now", ...:
+ long currentTime = System.currentTimeMillis() / 1000L;
+ // Index error messages for the past week (24 * 7 = 168 hours)
+ for (int i = 0; i < 168; i++) {
Review Comment:
minor: I find it a bit confusing that you index the data and create ranges in "reverse" starting from "now." Would it be easier for others to understand if you started "one week ago" and looped "forwards"? Since the demo code is here to help people understand the functionality, I want to make sure we don't create unnecessary confusion.
##########
lucene/demo/src/java/org/apache/lucene/demo/facet/RangeFacetsExample.java:
##########
@@ -73,6 +76,30 @@ public void index() throws IOException {
indexWriter.addDocument(doc);
}
+ // Add documents with a fake timestamp, 3600 sec (1 hour) before "now", 7200 sec before (2
+ // hours) before "now", ...:
+ long currentTime = System.currentTimeMillis() / 1000L;
+ // Index error messages for the past week (24 * 7 = 168 hours)
+ for (int i = 0; i < 168; i++) {
+ long then = currentTime - (i + 1) * 3600;
+
+ // conditionally add different number of error messages to the past hour slot
+ for (int j = 0; j < i % 35; j++) {
+ Document doc = new Document();
+ doc.add(new NumericDocValuesField("error log", then));
Review Comment:
Could we add some "jitter" to what gets indexed so all the timestamps of the "fake error logs" don't fall right on hour boundaries? That would be a bit of a more realistic example.
##########
lucene/demo/src/java/org/apache/lucene/demo/facet/RangeFacetsExample.java:
##########
@@ -73,6 +76,30 @@ public void index() throws IOException {
indexWriter.addDocument(doc);
}
+ // Add documents with a fake timestamp, 3600 sec (1 hour) before "now", 7200 sec before (2
+ // hours) before "now", ...:
+ long currentTime = System.currentTimeMillis() / 1000L;
+ // Index error messages for the past week (24 * 7 = 168 hours)
+ for (int i = 0; i < 168; i++) {
+ long then = currentTime - (i + 1) * 3600;
+
+ // conditionally add different number of error messages to the past hour slot
+ for (int j = 0; j < i % 35; j++) {
+ Document doc = new Document();
+ doc.add(new NumericDocValuesField("error log", then));
+ doc.add(
+ new StringField(
+ "Error msg", "[Error] Server encountered error at " + currentTime, Field.Store.NO));
Review Comment:
Shouldn't the "logged" timestamp here be the same as the one indexed in the previous line (i.e., `then` instead of `currentTime`)? I realize that doesn't impact the functionality of the example, but just trying to avoid 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] Yuti-G commented on a diff in pull request #974: LUCENE-10614: Properly support getTopChildren in RangeFacetCounts
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on code in PR #974:
URL: https://github.com/apache/lucene/pull/974#discussion_r909227446
##########
lucene/facet/src/java/org/apache/lucene/facet/range/RangeFacetCounts.java:
##########
@@ -232,20 +233,43 @@ 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
@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<LabelAndValue> pq =
+ new PriorityQueue<>(resultSize) {
+ @Override
+ protected boolean lessThan(LabelAndValue a, LabelAndValue b) {
+ int cmp = Integer.compare(a.value.intValue(), b.value.intValue());
+ if (cmp == 0) {
+ cmp = b.label.compareTo(a.label);
+ }
+ return cmp < 0;
+ }
+ };
+
+ for (int i = 0; i < counts.length; i++) {
+ if (pq.size() < resultSize) {
+ pq.add(new LabelAndValue(ranges[i].label, counts[i]));
Review Comment:
True, I think this can actually make a good use case for users who want to get all results regardless of the count. If users want to get FacetResults with count > 0, they can call `getTopChildren` instead. I will open a spin-off issue to change `getAllChildren` to also return children with a zero-count in all Facets implementation, and add more to the `getAllChildren` javadoc in Facets to explain 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] Yuti-G commented on a diff in pull request #974: LUCENE-10614: Properly support getTopChildren in RangeFacetCounts
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on code in PR #974:
URL: https://github.com/apache/lucene/pull/974#discussion_r912299921
##########
lucene/demo/src/java/org/apache/lucene/demo/facet/DistanceFacetsExample.java:
##########
@@ -212,7 +212,26 @@ public static Query getBoundingBoxQuery(
}
/** User runs a query and counts facets. */
- public FacetResult search() throws IOException {
+ public FacetResult searchAllChildren() throws IOException {
+
+ FacetsCollector fc = searcher.search(new MatchAllDocsQuery(), new FacetsCollectorManager());
+
+ Facets facets =
+ new DoubleRangeFacetCounts(
+ "field",
+ getDistanceValueSource(),
+ fc,
+ getBoundingBoxQuery(ORIGIN_LATITUDE, ORIGIN_LONGITUDE, 10.0),
+ ONE_KM,
+ TWO_KM,
+ FIVE_KM,
+ TEN_KM);
+
+ return facets.getAllChildren("field");
+ }
+
+ /** User runs a query and counts facets. */
+ public FacetResult searchTopChildren() throws IOException {
Review Comment:
Thanks for providing an example! I've added it to the RangeFacetsExample, and would appreciate your feedback. 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 diff in pull request #974: LUCENE-10614: Properly support getTopChildren in RangeFacetCounts
Posted by GitBox <gi...@apache.org>.
gsmiller commented on code in PR #974:
URL: https://github.com/apache/lucene/pull/974#discussion_r916216255
##########
lucene/demo/src/java/org/apache/lucene/demo/facet/RangeFacetsExample.java:
##########
@@ -73,6 +76,35 @@ public void index() throws IOException {
indexWriter.addDocument(doc);
}
+ // Add documents with a fake timestamp, 3600 sec (1 hour) after "now", 7200 sec (2
+ // hours) after "now", ...:
+ long startTime = 0;
+ // Index error messages since a week (24 * 7 = 168 hours) ago
+ for (int i = 0; i < 168; i++) {
+ long endTime = startTime + (i + 1) * 3600;
+
+ // Choose a relatively larger number, e,g., "35", in order to create variation in count for
+ // the top-n children, so that getTopChildren(10) in the searchTopChildren functionality
+ // can return children with different counts
+ for (int j = 0; j < i % 35; j++) {
+ Document doc = new Document();
+ // index document at a different timestamp by using endTime - i * j
Review Comment:
Sorry, I'm sure what you're doing is really obvious to you, but it's just confusing to me. I find myself really stuck on things like `endTime - i * j`, or `i % 35` as a way to generate different numbers of log events within an hour block. What's wrong with just using `Random`? Would that just make it impossible to test? Sorry to be a pain with this, but if I were a user just trying to understand range faceting and I looked at this code, I'd be spending all my time just trying to figure out what we're trying to simulate here instead of understanding faceting. There has to be a simpler way right?
As a suggestion, maybe we create a separate Jira issue to add a top-n range faceting example and revert out this work for now? That would let us get the actual change merged in the meantime.
##########
lucene/demo/src/java/org/apache/lucene/demo/facet/RangeFacetsExample.java:
##########
@@ -73,6 +76,35 @@ public void index() throws IOException {
indexWriter.addDocument(doc);
}
+ // Add documents with a fake timestamp, 3600 sec (1 hour) after "now", 7200 sec (2
+ // hours) after "now", ...:
+ long startTime = 0;
+ // Index error messages since a week (24 * 7 = 168 hours) ago
+ for (int i = 0; i < 168; i++) {
+ long endTime = startTime + (i + 1) * 3600;
+
+ // Choose a relatively larger number, e,g., "35", in order to create variation in count for
+ // the top-n children, so that getTopChildren(10) in the searchTopChildren functionality
+ // can return children with different counts
+ for (int j = 0; j < i % 35; j++) {
+ Document doc = new Document();
+ // index document at a different timestamp by using endTime - i * j
+ doc.add(new NumericDocValuesField("error log", endTime - i * j));
Review Comment:
Maybe "error timestamp" would be a better name?
--
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 diff in pull request #974: LUCENE-10614: Properly support getTopChildren in RangeFacetCounts
Posted by GitBox <gi...@apache.org>.
gsmiller commented on code in PR #974:
URL: https://github.com/apache/lucene/pull/974#discussion_r908942536
##########
lucene/demo/src/java/org/apache/lucene/demo/facet/DistanceFacetsExample.java:
##########
@@ -212,7 +212,26 @@ public static Query getBoundingBoxQuery(
}
/** User runs a query and counts facets. */
- public FacetResult search() throws IOException {
+ public FacetResult searchAllChildren() throws IOException {
+
+ FacetsCollector fc = searcher.search(new MatchAllDocsQuery(), new FacetsCollectorManager());
+
+ Facets facets =
+ new DoubleRangeFacetCounts(
+ "field",
+ getDistanceValueSource(),
+ fc,
+ getBoundingBoxQuery(ORIGIN_LATITUDE, ORIGIN_LONGITUDE, 10.0),
+ ONE_KM,
+ TWO_KM,
+ FIVE_KM,
+ TEN_KM);
+
+ return facets.getAllChildren("field");
+ }
+
+ /** User runs a query and counts facets. */
+ public FacetResult searchTopChildren() throws IOException {
Review Comment:
OK thanks. I'm not opposed to demoing a `getTopChildren` example for what it's worth, but if we want to do that, let's try to come up with a real-world type use-case where a user might care about the "top" values?
--
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 diff in pull request #974: LUCENE-10614: Properly support getTopChildren in RangeFacetCounts
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on code in PR #974:
URL: https://github.com/apache/lucene/pull/974#discussion_r909227446
##########
lucene/facet/src/java/org/apache/lucene/facet/range/RangeFacetCounts.java:
##########
@@ -232,20 +233,43 @@ 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
@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<LabelAndValue> pq =
+ new PriorityQueue<>(resultSize) {
+ @Override
+ protected boolean lessThan(LabelAndValue a, LabelAndValue b) {
+ int cmp = Integer.compare(a.value.intValue(), b.value.intValue());
+ if (cmp == 0) {
+ cmp = b.label.compareTo(a.label);
+ }
+ return cmp < 0;
+ }
+ };
+
+ for (int i = 0; i < counts.length; i++) {
+ if (pq.size() < resultSize) {
+ pq.add(new LabelAndValue(ranges[i].label, counts[i]));
Review Comment:
True, I think this can actually make a good use case for users who want to get all results regardless of the count. If users want to get FacetResults with count > 0, they can call `getTopChildren` instead. I will open a spin-off issue to change `getAllChildren` to also return children with a zero-count in all Facets implementations, and add more to the `getAllChildren` javadoc in Facets to explain 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] Yuti-G commented on a diff in pull request #974: LUCENE-10614: Properly support getTopChildren in RangeFacetCounts
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on code in PR #974:
URL: https://github.com/apache/lucene/pull/974#discussion_r915448876
##########
lucene/demo/src/java/org/apache/lucene/demo/facet/DistanceFacetsExample.java:
##########
@@ -212,7 +212,26 @@ public static Query getBoundingBoxQuery(
}
/** User runs a query and counts facets. */
- public FacetResult search() throws IOException {
+ public FacetResult searchAllChildren() throws IOException {
+
+ FacetsCollector fc = searcher.search(new MatchAllDocsQuery(), new FacetsCollectorManager());
+
+ Facets facets =
+ new DoubleRangeFacetCounts(
+ "field",
+ getDistanceValueSource(),
+ fc,
+ getBoundingBoxQuery(ORIGIN_LATITUDE, ORIGIN_LONGITUDE, 10.0),
+ ONE_KM,
+ TWO_KM,
+ FIVE_KM,
+ TEN_KM);
+
+ return facets.getAllChildren("field");
+ }
+
+ /** User runs a query and counts facets. */
+ public FacetResult searchTopChildren() throws IOException {
Review Comment:
Sorry for the confusion. I've renamed the variables and addressed the comments in the latest commit. Please do let me know if there is anything that still look confusing. Thanks again for all the great suggestions!
--
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 diff in pull request #974: LUCENE-10614: Properly support getTopChildren in RangeFacetCounts
Posted by GitBox <gi...@apache.org>.
gsmiller commented on code in PR #974:
URL: https://github.com/apache/lucene/pull/974#discussion_r910485371
##########
lucene/demo/src/java/org/apache/lucene/demo/facet/DistanceFacetsExample.java:
##########
@@ -212,7 +212,26 @@ public static Query getBoundingBoxQuery(
}
/** User runs a query and counts facets. */
- public FacetResult search() throws IOException {
+ public FacetResult searchAllChildren() throws IOException {
+
+ FacetsCollector fc = searcher.search(new MatchAllDocsQuery(), new FacetsCollectorManager());
+
+ Facets facets =
+ new DoubleRangeFacetCounts(
+ "field",
+ getDistanceValueSource(),
+ fc,
+ getBoundingBoxQuery(ORIGIN_LATITUDE, ORIGIN_LONGITUDE, 10.0),
+ ONE_KM,
+ TWO_KM,
+ FIVE_KM,
+ TEN_KM);
+
+ return facets.getAllChildren("field");
+ }
+
+ /** User runs a query and counts facets. */
+ public FacetResult searchTopChildren() throws IOException {
Review Comment:
Yeah maybe. I think if you can come up with a real-world example that has a somewhat high cardinality of children but where you only want a small subset, then building an example around that could be useful. Here's one I just thought of, but maybe you can come up with something else?
What if, as an example, you indexed error messages in a service log so you could do analysis over them. Each document could be an error log entry that contains the log message string and also a timestamp for when it occurred. Then let's say you wanted to find the top 5 hour periods that had the most errors over the past week. To do this, you could create 168 ranges (each for a one hour time period; 7 * 24 = 268) and facet on them. Then you could ask for the top-5 by count. That would give you the five hour periods over the last week with the most errors.
--
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 diff in pull request #974: LUCENE-10614: Properly support getTopChildren in RangeFacetCounts
Posted by GitBox <gi...@apache.org>.
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-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 diff in pull request #974: LUCENE-10614: Properly support getTopChildren in RangeFacetCounts
Posted by GitBox <gi...@apache.org>.
gsmiller commented on code in PR #974:
URL: https://github.com/apache/lucene/pull/974#discussion_r912362078
##########
lucene/demo/src/java/org/apache/lucene/demo/facet/DistanceFacetsExample.java:
##########
@@ -212,7 +212,26 @@ public static Query getBoundingBoxQuery(
}
/** User runs a query and counts facets. */
- public FacetResult search() throws IOException {
+ public FacetResult searchAllChildren() throws IOException {
+
+ FacetsCollector fc = searcher.search(new MatchAllDocsQuery(), new FacetsCollectorManager());
+
+ Facets facets =
+ new DoubleRangeFacetCounts(
+ "field",
+ getDistanceValueSource(),
+ fc,
+ getBoundingBoxQuery(ORIGIN_LATITUDE, ORIGIN_LONGITUDE, 10.0),
+ ONE_KM,
+ TWO_KM,
+ FIVE_KM,
+ TEN_KM);
+
+ return facets.getAllChildren("field");
+ }
+
+ /** User runs a query and counts facets. */
+ public FacetResult searchTopChildren() throws IOException {
Review Comment:
Thanks! I don't think this is quite what I was suggesting, but maybe it's OK? Or maybe I haven't had enough coffee yet and am misreading your change? But the idea I was proposing would be to faceting on one-hour buckets, not a trailing window as you have. So each range would only be one hour "wide", but we'd cover an entire week's worth of them. This type of log analysis is something you might want to do in the real-world, where you want to understand how many errors occurred in each hour over the last week. So you're essentially creating a histogram of error counts over a week-long time period (in one-hour granularity). And then you're answering the question, "what are the top-5 hours that contained the most errors?" Does this make sense?
I think what you have here is a "trailing window" sort of thing that grows but the endpoint is always at "now"? I'm not sure this is all that interesting of an example since the larger ranges will _always_ contain more errors right? Sorry if I'm mis-reading your new 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 diff in pull request #974: LUCENE-10614: Properly support getTopChildren in RangeFacetCounts
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on code in PR #974:
URL: https://github.com/apache/lucene/pull/974#discussion_r912548774
##########
lucene/demo/src/java/org/apache/lucene/demo/facet/DistanceFacetsExample.java:
##########
@@ -212,7 +212,26 @@ public static Query getBoundingBoxQuery(
}
/** User runs a query and counts facets. */
- public FacetResult search() throws IOException {
+ public FacetResult searchAllChildren() throws IOException {
+
+ FacetsCollector fc = searcher.search(new MatchAllDocsQuery(), new FacetsCollectorManager());
+
+ Facets facets =
+ new DoubleRangeFacetCounts(
+ "field",
+ getDistanceValueSource(),
+ fc,
+ getBoundingBoxQuery(ORIGIN_LATITUDE, ORIGIN_LONGITUDE, 10.0),
+ ONE_KM,
+ TWO_KM,
+ FIVE_KM,
+ TEN_KM);
+
+ return facets.getAllChildren("field");
+ }
+
+ /** User runs a query and counts facets. */
+ public FacetResult searchTopChildren() throws IOException {
Review Comment:
Hmmm, I think the added example is intending to do what you described. It is faceting on one-hour bucket and tracing the error message in each one-hour slot from the past 168 hours (entire week). Only the first range (past `0-1` hour) has an end point as now. For better understanding, I changed the starting index in the range label to start from (0-1) hour in the latest commit.
For example, if I call `getAllChildren("error log")`, the results are:
```
dim=error log path=[] value=2758 childCount=168
Past 0-1 hour (0)
Past 1-2 hour (1)
Past 2-3 hour (2)
Past 3-4 hour (3)
Past 4-5 hour (4)
Past 5-6 hour (5)
Past 6-7 hour (6)
Past 7-8 hour (7)
Past 8-9 hour (8)
Past 9-10 hour (9)
Past 10-11 hour (10)
Past 11-12 hour (11)
Past 12-13 hour (12)
Past 13-14 hour (13)
Past 14-15 hour (14)
Past 15-16 hour (15)
Past 16-17 hour (16)
Past 17-18 hour (17)
Past 18-19 hour (18)
Past 19-20 hour (19)
Past 20-21 hour (20)
Past 21-22 hour (21)
Past 22-23 hour (22)
Past 23-24 hour (23)
Past 24-25 hour (24)
Past 25-26 hour (25)
Past 26-27 hour (26)
Past 27-28 hour (27)
Past 28-29 hour (28)
Past 29-30 hour (29)
Past 30-31 hour (30)
Past 31-32 hour (31)
Past 32-33 hour (32)
Past 33-34 hour (33)
Past 34-35 hour (34)
Past 35-36 hour (0)
Past 36-37 hour (1)
Past 37-38 hour (2)
Past 38-39 hour (3)
Past 39-40 hour (4)
Past 40-41 hour (5)
Past 41-42 hour (6)
Past 42-43 hour (7)
Past 43-44 hour (8)
Past 44-45 hour (9)
Past 45-46 hour (10)
Past 46-47 hour (11)
Past 47-48 hour (12)
Past 48-49 hour (13)
Past 49-50 hour (14)
Past 50-51 hour (15)
Past 51-52 hour (16)
Past 52-53 hour (17)
Past 53-54 hour (18)
Past 54-55 hour (19)
Past 55-56 hour (20)
Past 56-57 hour (21)
Past 57-58 hour (22)
Past 58-59 hour (23)
Past 59-60 hour (24)
Past 60-61 hour (25)
Past 61-62 hour (26)
Past 62-63 hour (27)
Past 63-64 hour (28)
Past 64-65 hour (29)
Past 65-66 hour (30)
Past 66-67 hour (31)
Past 67-68 hour (32)
Past 68-69 hour (33)
Past 69-70 hour (34)
Past 70-71 hour (0)
Past 71-72 hour (1)
Past 72-73 hour (2)
Past 73-74 hour (3)
Past 74-75 hour (4)
Past 75-76 hour (5)
Past 76-77 hour (6)
Past 77-78 hour (7)
Past 78-79 hour (8)
Past 79-80 hour (9)
Past 80-81 hour (10)
Past 81-82 hour (11)
Past 82-83 hour (12)
Past 83-84 hour (13)
Past 84-85 hour (14)
Past 85-86 hour (15)
Past 86-87 hour (16)
Past 87-88 hour (17)
Past 88-89 hour (18)
Past 89-90 hour (19)
Past 90-91 hour (20)
Past 91-92 hour (21)
Past 92-93 hour (22)
Past 93-94 hour (23)
Past 94-95 hour (24)
Past 95-96 hour (25)
Past 96-97 hour (26)
Past 97-98 hour (27)
Past 98-99 hour (28)
Past 99-100 hour (29)
Past 100-101 hour (30)
Past 101-102 hour (31)
Past 102-103 hour (32)
Past 103-104 hour (33)
Past 104-105 hour (34)
Past 105-106 hour (0)
Past 106-107 hour (1)
Past 107-108 hour (2)
Past 108-109 hour (3)
Past 109-110 hour (4)
Past 110-111 hour (5)
Past 111-112 hour (6)
Past 112-113 hour (7)
Past 113-114 hour (8)
Past 114-115 hour (9)
Past 115-116 hour (10)
Past 116-117 hour (11)
Past 117-118 hour (12)
Past 118-119 hour (13)
Past 119-120 hour (14)
Past 120-121 hour (15)
Past 121-122 hour (16)
Past 122-123 hour (17)
Past 123-124 hour (18)
Past 124-125 hour (19)
Past 125-126 hour (20)
Past 126-127 hour (21)
Past 127-128 hour (22)
Past 128-129 hour (23)
Past 129-130 hour (24)
Past 130-131 hour (25)
Past 131-132 hour (26)
Past 132-133 hour (27)
Past 133-134 hour (28)
Past 134-135 hour (29)
Past 135-136 hour (30)
Past 136-137 hour (31)
Past 137-138 hour (32)
Past 138-139 hour (33)
Past 139-140 hour (34)
Past 140-141 hour (0)
Past 141-142 hour (1)
Past 142-143 hour (2)
Past 143-144 hour (3)
Past 144-145 hour (4)
Past 145-146 hour (5)
Past 146-147 hour (6)
Past 147-148 hour (7)
Past 148-149 hour (8)
Past 149-150 hour (9)
Past 150-151 hour (10)
Past 151-152 hour (11)
Past 152-153 hour (12)
Past 153-154 hour (13)
Past 154-155 hour (14)
Past 155-156 hour (15)
Past 156-157 hour (16)
Past 157-158 hour (17)
Past 158-159 hour (18)
Past 159-160 hour (19)
Past 160-161 hour (20)
Past 161-162 hour (21)
Past 162-163 hour (22)
Past 163-164 hour (23)
Past 164-165 hour (24)
Past 165-166 hour (25)
Past 166-167 hour (26)
Past 167-168 hour (27)
```
If I call `getTopChildren(10, "error log")`, the results are
```
dim=error log path=[] value=2758 childCount=163
Past 104-105 hour (34)
Past 139-140 hour (34)
Past 34-35 hour (34)
Past 69-70 hour (34)
Past 103-104 hour (33)
Past 138-139 hour (33)
Past 33-34 hour (33)
Past 68-69 hour (33)
Past 102-103 hour (32)
Past 137-138 hour (32)
```
Please let me know if there is any question/issue, thank you very much!
--
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