Solr
  1. Solr
  2. SOLR-6022

Rename getAnalyzer to getIndexAnalyzer

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.9, 6.0
    • Component/s: None
    • Labels:
      None

      Description

      We have separate index/query analyzer chains, but the access methods for the analyzers do not match up with the names. This can lead to unknowingly using the wrong analyzer chain (as it did in SOLR-6017). We should do this renaming in trunk, and deprecate the old getAnalyzer function in 4x.

      1. SOLR-6022.branch_4x-deprecation.patch
        4 kB
        Ryan Ernst
      2. SOLR-6022.patch
        37 kB
        Ryan Ernst
      3. SOLR-6022.patch
        34 kB
        Ryan Ernst
      4. SOLR-6022.patch
        33 kB
        Ryan Ernst

        Activity

        Hide
        Ryan Ernst added a comment -

        Here's a patch against trunk that does the rename for IndexSchema and FieldType. When merging back to branch_4x I'll add getAnalyzer back with as deprecated.

        Show
        Ryan Ernst added a comment - Here's a patch against trunk that does the rename for IndexSchema and FieldType. When merging back to branch_4x I'll add getAnalyzer back with as deprecated.
        Hide
        Erick Erickson added a comment -

        Ryan:

        Cool, way to jump on it! And when I did my quick scan for calls to getAnalyzer, I didn't think about the local variables, subclasses that overrode the method call etc., good catch.

        Can we really just change the public method without providing a back-compat? Something like

        public Analyzer getIndexAnalyzer()

        { return indexAnalyzer; }

        /**

        • @deprecated As of release 5.0, replaced by {@link #getIndexAnalyzer()}

          */
          @Deprecated // Remove in 5.0
          public Analyzer getAnalyzer()

          { return getIndexAnalyzer(); }

        And a CHANGES.txt entry too.

        Or is it OK in a case like this to just rename the method and provide a CHANGES.txt entry? What do others think?

        Show
        Erick Erickson added a comment - Ryan: Cool, way to jump on it! And when I did my quick scan for calls to getAnalyzer, I didn't think about the local variables, subclasses that overrode the method call etc., good catch. Can we really just change the public method without providing a back-compat? Something like public Analyzer getIndexAnalyzer() { return indexAnalyzer; } /** @deprecated As of release 5.0, replaced by {@link #getIndexAnalyzer()} */ @Deprecated // Remove in 5.0 public Analyzer getAnalyzer() { return getIndexAnalyzer(); } And a CHANGES.txt entry too. Or is it OK in a case like this to just rename the method and provide a CHANGES.txt entry? What do others think?
        Hide
        Uwe Schindler added a comment - - edited

        The sophisticated backwards policeman says:

        To provide backwards compatibility for those that implement getAnalyzer, the delegation must be the other way round:

        • In trunk, all like it is in the patch.
        • In 4.x, getAnalyzer() stays as it is - no code changes, but gets deprecated. getIndexAnalyzer is new method, but delegates in 4.x to the deprecated getAnalyzer() with a @SuppressWranings("deprecation") on the call. By this we have two good things: both methods behave identical behavior, so people can call both. But people who implement only the old one will still get called by Solr's code, so they don't loose functionality. This is the general pattern to replace methods (also used by the JDK) - it just "feels" unnatural, but is correct way to deprecate

        Uwe

        Show
        Uwe Schindler added a comment - - edited The sophisticated backwards policeman says: To provide backwards compatibility for those that implement getAnalyzer, the delegation must be the other way round: In trunk, all like it is in the patch. In 4.x, getAnalyzer() stays as it is - no code changes, but gets deprecated. getIndexAnalyzer is new method, but delegates in 4.x to the deprecated getAnalyzer() with a @SuppressWranings("deprecation") on the call. By this we have two good things: both methods behave identical behavior, so people can call both. But people who implement only the old one will still get called by Solr's code, so they don't loose functionality. This is the general pattern to replace methods (also used by the JDK) - it just "feels" unnatural, but is correct way to deprecate Uwe
        Hide
        Erick Erickson added a comment -

        Ryan:

        That'll teach me to read the entire comment, you already are on the deprecation thing.

        Uwe:

        Thanks for the tutorial...

        Show
        Erick Erickson added a comment - Ryan: That'll teach me to read the entire comment, you already are on the deprecation thing. Uwe: Thanks for the tutorial...
        Hide
        Ryan Ernst added a comment -

        Here are two more patches:

        1. The first is still for trunk, and changes the indexAnalyzer/queryAnalyzer members in FieldType to private scope. This will be a "hard fail" for anyone that is subclassing FieldType and using these, but they should be using get/setAnalyzer anyways. It also adds CHANGES.txt entries for review.
        2. The second patch shows exactly how I will do the deprecation in branch_4x (I believe I should be able to just apply the patch after doing a merge back from trunk).
        Show
        Ryan Ernst added a comment - Here are two more patches: The first is still for trunk, and changes the indexAnalyzer/queryAnalyzer members in FieldType to private scope. This will be a "hard fail" for anyone that is subclassing FieldType and using these, but they should be using get/setAnalyzer anyways. It also adds CHANGES.txt entries for review. The second patch shows exactly how I will do the deprecation in branch_4x (I believe I should be able to just apply the patch after doing a merge back from trunk).
        Hide
        Uwe Schindler added a comment -

        I think that looks good.

        Show
        Uwe Schindler added a comment - I think that looks good.
        Hide
        Tomás Fernández Löbbe added a comment -

        I don't see why in 4x calls to getAnalyzer() can't be changed to getIndexAnalyzer(), It wouldn't break compatibility and it would avoid creating many warnings.

        Show
        Tomás Fernández Löbbe added a comment - I don't see why in 4x calls to getAnalyzer() can't be changed to getIndexAnalyzer(), It wouldn't break compatibility and it would avoid creating many warnings.
        Hide
        Uwe Schindler added a comment - - edited

        Tomás Fernández Löbbe: The patch for 4.x has to be applied after merge from trunk, so first apply patch to trunk, merge that to 4.x and then apply the 4.x patch on-top (which restores the deprecated method):

        The second patch shows exactly how I will do the deprecation in branch_4x (I believe I should be able to just apply the patch after doing a merge back from trunk).

        To test that everything works without merging/committing, apply both patches to the same 4.x checkout, the general first, then the 4.x one.

        Show
        Uwe Schindler added a comment - - edited Tomás Fernández Löbbe : The patch for 4.x has to be applied after merge from trunk, so first apply patch to trunk, merge that to 4.x and then apply the 4.x patch on-top (which restores the deprecated method): The second patch shows exactly how I will do the deprecation in branch_4x (I believe I should be able to just apply the patch after doing a merge back from trunk). To test that everything works without merging/committing, apply both patches to the same 4.x checkout, the general first, then the 4.x one.
        Hide
        Tomás Fernández Löbbe added a comment -

        The patch for 4.x has to be applied after merge from trunk

        Got it, thanks for explaining

        Show
        Tomás Fernández Löbbe added a comment - The patch for 4.x has to be applied after merge from trunk Got it, thanks for explaining
        Hide
        Ryan Ernst added a comment -

        Another trunk patch. I had to rework how subclasses enable analyzers for their type. Before subclasses had to override setAnalyzer, and implement it to set the protected member var. With this patch they instead override supportsAnalyzers() to return true.

        Show
        Ryan Ernst added a comment - Another trunk patch. I had to rework how subclasses enable analyzers for their type. Before subclasses had to override setAnalyzer, and implement it to set the protected member var. With this patch they instead override supportsAnalyzers() to return true.
        Hide
        Anshum Gupta added a comment -

        Good stuff. Makes things easier to comprehend.

        I am not sure if this should be the IndexAnalyzer or the QueryAnalyzer as AFAIR, this tries to construct a query out of the terms from a document (given an id).

        MoreLikeThisHandler.java
        -      mlt.setAnalyzer( searcher.getSchema().getAnalyzer() );
        +      mlt.setAnalyzer( searcher.getSchema().getIndexAnalyzer() );
        
        Show
        Anshum Gupta added a comment - Good stuff. Makes things easier to comprehend. I am not sure if this should be the IndexAnalyzer or the QueryAnalyzer as AFAIR, this tries to construct a query out of the terms from a document (given an id). MoreLikeThisHandler.java - mlt.setAnalyzer( searcher.getSchema().getAnalyzer() ); + mlt.setAnalyzer( searcher.getSchema().getIndexAnalyzer() );
        Hide
        Ryan Ernst added a comment -

        I am not sure if this should be the IndexAnalyzer or the QueryAnalyzer as AFAIR, this tries to construct a query out of the terms from a document (given an id).

        I only kept what was there before. I don't know enough about MLT to make a statement whether it is correct or not. If it is broken, I think another jira should be opened?

        Show
        Ryan Ernst added a comment - I am not sure if this should be the IndexAnalyzer or the QueryAnalyzer as AFAIR, this tries to construct a query out of the terms from a document (given an id). I only kept what was there before. I don't know enough about MLT to make a statement whether it is correct or not. If it is broken, I think another jira should be opened?
        Hide
        Anshum Gupta added a comment -

        Sure, let that be. Will look into it more and open a new JIRA if required.
        Thanks.

        Show
        Anshum Gupta added a comment - Sure, let that be. Will look into it more and open a new JIRA if required. Thanks.
        Hide
        ASF subversion and git services added a comment -

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

        SOLR-6022: Rename getAnalyzer() to getIndexAnalyzer()

        Show
        ASF subversion and git services added a comment - Commit 1592076 from Ryan Ernst in branch 'dev/trunk' [ https://svn.apache.org/r1592076 ] SOLR-6022 : Rename getAnalyzer() to getIndexAnalyzer()
        Hide
        ASF subversion and git services added a comment -

        Commit 1592127 from Ryan Ernst in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1592127 ]

        SOLR-6022: Deprecate getAnalyzer() in IndexField and FieldType, and add getIndexAnalyzer()

        Show
        ASF subversion and git services added a comment - Commit 1592127 from Ryan Ernst in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1592127 ] SOLR-6022 : Deprecate getAnalyzer() in IndexField and FieldType, and add getIndexAnalyzer()

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development