Looks like quite a bug, Shai! Thanks. It came in with LUCENE-1483.
I would say add test case & fix it under 1575.
Mike
On Mon, Mar 30, 2009 at 3:50 AM, Shai Erera <ser...@gmail.com> wrote:
> Hi
>
> As I prepared the patch for 1575, I noticed a strange implementation in
> TopFieldCollector's topDocs():
>
> ScoreDoc[] scoreDocs = new ScoreDoc[queue.size()];
> if (fillFields) {
> for (int i = queue.size() - 1; i >= 0; i--) {
> scoreDocs[i] = queue.fillFields((FieldValueHitQueue.Entry)
> queue.pop());
> }
> } else {
> Entry entry = (FieldValueHitQueue.Entry) queue.pop();
> for (int i = queue.size() - 1; i >= 0; i--) {
> scoreDocs[i] = new FieldDoc(entry.docID,
> entry.score);
> }
> }
>
> return new TopFieldDocs(totalHits, scoreDocs, queue.getFields(),
> maxScore);
>
>
> Notice that if fillFields is true, then documents are popped from the queue.
> However if it's false, then the first document is popped out of the queue
> and used to populate the entire ScoreDoc[]? I believe that's wrong, right?
> Otherwise, the returned TopFieldDocs.scoreDocs array will include the same
> document and score?
>
> I noticed there's no test case for that ... TopFieldCollector's constructor
> is called only from IndexSearcher.search(Weight, Filter, int, Sort, boolean
> /* fillFields */) which is called from IndexSearcher.search(Weight, Filter,
> int, sort) with fillFields always set to true. So perhaps that's why this
> hasn't showed up?
>
> BTW, the now deprecated TopFieldDocCollector's topDocs() implementation
> looks like this:
>
> FieldSortedHitQueue fshq = (FieldSortedHitQueue)hq;
> ScoreDoc[] scoreDocs = new ScoreDoc[fshq.size()];
> for (int i = fshq.size()-1; i >= 0; i--) // put docs in array
> scoreDocs[i] = fshq.fillFields ((FieldDoc) fshq.pop());
>
> return new TopFieldDocs(totalHits, scoreDocs,
> fshq.getFields(), fshq.getMaxScore());
>
> It assumes fillFields is always true and always pops elements out of the
> queue.
>
> If this is a bug, I can fix it as part of 1575, as I'm touching that class
> anyway. I can also add a test case ... The fix is very simple BTW, just move
> the line "Entry entry = (FieldValueHitQueue.Entry) queue.pop();" inside the
> for loop.
>
> Shai
>
---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: java-dev-h...@lucene.apache.org
