LUCENE-10002: Deprecate IndexSearch#search(Query, Collector) in favor of IndexSearcher#search(Query, CollectorManager) - TopFieldCollectorManager & TopScoreDocCollectorManager #240
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,9 +66,9 @@ | |
| * match lots of documents, counting the number of hits may take much longer than computing the top | ||
| * hits so this trade-off allows to get some minimal information about the hit count without slowing | ||
| * down search too much. The {@link TopDocs#scoreDocs} array is always accurate however. If this | ||
| * behavior doesn't suit your needs, you should create collectorManagers manually with either {@link | ||
| * TopScoreDocCollectorManager} or {@link TopFieldCollectorManager} and call {@link #search(Query, | ||
| * CollectorManager)}. | ||
| * | ||
| * <p><a id="thread-safety"></a> | ||
| * | ||
|
|
@@ -478,34 +478,10 @@ public TopDocs searchAfter(ScoreDoc after, Query query, int numHits) throws IOEx | |
| } | ||
|
|
||
| final int cappedNumHits = Math.min(numHits, limit); | ||
| final boolean supportsConcurrency = executor != null && leafSlices.length > 1; | ||
| CollectorManager<TopScoreDocCollector, TopDocs> manager = | ||
| new TopScoreDocCollectorManager( | ||
| cappedNumHits, after, TOTAL_HITS_THRESHOLD, supportsConcurrency); | ||
|
|
||
| return search(query, manager); | ||
| } | ||
|
|
@@ -527,7 +503,10 @@ public TopDocs search(Query query, int n) throws IOException { | |
| * | ||
| * @throws TooManyClauses If a query would exceed {@link IndexSearcher#getMaxClauseCount()} | ||
| * clauses. | ||
| * @deprecated This method is being deprecated in favor of {@link IndexSearcher#search(Query, | ||
|
There was a problem hiding this comment. I think we like the Is your reasoning behind keeping this in there for now that all of our uses of this (internal to Lucene) haven't yet migrated as part of this change? Would the plan me to migrate all usages off of this internally and then actually remove it on There was a problem hiding this comment.
Do you mean
As explained in https://issues.apache.org/jira/browse/LUCENE-10002?focusedCommentId=17397827&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17397827, I use deprecation instead of direct removal in this PR exactly for the reasons you mentioned:
I would be interested in seeing how folks feel about the timing of removal, but after #1 is completed, complete removal of IndexSearcher#search(Query, Collector) in lucene codebase should require only straightforward deletion changes. There was a problem hiding this comment. I think we should try to keep There was a problem hiding this comment. +1 makes sense to keep it in 9.0. As for my comment about There was a problem hiding this comment.
Though, I think there are both |
||
| * CollectorManager)} due to its support for concurrency in IndexSearcher | ||
| */ | ||
| @Deprecated | ||
| public void search(Query query, Collector results) throws IOException { | ||
| query = rewrite(query); | ||
| search(leafContexts, createWeight(query, results.scoreMode(), 1), results); | ||
|
|
@@ -613,34 +592,10 @@ private TopFieldDocs searchAfter( | |
| final int cappedNumHits = Math.min(numHits, limit); | ||
| final Sort rewrittenSort = sort.rewrite(this); | ||
|
|
||
| final boolean supportsConcurrency = executor != null && leafSlices.length > 1; | ||
| final CollectorManager<TopFieldCollector, TopFieldDocs> manager = | ||
| new TopFieldCollectorManager( | ||
|
There was a problem hiding this comment. Wonderful to see all this redundant code go away! |
||
| rewrittenSort, cappedNumHits, after, TOTAL_HITS_THRESHOLD, supportsConcurrency); | ||
|
|
||
| TopFieldDocs topDocs = search(query, manager); | ||
| if (doDocScores) { | ||
|
|
@@ -659,9 +614,12 @@ public TopFieldDocs reduce(Collection<TopFieldCollector> collectors) throws IOEx | |
| */ | ||
| public <C extends Collector, T> T search(Query query, CollectorManager<C, T> collectorManager) | ||
| throws IOException { | ||
| if (executor == null || leafSlices.length == 0) { | ||
gsmiller marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| final C collector = collectorManager.newCollector(); | ||
| final Query rewrittenQuery = rewrite(query); | ||
|
|
||
| search(leafContexts, createWeight(rewrittenQuery, collector.scoreMode(), 1), collector); | ||
|
|
||
| return collectorManager.reduce(Collections.singletonList(collector)); | ||
| } else { | ||
| final List<C> collectors = new ArrayList<>(leafSlices.length); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if anyone out there has sub-classed this to provide their own
Collectorthrough overriding this method? It's possible right? Would we want to support this with aCollectorManagernow instead? You might consider creating a new protected method--createCollectorManager--that sub-classes could override if they want, then add javadoc here with a@lucene.deprecatedtag pointing users to the new method.Also, if this change is going onto
main, I might kill this method entirely and then add it back in as deprecated if you choose to backport to 8x.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm you are right. I reverted some changes in this class to still use the
ReadTask#createCollectormethod, in case there's overriding from users.I think this
createCollectormethod was originally added (11 years ago) to support benchmarking, and not sure how widely this is used actually? So hopefully marking it as deprecated for release 9.0 and removing it entirely once we release 10.0 should be good enough?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was more thinking along the lines of keeping the change you had but providing a protected method that would allow sub-classes to provide their own
CollectorManagerif they wanted to do so (which would give users a migration path if they were previously providing their ownCollector). So in 9.0, I think you'd move completely to theCollectorManagerapproach like you had. What do you think of that approach?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see where you are coming from. However, upon looking at this method more closely, I'm afraid this method is effectively not useful, since the result of using this collector was commented out :D :
lucene/lucene/benchmark/src/java/org/apache/lucene/benchmark/byTask/tasks/ReadTask.java
Lines 119 to 121 in 3b3f960
So I'm now leaning more towards just removing this method altogether if no users ever noticed / complaint about this. What do you think @gsmiller @jpountz ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect it would still be useful to assess the performance of a collector but you probably have a point that if performance is the only thing that the benchmark module reports, then there's a chance that it's not actually used much.
I did a quick GitHub search to see how much code I could find that extends this code:
"extends ReadTask" createCollector: no hits"extends SearchTask" createCollector -SearchWithCollectorTask: no hits (I had to excludeSearchWithCollectorTaskor all Lucene forks would be returned).Given that the benchmark module is quite expert, I'd be ok with breaking hard here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! I've removed this method, and also fixed the commented out code.