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

      The current back compat introduced in LUCENE-4524, does not really help the users calling e.g. LeafReader.termDocsEnum() or LeafReader.termPositionsEnum(), because the former's return value changes to PostingsEnum, its superclass, and the latter got removed.

      It also does not help users using TermsEnum.docs() or TermsEnum.docsAndPositions() which got removed and just replaced with postings().

      DocsEnum is different, but not deprecated, instead only used by some codecs as a convenience class. DocsAndPositionsEnum is removed.

      I think we can do this a little better. First, we need to fix trunk to work the way we want it to look. I think we should have LeafReader.postings() and TermsEnum.postings(), and everything should use PostingsEnum. This is simplest.

      But in 5.x, I think we should have DocsEnum and DocsAndPositionsEnum which are deprecated, to help guide the user.

      The "sugar" methods on LeafReader that exist in 5.0 (termDocsEnum(), termPositionsEnum()) should be deprecated (with message to use postings()) and final, and can just wrap PostingsEnum. There is no reuse and flags here so this is very simple.

      On TermsEnum its more complicated, but i dont think impossible. We should add back deprecated and final termDocsEnum() and termPositionsEnum() (with message to use postings()) and these deprecated ones can have an instanceof check, unwrapping back to PostingsEnum before they invoke postings behind the scenes.

      For the 2 remaining ones on TermsEnum that take flags, thats the most tricky. I actually think we shouldn't change the existing constant values when we dont have to. And I don't think the names FLAG_FREQS are special, i'd rather these just be constants like FREQS. I looked thru JDK constants (http://docs.oracle.com/javase/7/docs/api/constant-values.html) and only one class uses this FLAG_xxx prefix.

      So I think we should have PostingsEnum.FREQS etc with new values, not conflicting with the old FLAG_FREQS etc values (which we can add back, deprecated, to DocsEnum and DocsAndPositionsEnum). We can even add a check to the deprecated methods that only valid values are passed.

      This just means we have contained back compat, only for deprecated and final sugar methods in LeafReader and TermsEnum, and the 2 deprecated classes. I think we can live with that and it would save users pain.

      1. LUCENE-6246_backwards.patch
        66 kB
        Robert Muir
      2. LUCENE-6246-flags-cleanup.patch
        17 kB
        Ryan Ernst
      3. LUCENE-6246-flags-cleanup.patch
        21 kB
        Ryan Ernst
      4. LUCENE-6246-trunk.patch
        151 kB
        Robert Muir

        Activity

        Hide
        Robert Muir added a comment -

        FLAG_ALL is also buggy today. The generated bitmask does not actually include offsets. It would need to be 0xF.

        The logic is also wrong here for OFFSETS and PAYLOADS i think too? With the current constants, OFFSETS implies positions but PAYLOADS does not. This doesn't make sense...

        Show
        Robert Muir added a comment - FLAG_ALL is also buggy today. The generated bitmask does not actually include offsets. It would need to be 0xF. The logic is also wrong here for OFFSETS and PAYLOADS i think too? With the current constants, OFFSETS implies positions but PAYLOADS does not. This doesn't make sense...
        Hide
        Robert Muir added a comment -

        Here is a patch with the API changes for trunk.

        This is just the conversion to postings(), removal of DocsEnum, renaming of FLAG_XXX constants, and fixing some documentation bugs.

        I'd like to also backport this to 5.x

        I didn't change any FLAG constant values yet, because I think this is buggy and would prefer to have better tests. But I think this is the API we want in trunk? And it allows us to implement simple deprecations and back compat on 5.x afterwards.

        Show
        Robert Muir added a comment - Here is a patch with the API changes for trunk. This is just the conversion to postings(), removal of DocsEnum, renaming of FLAG_XXX constants, and fixing some documentation bugs. I'd like to also backport this to 5.x I didn't change any FLAG constant values yet, because I think this is buggy and would prefer to have better tests. But I think this is the API we want in trunk? And it allows us to implement simple deprecations and back compat on 5.x afterwards.
        Hide
        Michael McCandless added a comment -

        +1 to the plan and to the trunk patch.

        Show
        Michael McCandless added a comment - +1 to the plan and to the trunk patch.
        Hide
        Robert Muir added a comment -

        Thanks for reviewing. Anyone else please have a look if you have the chance.

        There is a fair amount of work to do on this issue, but I will do it. I just want to do it iteratively.

        Show
        Robert Muir added a comment - Thanks for reviewing. Anyone else please have a look if you have the chance. There is a fair amount of work to do on this issue, but I will do it. I just want to do it iteratively.
        Hide
        Ryan Ernst added a comment -

        +1 The trunk patch and plan also lgtm.

        Show
        Ryan Ernst added a comment - +1 The trunk patch and plan also lgtm.
        Hide
        Alan Woodward added a comment -

        +1, thanks Rob, this makes the transition much cleaner

        Show
        Alan Woodward added a comment - +1, thanks Rob, this makes the transition much cleaner
        Hide
        ASF subversion and git services added a comment -

        Commit 1660366 from Robert Muir in branch 'dev/trunk'
        [ https://svn.apache.org/r1660366 ]

        LUCENE-6246: Fix DocsEnum -> PostingsEnum transition (phase 1)

        Show
        ASF subversion and git services added a comment - Commit 1660366 from Robert Muir in branch 'dev/trunk' [ https://svn.apache.org/r1660366 ] LUCENE-6246 : Fix DocsEnum -> PostingsEnum transition (phase 1)
        Hide
        ASF subversion and git services added a comment -

        Commit 1660375 from Robert Muir in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1660375 ]

        LUCENE-6246: Fix DocsEnum -> PostingsEnum transition (phase 1)

        Show
        ASF subversion and git services added a comment - Commit 1660375 from Robert Muir in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1660375 ] LUCENE-6246 : Fix DocsEnum -> PostingsEnum transition (phase 1)
        Hide
        ASF subversion and git services added a comment -

        Commit 1660405 from Robert Muir in branch 'dev/trunk'
        [ https://svn.apache.org/r1660405 ]

        LUCENE-6246: add simple test for postings reuse/flags/behavior (DOCS_ONLY so far), fix asserting to support reuse

        Show
        ASF subversion and git services added a comment - Commit 1660405 from Robert Muir in branch 'dev/trunk' [ https://svn.apache.org/r1660405 ] LUCENE-6246 : add simple test for postings reuse/flags/behavior (DOCS_ONLY so far), fix asserting to support reuse
        Hide
        ASF subversion and git services added a comment -

        Commit 1660422 from Robert Muir in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1660422 ]

        LUCENE-6246: add simple test for postings reuse/flags/behavior (DOCS_ONLY so far), fix asserting to support reuse

        Show
        ASF subversion and git services added a comment - Commit 1660422 from Robert Muir in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1660422 ] LUCENE-6246 : add simple test for postings reuse/flags/behavior (DOCS_ONLY so far), fix asserting to support reuse
        Hide
        ASF subversion and git services added a comment -

        Commit 1660441 from Robert Muir in branch 'dev/trunk'
        [ https://svn.apache.org/r1660441 ]

        LUCENE-6246: simple tests for other index/flags possibilities

        Show
        ASF subversion and git services added a comment - Commit 1660441 from Robert Muir in branch 'dev/trunk' [ https://svn.apache.org/r1660441 ] LUCENE-6246 : simple tests for other index/flags possibilities
        Hide
        ASF subversion and git services added a comment -

        Commit 1660442 from Robert Muir in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1660442 ]

        LUCENE-6246: simple tests for other index/flags possibilities

        Show
        ASF subversion and git services added a comment - Commit 1660442 from Robert Muir in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1660442 ] LUCENE-6246 : simple tests for other index/flags possibilities
        Hide
        Robert Muir added a comment -

        Here is the backwards patch as described. its only for branch_5x... kinda large due to the test.

        I didn't yet renumber any FLAGS or anything, since my tests are passing. We can look into that separately next, at least so its easier to reason about?

        Show
        Robert Muir added a comment - Here is the backwards patch as described. its only for branch_5x... kinda large due to the test. I didn't yet renumber any FLAGS or anything, since my tests are passing. We can look into that separately next, at least so its easier to reason about?
        Hide
        Ryan Ernst added a comment -

        +1, great tests

        Show
        Ryan Ernst added a comment - +1, great tests
        Hide
        Adrien Grand added a comment -

        +1!

        Show
        Adrien Grand added a comment - +1!
        Hide
        Michael McCandless added a comment -

        +1

        Show
        Michael McCandless added a comment - +1
        Hide
        ASF subversion and git services added a comment -

        Commit 1660489 from Robert Muir in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1660489 ]

        LUCENE-6246: add backwards layer

        Show
        ASF subversion and git services added a comment - Commit 1660489 from Robert Muir in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1660489 ] LUCENE-6246 : add backwards layer
        Hide
        Ryan Ernst added a comment -

        Here is a patch which cleans up the flags. Each feature now is represented by a single bit, which starts above the backcompat bits for DocsEnum and DocsAndPositionsEnum constants, and there is a general purpose helper method for checking whether a feature was requested. The constants are now also shorts, so that the helper method can have different types for the flags vs feature, to avoid confusion between those two parameters.

        Show
        Ryan Ernst added a comment - Here is a patch which cleans up the flags. Each feature now is represented by a single bit, which starts above the backcompat bits for DocsEnum and DocsAndPositionsEnum constants, and there is a general purpose helper method for checking whether a feature was requested. The constants are now also shorts, so that the helper method can have different types for the flags vs feature, to avoid confusion between those two parameters.
        Hide
        Alan Woodward added a comment -

        There are some changes to SimpleTextFieldsReader and TestUtil that I think you probably didn't mean to include? Other than that, though, it looks splendid. +1! And thanks for cleaning this up.

        Show
        Alan Woodward added a comment - There are some changes to SimpleTextFieldsReader and TestUtil that I think you probably didn't mean to include? Other than that, though, it looks splendid. +1! And thanks for cleaning this up.
        Hide
        Ryan Ernst added a comment -

        Yeah I had a bad merge. Here's a new patch that fixes that.

        Show
        Ryan Ernst added a comment - Yeah I had a bad merge. Here's a new patch that fixes that.
        Hide
        Robert Muir added a comment -

        +1, thanks for cleaning up.

        Show
        Robert Muir added a comment - +1, thanks for cleaning up.
        Hide
        ASF subversion and git services added a comment -

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

        LUCENE-6246: simplify flags for PostingsEnum

        Show
        ASF subversion and git services added a comment - Commit 1661822 from Ryan Ernst in branch 'dev/trunk' [ https://svn.apache.org/r1661822 ] LUCENE-6246 : simplify flags for PostingsEnum
        Hide
        ASF subversion and git services added a comment -

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

        LUCENE-6246: simplify flags for PostingsEnum (merged 1661822)

        Show
        ASF subversion and git services added a comment - Commit 1661829 from Ryan Ernst in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1661829 ] LUCENE-6246 : simplify flags for PostingsEnum (merged 1661822)
        Hide
        ASF subversion and git services added a comment -

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

        LUCENE-6246: fix missed checks on PostingsEnum.POSITIONS

        Show
        ASF subversion and git services added a comment - Commit 1662129 from Ryan Ernst in branch 'dev/trunk' [ https://svn.apache.org/r1662129 ] LUCENE-6246 : fix missed checks on PostingsEnum.POSITIONS
        Hide
        ASF subversion and git services added a comment -

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

        LUCENE-6246: fix missed checks on PostingsEnum.POSITIONS (merged 1662129)

        Show
        ASF subversion and git services added a comment - Commit 1662130 from Ryan Ernst in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1662130 ] LUCENE-6246 : fix missed checks on PostingsEnum.POSITIONS (merged 1662129)
        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:
            Unassigned
            Reporter:
            Robert Muir
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development