Refactoring Lucene collectors (HitCollector and extensions) [LUCENE-1575]

[asfimport]

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?)

---

Migrated from LUCENE-1575 by Shai Erera (@shaie), 1 vote, resolved Apr 24 2009 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.7.patch, LUCENE-1575.8.patch, LUCENE-1575.9.patch (versions: 2), LUCENE-1575.patch (versions: 5), PerfTest.java, sortBench5.py, sortCollate5.py

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

Looks good! Thanks Shai. Some responses:

Create a final class HitCollectorWrapper, and use it in the deprecated methods in IndexSearcher, wrapping the given HitCollector.

This turns deprecated HitCollector into a Collector? Seems like it should be package private?

Change TopFieldDocCollector (deprecated) to extend TopDocsCollector, instead of TopScoreDocCollector. Implement topDocs(start, howMany)

This is deprecated, so we shouldn't add topDocs(start, howMany)? I think just switch it back to extending the deprecated TopDocCollector (like it does in 2.4)?

What if during collect() Scorer is null? (i.e., not set) - is it even possible?

I think Lucene should guarantee not to do that?

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?

Hmmmm good point. I would love to stop screening for 0 score in the core collectors (like Solr). Maybe we fix the core collectors to not screen by zero score, but we add a new "only keep positive scores" collector chain/wrapper class that does the filtering and the forwards collection to another collector? This way there's a migration path if somehow users are relying on this.

And we should note this difference clearly in the javadocs for the new hierarchy.

There might be even a 3rd patch which handles the setScorer thing in Collector (maybe even a different issue?)

I think it's fine if it's the same issue, though doing it as 2 patches is going to make life difficult. I think a single patch covering changes to src/java, and one to src/test is OK, though I'd personally prefer just one patch overall.

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

This turns deprecated HitCollector into a Collector? Seems like it should be package private?

Initially I wrote it but then deleted. I decided to make the decision as I create the patch. If this will be used only in IndexSearcher, then it should be a private static final class in IndexSearcher, otherwise a package private one. However, if it turns out we'd want to use it for now in other places too where we deprecate the HitCollector methods, then it will be public. Anyway, it will be marked deprecated, and I have the intention to make it as 'invisible' as possible.

This is deprecated, so we shouldn't add topDocs(start, howMany)? I think just switch it back to extending the deprecated TopDocCollector (like it does in 2.4)?

That's a good idea.

Hmmmm good point. I would love to stop screening for 0 score in the core collectors (like Solr). Maybe we fix the core collectors to not screen by zero score, but we add a new "only keep positive scores" collector chain/wrapper class that does the filtering and the forwards collection to another collector? This way there's a migration path if somehow users are relying on this.

I can do that. Create a FilterZeroScoresCollector which wraps a Collector and passes forward only documents with score > 0. BTW, how can a document get a zero score?

I thought to split patches to code and test since I believe the code patch can be ready sooner for review. The test patch will just fix test cases. If that matters so much, I can create a final patch in the end which contains all the changes for easier commit?

[asfimport]

Marvin Humphrey (migrated from JIRA)

> BTW, how can a document get a zero score?

Any number of ways, since Query and Scorer are extensible. How about a RandomScoreQuery that uses floor(rand(1.9))? Or say that you have a bitset of docs which should match and you use that to feed a scorer. What score should you assign? Why not 0? Why not -1? Should it matter?

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

I thought to split patches to code and test since I believe the code patch can be ready sooner for review. The test patch will just fix test cases. If that matters so much, I can create a final patch in the end which contains all the changes for easier commit?

OK that sounds great. The back-compat tests will also assert nothing broke.

Anyway, it will be marked deprecated, and I have the intention to make it as 'invisible' as possible.

OK.

BTW, how can a document get a zero score?

I've wondered the same thing. There was this thread recently:

http://www.nabble.com/TopDocCollector-td22244245.html

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

After I posted the question on how can a document get a 0 score, I realized that it's possible due to extensions of Similarity for example. Thanks Marvin for clearing that up. I guess though that the Lucene core classes will not assign <= 0 score to a document?

Anyway, whether it's true or not, I think I agree with Mike saying we should remove this screening from the core collectors. If my application extends Lucene in a way that it can assign <= 0 scores to documents, and it has the intention of screening those documents, it should use the new FilterZeroScoresCollector (maybe call it OnlyPositiveScoresCollector?)

I don't think that assigning <= 0 score to a document necessarily means it should be removed from the result set.

However, Mike (and others) - isn't there a back-compatibility issue with changing the core collectors to not screen on <=0 score documents? I mean, what if my application relies on that and extended Lucene in a way that it sometimes assigns 0 scores to documents? Now when I'll switch to 2.9, those documents won't be filtered. I will be able to use the new FilterZeroScoresCollector, but that'll require me to change my app's code.

Maybe just do it for the new collectors (TopScoreDocCollector and TopFieldCollector)? I need to change my app's code anyway if I want to use them, so as long as we document this fact in their javadocs, we should be fine?

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

However, Mike (and others) - isn't there a back-compatibility issue with changing the core collectors to not screen on <=0 score documents?

Hmm right there is, because the search methods will use the new collectors.

I need to change my app's code anyway if I want to use them, so as long as we document this fact in their javadocs, we should be fine?

Actually there's no change to your code required (the search methods should use the new collectors). So we do have a back-compat difference.

We could make the change (turn off filtering), but put a setter on IndexSearcher to have it insert the "PositiveScoresOnlyCollector" wrapper? I think the vast majority of users are not relying on <= 0 scoring docs to be filtered out.

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

We could make the change (turn off filtering), but put a setter on IndexSearcher to have it insert the "PositiveScoresOnlyCollector" wrapper?

Then why do that at all? If I need to call searcher.setKeepOnlyPositiveScores, then it means a change to my code. I could then just pass in the PositiveScoresOnlyCollector to the search methods instead, right?

I guess you are referring to the methods which don't take a collector as a parameter and instantiate a new TopScoreDocCollector internally? I tend to think that if someone uses those, it is just because they are simple, and I find it very hard to imagine that that someone relies on the filtering. So perhaps we can get away with just documenting the change in behavior?

I think the vast majority of users are not relying on <= 0 scoring docs to be filtered out.

I tend to agree. This has been around for quite some time. I checked my custom collectors, and they do the same check. I only now realize I just followed the code practice I saw in Lucene's code, never giving it much thought of whether this can actually happen. I believe that if I'd have extended Lucene in a way such that it returns <=0 scores, I'd be aware of that and probably won't use the built-in collectors. I see no reason to filter <= 0 scored docs anyway, and if I wanted that, I'd probably write my own filtering collector ...

I think that if we don't believe people rely on the <= 0 filtering, let's just document it. I'd hate to add a setter method to IndexSearcher, and a unit test, and check where else it should be added (i.e., in extending searcher classes) and introduce a new API which we might need to deprecate some day ... People who'll need that functionality can move to use the methods that accept a Collector, and pass in the PositiveScoresOnlyCollector. That way we also keep the 'fast and easy' search methods really simple, fast and easy.

Is that acceptable?

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

Then why do that at all? If I need to call searcher.setKeepOnlyPositiveScores, then it means a change to my code. I could then just pass in the PositiveScoresOnlyCollector to the search methods instead, right?

OK, I agree. Let's add an entry to the top of CHANGES.txt that states this [minor] break in back compatibility, as well as the code fragment showing how to use that filter to get back to the pre-2.9 way?

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

Great !

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

BooleanScorer defines an internal package private static final Collector class. Two questions:

1. May I change it to BooleanCollector? (the name conflicts with the Collector name we want to give to all base collectors)
2. May I change it to private static final? It is used only in BooleanScorer's newCollector() method. I think the two are safe because it's already package-private and there's no other Lucene code which uses it.

BTW, we might wanna review BooleanScorer's internal classes visibility. They are all package-private, with some public methods, however used by BooleanScorer only ... But that's something for a different issue.

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

May I change it to BooleanCollector? (the name conflicts with the Collector name we want to give to all base collectors)

May I change it to private static final? It is used only in BooleanScorer's newCollector() method.

I think these are fine.

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

I think as part of this we should allow TopFieldCollector to NOT get the score of each hit? EG another boolean to the ctor?

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

I am not sure what you mean - score is used all over the place in collect() as well as other methods. updateBottom for example takes a score, updates bottom.score and then calls adjustTop(). Do you mean that if ignoreScore is true (in ctor), then setScorer should not save the Scorer and not call scorer.score()? If so, what should I do with all the methods that accept score? Create another code path in TopFieldCollector which ignore the score?

Also, what should the default value be? true (for ignoring scores)?

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

Ok I now understand better where score is used in TopFieldCollector ... It is used in a number of places, two important are:

1. Maintain maxScore for returning in TopFieldDocs.
2. Passed to FieldComparator.compareBottom which takes a doc and score parameters. There is one comparator RelevanceComparator which makes use of the score passed, however that's part of the method signature.
3. Passed to FieldComparator.copy - again used by RelevanceComparator only.
4. Passed to updateBottom, which updates the score of the least element in the queue and then calls adjustTop().

• Number 2, 3 and 4 can be resolved by adding a setScorer to FieldComparator (as empty implementation) which TopFieldCollector will call in each collect() call, passing the Scorer that was given to it in its setScorer.
• Then, we override that in RelevanceComparator, saving the Scorer and using it whenever the score is needed. Of course we'll need to save the current score, so that we don't call score() too many times for the same document.
• This eliminates the need to define on TopFieldCollector whether scores should be saved. The reason is that the Sort parameter may include a SortField.SCORE field, which will invoke the RelevanceComparator.

The question is what to do with maxScore? It is needed for TopDocs / TopFieldDocs. It may also be important to know the maxScore of a query, even if you sort it by something which is not a score.

Question is - if the steps above make sense, why should we do them at all? :) Now the score is computed and passed on to every FieldComparator we received in Sort. Cleaning the method signature means additional code overhead in RelevanceComparator. If we want to compute maxScore as well, it means the score will be computed twice, once in collect() and once in RelevanceComparator.

We can solve the double score() computation by using an internal ScoreCacheScorer which keeps the score of the current document and returns it whenever score() is called, unless it's a new document and then it delegates the call to the wrapped Scorer. TopFieldCollector can instantiate it in setScorer.

But this looks quite a lot for cleaning a method signature, don't you think? Of course if you can suggest how we somehow remove the maxScore computation, then it might be a good change, since only if SortField.SCORE is used, will the score be computed.

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

The question is what to do with maxScore? It is needed for TopDocs / TopFieldDocs.

This is why I was thinking you'd have to tell TopFieldCollector whether or not it should track scores. Furthermore, even if SortField.SCORE is not in your SortFields, an app may still want the scores to be enrolled in the TopFieldDocs, for presentation.

Turning off scoring in TopFieldCollector's ctor just means 1) TopFieldCollector won't track max score, and 2) TopFieldCollector will leave score at 0 in the returned ScoreDoc array.

Number 2, 3 and 4 can be resolved by adding a setScorer to FieldComparator (as empty implementation) which TopFieldCollector will call in each collect() call, passing the Scorer that was given to it in its setScorer.

+1

It makes sense to push the same improvement (not always passing a score; instead, you ask the scorer for score if you need it) down into the FieldCollector API.

We can solve the double score() computation by using an internal ScoreCacheScorer which keeps the score of the current document and returns it whenever score() is called, unless it's a new document and then it delegates the call to the wrapped Scorer. TopFieldCollector can instantiate it in setScorer.

+1

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

Turning off scoring in TopFieldCollector's ctor just means 1) TopFieldCollector won't track max score, and 2) TopFieldCollector will leave score at 0 in the returned ScoreDoc array.

Just to be clear - TopDocs as well as TopFieldDocs require a maxScore parameter in their ctor. So are you suggesting to pass something like Float.NaN as maxScore if scoring is turned off? Or introducing a new ctor which does not require maxScore, and defaults to Float.NaN? (or both?)

Furthermore, even if SortField.SCORE is not in your SortFields, an app may still want the scores to be enrolled in the TopFieldDocs, for presentation.

Right - we should separate between getting score out of the FieldComparator API and tracking scores in TFC. If I don't have SortField.SCORE in my list of sort fields, then scorer.score() will not be called at all from the FieldComparators layer.

Tracking scores in TFC is what I'm having troubles with. Turning it off does not necessarily improve anything .. Might be, and might not. In setScorer() I'd still need to register Scorer for passing on to FieldComparator. In collect() I'd still need to check whether score tracking is on, and if so, call scorer.score() and track maxScore. Note that if ScoreCacheScorer is used, then calling scorer.score() in collect does not have too much overhead.

Also, what will be the default from a Lucene perspective? true - i.e., always keep track of scores?

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

Or introducing a new ctor which does not require maxScore, and defaults to Float.NaN?

I think that's a good approach, though for TopDocs the ctor should be package private I think (only called from TopFieldDocs' ctor)? And the javadocs should clearly spell out that this could happen (so people don't get scared on seeing Float.NaN coming back).

Turning it off does not necessarily improve anything .. Might be, and might not. In setScorer() I'd still need to register Scorer for passing on to FieldComparator. In collect() I'd still need to check whether score tracking is on, and if so, call scorer.score() and track maxScore. Note that if ScoreCacheScorer is used, then calling scorer.score() in collect does not have too much overhead.

I think this is an improvement? (Scorer.score() will not have been called... that's the goal here).

I guess we could also consider making a separate TopFieldCollector (NonScoringTopFieldCollector or some such), instead of sprinkling if statements all over the place.

Also, what will be the default from a Lucene perspective? true - i.e., always keep track of scores?

Good question... we have the freedom to choose. Perhaps default to off? But say clearly in the migration javadocs that you have to set that to true to get same behavior as TSDC?

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

ok I'll add another package-private ctor to TopDocs which does not get maxScore and defaults to NaN, as well as update the javadocs. No back-compat here since the only code that will use it is TFC, which is new.

I think this is an improvement? (Scorer.score() will not have been called... that's the goal here).

Well, if we use ScoreCacheScorer, then this call is really fast, returning immediately and w/o computing the score.

I guess we could also consider making a separate TopFieldCollector (NonScoringTopFieldCollector or some such), instead of sprinkling if statements all over the place.

I always like such approaches. How's that sound:

1. Create a base NSTFC, which has a protected score member, initialized to 0, and is exactly like the current TFC, only w/o maxScore.
2. Have TFC extend NSTFC, override collect() and:
   1. Set super.score = scorer.score(). That is required for updateBottom which updates the score on the ScoreDoc in pq.
   2. Compute maxScore.
   3. Call super.collect().
   4. Override topDocs(start, howMany) to provide one with maxScore.

Good question... we have the freedom to choose. Perhaps default to off? But say clearly in the migration javadocs that you have to set that to true to get same behavior as TSDC?

So you suggest the methods on IndexSearcher today that take a Sort as parameter will default to NSTFC? As long as we document it it's ok? Are all of these new?

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

How's that sound:

That sounds good! So to be consistent maybe we create ScoringTopFieldCollector and NonScoringTopFieldCollector?

This means we don't need ScoreCacheScorer? (because ScoringTopFieldCollector will always grab the score). Though how do we change FieldComparator API so as to not pass score around? All comparators except RelevanceComparator don't use it.

Well, if we use ScoreCacheScorer, then this call is really fast, returning immediately and w/o computing the score.

I'm actually torn on how fast this will be: I think that will be an if statement that's hard for the CPU to predict, which is costly.

So you suggest the methods on IndexSearcher today that take a Sort as parameter will default to NSTFC? As long as we document it it's ok? Are all of these new?

Hmmm... actually, no, I think those must continue to use NSTFC for the existing methods (to remain back compatible), but add a new search method that takes a boolean trackScore?

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

So to be consistent maybe we create ScoringTopFieldCollector and NonScoringTopFieldCollector?

And have STFC extend NSTFC? I see no reason to create an abstract TopFieldCollector.

This means we don't need ScoreCacheScorer? (because ScoringTopFieldCollector will always grab the score). Though how do we change FieldComparator API so as to not pass score around? All comparators except RelevanceComparator don't use it.

I was actually thinking of that class for RelevanceComparator. So perhaps I can implement the logic inside RelevanceComparator? Although this sounds like a nice utility class, now that we have a setScorer on Collector - others may find it useful too. Remember that score-tracking is done for maxScore and ScoreDoc purposes (inside STFC). The score in the FieldComparator API is used only in RelevanceComparator, whether it's STFC or NSTFC.

I think those must continue to use NSTFC for the existing methods (to remain back compatible)

Did you mean continue to use STFC? The current behavior is that scoring is tracked, I think.

add a new search method that takes a boolean trackScore?

I actually prefer not to expose any more methods. IndexSearcher already has plenty of them. Instead, one can use the very generic, simple and useful method search(Query, Collector) and pass in a NSTFC instance. Otherwise we'll end up adding many search() methods to IndexSearcher, if we continue with that approach going forward.

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

And have STFC extend NSTFC? I see no reason to create an abstract TopFieldCollector.

Yes.

Although this sounds like a nice utility class, now that we have a setScorer on Collector - others may find it useful too.

OK I agree, it would be useful to have for general usage (eg chaining collectors).

But what is the plan now for the FieldComparator API? We no longer pass score all around, but expose access to scorer, which only RelevanceComparator (in core) will use?

Did you mean continue to use STFC? The current behavior is that scoring is tracked, I think.

Sorry, yes, STFC. Beginning to lose mind...

I actually prefer not to expose any more methods. IndexSearcher already has plenty of them. Instead, one can use the very generic, simple and useful method search(Query, Collector) and pass in a NSTFC instance. Otherwise we'll end up adding many search() methods to IndexSearcher, if we continue with that approach going forward.

I think that'd be OK... the only thing that bothers me is I think the natural default when sorting by field is to not gather the score. Ie I don't want someone evaluating Lucene in the future to say our field sort is too slow when they didn't realize they had to go use this advanced API that turns off scoring.

What if we add a new method, and deprecate the old one? This way come 3.0 we will not have added any methods, and then when sorting by field you see that you have to choose with or without scores.

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

But what is the plan now for the FieldComparator API? We no longer pass score all around, but expose access to scorer, which only RelevanceComparator (in core) will use?

Yes. FieldComparator will have a default empty setScorer() method, which will be overridden by RelevanceComparator. In TopFieldCollector (forget the final name now) setScorer() method we set the Scorer on all FieldComparators. During collect(), only RelevanceComparator, if it exists, will compute the score.

What if we add a new method, and deprecate the old one?

The current methods are:

• Searchable - search(Weight, Filter, int, Sort)
• Searcher - search(Query, Filter, int, Sort)
• IndexSearcher - search(Weight, Filter, int, Sort, boolean /* fillFields */)

Deprecate all three, and add the same but take another boolean as a parameter? I have two comments regarding that:

1. The current methods need to call the new ones with trackScores = true since that's the current behavior.
2. When we are left with only the new versions, I'm afraid those methods will not look 'simple fast' to a user - I now have to decide whether I want to track scores or not, something I haven't given much thought to before. I kind of like the current signature, but I understand your concern regarding defaults.

BTW, Searchable is an interface, so we cannot add it there. Searcher is an abstract class and we cannot add the method to it with default implementation (as I believe the other search methods will call the new one with default=true). So it only leaves IndexSearcher as an option. But then what if someone uses MultiSearcher? ParallelMultiSearcher? etc.

Is it possible to deprecate a method, documenting that its runtime behavior will change in 3.0 and then in 3.0 change to not track scores?

If we're touching TopFieldCollector in such a way, I'd like to propose the following refactoring. It stems from the current complex implementation in collect() which checks in every collect call if we have just one Comparator or Multi, and we're talking about having two versions w/ and w/o score tracking:

• Keep TopFieldCollector as abstract class with a static create() factory method, and an abstract updateBottom() method. It will still implement topDocs(start, howMany).
• Have the factory method create one of 4 instances, all extend TFC, but are private internal classes, which do not concern the user:
  1. OneComparatorNonScoringTopFieldCollector - assumes just one FieldComparator exists as well as scoring should not be tracked.
  2. OneComparatorScoringTopFieldCollector - assumes just one FieldComparator exists as well as scoring should be tracked.
  3. MultiComparatorNonScoringTopFieldCollector - assumes more than one FieldComparator exists, as well as scoring should not be tracked.
  4. MultiComparatorScoringTopFieldCollector - assumes more than one FieldComparator exists, as well as scoring should be tracked.

The advantages are:

• We simplify the API - the user is only aware of TopFieldCollector, and instead of calling a ctor it calls a static create method, which takes all the arguments as the ctor takes.
• We are free to create whatever instance is the right and most optimized one given the input parameters. The user does not care how the instance is called. Hence the long names - they are internal anyway.
• The code is much cleaner and easy to understand. It also does not need to check if we have just one comparator or more in every call to collect.
• We don't need to add a protected score to a NonScoring collector (read above about code readability) just because a Scoring one extends it and will make use of it.

Since TopFieldCollector is new, we have the freedom to do it right w/o deprecating anything. I think it's a much cleaner design. It is orthogonal to the discussion we're having regarding the search methods and parameters. They will use the create() factory method instead of creating a collector, passing whatever arguments they have. So let's not confuse the two.

The patch for this issue is ready. As soon as we agree on how to proceed with TFC, I'll add the changes and submit the patch.

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

If we're touching TopFieldCollector in such a way, I'd like to propose the following refactoring

+1. That looks great.

When we are left with only the new versions, I'm afraid those methods will not look 'simple fast' to a user

I agree, which is why I'd like in 3.0 for the default to be "don't score when sorting by fields".

Is it possible to deprecate a method, documenting that its runtime behavior will change in 3.0 and then in 3.0 change to not track scores?

I think this may in fact be our best option: don't deprecate the method, but document that in 3.0 this method will no longer do scoring. There is a precedent here: in 3.0, IndexReader.open is going to return readOnly readers by default (vs read/write today). We have also done similar fixes within a minor release, eg fixes to StandardAnalyzer. I think there are other things we should do (eg, StopFilter should enable position increment by default, which it doesn't today – LUCENE-1258). If we do this approach, on committing this issue you should open a new one w/ fix version 3.0 to switch up the default.

I think, with 3.0, if we clearly document in CHANGES, as well as on the particular APIs, the changes to Lucene's defaults, that's sufficient?

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

If we do this approach, on committing this issue you should open a new one w/ fix version 3.0 to switch up the default.

Ok then let's do that. I'll add a TODO to these methods that it should be changed in 3.0, and also open another issue. (The TODO is in case I forget to open the issue).

I'll also add documentation to the CHANGES file as well as the API.

Ok, I think the patch should be ready soon then. Just need to complete the refactoring to TopFieldCollector.

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

When I was about to make the changes to FieldComparator (add setScorer and calling scorer.score() when necessary) I noticed that scorer.score() declares it throws IOException, while the FieldComparatro methods don't. So two ways to handle it:

1. catch the IOException and ignore it, assuming a 0 score. This is if we think no Scorer will actually throw an IOException.
2. Change FieldComparator APIs to declare throwing IOE, which will give us flexibility in the future. Since the Lucene code throws IOE in many places, I don't think it's a problem.

I'm in favor of (2) (I had to add IOE to Collector.collect() for that reason).

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

I like 2 as well. I find reserving that freedom to be very helpful, while preventing it to be a real hassle later on...

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

Eventually I decided to include just one patch file (instead of code and test) since it was simpler after all. Please be sure to review the following:

1. Collector class and documentation.
2. New TopDocsCollector class.
3. TopFieldCollector refactoring.
4. Methods deprecation.
5. New TestTopDocsCollector as well as test cases in TestSort.

[asfimport]

Uwe Schindler (@uschindler) (migrated from JIRA)

I just wonder, why HitCollectorWrapper implements:

public void collect(int doc, float score) {
    collector.collect(doc + base, score);
}

This is not needed by Collector abstract class and never called.

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

oops :) leftovers from when it extended MultiReaderHitCollector (now called Collector)

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

Shai, it looks like you "svn copy"'d MultiReaderHitCollector.java --> Collector.java? It causes "patch" to be confused when applying the patch. The simple workaround is to pre-copy that file yourself, manually, before appying the patch.

[asfimport]

Uwe Schindler (@uschindler) (migrated from JIRA)

oops leftovers from when it extended MultiReaderHitCollector (now called Collector)

This is why we really should move to Java 1.5 soon and its @Override annotation...

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

I did not do any "svn copy", just used Eclipse refactoring to change the name of the class to Collector. I did not understand though from your comment if I should do it differently and post another patch, or is that a hint to how someone can still apply the patch?

[asfimport]

Uwe Schindler (@uschindler) (migrated from JIRA)

JavaDoc errors:

• The Collector javadoc example still contains the score in its collect() method.
• The javadocs of ParallelMultiSearcher's new Collector's

  public void search(Weight weight, Filter filter, final Collector collector)

  has still HitCollector in its JavaDocs.

This is what I found out when reading the new generated Javadocs.

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

Thanks Mike. I ran the javadocs task and found other mentions of MultiReaderHitCollector as well as fixed some more javadocs. BTW, the javadoc Ant task outputs many errors on missing files/names, but that something for another issue.

[asfimport]

Andrzej Bialecki (@sigram) (migrated from JIRA)

I'm late to this discussion, so I may have missed something. Is there any provision in the new API for the early termination of hit collection? Currently the (time | count)-limited collectors that are used in Nutch and Solr have to throw RuntimeException to break the loop. It would be much more elegant if the new Collector.collect() had a way to signal the caller that it should stop looping without incurring the cost of throwing an Exception. E.g. by returning a boolean, or setting a flag in the caller.

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

Thanks Mike.

That was Uwe ;)

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

Looks great! Thanks Shai.

My biggest question/issue is how TopDocsCollector now (still?) requires that the PQ you give it is sorting primarily by score (eg getMaxScore() assumes the max is in the PQ; topDocs() uses results[0]'s score when start == 0). We've sort of come full circle (a "hidden assumption" that you are sorting by score was what started the whole thread in the beginning) ;)

We get away with that with TopFieldCollector because that overrides all the score-related processing.

I'm not sure how to cleanly fix this... maybe TopDocsCollector should make maxScore() abstract? Or... it's as if we need the scoring/non-scoring bifurcation up higher (moved out of TopFieldCollector to above TopDocsCollector)?

EG say I provide my own PQ that's sorting by date (by loading date from some external source, say); I may or may not care for score.

Maybe we make a ScoringWrapperCollector that grabs score in its own collect, then calls collect() on its child?

A few other small things:

• In the private final static classes inside TopFieldCollector, you can make the members final too (eg reverseMul, comparator), in case it helps compiler.

• Can you add a paragraph @ top of CHANGES stating the pending default swap in 3.0 (ie the same note you added to IndexSearcher.search). Add a new "Changes in backwards compatibility policy" section at the very top (look at how 2.4.0 release did it). And can you give explicit code fragment showing how to get back to the old way (ie show that you must pass in "true" for trackDocScores).

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

It would be much more elegant if the new Collector.collect() had a way to signal the caller that it should stop looping without incurring the cost of throwing an Exception. E.g. by returning a boolean, or setting a flag in the caller.

I agree: we should work into this new collection API a way to stop early.

But I'm nervous about the cost of checking a returned boolean on every collect call vs the cost of throwing/catching an exception. Adding the boolean check slows down every single collect() call (by just a bit, but bits really count here), even those that never use stop early (the majority of apps today). Throwing an exception adds a clear cost when you actually throw & catch it, but presumably that cost is proportionally tiny because you only throw it on searches that have been running for a long time, anyway.

We could upgrade the exception to a checked exception; then we'd need to add "throws XXX" to the search methods in 3.0 (perhaps wrapping as RuntimeException until 3.0). But then I also wonder if the checked exception logic would add instructions in the collect() path.

[asfimport]

Uwe Schindler (@uschindler) (migrated from JIRA)

I think, a checked Exception to stop collecting would be the best. The "cost" of the exception is very minimal (it is only thrown once in the collector and catched somewhere at top level). So where would be the costly part? Microseconds per search?

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

Sorry – thanks Uwe :)

On Wed, Apr 1, 2009 at 1:01 PM, Michael McCandless (JIRA)

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

I think, a checked Exception to stop collecting would be the best.

I actually think a RuntimeException is better for the following reasons:

• The effects of a checked exception added to collect() will not touch IndexSearcher only, but also some Scorer implementations.
• That only exists in the TimeLimitedCollector, which is not instantiated by Lucene code, but rather by my application code. Therefore, reading its javadocs, I know I should wrap the searcher.search with a try-catch. On the other hand, the rest of the Lucene users will not need to do that since they don't use it.
• Going forward, someone may come up with another limiting Collector implementation, for example "I want to stop collecting and searching as soon as I hit 10 documents" - what would we do then - add another checked exception?

My point is - since such Collectors (at least now) are now instantiated by the search application and not by the Lucene code itself, RuntimeException is as good as checked exception, only they don't require any changes to the methods signature.

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

Maybe we make a ScoringWrapperCollector that grabs score in its own collect, then calls collect() on its child?

How about this:

1. I remove maxScore() completely from TopDocsCollector.
2. I define newTopDocs to return a TopDocs() w/o a maxScore (like we said, defaulting to Float.NaN). Also change the method signature to receive the start as well as ScoreDoc[] that was collected so far.
3. TopScoreDocCollector will override it, fetching maxScore and returning a TopDocs.
   • TopFieldCollector will do the same, but returning TopFieldDocs.

Can you add a paragraph @ top of CHANGES stating the pending default swap in 3.0

Done.

In the private final static classes inside TopFieldCollector, you can make the members final too

Done.

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

Includes the latest comments from Mike.

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

How about this:

OK that looks good!

Though we now don't have a single shared ancestor class that can give you a maxScore() when sorting by score or when sorting by fields. Not sure how big a loss that is though...

More comments:

• Should we implement a default TopDocsCollector.collect/setScorer/setNextReader? Ie, TopDocsCollector is supposed to be the "you provide the PQ and we do the rest" collector, most closely matching TopDocCollector. Or.... maybe we don't, because we don't know if we should compute the score for you, you may want to put something other than ScoreDoc into the queue, etc. Who knows what your class considers "top".

• What happened to OnlyPositiveScoresFilter? (I don't see it).

• It's a bit of a trap on calling either topDocs method in TopDocsCollector (or its subclasses) that it has popped everything from your PQ as a side-effect. This is technically a pre-existing issue, but the new bracketed version makes the trap more "trappy". For example, I can't call it N times once for each page – I have to re-run the search to get another page's worth of results. Can you update javadocs eg something like "NOTE: you cannot call this method more than once" .

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

I did not understand though from your comment if I should do it differently and post another patch, or is that a hint to how someone can still apply the patch?

The patch command gets confused because it sees set of diffs against what looks to be a pre-existing Collector.java, but of course I have no Collector.java locally. "svn diff" did this because it knows you had renamed MRHC --> C.

I think for now it's fine if those of us that need to apply the patch simply first copy MultiReaderHitCollector.java --> Collector.java.

This is yet another example of how "patch" needs to be better integrated with svn: there needs to be a mirror "svn patch" to "svn diff" that's able to properly carry over all svn changes, perhaps do a 3way merge, etc.

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

Though we now don't have a single shared ancestor class that can give you a maxScore() when sorting by score or when sorting by fields

I don't think it's a big issue. We have just two extensions to TopDocsCollector, each tracking maxScore differently. Perhaps later on if more extensions are created, or a demand comes from users, we add a ScoringTopDocsCollector class?

Should we implement a default TopDocsCollector.collect/setScorer/setNextReader?

I actually think that TopDocsCollector gives you exactly what I had in mind. When I started it, there was only collect(doc, score) method, and I wanted to have a base class that will implement getTotalHits and topDocs for you, while you only need to provide an implementation to collect(). Now collect has really become setNextReader, setScorer and collect. Meaning, you know what you want to do, TDC just takes care of creating the TopDocs for you, and you can even override topDocs(start, howMany) if you want to return a different TopDocs instance.

I think now it's clean and simple.

What happened to OnlyPositiveScoresFilter? (I don't see it).

Well ... you don't see it because I forgot to implement it :). Funny thing - all the tests pass without it, meaning all that time, we were filtering <= 0 scored documents, but never really tested it ... I guess if it's because we never really believed it'll happen?

Anyway, I'll add such a class and a suitable test class (make sure it fails w/o using that wrapper first).

Can you update javadocs eg something like "NOTE: you cannot call this method more than once" .

That has always been the case. Previously if you called topDocs() everything has been popped out of the queue for you. I understand what you say though, since we have the topDocs(start, howMany) API, nothing prevents a user from calling topDocs(0, 10) and topDocs(10, 10), only the second call will fail. However, I don't really think that's how people will use it ... and if they do, then perhaps they should just call topDocs() and do whatever they need on these ranges using the TopDocs object? I'll add that to the documentation as well.

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

Adds:

• PositiveScoresOnlyCollector and TestPositiveScoresOnlyCollector.
• Relevant comments in CHANGES
• Comments to TopDocsCollector.topDocs(start, howMany) and topDocs(start)

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

Funny thing - all the tests pass without it, meaning all that time, we were filtering <= 0 scored documents, but never really tested it

Hmmm!

I actually think that TopDocsCollector gives you exactly what I had in mind.

OK let's keep the current approach.

Perhaps later on if more extensions are created, or a demand comes from users, we add a ScoringTopDocsCollector class?

OK.

I think patch looks good – I don't have any more comments now.

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

I think patch looks good - I don't have any more comments now.

Great !

Will you be committing it (perhaps after we give some more time for people to review it)?

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

Will you be committing it (perhaps after we give some more time for people to review it)?

Yes... but let's let it age some so others can review. Plus, I find coming back and looking at something again after just a couple of days results in a fresh perspective.