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 <[email protected]> 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: [email protected] For additional commands, e-mail: [email protected]
