Return-Path: Delivered-To: apmail-lucene-java-dev-archive@www.apache.org Received: (qmail 697 invoked from network); 30 Mar 2009 08:40:06 -0000 Received: from hermes.apache.org (HELO mail.apache.org) (140.211.11.3) by minotaur.apache.org with SMTP; 30 Mar 2009 08:40:06 -0000 Received: (qmail 79002 invoked by uid 500); 30 Mar 2009 08:40:03 -0000 Delivered-To: apmail-lucene-java-dev-archive@lucene.apache.org Received: (qmail 78917 invoked by uid 500); 30 Mar 2009 08:40:02 -0000 Mailing-List: contact java-dev-help@lucene.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: java-dev@lucene.apache.org Delivered-To: mailing list java-dev@lucene.apache.org Received: (qmail 78701 invoked by uid 99); 30 Mar 2009 08:40:02 -0000 Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 30 Mar 2009 08:40:02 +0000 X-ASF-Spam-Status: No, hits=3.4 required=10.0 tests=HTML_MESSAGE,SPF_NEUTRAL X-Spam-Check-By: apache.org Received-SPF: neutral (athena.apache.org: local policy) Received: from [80.190.230.99] (HELO mail.troja.net) (80.190.230.99) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 30 Mar 2009 08:39:52 +0000 Received: from localhost (localhost [127.0.0.1]) by mail.troja.net (Postfix) with ESMTP id 5BD6450EC4 for ; Mon, 30 Mar 2009 10:39:29 +0200 (CEST) Received: from mail.troja.net ([127.0.0.1]) by localhost (cyca.troja.net [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 27700-09 for ; Mon, 30 Mar 2009 10:39:26 +0200 (CEST) Received: from VEGA (unknown [134.102.249.74]) (using SSLv3 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mail.troja.net (Postfix) with ESMTP id 29EC950906 for ; Mon, 30 Mar 2009 10:39:26 +0200 (CEST) From: "Uwe Schindler" To: References: <786fde50903300050x576cb814g550df2128b45fb60@mail.gmail.com> <9ac0c6aa0903300107t35b31f80u5df09d618692178c@mail.gmail.com> <786fde50903300120s411c91cch5df579c651c104fb@mail.gmail.com> Subject: RE: Bug in TopFieldCollector? Date: Mon, 30 Mar 2009 10:39:32 +0200 Message-ID: <2139BAF9FE46466BB404DDA4962A8AB9@VEGA> MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="----=_NextPart_000_0003_01C9B123.D00EC390" X-Mailer: Microsoft Office Outlook 11 In-Reply-To: <786fde50903300120s411c91cch5df579c651c104fb@mail.gmail.com> Thread-Index: AcmxEHZaTq9qdWSaQfq+xu64Dwt5UAAAdasA X-MimeOLE: Produced By Microsoft MimeOLE V6.00.2900.5579 X-Virus-Checked: Checked by ClamAV on apache.org ------=_NextPart_000_0003_01C9B123.D00EC390 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Why not call IndexSearcher.getIndexReader().getSequentialSubReaders() (see http://hudson.zones.apache.org/hudson/job/Lucene-trunk/javadoc/all/org/apach e/lucene/index/IndexReader.html). Its public and documented as this: public IndexReader[] getSequentialSubReaders() Expert: returns the sequential sub readers that this reader is logically composed of. For example, IndexSearcher uses this API to drive searching by one sub reader at a time. If this reader is not composed of sequential child readers, it should return null. If this method returns an empty array, that means this reader is a null reader (for example a MultiReader that has no sub readers). NOTE: for a MultiSegmentReader, which is obtained by open(java.lang.String) when the index has more than one segment, you should not use the sub-readers returned by this method to make any changes (setNorm, deleteDocument, etc.). Doing so will likely lead to index corruption. Use the parent reader instead. You only have the problem to replicate the code that gathers the subreaders of the subreaders itself recursively. Uwe ----- Uwe Schindler H.-H.-Meier-Allee 63, D-28213 Bremen http://www.thetaphi.de eMail: uwe@thetaphi.de _____ From: Shai Erera [mailto:serera@gmail.com] Sent: Monday, March 30, 2009 10:20 AM To: java-dev@lucene.apache.org Subject: Re: Bug in TopFieldCollector? Already did ! Another question - I think we somehow broke TopFieldCollector ... Previously, in TopFieldDocCollector, it accepted an IndexReader as a parameter, and now it requires IndexReader[], which is called subReaders. Calling the 'fast' search methods with Sort has no problem obtaining that IndexReader[] (and sort it), but how would someone use TopFieldCollector w/o calling the appropriate Searcher methods? For example, since all the Searcher methods pass in fillFields = true, I wanted to use the method Searcher.search(Query, TopFieldCollector) in the test case I wrote, which BTW looks like this: public void testSortWithoutFillFields() throws Exception { // There was previously a bug in TopFieldCollector when fillFields was set // to false - the same doc and score was set in ScoreDoc[] array. This test // asserts that if fillFields is false, the documents are set properly. It // does not use Searcher's default search methods (with Sort) since all set // fillFields to true. Sort sort = new Sort(); int nDocs=10; TopDocsCollector tdc = new TopFieldCollector(sort, nDocs, new IndexReader[] { ((IndexSearcher) full).getIndexReader() }, false); full.search(new MatchAllDocsQuery(), tdc); ScoreDoc[] sd = tdc.topDocs().scoreDocs; for (int i = 1; i < sd.length; i++) { assertTrue(sd[i].doc != sd[i - 1].doc); } } You'll notice that creating a TopFieldCollector now is much more complicated and *ugly*. As a user of IndexSearcher, I can only call getIndexReader() which returns a single IndexReader. I don't have access to gatherSubReaders and sortSubReaders. I don't see why I should have access to them. So it forces me to create a dummy array with a single IndexReader. There are two ways I see to solve it: 1. Introduce a getIndexReaders() method on IndexSearcher, which will return an array of (sorted?) IndexReader. 2. Introduce a new constructor in TopFieldCollector which accepts a single IndexReader and make the other one package-private (for use by IndexSearcher only). That constructor can internally create a dummy array of readers, but at least it's private to the constructor and not exposed to the rest of the world. Otherwise, I think it ruins TopFieldCollector and will make it a lot less intuitive to use. At least, people who'd want to move from TopFieldDocCollector to TopFieldCollector, will find it very inconvenient and strange. What do you think? I can do that (2) as part of 1575. If (1) is better, then I think a different issue should be opened, because we might want to return such an array as sorted or something, which makes it less trivial. Shai On Mon, Mar 30, 2009 at 11:07 AM, Michael McCandless wrote: 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 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-unsubscribe@lucene.apache.org For additional commands, e-mail: java-dev-help@lucene.apache.org ------=_NextPart_000_0003_01C9B123.D00EC390 Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

Why not call IndexSearcher.getIndexReader().getSequentialSubReaders() (see http://hudson.zones.apache.or= g/hudson/job/Lucene-trunk/javadoc/all/org/apache/lucene/index/IndexReader= .html). Its public and documented as this:

 =

public IndexReader[] getSequentialSubReaders()

 

Expert: returns the sequential sub readers that this reader is logically = composed of. For example, IndexSearcher uses this API to drive searching by one sub = reader at a time. If this reader is not composed of sequential child readers, it = should return null. If this method returns an empty array, that means this = reader is a null reader (for example a MultiReader that has no sub readers). =

NOTE: for a MultiSegmentReader, = which is obtained by = open(java.lang.String) when the index has more than one segment, you should not = use the sub-readers returned by this method to make any changes (setNorm, deleteDocument, etc.). Doing so will likely lead to index corruption. = Use the parent reader instead.

 =

You only have = the problem to replicate the code that gathers the subreaders of the subreaders = itself recursively.

 =

Uwe

From: Shai = Erera [mailto:serera@gmail.com]
Sent: Monday, March 30, = 2009 10:20 AM
To: java-dev@lucene.apache.org
Subject: Re: Bug in TopFieldCollector?

 

Already did = !

Another question - I think we somehow broke TopFieldCollector ... = Previously, in TopFieldDocCollector, it accepted an IndexReader as a parameter, and = now it requires IndexReader[], which is called subReaders. Calling the 'fast' = search methods with Sort has no problem obtaining that IndexReader[] (and sort = it), but how would someone use TopFieldCollector w/o calling the appropriate Searcher methods?

For example, since all the Searcher methods pass in fillFields =3D true, = I wanted to use the method Searcher.search(Query, TopFieldCollector) in the test = case I wrote, which BTW looks like this:

  public void testSortWithoutFillFields() throws Exception {
   
    // There was previously a bug in TopFieldCollector = when fillFields was set
    // to false - the same doc and score was set in = ScoreDoc[] array. This test
    // asserts that if fillFields is false, the documents = are set properly. It
    // does not use Searcher's default search methods = (with Sort) since all set
    // fillFields to true.
    Sort sort =3D new Sort();
    int nDocs=3D10;
   
    TopDocsCollector tdc =3D new TopFieldCollector(sort, = nDocs,
        new IndexReader[] { = ((IndexSearcher) full).getIndexReader() }, false);
   
    full.search(new MatchAllDocsQuery(), tdc);

    ScoreDoc[] sd =3D tdc.topDocs().scoreDocs;
    for (int i =3D 1; i < sd.length; i++) {
      assertTrue(sd[i].doc !=3D sd[i - = 1].doc);
    }
  }

You'll notice that creating a TopFieldCollector now is much more = complicated and *ugly*. As a user of IndexSearcher, I can only call getIndexReader() = which returns a single IndexReader. I don't have access to gatherSubReaders = and sortSubReaders. I don't see why I should have access to them. So it = forces me to create a dummy array with a single IndexReader.

There are two ways I see to solve it:
1. Introduce a getIndexReaders() method on IndexSearcher, which will = return an array of (sorted?) IndexReader.
2. Introduce a new constructor in TopFieldCollector which accepts a = single IndexReader and make the other one package-private (for use by = IndexSearcher only). That constructor can internally create a dummy array of readers, = but at least it's private to the constructor and not exposed to the rest of the = world.

Otherwise, I think it ruins TopFieldCollector and will make it a lot = less intuitive to use. At least, people who'd want to move from = TopFieldDocCollector to TopFieldCollector, will find it very inconvenient and strange.

What do you think? I can do that (2) as part of 1575. If (1) is better, = then I think a different issue should be opened, because we might want to = return such an array as sorted or something, which makes it less trivial.

Shai

On Mon, Mar 30, 2009 at 11:07 AM, Michael McCandless <lucene@mikemccandless.com&g= t; wrote:

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 <serera@gmail.com> wrote:
> Hi
>
> As I prepared the patch for 1575, I noticed a strange = implementation in
> TopFieldCollector's topDocs():
>
>     ScoreDoc[] scoreDocs =3D new = ScoreDoc[queue.size()];
>     if (fillFields) {
>       for (int i =3D queue.size() - 1; i = >=3D 0; i--) {
>         scoreDocs[i] =3D = queue.fillFields((FieldValueHitQueue.Entry)
> queue.pop());
>       }
>     } else {
>       Entry entry =3D = (FieldValueHitQueue.Entry) queue.pop();
>       for (int i =3D queue.size() - 1; i = >=3D 0; i--) {
>         scoreDocs[i] =3D new FieldDoc(entry.docID,
> =             &= nbsp;           &n= bsp;           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 =3D = (FieldSortedHitQueue)hq;
>     ScoreDoc[] scoreDocs =3D new = ScoreDoc[fshq.size()];
>     for (int i =3D fshq.size()-1; i >=3D 0; i--)      // put docs in array
>       scoreDocs[i] =3D fshq.fillFields = ((FieldDoc) fshq.pop());
>
>     return new TopFieldDocs(totalHits, = scoreDocs,
>             &= nbsp;           &n= bsp;   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 =3D (FieldValueHitQueue.Entry) = queue.pop();" inside the
> for loop.
>
> Shai
>

----------------------------------------------= -----------------------
To unsubscribe, e-mail: java-dev-unsubscri= be@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apac= he.org

 

------=_NextPart_000_0003_01C9B123.D00EC390--