Lucene - Core
  1. Lucene - Core
  2. LUCENE-2013

QueryScorer and SpanRegexQuery are incompatible.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.9
    • Fix Version/s: 2.9.1, 3.0
    • Component/s: modules/highlighter
    • Labels:
      None
    • Environment:

      Lucene-Java 2.9

    • Lucene Fields:
      New, Patch Available

      Description

      Since the resolution of #LUCENE-1685, users are not supposed to rewrite their queries before submitting them to QueryScorer:

      ------------------------------------------------------------------------

      r800796 | markrmiller | 2009-08-04 06:56:11 -0700 (Tue, 04 Aug 2009) | 1 line

      LUCENE-1685: The position aware SpanScorer has become the default scorer for Highlighting. The SpanScorer implementation has replaced QueryScorer and the old term highlighting QueryScorer has been renamed to QueryTermScorer. Multi-term queries are also now expanded by default. If you were previously rewritting the query for multi-term query highlighting, you should no longer do that (unless you switch to using QueryTermScorer). The SpanScorer API (now QueryScorer) has also been improved to more closely match the API of the previous QueryScorer implementation.

      ------------------------------------------------------------------------

      This is a great convenience for the most part, but it's causing me difficulties with SpanRegexQuerys, as the WeightedSpanTermExtractor uses Query.extractTerms() to collect the fields used in the query, but SpanRegexQuery does not implement this method, so highlighting any query with a SpanRegexQuery throws an UnsupportedOpertationException. If this issue is circumvented, there is still the issue of SpanRegexQuery throwing an exception when someone calls its getSpans() method.

      I can provide the patch that I am currently using, but I'm not sure that my solution is optimal. It adds two methods to SpanQuery: extractFields(Set<String> fields) which is equivalent to fields.add(getField()) except when MaskedFieldQuerys get involved, and mustBeRewrittenToGetSpans() which returns true for SpanQuery, false for SpanTermQuery, and is overridden in each composite SpanQuery to return a value depending on its components. In this way SpanRegexQuery (and any other custom SpanQuerys) do not need to be adjusted.

      Currently the collection of fields and non-weighted terms are done in a single step. In the proposed patch the WeightedSpanTerm extraction from a SpanQuery proceeds in two steps. First, if the QueryScorer's field is null, then the fields are collected from the SpanQuery using the extractFields() method. Second the terms are collected using extractTerms(), rewriting the query for each field if mustBeRewrittenToGetSpans() returns true.

      1. LUCENE-2013.patch
        5 kB
        Mark Miller
      2. lucene-2013-2009-10-28.patch
        7 kB
        Benjamin Keil
      3. lucene-2013-2009-10-28-2135.patch
        7 kB
        Benjamin Keil
      4. lucene-2013-2009-10-29-0136.patch
        7 kB
        Benjamin Keil

        Issue Links

          Activity

          Hide
          Benjamin Keil added a comment -

          Patch for LUCENE-2013

          Show
          Benjamin Keil added a comment - Patch for LUCENE-2013
          Hide
          Benjamin Keil added a comment -

          Resolving LUCENE-1685 has caused some difficulties for non-core queries in contrib-regex and in client code.

          Show
          Benjamin Keil added a comment - Resolving LUCENE-1685 has caused some difficulties for non-core queries in contrib-regex and in client code.
          Hide
          Benjamin Keil added a comment -

          This patch does a better job of collecting fields from the queries. The previous patch only had the desired result if there were no FieldMaskingSpanQueries, or if there was just one and it was at the very root of the hierarchy.

          Show
          Benjamin Keil added a comment - This patch does a better job of collecting fields from the queries. The previous patch only had the desired result if there were no FieldMaskingSpanQueries, or if there was just one and it was at the very root of the hierarchy.
          Hide
          Benjamin Keil added a comment -

          Removed failed attempts at monospace font in description.

          Show
          Benjamin Keil added a comment - Removed failed attempts at monospace font in description.
          Hide
          Mark Miller added a comment -

          Thanks for the report Benjamin -

          Not sure I like adding the methods to SpanQuerys though -

          how about putting a check for regex query before the check for spanquery, and rewriting if we see it? It means adding the contrib with regex as a dependency of the highlighter, but it lets us avoid modifying any core classes.

          Show
          Mark Miller added a comment - Thanks for the report Benjamin - Not sure I like adding the methods to SpanQuerys though - how about putting a check for regex query before the check for spanquery, and rewriting if we see it? It means adding the contrib with regex as a dependency of the highlighter, but it lets us avoid modifying any core classes.
          Hide
          Mark Miller added a comment -

          Here is what I would propose - with additional handling of regular regex queries if needed (havn't check yet).

          Show
          Mark Miller added a comment - Here is what I would propose - with additional handling of regular regex queries if needed (havn't check yet).
          Hide
          Benjamin Keil added a comment -

          The problem with that solution is that it doesn't handle any kind of nested span queries. Let's update the test case to use this query:

          public void testSpanRegexQuery() throws Exception {
          query = new SpanOrQuery(new SpanQuery []

          { new SpanRegexQuery(new Term(FIELD_NAME, "ken.*")) }

          );

          I've attached a counter-counter-proposal that does not add SpanRegexQuery as a dependency of WeightedSpanTermExtractor (although the test-case still needs it ... does the build have a concept of test-only dependencies?).

          It doesn't add any methods to the core classes, but this also means that no third-party queries have any way to influence their highlighting.

          Show
          Benjamin Keil added a comment - The problem with that solution is that it doesn't handle any kind of nested span queries. Let's update the test case to use this query: public void testSpanRegexQuery() throws Exception { query = new SpanOrQuery(new SpanQuery [] { new SpanRegexQuery(new Term(FIELD_NAME, "ken.*")) } ); I've attached a counter-counter-proposal that does not add SpanRegexQuery as a dependency of WeightedSpanTermExtractor (although the test-case still needs it ... does the build have a concept of test-only dependencies?). It doesn't add any methods to the core classes, but this also means that no third-party queries have any way to influence their highlighting.
          Hide
          Mark Miller added a comment -

          Nice catch - I think I like this method better than the core modifications.

          but this also means that no third-party queries have any way to influence their highlighting.

          Unfortunately, I think thats already the deal in many cases. The Highlighter is very special case - ugly, but the current state of things. We will hopefully get away from that eventually.

          Show
          Mark Miller added a comment - Nice catch - I think I like this method better than the core modifications. but this also means that no third-party queries have any way to influence their highlighting. Unfortunately, I think thats already the deal in many cases. The Highlighter is very special case - ugly, but the current state of things. We will hopefully get away from that eventually.
          Hide
          Mark Miller added a comment -

          Thanks Benjamin!

          Show
          Mark Miller added a comment - Thanks Benjamin!
          Hide
          Benjamin Keil added a comment -

          Awesome, Mark. Thank you!

          Show
          Benjamin Keil added a comment - Awesome, Mark. Thank you!
          Hide
          Mark Miller added a comment -

          No problem - and we can refine if we need to for the next release - I plopped it in now to make sure at least this fix gets into 2.9.1

          Show
          Mark Miller added a comment - No problem - and we can refine if we need to for the next release - I plopped it in now to make sure at least this fix gets into 2.9.1
          Hide
          Mark Miller added a comment - - edited

          bug alert - I screwed up the backport

          Show
          Mark Miller added a comment - - edited bug alert - I screwed up the backport
          Hide
          Michael McCandless added a comment -

          Mark, does this require a 2.9.1 respin?

          Show
          Michael McCandless added a comment - Mark, does this require a 2.9.1 respin?
          Hide
          Mark Miller added a comment - - edited

          Umm - yes. Unless we want to draw a line. Its worse than the bug we fixed. Very sorry - entirely my fault.

          Show
          Mark Miller added a comment - - edited Umm - yes. Unless we want to draw a line. Its worse than the bug we fixed. Very sorry - entirely my fault.
          Hide
          Michael McCandless added a comment -

          OK, no problem, I'll respin!

          Show
          Michael McCandless added a comment - OK, no problem, I'll respin!
          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:
              Benjamin Keil
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development