Lucene - Core
  1. Lucene - Core
  2. LUCENE-4934

AssertingIndexSearcher should do basic QueryUtils/etc checks on every query

    Details

    • Type: Test Test
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      We can start with QueryUtils.check(query): which does some basic hashcode/equals checks.

      Ideally we'd strengthen the checks as we fix problems: e.g. add explanations verifications (checkExplanations) and then finally the more intense check() that does more verifications with deleted docs/next/advance.

      1. LUCENE-4934.patch
        16 kB
        Robert Muir
      2. LUCENE-4934.patch
        10 kB
        Robert Muir
      3. LUCENE-4934.patch
        6 kB
        Robert Muir
      4. LUCENE-4934.patch
        5 kB
        Robert Muir

        Activity

        Hide
        Adrien Grand added a comment -

        +1

        Show
        Adrien Grand added a comment - +1
        Hide
        Robert Muir added a comment -

        Here's a start just doing the simplest check. I am currently running tests fixing the bugs this one found... I'm gonna see if we can do the explanations test though (at some point we will hit bugs in spans/payloads like LUCENE-4219)

        Show
        Robert Muir added a comment - Here's a start just doing the simplest check. I am currently running tests fixing the bugs this one found... I'm gonna see if we can do the explanations test though (at some point we will hit bugs in spans/payloads like LUCENE-4219 )
        Hide
        Robert Muir added a comment -

        There are hashcode/equals bugs in DrillDownQuery

        Show
        Robert Muir added a comment - There are hashcode/equals bugs in DrillDownQuery
        Hide
        Robert Muir added a comment -

        updated patch: now the next bug to fix is hashcode/equals in DrillSidewaysQuery.

        I don't know why it throws UOE today... we can't do stuff like this.

        Show
        Robert Muir added a comment - updated patch: now the next bug to fix is hashcode/equals in DrillSidewaysQuery. I don't know why it throws UOE today... we can't do stuff like this.
        Hide
        Robert Muir added a comment -

        updated patch: fixes some more equals/hashcode bugs.

        I also implemented the assert differently: we check both before rewrite() and after.

        Show
        Robert Muir added a comment - updated patch: fixes some more equals/hashcode bugs. I also implemented the assert differently: we check both before rewrite() and after.
        Hide
        Robert Muir added a comment -

        3 more queries with bugs i still havent fixed:

        [junit4:junit4] Tests with failures:
        [junit4:junit4] - org.apache.lucene.queryparser.xml.TestParser.testLikeThisQueryXML
        [junit4:junit4] - org.apache.lucene.queryparser.xml.TestParser.testBoostingQueryXML
        [junit4:junit4] - org.apache.lucene.queryparser.xml.TestParser.testFuzzyLikeThisQueryXML

        Its a little disturbing that tests in lucene/queries arent finding these problems, only a queryparser test.

        Maybe they arent using newSearcher.

        Show
        Robert Muir added a comment - 3 more queries with bugs i still havent fixed: [junit4:junit4] Tests with failures: [junit4:junit4] - org.apache.lucene.queryparser.xml.TestParser.testLikeThisQueryXML [junit4:junit4] - org.apache.lucene.queryparser.xml.TestParser.testBoostingQueryXML [junit4:junit4] - org.apache.lucene.queryparser.xml.TestParser.testFuzzyLikeThisQueryXML Its a little disturbing that tests in lucene/queries arent finding these problems, only a queryparser test. Maybe they arent using newSearcher.
        Hide
        Uwe Schindler added a comment -

        +1, I also like the Term.toString() method We may use a similar approach in Solr's AnalysisRequestHandler when completely binary terms are generated (currently it prints the raw binary term and the string rep next to each other). If the latter fails, it should only return the binary term in the NämedList.

        Show
        Uwe Schindler added a comment - +1, I also like the Term.toString() method We may use a similar approach in Solr's AnalysisRequestHandler when completely binary terms are generated (currently it prints the raw binary term and the string rep next to each other). If the latter fails, it should only return the binary term in the NämedList.
        Hide
        Robert Muir added a comment -

        OK updated patch. I think i fixed the big issues in all these equals/hashcodes.

        BlockJoin should be revisited (in another issue: please). Its doing complicated stuff like equals based on unrewritten-query, I think this is wrong (e.g. in the case of a MTQ query rewritten from anotehr reader). But i dont want to fix this here.

        I want to commit this to make some progress and then look at trying to improve the check (e.g. explains would be a nice step)

        Show
        Robert Muir added a comment - OK updated patch. I think i fixed the big issues in all these equals/hashcodes. BlockJoin should be revisited (in another issue: please). Its doing complicated stuff like equals based on unrewritten-query, I think this is wrong (e.g. in the case of a MTQ query rewritten from anotehr reader). But i dont want to fix this here. I want to commit this to make some progress and then look at trying to improve the check (e.g. explains would be a nice step)
        Hide
        Michael McCandless added a comment -

        +1

        Show
        Michael McCandless added a comment - +1
        Hide
        Uwe Schindler added a comment -

        I also implemented the assert differently: we check both before rewrite() and after.

        Hey, that was my idea to move that into IS.rewrite()

        Show
        Uwe Schindler added a comment - I also implemented the assert differently: we check both before rewrite() and after. Hey, that was my idea to move that into IS.rewrite()

          People

          • Assignee:
            Unassigned
            Reporter:
            Robert Muir
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:

              Development