You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "javanna (via GitHub)" <gi...@apache.org> on 2024/09/06 11:47:09 UTC
[PR] Replace static FacetsCollector#search methods [lucene]
javanna opened a new pull request, #13733:
URL: https://github.com/apache/lucene/pull/13733
We have a few public static utility search methods in FacetsCollector that accept a Collector as last argument. In practice, these are expected to be called providing a `FacetsCollector` as last argument. Also, we'd like to remove all the search methods that take a `Collector` in favour of those that take a `CollectorManager` (see #12892).
This commit adds the corresponding functionality to `FacetsCollectorManager`. The new methods take a `FacetsCollectorManager` as last argument. The return type has to be adapted to include also the facets results that were before made available through the collector argument.
In order for tests to all work I had to add support for `keepScores` to `FacetsCollectorManager` which was missing.
Closes #13725
--
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
Re: [PR] Replace static FacetsCollector#search methods [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on code in PR #13733:
URL: https://github.com/apache/lucene/pull/13733#discussion_r1746991600
##########
lucene/facet/src/java/org/apache/lucene/facet/FacetsCollectorManager.java:
##########
@@ -54,4 +79,135 @@ public ReducedFacetsCollector(final Collection<FacetsCollector> facetsCollectors
facetsCollector -> matchingDocs.addAll(facetsCollector.getMatchingDocs()));
}
}
+
+ /** Utility method, to search and also collect all hits into the provided {@link Collector}. */
+ public static FacetsResult search(
+ IndexSearcher searcher, Query q, int n, FacetsCollectorManager fcm) throws IOException {
+ return doSearch(searcher, null, q, n, null, false, fcm);
+ }
+
+ /** Utility method, to search and also collect all hits into the provided {@link Collector}. */
+ public static FacetsResult search(
+ IndexSearcher searcher, Query q, int n, Sort sort, FacetsCollectorManager fcm)
+ throws IOException {
+ if (sort == null) {
+ throw new IllegalArgumentException("sort must not be null");
+ }
+ return doSearch(searcher, null, q, n, sort, false, fcm);
+ }
+
+ /** Utility method, to search and also collect all hits into the provided {@link Collector}. */
+ public static FacetsResult search(
+ IndexSearcher searcher,
+ Query q,
+ int n,
+ Sort sort,
+ boolean doDocScores,
+ FacetsCollectorManager fcm)
+ throws IOException {
+ if (sort == null) {
+ throw new IllegalArgumentException("sort must not be null");
+ }
+ return doSearch(searcher, null, q, n, sort, doDocScores, fcm);
+ }
+
+ /** Utility method, to search and also collect all hits into the provided {@link Collector}. */
+ public static FacetsResult searchAfter(
+ IndexSearcher searcher, ScoreDoc after, Query q, int n, FacetsCollectorManager fcm)
+ throws IOException {
+ return doSearch(searcher, after, q, n, null, false, fcm);
+ }
+
+ /** Utility method, to search and also collect all hits into the provided {@link Collector}. */
+ public static FacetsResult searchAfter(
+ IndexSearcher searcher, ScoreDoc after, Query q, int n, Sort sort, FacetsCollectorManager fcm)
+ throws IOException {
+ if (sort == null) {
+ throw new IllegalArgumentException("sort must not be null");
+ }
+ return doSearch(searcher, after, q, n, sort, false, fcm);
+ }
+
+ /** Utility method, to search and also collect all hits into the provided {@link Collector}. */
+ public static FacetsResult searchAfter(
+ IndexSearcher searcher,
+ ScoreDoc after,
+ Query q,
+ int n,
+ Sort sort,
+ boolean doDocScores,
+ FacetsCollectorManager fcm)
+ throws IOException {
+ if (sort == null) {
+ throw new IllegalArgumentException("sort must not be null");
+ }
+ return doSearch(searcher, after, q, n, sort, doDocScores, fcm);
+ }
+
+ private static FacetsResult doSearch(
+ IndexSearcher searcher,
+ ScoreDoc after,
+ Query q,
+ int n,
+ Sort sort,
+ boolean doDocScores,
+ FacetsCollectorManager fcm)
+ throws IOException {
+
+ int limit = searcher.getIndexReader().maxDoc();
+ if (limit == 0) {
+ limit = 1;
+ }
+ n = Math.min(n, limit);
+
+ if (after != null && after.doc >= limit) {
+ throw new IllegalArgumentException(
+ "after.doc exceeds the number of documents in the reader: after.doc="
+ + after.doc
+ + " limit="
+ + limit);
+ }
+
+ final TopDocs topDocs;
+ final FacetsCollector facetsCollector;
+ if (n == 0) {
+ TotalHitCountCollectorManager hitCountCollectorManager = new TotalHitCountCollectorManager();
+ MultiCollectorManager multiCollectorManager =
+ new MultiCollectorManager(hitCountCollectorManager, fcm);
+ Object[] result = searcher.search(q, multiCollectorManager);
+ topDocs =
+ new TopDocs(
+ new TotalHits((Integer) result[0], TotalHits.Relation.EQUAL_TO), new ScoreDoc[0]);
+ facetsCollector = (FacetsCollector) result[1];
+ } else {
+ final MultiCollectorManager multiCollectorManager;
+ if (sort != null) {
+ if (after != null && !(after instanceof FieldDoc)) {
+ // TODO: if we fix type safety of TopFieldDocs we can
+ // remove this
+ throw new IllegalArgumentException("after must be a FieldDoc; got " + after);
+ }
+ TopFieldCollectorManager topFieldCollectorManager =
+ new TopFieldCollectorManager(sort, n, (FieldDoc) after, Integer.MAX_VALUE, true);
+ multiCollectorManager = new MultiCollectorManager(topFieldCollectorManager, fcm);
+ } else {
+ TopScoreDocCollectorManager topScoreDocCollectorManager =
+ new TopScoreDocCollectorManager(n, after, Integer.MAX_VALUE, true);
+ multiCollectorManager = new MultiCollectorManager(topScoreDocCollectorManager, fcm);
+ }
+ Object[] result = searcher.search(q, multiCollectorManager);
+ topDocs = (TopDocs) result[0];
+ if (doDocScores) {
+ TopFieldCollector.populateScores(topDocs.scoreDocs, searcher, q);
+ }
+ facetsCollector = (FacetsCollector) result[1];
+ }
+ return new FacetsResult(topDocs, facetsCollector);
+ }
+
+ /**
+ * Holds results of a search run via static utility methods exposed by this class. Those include
+ * {@link TopDocs} as well as facets result included in the returned {@link FacetsCollector}
+ */
+ public record FacetsResult(TopDocs topDocs, FacetsCollector facetsCollector) {}
Review Comment:
I think that using a collector to expose results is a bit of a hack, but that is the case for a while already, so perhaps it's not something to address in this PR.
--
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
Re: [PR] Replace static FacetsCollector#search methods [lucene]
Posted by "gsmiller (via GitHub)" <gi...@apache.org>.
gsmiller commented on code in PR #13733:
URL: https://github.com/apache/lucene/pull/13733#discussion_r1747170856
##########
lucene/facet/src/java/org/apache/lucene/facet/FacetsCollectorManager.java:
##########
@@ -54,4 +79,138 @@ public ReducedFacetsCollector(final Collection<FacetsCollector> facetsCollectors
facetsCollector -> matchingDocs.addAll(facetsCollector.getMatchingDocs()));
}
}
+
+ /** Utility method, to search and also collect all hits into the provided {@link Collector}. */
+ public static FacetsResult search(
+ IndexSearcher searcher, Query q, int n, FacetsCollectorManager fcm) throws IOException {
Review Comment:
Mostly just thinking out loud here, but now that these methods are responsible for creating the `FacetsCollector` that gets returned in the wrapped `FacetsResult` (instead of previously where the user would provide it), it feels a little strange that the user would be responsible for passing in a `FacetsCollectorManager`. Another option would be to have these methods just setup the `FacetsCollectorManager` internally. Is there a need for users to provide their own? I suppose it's useful if users have some use-case where they want to extend FCM with their own customization? And it's also the only way to specify `keepScores`? So maybe we keep this. At the same time, these methods feel aimed at users that want out-of-the-box functionality, so maybe we simplify and take the parameter away? If we did that, I'm not sure how we'd deal with `keepScores` though unless we added yet another parameter to these static methods.
--
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
Re: [PR] Replace static FacetsCollector#search methods [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on code in PR #13733:
URL: https://github.com/apache/lucene/pull/13733#discussion_r1747548475
##########
lucene/facet/src/java/org/apache/lucene/facet/FacetsCollectorManager.java:
##########
@@ -54,4 +79,138 @@ public ReducedFacetsCollector(final Collection<FacetsCollector> facetsCollectors
facetsCollector -> matchingDocs.addAll(facetsCollector.getMatchingDocs()));
}
}
+
+ /** Utility method, to search and also collect all hits into the provided {@link Collector}. */
+ public static FacetsResult search(
+ IndexSearcher searcher, Query q, int n, FacetsCollectorManager fcm) throws IOException {
Review Comment:
Yes! I was going to make the same comment. I was going to ask if we need to care for scenarios where the manager is a subclass of `FacetsCollectorManager`. But the `keepScores` flag made it kind of necessary to accept the collector manager, unless we want to accept the flag instead which is a little cryptic?
--
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
Re: [PR] Replace static FacetsCollector#search methods [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on PR #13733:
URL: https://github.com/apache/lucene/pull/13733#issuecomment-2334786138
Thanks a lot for the speedy review @gsmiller !
--
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
Re: [PR] Replace static FacetsCollector#search methods [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on code in PR #13733:
URL: https://github.com/apache/lucene/pull/13733#discussion_r1746990060
##########
lucene/facet/src/java/org/apache/lucene/facet/FacetsCollector.java:
##########
@@ -155,118 +141,4 @@ public void finish() throws IOException {
scores = null;
context = null;
}
-
- /** Utility method, to search and also collect all hits into the provided {@link Collector}. */
- public static TopDocs search(IndexSearcher searcher, Query q, int n, Collector fc)
Review Comment:
If we agree on the approach, I will deprecate these methods in branch_9x and backport the rest of this PR as well (minus removal) so that the new methods are already as soon as the old are deprecated.
--
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
Re: [PR] Replace static FacetsCollector#search methods [lucene]
Posted by "gsmiller (via GitHub)" <gi...@apache.org>.
gsmiller commented on code in PR #13733:
URL: https://github.com/apache/lucene/pull/13733#discussion_r1747671919
##########
lucene/facet/src/java/org/apache/lucene/facet/FacetsCollectorManager.java:
##########
@@ -54,4 +79,138 @@ public ReducedFacetsCollector(final Collection<FacetsCollector> facetsCollectors
facetsCollector -> matchingDocs.addAll(facetsCollector.getMatchingDocs()));
}
}
+
+ /** Utility method, to search and also collect all hits into the provided {@link Collector}. */
+ public static FacetsResult search(
+ IndexSearcher searcher, Query q, int n, FacetsCollectorManager fcm) throws IOException {
Review Comment:
Yeah... hmm. I think it's fine as you have it (because of `keepScores` in particular). Let's leave it like 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
Re: [PR] Replace static FacetsCollector#search methods [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna merged PR #13733:
URL: https://github.com/apache/lucene/pull/13733
--
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