Lucene - Core
  1. Lucene - Core
  2. LUCENE-5260

Make older Suggesters more accepting of TermFreqPayloadIterator

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.6, 6.0
    • Component/s: core/search
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      As discussed in https://issues.apache.org/jira/browse/LUCENE-5251, it would be nice to make the older suggesters accepting of TermFreqPayloadIterator and throw an exception if payload is found (if it cannot be used).

      This will also allow us to nuke most of the other interfaces for BytesRefIterator.

      1. LUCENE-5260.patch
        126 kB
        Areek Zillur
      2. LUCENE-5260.patch
        33 kB
        Areek Zillur

        Activity

        Hide
        Michael McCandless added a comment -

        +1

        It seems like we should remove TermFreqIterator, and only keep TermFreqPayloadIterator.

        Show
        Michael McCandless added a comment - +1 It seems like we should remove TermFreqIterator, and only keep TermFreqPayloadIterator.
        Hide
        Areek Zillur added a comment -

        Hey Michael, I was thinking about how to nicely replace TermFreqIterator.

        • I was thinking about having some kind of wrapper for TermFreqPayloadIterator that will nullify the payload field for the current TermFreqIterator consumers and a way for the wrapper to signal early on to the consumers that they dont need to deal with the payload at all.
        • Also It seems like there are a lot of implementations for TermFreqIterator (e.g BufferedTermFreqIteratorWrapper, SortedTermFreqIteratorWrapper); I will make sure all these implementation work with TermFreqPayloadIterator and its new wrapper (for mimicking TermFreqIterator).

        Any thoughts? I will try to come up with a rough patch soon.

        Show
        Areek Zillur added a comment - Hey Michael, I was thinking about how to nicely replace TermFreqIterator. I was thinking about having some kind of wrapper for TermFreqPayloadIterator that will nullify the payload field for the current TermFreqIterator consumers and a way for the wrapper to signal early on to the consumers that they dont need to deal with the payload at all. Also It seems like there are a lot of implementations for TermFreqIterator (e.g BufferedTermFreqIteratorWrapper, SortedTermFreqIteratorWrapper); I will make sure all these implementation work with TermFreqPayloadIterator and its new wrapper (for mimicking TermFreqIterator). Any thoughts? I will try to come up with a rough patch soon.
        Hide
        Areek Zillur added a comment -

        Uploaded Patch:

        • changed the input to lookup.build to take TermFreqPayloadIterator instead of TermFreqPayloadIterator
        • made all suggesters compatible with termFreqPayloadIterator (but error if payload is present but cannot be used)
        • nuked all implementations of TermFreq and made them work with termFreqPayload instead (Except for SortedTermFreqIteratorWrapper).
        • got rid of all the references to termFreqIter

        Still todo:

        • actually nuke TermFreqIterator
        • change the names of the implementations to reflect that they are implementations of TermFreqPayloadIter
        • add tests to ensure that all the implementations work with payload
        • support payloads in SortedTermFreqIteratorWrapper
        Show
        Areek Zillur added a comment - Uploaded Patch: changed the input to lookup.build to take TermFreqPayloadIterator instead of TermFreqPayloadIterator made all suggesters compatible with termFreqPayloadIterator (but error if payload is present but cannot be used) nuked all implementations of TermFreq and made them work with termFreqPayload instead (Except for SortedTermFreqIteratorWrapper). got rid of all the references to termFreqIter Still todo: actually nuke TermFreqIterator change the names of the implementations to reflect that they are implementations of TermFreqPayloadIter add tests to ensure that all the implementations work with payload support payloads in SortedTermFreqIteratorWrapper
        Hide
        Michael McCandless added a comment -

        Thanks Areek, patch looks great! I like the hasPayloads() up-front
        introspection.

        In UnsortedTermFreqIteratorWrapper.payload(), why do we set currentOrd
        as a side effect? Shouldn't next() already do that? Maybe, we should
        instead assert currentOrd == ords[curPos]? Also, can we break that
        sneaky currentOrd assignment in next into its own line before?

        Show
        Michael McCandless added a comment - Thanks Areek, patch looks great! I like the hasPayloads() up-front introspection. In UnsortedTermFreqIteratorWrapper.payload(), why do we set currentOrd as a side effect? Shouldn't next() already do that? Maybe, we should instead assert currentOrd == ords [curPos] ? Also, can we break that sneaky currentOrd assignment in next into its own line before?
        Hide
        Areek Zillur added a comment -

        Thanks for the review, Michael! I fixed the UnsortedTermFreqIteratorWrapper issue and added the asserts.

        Uploaded patch

        • added support for payloads in SortedTermFreqPayloadIteratorWrapper
        • nuked all termFreqIter and co
        • renamed all the termFreq implementations
        • added tests
        Show
        Areek Zillur added a comment - Thanks for the review, Michael! I fixed the UnsortedTermFreqIteratorWrapper issue and added the asserts. Uploaded patch added support for payloads in SortedTermFreqPayloadIteratorWrapper nuked all termFreqIter and co renamed all the termFreq implementations added tests
        Hide
        ASF subversion and git services added a comment -

        Commit 1531664 from Michael McCandless in branch 'dev/trunk'
        [ https://svn.apache.org/r1531664 ]

        LUCENE-5260: cutover all suggesters to TermFreqPayloadIterator

        Show
        ASF subversion and git services added a comment - Commit 1531664 from Michael McCandless in branch 'dev/trunk' [ https://svn.apache.org/r1531664 ] LUCENE-5260 : cutover all suggesters to TermFreqPayloadIterator
        Hide
        ASF subversion and git services added a comment -

        Commit 1531666 from Michael McCandless in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1531666 ]

        LUCENE-5260: cutover all suggesters to TermFreqPayloadIterator

        Show
        ASF subversion and git services added a comment - Commit 1531666 from Michael McCandless in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1531666 ] LUCENE-5260 : cutover all suggesters to TermFreqPayloadIterator
        Hide
        Michael McCandless added a comment -

        Thanks Areek!

        Show
        Michael McCandless added a comment - Thanks Areek!
        Hide
        ASF subversion and git services added a comment -

        Commit 1531667 from Michael McCandless in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1531667 ]

        LUCENE-5260: don't use java7-only API

        Show
        ASF subversion and git services added a comment - Commit 1531667 from Michael McCandless in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1531667 ] LUCENE-5260 : don't use java7-only API

          People

          • Assignee:
            Unassigned
            Reporter:
            Areek Zillur
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development