Lucene - Core
  1. Lucene - Core
  2. LUCENE-5313

Add "preservePositionIncrements" to AnalyzingSuggester and FuzzySuggester constructors

    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, Patch Available

      Description

      It would be convenient to have "preservePositionIncrements" in the suggesters constructor, rather than having a setPreservePositionIncrements method. That way it could be nicely used with the factory model already used by Solr.

      1. LUCENE-5313.patch
        25 kB
        Areek Zillur
      2. LUCENE-5313.patch
        9 kB
        Areek Zillur
      3. LUCENE-5313.patch
        5 kB
        Areek Zillur

        Activity

        Hide
        Areek Zillur added a comment -

        Patch adding "preservePositionIncrements" to base constructors for FuzzySuggester and AnalyzingSuggester.

        Show
        Areek Zillur added a comment - Patch adding "preservePositionIncrements" to base constructors for FuzzySuggester and AnalyzingSuggester.
        Hide
        Robert Muir added a comment -

        Makes sense to me. I don't know why we have this lone setter in this class when everything else is controlled via the ctor.

        Show
        Robert Muir added a comment - Makes sense to me. I don't know why we have this lone setter in this class when everything else is controlled via the ctor.
        Hide
        Adrien Grand added a comment -

        I think it is me: when I deprecated FilteringTokenFilter.setEnablePositionIncrements (because it could break token streams), I had to give AnalyzingSuggester a way to deal with holes in token streams (it used to rely on a setEnablePositionIncrements) so I added a setter. I must admit I haven't thought a lot when doing it and since I was deprecating a setter, the first idea that came to my mind was to add a setter to the suggester. However, I agree it would be better to have it as a parameter of the constructor!

        Show
        Adrien Grand added a comment - I think it is me: when I deprecated FilteringTokenFilter.setEnablePositionIncrements (because it could break token streams), I had to give AnalyzingSuggester a way to deal with holes in token streams (it used to rely on a setEnablePositionIncrements) so I added a setter. I must admit I haven't thought a lot when doing it and since I was deprecating a setter, the first idea that came to my mind was to add a setter to the suggester. However, I agree it would be better to have it as a parameter of the constructor!
        Hide
        Michael McCandless added a comment -

        I think we should just remove the setter (ie, you can only set this in the ctor)?

        Show
        Michael McCandless added a comment - I think we should just remove the setter (ie, you can only set this in the ctor)?
        Hide
        Robert Muir added a comment -

        +1

        Show
        Robert Muir added a comment - +1
        Hide
        Areek Zillur added a comment -

        Updated patch

        • Nuked lone setter
          It does make the ctor scary when you want to preservePositionIncrements though!
        Show
        Areek Zillur added a comment - Updated patch Nuked lone setter It does make the ctor scary when you want to preservePositionIncrements though!
        Hide
        Michael McCandless added a comment -

        I think we don't need that ctor defaulting preservePosIncr to true? Ie, just three ctors: Analyzer, Analyzer + Analyzer, and then the kitchen sink.

        Show
        Michael McCandless added a comment - I think we don't need that ctor defaulting preservePosIncr to true? Ie, just three ctors: Analyzer, Analyzer + Analyzer, and then the kitchen sink.
        Hide
        Areek Zillur added a comment -

        Patch Uploaded:

        • got rid of ctor taking everything except "preservePositionIncrements" for AnalyzingSuggester and FuzzySuggester
        • updated Solr suggester lookup factories to use the new ctor (depended on the removed ctor)
        Show
        Areek Zillur added a comment - Patch Uploaded: got rid of ctor taking everything except "preservePositionIncrements" for AnalyzingSuggester and FuzzySuggester updated Solr suggester lookup factories to use the new ctor (depended on the removed ctor)
        Hide
        Michael McCandless added a comment -

        Thanks Areek, new patch looks great; I'll commit shortly.

        Show
        Michael McCandless added a comment - Thanks Areek, new patch looks great; I'll commit shortly.
        Hide
        ASF subversion and git services added a comment -

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

        LUCENE-5313: move preservePositionIncrements from setter to ctor

        Show
        ASF subversion and git services added a comment - Commit 1537038 from Michael McCandless in branch 'dev/trunk' [ https://svn.apache.org/r1537038 ] LUCENE-5313 : move preservePositionIncrements from setter to ctor
        Hide
        ASF subversion and git services added a comment -

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

        LUCENE-5313: move preservePositionIncrements from setter to ctor

        Show
        ASF subversion and git services added a comment - Commit 1537039 from Michael McCandless in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1537039 ] LUCENE-5313 : move preservePositionIncrements from setter to ctor
        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 1537102 from Michael McCandless in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1537102 ]

        LUCENE-5313: leave the default to true for enablePositionIncrements

        Show
        ASF subversion and git services added a comment - Commit 1537102 from Michael McCandless in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1537102 ] LUCENE-5313 : leave the default to true for enablePositionIncrements
        Hide
        ASF subversion and git services added a comment -

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

        LUCENE-5313: leave the default to true for enablePositionIncrements

        Show
        ASF subversion and git services added a comment - Commit 1537104 from Michael McCandless in branch 'dev/trunk' [ https://svn.apache.org/r1537104 ] LUCENE-5313 : leave the default to true for enablePositionIncrements

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development