Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-9237

DefaultSolrHighlighter.doHighlightingByFastVectorHighlighter can't be overidden

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 6.1
    • Fix Version/s: 6.2, master (7.0)
    • Component/s: highlighter
    • Labels:
      None

      Description

      With 6.1.0 the protected method DefaultSolrHighlighter.doHighlightingByFastVectorHighlighter passes in a private class called FvhContainer which makes it very difficult to extend.

      I have code that extends DefaultSolrHighlighter which I can't fix due to this issue.
      Could you consider making FvhContainer "protected" and use a constructor?

      1. SOLR-9237.patch
        2 kB
        Jan Høydahl

        Activity

        Hide
        covolution Gethin James added a comment -
        Show
        covolution Gethin James added a comment - The commit that added the FvhContainer was https://github.com/covolution/lucene-solr/commit/e37e49ed46c42da4ea4fbd74f08de1ba10af7923 by Jan Høydahl
        Hide
        janhoy Jan Høydahl added a comment -

        Nice catch. I'm gonna make both FvhContainer and doHighlightingOfField() protected just to follow existing practice for this class, ok?

        Show
        janhoy Jan Høydahl added a comment - Nice catch. I'm gonna make both FvhContainer and doHighlightingOfField() protected just to follow existing practice for this class, ok?
        Hide
        janhoy Jan Høydahl added a comment -

        Proposed patch attached. David Smiley

        Show
        janhoy Jan Høydahl added a comment - Proposed patch attached. David Smiley
        Hide
        joel.bernstein Joel Bernstein added a comment -

        +1

        Show
        joel.bernstein Joel Bernstein added a comment - +1
        Hide
        dsmiley David Smiley added a comment -

        +1. Personally I wouldn't have bothered with that constructor only to call it with (null,null) but whatever.

        Show
        dsmiley David Smiley added a comment - +1. Personally I wouldn't have bothered with that constructor only to call it with (null,null) but whatever.
        Hide
        janhoy Jan Høydahl added a comment -

        Sure, however it makes the initialization a bit more explicit and enables inline construction with new - sounds like Gethin James's custom code would benefit, so I'm OK with that..

        Show
        janhoy Jan Høydahl added a comment - Sure, however it makes the initialization a bit more explicit and enables inline construction with new - sounds like Gethin James 's custom code would benefit, so I'm OK with that..
        Hide
        janhoy Jan Høydahl added a comment -

        I did a small test of subclassing DefaultSolrHighlighter and overriding doHighlightingByFastVectorHighlighter, accessing fvhContainer.fieldQuery and creating a new FvhContainer.

        The only problem I saw was that IntelliJ did not like my subclass accessing fvhContainer.fieldQuery. It says Cannot access org.apache.lucene.search.vectorhighlight.FieldQuery. However, compiling with ant works.

        Gethin James do you have a chance to verify that the proposed patch solves your concerns?

        Show
        janhoy Jan Høydahl added a comment - I did a small test of subclassing DefaultSolrHighlighter and overriding doHighlightingByFastVectorHighlighter , accessing fvhContainer.fieldQuery and creating a new FvhContainer . The only problem I saw was that IntelliJ did not like my subclass accessing fvhContainer.fieldQuery . It says Cannot access org.apache.lucene.search.vectorhighlight.FieldQuery . However, compiling with ant works. Gethin James do you have a chance to verify that the proposed patch solves your concerns?
        Hide
        covolution Gethin James added a comment -

        I am currently on vacation but, from memory, I believe I just need a constructor to pass in the 2 objects to create a new Fvhcontainer. I don't need to access the fields after.

        Show
        covolution Gethin James added a comment - I am currently on vacation but, from memory, I believe I just need a constructor to pass in the 2 objects to create a new Fvhcontainer. I don't need to access the fields after.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 319f89210ac29c53828bd16e1a77e01cd08c164b in lucene-solr's branch refs/heads/branch_6x from Jan Høydahl
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=319f892 ]

        SOLR-9237: DefaultSolrHighlighter.doHighlightingByFastVectorHighlighter can't be overidden
        (cherry picked from commit 7eb7702)

        Show
        jira-bot ASF subversion and git services added a comment - Commit 319f89210ac29c53828bd16e1a77e01cd08c164b in lucene-solr's branch refs/heads/branch_6x from Jan Høydahl [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=319f892 ] SOLR-9237 : DefaultSolrHighlighter.doHighlightingByFastVectorHighlighter can't be overidden (cherry picked from commit 7eb7702)
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 7eb77027bb322a562bca17d23323f95ce58dd9d6 in lucene-solr's branch refs/heads/master from Jan Høydahl
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7eb7702 ]

        SOLR-9237: DefaultSolrHighlighter.doHighlightingByFastVectorHighlighter can't be overidden

        Show
        jira-bot ASF subversion and git services added a comment - Commit 7eb77027bb322a562bca17d23323f95ce58dd9d6 in lucene-solr's branch refs/heads/master from Jan Høydahl [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7eb7702 ] SOLR-9237 : DefaultSolrHighlighter.doHighlightingByFastVectorHighlighter can't be overidden
        Hide
        covolution Gethin James added a comment -

        I just got a chance to validate the issue. Can you make the constructor public please? I can't use it outside the package.

        Show
        covolution Gethin James added a comment - I just got a chance to validate the issue. Can you make the constructor public please? I can't use it outside the package.
        Hide
        janhoy Jan Høydahl added a comment -

        Have you validated that this is the only necessary change?

        Show
        janhoy Jan Høydahl added a comment - Have you validated that this is the only necessary change?
        Hide
        covolution Gethin James added a comment -

        It needs to be public. This should work!

          public class FvhContainer {
            FastVectorHighlighter fvh;
            FieldQuery fieldQuery;
        
            public FvhContainer(FastVectorHighlighter fvh, FieldQuery fieldQuery) {
              this.fvh = fvh;
              this.fieldQuery = fieldQuery;
            }
          }
        
        Show
        covolution Gethin James added a comment - It needs to be public. This should work! public class FvhContainer { FastVectorHighlighter fvh; FieldQuery fieldQuery; public FvhContainer(FastVectorHighlighter fvh, FieldQuery fieldQuery) { this .fvh = fvh; this .fieldQuery = fieldQuery; } }
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 81c23f1c5b26c90ac4b060dcb687d41fcee76a98 in lucene-solr's branch refs/heads/branch_6x from Jan Høydahl
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=81c23f1 ]

        SOLR-9237: Make FvhContainer public
        (cherry picked from commit 4f45226)

        Show
        jira-bot ASF subversion and git services added a comment - Commit 81c23f1c5b26c90ac4b060dcb687d41fcee76a98 in lucene-solr's branch refs/heads/branch_6x from Jan Høydahl [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=81c23f1 ] SOLR-9237 : Make FvhContainer public (cherry picked from commit 4f45226)
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 4f45226174c4f1cdd5364b044b5d7ee6c2001522 in lucene-solr's branch refs/heads/master from Jan Høydahl
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=4f45226 ]

        SOLR-9237: Make FvhContainer public

        Show
        jira-bot ASF subversion and git services added a comment - Commit 4f45226174c4f1cdd5364b044b5d7ee6c2001522 in lucene-solr's branch refs/heads/master from Jan Høydahl [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=4f45226 ] SOLR-9237 : Make FvhContainer public
        Hide
        janhoy Jan Høydahl added a comment -

        This should do the trick

        Show
        janhoy Jan Høydahl added a comment - This should do the trick
        Hide
        mikemccand Michael McCandless added a comment -

        Bulk close resolved issues after 6.2.0 release.

        Show
        mikemccand Michael McCandless added a comment - Bulk close resolved issues after 6.2.0 release.

          People

          • Assignee:
            janhoy Jan Høydahl
            Reporter:
            covolution Gethin James
          • Votes:
            0 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development