Solr
  1. Solr
  2. SOLR-1319

Upgrade custom Solr Highlighter classes to new Lucene Highlighter API

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.4
    • Component/s: highlighter
    • Labels:
      None

      Issue Links

        Activity

        Hide
        Grant Ingersoll added a comment -

        Bulk close for Solr 1.4

        Show
        Grant Ingersoll added a comment - Bulk close for Solr 1.4
        Hide
        Mark Miller added a comment -

        Well, I've got a patch for this:

        Changes entry and some cleanup around the break

        But currently JIRA is completely whacked - so I'll attach when its back.

        Show
        Mark Miller added a comment - Well, I've got a patch for this: Changes entry and some cleanup around the break But currently JIRA is completely whacked - so I'll attach when its back.
        Hide
        Mark Miller added a comment -

        Right - I don't think its a big deal either. But the Highlighter framework has things like protected methods that are not overridden internally - seems to suggest that users could/might override them - whether we know they do or not, I think thats worth a mention in Changes on the break. Certainly easy enough to do anyway.

        The break itself is even less likely to be an issue than a custom highlighter in general - I like the completeness myself though. Its an extendable public class and Solr allows for plugins.

        Show
        Mark Miller added a comment - Right - I don't think its a big deal either. But the Highlighter framework has things like protected methods that are not overridden internally - seems to suggest that users could/might override them - whether we know they do or not, I think thats worth a mention in Changes on the break. Certainly easy enough to do anyway. The break itself is even less likely to be an issue than a custom highlighter in general - I like the completeness myself though. Its an extendable public class and Solr allows for plugins.
        Hide
        Yonik Seeley added a comment -

        if I'm reading this correctly, the back compat break is for those providing their own custom highlighter? That's gotta be almost no one... doesn't seem like a big deal as it seems more like internal implementation than interface.

        Show
        Yonik Seeley added a comment - if I'm reading this correctly, the back compat break is for those providing their own custom highlighter? That's gotta be almost no one... doesn't seem like a big deal as it seems more like internal implementation than interface.
        Hide
        Mark Miller added a comment -

        Given there is no patch, should this be pushed to 1.5?

        Nope, the actual work was part of another issue - the only reason this is still open was because there was a back compat break (due to a back compat break in Lucene's Highlighter, which doesn't promise back compat). So this is open as a marker to somehow deal with the break in Solr -

        Our options are fairly limited - basically, I think it means adding a notice in Changes about the whole affair. I'll try and work something up.

        Show
        Mark Miller added a comment - Given there is no patch, should this be pushed to 1.5? Nope, the actual work was part of another issue - the only reason this is still open was because there was a back compat break (due to a back compat break in Lucene's Highlighter, which doesn't promise back compat). So this is open as a marker to somehow deal with the break in Solr - Our options are fairly limited - basically, I think it means adding a notice in Changes about the whole affair. I'll try and work something up.
        Hide
        Grant Ingersoll added a comment -

        Given there is no patch, should this be pushed to 1.5?

        Show
        Grant Ingersoll added a comment - Given there is no patch, should this be pushed to 1.5?
        Hide
        Mark Miller added a comment -

        The break is that

        protected QueryTermScorer getQueryScorer(Query query, String fieldName, SolrQueryRequest request) 

        now has a different return type. Its too bad it wasnt done the same way as the SpanScorer - there you can just override the getSpanHighlighter call, but getting the Scorer is private.

        So its a break, though no one was overriding it likely - we should almost make it private - you can anything you need to overriding the getHighlighter call. At least, I should have changed it to just Scorer rather than QueryTermScorer.

        Show
        Mark Miller added a comment - The break is that protected QueryTermScorer getQueryScorer(Query query, String fieldName, SolrQueryRequest request) now has a different return type. Its too bad it wasnt done the same way as the SpanScorer - there you can just override the getSpanHighlighter call, but getting the Scorer is private. So its a break, though no one was overriding it likely - we should almost make it private - you can anything you need to overriding the getHighlighter call. At least, I should have changed it to just Scorer rather than QueryTermScorer.
        Hide
        Mark Miller added a comment -

        We should probably be careful here in the future, and document anything thats based on code in Lucene without a backcompat policy to have similar looseness in Solr - or hide the Lucene implementation from the Solr public API's.

        Show
        Mark Miller added a comment - We should probably be careful here in the future, and document anything thats based on code in Lucene without a backcompat policy to have similar looseness in Solr - or hide the Lucene implementation from the Solr public API's.
        Hide
        Mark Miller added a comment -

        Bah - I really gave myself a headache here. Lucene Highlighter broke back compat - which ripples here a bit. I think the impact can be mitigated if we switch to using the PhraseHighlighter by default (SOLR-1221) though.

        Show
        Mark Miller added a comment - Bah - I really gave myself a headache here. Lucene Highlighter broke back compat - which ripples here a bit. I think the impact can be mitigated if we switch to using the PhraseHighlighter by default ( SOLR-1221 ) though.
        Hide
        Mark Miller added a comment -

        the regex fragementer has to be updated (got a patch for that) and some work has to be done now that SpanScorer is gone, and the semantics/syntax for it is a bit different.

        Prob makes sense to do SOLR-1221 with this one.

        Show
        Mark Miller added a comment - the regex fragementer has to be updated (got a patch for that) and some work has to be done now that SpanScorer is gone, and the semantics/syntax for it is a bit different. Prob makes sense to do SOLR-1221 with this one.

          People

          • Assignee:
            Mark Miller
            Reporter:
            Mark Miller
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development