Lucene - Core
  1. Lucene - Core
  2. LUCENE-1929

Highlighter doesn't support NumericRangeQuery or deprecated RangeQuery

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.9
    • Fix Version/s: 2.9.1
    • Component/s: modules/highlighter
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      Sucks. Will throw a NullPointer exception.

      Only NumericRangeQuery will throw the exception.
      RangeQuery just won't highlight.

        Issue Links

          Activity

          Hide
          Mark Miller added a comment -

          So making NumericQuery Hilightable is not going to be so quick and easy - best first step is prob just make it be skipped, rather than throwing a NullPointer exception. Highlight support can come later.

          Show
          Mark Miller added a comment - So making NumericQuery Hilightable is not going to be so quick and easy - best first step is prob just make it be skipped, rather than throwing a NullPointer exception. Highlight support can come later.
          Hide
          Uwe Schindler added a comment -

          Does it throw a NPE for every "unknown" query type? Or is this because of it is a subclass of MTQ?

          Show
          Uwe Schindler added a comment - Does it throw a NPE for every "unknown" query type? Or is this because of it is a subclass of MTQ?
          Hide
          Uwe Schindler added a comment -

          I read what stands in SOLR-1221: It calls MTQ.getTerm() which is deprecated. The getTerm() method is moved downto the sub classes, so MTQ.getTerm() will be removed in 3.0. Because of this there was no intention to implement it in NumericRQ.

          Show
          Uwe Schindler added a comment - I read what stands in SOLR-1221 : It calls MTQ.getTerm() which is deprecated. The getTerm() method is moved downto the sub classes, so MTQ.getTerm() will be removed in 3.0. Because of this there was no intention to implement it in NumericRQ.
          Hide
          Mark Miller added a comment -

          Yeah - eventually (as in when getTerm() is removed), the Highlighter was going to have to deal with that anyway. Deprecated methods should still work though - so whether return null from getTerm is right or wrong, I dunno.

          But thats the issue - except for TermRangeQuery, the Highlighter looks for the Field by calling getTerm and asking for the field - made the exception for TermRangeQuery, but not NumericRangeQuery - so it throw a nullpointer exception. Easy to sidestep in Lucene, more of a pain in Solr (as the plan is to release with 2.9.0)

          Show
          Mark Miller added a comment - Yeah - eventually (as in when getTerm() is removed), the Highlighter was going to have to deal with that anyway. Deprecated methods should still work though - so whether return null from getTerm is right or wrong, I dunno. But thats the issue - except for TermRangeQuery, the Highlighter looks for the Field by calling getTerm and asking for the field - made the exception for TermRangeQuery, but not NumericRangeQuery - so it throw a nullpointer exception. Easy to sidestep in Lucene, more of a pain in Solr (as the plan is to release with 2.9.0)
          Hide
          Mark Miller added a comment -

          so whether return null from getTerm is right or wrong, I dunno.

          In fact, its probably wrong - but at this point its a moot point.

          Show
          Mark Miller added a comment - so whether return null from getTerm is right or wrong, I dunno. In fact, its probably wrong - but at this point its a moot point.
          Hide
          Uwe Schindler added a comment -

          In fact, its probably wrong - but at this point its a moot point.

          It can only return a "pattern term" if there is one. What is the pattern term for TermRangeQuery or NumericRangeQuery. Both return null and that is fine (and not a backwards break, as both classes are new in 2.9). That was exactly the discussion behind removing the getTerm() stuff from MTQ.

          Solr could maybe remove the NRQ from the highligter query.

          Show
          Uwe Schindler added a comment - In fact, its probably wrong - but at this point its a moot point. It can only return a "pattern term" if there is one. What is the pattern term for TermRangeQuery or NumericRangeQuery. Both return null and that is fine (and not a backwards break, as both classes are new in 2.9). That was exactly the discussion behind removing the getTerm() stuff from MTQ. Solr could maybe remove the NRQ from the highligter query.
          Hide
          Mark Miller added a comment -

          I realize the difficulties - I wasn't blaming you for the problem - but it is/was a problem.

          We deprecated getTerm() because of the problem (I think I did it? Don't remember though).

          However, unless it was documented to return null, just deprecating it doesn't really fix things. Code thats trying to use it on the base MultiTermQuery isn't going
          to be checking if its null unless its documented as such.

          I agree its screwed and it will be nice when its gone.

          Like I said, its a moot point. But I'll argue any point, moot or not

          Show
          Mark Miller added a comment - I realize the difficulties - I wasn't blaming you for the problem - but it is/was a problem. We deprecated getTerm() because of the problem (I think I did it? Don't remember though). However, unless it was documented to return null, just deprecating it doesn't really fix things. Code thats trying to use it on the base MultiTermQuery isn't going to be checking if its null unless its documented as such. I agree its screwed and it will be nice when its gone. Like I said, its a moot point. But I'll argue any point, moot or not
          Hide
          Mark Miller added a comment -

          and not a backwards break, as both classes are new in 2.9

          I didn't claim its back compat break - but any method on a base class than can return null really needs to be documented as such. I should have caught that and done a better doc job on MulitTermQuery. I'm not claiming back compat issues, I'm claiming its wrong.

          On a side note, its a bummer we seem to have lost the history on RangeQuery.

          Show
          Mark Miller added a comment - and not a backwards break, as both classes are new in 2.9 I didn't claim its back compat break - but any method on a base class than can return null really needs to be documented as such. I should have caught that and done a better doc job on MulitTermQuery. I'm not claiming back compat issues, I'm claiming its wrong. On a side note, its a bummer we seem to have lost the history on RangeQuery.
          Hide
          Uwe Schindler added a comment -

          On a side note, its a bummer we seem to have lost the history on RangeQuery.

          No we didn't. As RangeQuery is just a wrapper and will be removed in 3.0, I renamed the old RangeQuery to the new TermRangeQuery (so the history is there) and created a new class RangeQuery for the BW compatibility. This is better than loosing the history in 3.0.

          Show
          Uwe Schindler added a comment - On a side note, its a bummer we seem to have lost the history on RangeQuery. No we didn't. As RangeQuery is just a wrapper and will be removed in 3.0, I renamed the old RangeQuery to the new TermRangeQuery (so the history is there) and created a new class RangeQuery for the BW compatibility. This is better than loosing the history in 3.0.
          Hide
          Mark Miller added a comment -

          Ah, thanks - didn't catch that. Wondered how it could happen - looked weird. Faint memory of the whole thing returning though.

          Show
          Mark Miller added a comment - Ah, thanks - didn't catch that. Wondered how it could happen - looked weird. Faint memory of the whole thing returning though.
          Hide
          Mark Miller added a comment -

          I wasn't blaming you for the problem

          In fact, now that I can see the history to remember, if you go back and look, I am completely to blame

          Show
          Mark Miller added a comment - I wasn't blaming you for the problem In fact, now that I can see the history to remember, if you go back and look, I am completely to blame
          Hide
          Uwe Schindler added a comment -

          Another option to fix Solr's highlighter is to fix this in lucene and release solr with 2.9.0, but an updated highlighter package. As it is contrib, that shouldn't be a problem?

          In my opinion, the whole highlighter stuff should somehow be implemented without tons of instanceof checks (thats really wired). Maybe we extend Query by some highligther addons in future. I think we had an issue about that?

          Show
          Uwe Schindler added a comment - Another option to fix Solr's highlighter is to fix this in lucene and release solr with 2.9.0, but an updated highlighter package. As it is contrib, that shouldn't be a problem? In my opinion, the whole highlighter stuff should somehow be implemented without tons of instanceof checks (thats really wired). Maybe we extend Query by some highligther addons in future. I think we had an issue about that?
          Hide
          Mark Miller added a comment -

          Another option to fix Solr's highlighter is to fix this in lucene and release solr with 2.9.0, but an updated highlighter package. As it is contrib, that shouldn't be a problem?

          Thats a good point and prob the way to go.

          In my opinion, the whole highlighter stuff should somehow be implemented without tons of instanceof checks (thats really wired). Maybe we extend Query by some highligther addons in future. I think we had an issue about that?

          Yes - would be wonderful to lose all of the instanceOf - its just kind of legacy - how its worked in the past. Getting away from it would be fantastic.

          Show
          Mark Miller added a comment - Another option to fix Solr's highlighter is to fix this in lucene and release solr with 2.9.0, but an updated highlighter package. As it is contrib, that shouldn't be a problem? Thats a good point and prob the way to go. In my opinion, the whole highlighter stuff should somehow be implemented without tons of instanceof checks (thats really wired). Maybe we extend Query by some highligther addons in future. I think we had an issue about that? Yes - would be wonderful to lose all of the instanceOf - its just kind of legacy - how its worked in the past. Getting away from it would be fantastic .
          Hide
          Mark Miller added a comment -

          The 2.9.1 fix

          Show
          Mark Miller added a comment - The 2.9.1 fix
          Hide
          Uwe Schindler added a comment - - edited

          This is fixed also in trunk, but different where MTQ.getTerm() is not available (LUCENE-1977)

          Show
          Uwe Schindler added a comment - - edited This is fixed also in trunk, but different where MTQ.getTerm() is not available ( LUCENE-1977 )
          Hide
          Michael McCandless added a comment -

          Mark is this one reading to go into 2.9.1?

          Show
          Michael McCandless added a comment - Mark is this one reading to go into 2.9.1?
          Hide
          Mark Miller added a comment -

          Yeah - sorry - has been for some time. I can commit it shortly.

          Show
          Mark Miller added a comment - Yeah - sorry - has been for some time. I can commit it shortly.
          Hide
          Michael McCandless added a comment -

          Bulk close all 2.9.1 issues.

          Show
          Michael McCandless added a comment - Bulk close all 2.9.1 issues.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development