Uploaded image for project: 'Lucene - Core'
  1. Lucene - Core
  2. LUCENE-6256

PostingsEnum impls should respect documented behavior when no positions exist

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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
        rjernst 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
        rjernst 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
        romseygeek 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
        romseygeek 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
        rcmuir Robert Muir added a comment -

        +1

        Show
        rcmuir Robert Muir added a comment - +1
        Hide
        jira-bot 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
        jira-bot 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
        jira-bot 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
        jira-bot 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
        thelabdude Timothy Potter added a comment -

        Bulk close after 5.1 release

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

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development