[
https://issues.apache.org/jira/browse/LUCENE-1575?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12695513#action_12695513
]
Michael McCandless commented on LUCENE-1575:
--------------------------------------------
bq. I like TimeLimitingCollector better, as I think the name makes the class
more self explanatory.
OK let's go with that!
{quote}
At the end of the day, even the non-scoring collectors store a score in
ScoreDoc, which is Float.NaN. So they should pass a score. Unlike the scoring
ones, they always pass Float.NaN without ever calling scorer.score(). That's
the cleanest way I've found I can make the changes to that class, w/o
duplicating implementation all over the place. Notice that the scoring versions
extend the non-scoring, and just add score computation, which resulted in a
very clean implementation.
{quote}
OK... let's stick with this approach for now. Since the impl is
locked down (ctor for TopFieldCollector is private) we can freely
switch up this API in the future without breaking back compat, if we
want to optimize not passing/copying around the unused score.
Can't the scoring collector impls in TopFieldCollector be final?
bq. Since we pass Sorer to Collector, I thought we cannot really rely on anyone
not calling scorer.doc() or getSimilarity ever
Maybe instead make InternalScorer non-static, and then doc() can
return the doc from the TermScorer instance, instead of having to copy
"s.d = doc" each time? score can do a similar thing.
Actually, hang on: if I'm using a Collector that doesn't need the
score, TermScoring is still computing it? We don't want that right?
Can we simply pass "this" to setScorer(...)?
bq. If we add to Searcher a concrete impl which throws UOE, how would that work
in 3.0? How would anyone who extends Searcher know that it has to extend this
method? Maybe do it now, and document that in 3.0 it will become abstract again?
OK let's do that?
bq. About Searchable, I wonder how many do implement Searchable, rather than
extend IndexSearcher. Perhaps instead of making any changes in back-compat and
add documentation to CHANGES I'll just comment out this method with a TODO to
re-enstate in 3.0?
OK.
Make sure at the end of all of this, you open a new issue, marked as
fix version 3.0, that has all the "and then on 3.0 we do XYZ"s from
this.
> Refactoring Lucene collectors (HitCollector and extensions)
> -----------------------------------------------------------
>
> Key: LUCENE-1575
> URL: https://issues.apache.org/jira/browse/LUCENE-1575
> Project: Lucene - Java
> Issue Type: Improvement
> Components: Search
> Reporter: Shai Erera
> Assignee: Michael McCandless
> Fix For: 2.9
>
> Attachments: LUCENE-1575.1.patch, LUCENE-1575.2.patch,
> LUCENE-1575.3.patch, LUCENE-1575.4.patch, LUCENE-1575.5.patch,
> LUCENE-1575.6.patch, LUCENE-1575.patch, LUCENE-1575.patch
>
>
> This issue is a result of a recent discussion we've had on the mailing list.
> You can read the thread
> [here|http://www.nabble.com/Is-TopDocCollector%27s-collect()-implementation-correct--td22557419.html].
> We have agreed to do the following refactoring:
> * Rename MultiReaderHitCollector to Collector, with the purpose that it will
> be the base class for all Collector implementations.
> * Deprecate HitCollector in favor of the new Collector.
> * Introduce new methods in IndexSearcher that accept Collector, and deprecate
> those that accept HitCollector.
> ** Create a final class HitCollectorWrapper, and use it in the deprecated
> methods in IndexSearcher, wrapping the given HitCollector.
> ** HitCollectorWrapper will be marked deprecated, so we can remove it in 3.0,
> when we remove HitCollector.
> ** It will remove any instanceof checks that currently exist in IndexSearcher
> code.
> * Create a new (abstract) TopDocsCollector, which will:
> ** Leave collect and setNextReader unimplemented.
> ** Introduce protected members PriorityQueue and totalHits.
> ** Introduce a single protected constructor which accepts a PriorityQueue.
> ** Implement topDocs() and getTotalHits() using the PQ and totalHits members.
> These can be used as-are by extending classes, as well as be overridden.
> ** Introduce a new topDocs(start, howMany) method which will be used a
> convenience method when implementing a search application which allows paging
> through search results. It will also attempt to improve the memory
> allocation, by allocating a ScoreDoc[] of the requested size only.
> * Change TopScoreDocCollector to extend TopDocsCollector, use the topDocs()
> and getTotalHits() implementations as they are from TopDocsCollector. The
> class will also be made final.
> * Change TopFieldCollector to extend TopDocsCollector, and make the class
> final. Also implement topDocs(start, howMany).
> * Change TopFieldDocCollector (deprecated) to extend TopDocsCollector,
> instead of TopScoreDocCollector. Implement topDocs(start, howMany)
> * Review other places where HitCollector is used, such as in Scorer,
> deprecate those places and use Collector instead.
> Additionally, the following proposal was made w.r.t. decoupling score from
> collect():
> * Change collect to accecpt only a doc Id (unbased).
> * Introduce a setScorer(Scorer) method.
> * If during collect the implementation needs the score, it can call
> scorer.score().
> If we do this, then we need to review all places in the code where
> collect(doc, score) is called, and assert whether Scorer can be passed. Also
> this raises few questions:
> * What if during collect() Scorer is null? (i.e., not set) - is it even
> possible?
> * I noticed that many (if not all) of the collect() implementations discard
> the document if its score is not greater than 0. Doesn't it mean that score
> is needed in collect() always?
> Open issues:
> * The name for Collector
> * TopDocsCollector was mentioned on the thread as TopResultsCollector, but
> that was when we thought to call Colletor ResultsColletor. Since we decided
> (so far) on Collector, I think TopDocsCollector makes sense, because of its
> TopDocs output.
> * Decoupling score from collect().
> I will post a patch a bit later, as this is expected to be a very large
> patch. I will split it into 2: (1) code patch (2) test cases (moving to use
> Collector instead of HitCollector, as well as testing the new topDocs(start,
> howMany) method.
> There might be even a 3rd patch which handles the setScorer thing in
> Collector (maybe even a different issue?)
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: java-dev-h...@lucene.apache.org
