cbuescher commented on a change in pull request #913: LUCENE-8995:
TopSuggestDocsCollector#collect should be able to signal rejection
URL: https://github.com/apache/lucene-solr/pull/913#discussion_r335389135
##########
File path: lucene/core/src/java/org/apache/lucene/util/fst/Util.java
##########
@@ -460,11 +460,6 @@ public void addStartPaths(FST.Arc<T> node, T startOutput,
boolean allowEmptyStri
continue;
}
- if (results.size() == topN-1 && maxQueueDepth == topN) {
- // Last path -- don't bother w/ queue anymore:
- queue = null;
Review comment:
As far as I understand this optimization assumes we surely accept (and
collect) the path later in L516s acceptResult(), which always seems to be the
case for collectors that don't reject, but if the collector that is eventually
called via NRTSuggesters acceptResult() chooses to reject this option, we were
losing expected results. This surfaced in the prefix completion tests I added.
@jimczi might be able to explain this a bit better than me.
> Have you run the suggest benchmarks to see if removing this opto hurt
performance?
No, where are they and how can I run them?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
