Allow easy extensions of TopDocCollector

• Key: LUCENE-1356
• Type: Improvement
• Status: Closed
• Resolution: Fixed
• Priority: Minor
• Assignee: Doron Cohen
• Reporter: Shai Erera
• Component/s: Index
• Fix Version/s: 2.3.3, 2.4

Description

TopDocCollector's members and constructor are declared either private or package visible. It makes it hard to extend it as if you want to extend it you can reuse its hq and totatlHits members, but need to define your own. It also forces you to override getTotalHits() and topDocs().
By changing its members and constructor (the one that accepts a PQ) to protected, we allow users to extend it in order to get a different view of 'top docs' (like TopFieldCollector does), but still enjoy its getTotalHits() and topDocs() method implementations.

Comments

[Shai Erera]

The very trivial patch

Shai, Thanks for creating this issue and patch!

I noticed you also modified the private reusableSD to be protected.

This field is just for avoiding creating a new object at each insert to the priority queue.
Note that TopFieldDocCollector maintains its own reusable object for this matter, and it of a 'slightly' different type.

I am wondering if the right thing to do is to (1) leave that field private, or (2) make it protected but then make TopFieldDocCollector use it too.
I'm inclined for option 1.

What do you think? Others?

[Shai Erera]

IMO, TopFieldDocCollector should be changed to use reusableSD. FieldDoc extends ScoreDoc. That's the reason I modified it to protected - for extensions of TopDocCollector who maintain in PQ ScoreDoc types (either ScoreDoc or extensions).
I don't see any advantage in marking it private, nor any disadvantage if any extension to TopDocCollector will maintain its own ScoreDoc instance.
It's just that we have TopDocCollector, TopFieldDocCollector and my extension to TDC which insert ScoreDoc instances into PQ, so it made sense to me to change it to protected.

[Shai Erera]

Re-thinking this - resuableSD should remain private to both TDC and TFDC. The reason is the two classes use it differently and don't share any implementation which involves this member (unlike totalHits and hq).

I was in the middle of adding javadoc to the protected members and constructor (which accepts numHits and PQ) when I noticed that numHits is completely ignored by this constructor --> TopDocCollector(int numHits, PriortityQueue hq).
The reason is that PQ is be probably configured to hold a maximum number of hits.

What bothers me with this constructor is that it may falsely lead users of the API to think that it limits their PQ with a maximum number of hits. I think we should remove that parameter and expose two constructors:
public TopDocCollector(int numHits) AND public TopDocCollector(PriorityQueue hq).

If you agree, I'll reflect that in the 2nd patch I want to create (which adds javadoc).

[Doron Cohen]

You're right, this is confusing indeed.
Although it is not public or protected there may users code
(residing in same package) relying on this method so it can't
just be removed but rather just deprecated.

If you agree, I'll reflect that in the 2nd patch I want to create (which adds javadoc).

Yes thanks!

[Shai Erera]

Marked the constructor as deprecated, created another one (protected) which accepts PQ only and modified TopFieldDocCollector to use the new c'tor instead of the deprecated one.
Also added javadoc.

[Michael McCandless]

Doron is this one ready to go in?

[Doron Cohen]

It is, applies cleanly and seems correct.
Will commit as soon as tests complete.

[Doron Cohen]

Thanks Shai !