Lucene - Core
  1. Lucene - Core
  2. LUCENE-6256

PostingsEnum impls should respect documented behavior when no positions exist

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.1, 6.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      PostingsEnum.nextPositions says that if no positions exist, NO_MORE_POSITIONS will be returned on the first call (actually it refers to DocsEnum.NO_MORE_DOCS, which should be changed since DocsEnum doesn't exist on trunk). At least one impl (BlockDocsEnum) does assert false inside nextPosition(). This means if you have assertions turned on (e.g. in a test) you get an assertion here, when the behavior should return NO_MORE_POSITIONS. I'm still going through all the implementations, but I also see MultiTermHighlighting which returns Integer.MAX_VALUE. I think we should clean up all these implementations which have no positions (including maybe the fake scorers that are copied around in a lot of places?) to return NO_MORE_POSITIONS.

        Activity

        Hide
        Ryan Ernst added a comment -

        I was reminded that nextPosition() should be called freq() times only. Having a NO_MORE_POSITIONS constant doesn't make sense. The case that should return -1 is only for when positions do not exist.

        This patch removes asserts in the places that should just return -1, and updates the javadocs for nextPositions(). All tests pass.

        Show
        Ryan Ernst added a comment - I was reminded that nextPosition() should be called freq() times only. Having a NO_MORE_POSITIONS constant doesn't make sense. The case that should return -1 is only for when positions do not exist. This patch removes asserts in the places that should just return -1, and updates the javadocs for nextPositions() . All tests pass.
        Hide
        Alan Woodward added a comment -

        +1

        The javadoc is a straight error on my part, a remnant from a previous attempt at reworking the API. The assertions were in there to check that the codecs were returning the correct PostingsEnum implementation when postings() was called, but they're pretty superfluous.

        Show
        Alan Woodward added a comment - +1 The javadoc is a straight error on my part, a remnant from a previous attempt at reworking the API. The assertions were in there to check that the codecs were returning the correct PostingsEnum implementation when postings() was called, but they're pretty superfluous.
        Hide
        Robert Muir added a comment -

        +1

        Show
        Robert Muir added a comment - +1
        Hide
        ASF subversion and git services added a comment -

        Commit 1661061 from Ryan Ernst in branch 'dev/trunk'
        [ https://svn.apache.org/r1661061 ]

        LUCENE-6256: Change PostingsEnum.nextPosition() to consistently return -1 when positions are not available

        Show
        ASF subversion and git services added a comment - Commit 1661061 from Ryan Ernst in branch 'dev/trunk' [ https://svn.apache.org/r1661061 ] LUCENE-6256 : Change PostingsEnum.nextPosition() to consistently return -1 when positions are not available
        Hide
        ASF subversion and git services added a comment -

        Commit 1661062 from Ryan Ernst in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1661062 ]

        LUCENE-6256: Change PostingsEnum.nextPosition() to consistently return -1 when positions are not available (merged 1661061)

        Show
        ASF subversion and git services added a comment - Commit 1661062 from Ryan Ernst in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1661062 ] LUCENE-6256 : Change PostingsEnum.nextPosition() to consistently return -1 when positions are not available (merged 1661061)
        Hide
        Timothy Potter added a comment -

        Bulk close after 5.1 release

        Show
        Timothy Potter added a comment - Bulk close after 5.1 release

          People

          • Assignee:
            Ryan Ernst
            Reporter:
            Ryan Ernst
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development