Lucene - Core
  1. Lucene - Core
  2. LUCENE-1938

Precedence query parser using the contrib/queryparser framework

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.9
    • Fix Version/s: 3.1, 4.0-ALPHA
    • Component/s: modules/other
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      Extend the current StandardQueryParser on contrib so it supports boolean precedence

      1. LUCENE-1938.patch
        188 kB
        Robert Muir
      2. LUCENE-1938.patch
        43 kB
        Adriano Crestani

        Issue Links

          Activity

          Hide
          Adriano Crestani added a comment -

          This patch contains a Precedence query parser. It's basically an extension of the current Standard query parser, but enables the boolean precedence.

          The patch also contains the test cases from the current PrecedenceQueryParser, currently located under contrib/misc. Everything passes fine, except a test that tests if the query <A B C> is the same as <A OR B C>, when the default operator is OR. The problem is that this new PQP does not flat/optimize the boolean tree when the boolean operator is the same. It can be performed later, but for now it does not implement this kind of optimization.

          Another issue is that I created a class under contrib/queryparser called PrecedenceQueryParser within the same package declaration the PQP from contrib/misc is right now. Is that a big issue? Couldn't the old PQP be removed and we start using this one, so we have no class conflict.

          The greatest advantage of this implementation is that it basically reuses all the StandardQueryParser functiontalities and syntax, and any modification on the StandardQP will be automatically applied to the PrecedenceQP. No need to maintain 2 different implementations with the same code, as the old PQP does today.

          Show
          Adriano Crestani added a comment - This patch contains a Precedence query parser. It's basically an extension of the current Standard query parser, but enables the boolean precedence. The patch also contains the test cases from the current PrecedenceQueryParser, currently located under contrib/misc. Everything passes fine, except a test that tests if the query <A B C> is the same as <A OR B C>, when the default operator is OR. The problem is that this new PQP does not flat/optimize the boolean tree when the boolean operator is the same. It can be performed later, but for now it does not implement this kind of optimization. Another issue is that I created a class under contrib/queryparser called PrecedenceQueryParser within the same package declaration the PQP from contrib/misc is right now. Is that a big issue? Couldn't the old PQP be removed and we start using this one, so we have no class conflict. The greatest advantage of this implementation is that it basically reuses all the StandardQueryParser functiontalities and syntax, and any modification on the StandardQP will be automatically applied to the PrecedenceQP. No need to maintain 2 different implementations with the same code, as the old PQP does today.
          Hide
          Adriano Crestani added a comment -

          The patch uses some code from LUCENE-1937, so LUCENE-1937 needs to be applied before.

          Show
          Adriano Crestani added a comment - The patch uses some code from LUCENE-1937 , so LUCENE-1937 needs to be applied before.
          Hide
          Erik Hatcher added a comment -

          Yes, let's just remove the old PrecedenceQueryParser (which was just an experiment by me - is anyone actually using it?)

          Show
          Erik Hatcher added a comment - Yes, let's just remove the old PrecedenceQueryParser (which was just an experiment by me - is anyone actually using it?)
          Hide
          Luis Alves added a comment -

          Hi,

          I looked at the pipeline PrecedenceQueryNodeProcessorPipeline, it contains some complex logic off removing a processor from the StandardQueryNodeProcessorPipeline.

          I think it would be better to have a common ancestor, CommonQueryNodeProcessorPipeline, that both StandardQueryNodeProcessorPipeline and PrecedenceQueryNodeProcessorPipeline can extend.
          It should make it easier for people reading the code.
          What do you think?

          Show
          Luis Alves added a comment - Hi, I looked at the pipeline PrecedenceQueryNodeProcessorPipeline, it contains some complex logic off removing a processor from the StandardQueryNodeProcessorPipeline. I think it would be better to have a common ancestor, CommonQueryNodeProcessorPipeline, that both StandardQueryNodeProcessorPipeline and PrecedenceQueryNodeProcessorPipeline can extend. It should make it easier for people reading the code. What do you think?
          Hide
          Luis Alves added a comment - - edited

          Hi Erik,

          Should we remove the PrecedenceQueryParser on 3.0, or wait untill 3.1?

          Show
          Luis Alves added a comment - - edited Hi Erik, Should we remove the PrecedenceQueryParser on 3.0, or wait untill 3.1?
          Hide
          Adriano Crestani added a comment -

          I looked at the pipeline PrecedenceQueryNodeProcessorPipeline, it contains some complex logic off removing a processor from the StandardQueryNodeProcessorPipeline.

          I think it would be better to have a common ancestor, CommonQueryNodeProcessorPipeline, that both StandardQueryNodeProcessorPipeline and PrecedenceQueryNodeProcessorPipeline can extend.

          I don't think it's so complex, it just removes GroupQueryNodeProcessor and adds ModifierQueryNodeProcessor (I think this is the class name). I also added this description to the javadocs.

          Anyway, I would prefer to leave it like that, extending StandardQPP, instead of creating a common pipeline. The common pipeline would not make much sense right now, there is not QueryBuilder impl that would produce a correct Query instance from a query tree processed by it, so no one could just use it.

          In future, we could have a third QP implementation, that instead of replacing GroupQueryNodeProcessor by ModifierQueryNodeProcessor, it would replace another processor, or switch the processors position, etc... the CommonQueryNodeProcessorPipeline would not make sense anymore, it only make sense right now for PrecedenceQP and StandardQP, because we know what both have in common, but doesn't mean a third QP would have the same in common.

          So, +1 to leave it the way it's.

          Show
          Adriano Crestani added a comment - I looked at the pipeline PrecedenceQueryNodeProcessorPipeline, it contains some complex logic off removing a processor from the StandardQueryNodeProcessorPipeline. I think it would be better to have a common ancestor, CommonQueryNodeProcessorPipeline, that both StandardQueryNodeProcessorPipeline and PrecedenceQueryNodeProcessorPipeline can extend. I don't think it's so complex, it just removes GroupQueryNodeProcessor and adds ModifierQueryNodeProcessor (I think this is the class name). I also added this description to the javadocs. Anyway, I would prefer to leave it like that, extending StandardQPP, instead of creating a common pipeline. The common pipeline would not make much sense right now, there is not QueryBuilder impl that would produce a correct Query instance from a query tree processed by it, so no one could just use it. In future, we could have a third QP implementation, that instead of replacing GroupQueryNodeProcessor by ModifierQueryNodeProcessor, it would replace another processor, or switch the processors position, etc... the CommonQueryNodeProcessorPipeline would not make sense anymore, it only make sense right now for PrecedenceQP and StandardQP, because we know what both have in common, but doesn't mean a third QP would have the same in common. So, +1 to leave it the way it's.
          Hide
          Adriano Crestani added a comment -

          This patch could also be applied to branch_3x

          Show
          Adriano Crestani added a comment - This patch could also be applied to branch_3x
          Hide
          Robert Muir added a comment -

          attached is a patch that replaces the PrecedenceQueryParser with this implementation
          I didnt change any functionality, just brought it up to trunk and cleared javadocs errors and other minor things

          I think this is much easier to maintain... the patch is large because it will remove over 4000 lines of code.

          Show
          Robert Muir added a comment - attached is a patch that replaces the PrecedenceQueryParser with this implementation I didnt change any functionality, just brought it up to trunk and cleared javadocs errors and other minor things I think this is much easier to maintain... the patch is large because it will remove over 4000 lines of code.
          Hide
          Adriano Crestani added a comment -

          Thanks Robert! The patch looks good to me.

          Show
          Adriano Crestani added a comment - Thanks Robert! The patch looks good to me.
          Hide
          Robert Muir added a comment -

          Thanks for reviewing Adriano.

          I'll commit tomorrow unless anyone objects.

          Show
          Robert Muir added a comment - Thanks for reviewing Adriano. I'll commit tomorrow unless anyone objects.
          Hide
          Robert Muir added a comment -

          Committed revision 1025597, 1025606 (3x)

          Thanks Adriano!

          Show
          Robert Muir added a comment - Committed revision 1025597, 1025606 (3x) Thanks Adriano!
          Hide
          Adriano Crestani added a comment -

          Thank you for committing the code Robert.

          Show
          Adriano Crestani added a comment - Thank you for committing the code Robert.
          Hide
          Grant Ingersoll added a comment -

          Bulk close for 3.1

          Show
          Grant Ingersoll added a comment - Bulk close for 3.1

            People

            • Assignee:
              Robert Muir
              Reporter:
              Adriano Crestani
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development