Solr
  1. Solr
  2. SOLR-3039

ExtendedDismaxQParser should allow for extension of parsing-related behavior

    Details

      Description

      ExtendedDismaxQParser.parse does not currently allow for things like query pre-processing prior to its parsing, specifying the parser to be used, and whether particular clause should be included in the query being parsed. Furthermore, ExtendedDismaxQParser and inner ExtendedSolrQueryParser cannot be subclassed. By resolving this issue, we'll provide a way for Solr implementations to extend the parser and parsing related behavior.

      1. SOLR-3039.patch
        93 kB
        Danny Dvinov
      2. SOLR-3039.patch
        74 kB
        Danny Dvinov
      3. SOLR-3039.patch
        74 kB
        Danny Dvinov
      4. SOLR-3039.patch
        1 kB
        Danny Dvinov

        Issue Links

          Activity

          Hide
          Danny Dvinov added a comment -

          Thank you, Otis Gospodnetic and Tomás Fernández Löbbe, looked at Tomas' patch, and looks like it pretty much covers everything I was suggesting (and further improves the refactoring!). The only thing we won't be able to do out of the box is decide on whether or not a particular clause should be included/excluded while in splitIntoClauses (includeClause method from this suggested patch), but I suppose this is minor, and we could either override entire splitIntoClauses or just apply a minor patch to accomplish this. Thank you for looking into this!

          Show
          Danny Dvinov added a comment - Thank you, Otis Gospodnetic and Tomás Fernández Löbbe , looked at Tomas' patch, and looks like it pretty much covers everything I was suggesting (and further improves the refactoring!). The only thing we won't be able to do out of the box is decide on whether or not a particular clause should be included/excluded while in splitIntoClauses (includeClause method from this suggested patch), but I suppose this is minor, and we could either override entire splitIntoClauses or just apply a minor patch to accomplish this. Thank you for looking into this!
          Hide
          Otis Gospodnetic added a comment -

          Danny Dvinov maybe you can close after you've confirmed?

          Show
          Otis Gospodnetic added a comment - Danny Dvinov maybe you can close after you've confirmed?
          Hide
          Tomás Fernández Löbbe added a comment -

          Yes, it looks like this patch is based on an older revision, prior to SOLR-4208. Most of the things that are suggested here were implemented and committed.
          I didn't add a "preProecessUserQuery(...)", I thought about this, but I think now it is something easier to extend now (either wrapping the parse method, overriding the "getString()" method, when parsing the configuration, when getting the different clauses, depending on the needs). I didn't thought it was necessary to add an empty hook for this.

          Show
          Tomás Fernández Löbbe added a comment - Yes, it looks like this patch is based on an older revision, prior to SOLR-4208 . Most of the things that are suggested here were implemented and committed. I didn't add a "preProecessUserQuery(...)", I thought about this, but I think now it is something easier to extend now (either wrapping the parse method, overriding the "getString()" method, when parsing the configuration, when getting the different clauses, depending on the needs). I didn't thought it was necessary to add an empty hook for this.
          Hide
          Otis Gospodnetic added a comment -

          Haven't looked at the patch but I suspect it might be in conflict with Tomás Fernández Löbbe's SOLR-4208. Oh, maybe not, SOLR-4208 was committed... assuming Danny based his latest patch on the more recent trunk revision? I'm eyeing this with LUCENE-4499 in mind.

          Show
          Otis Gospodnetic added a comment - Haven't looked at the patch but I suspect it might be in conflict with Tomás Fernández Löbbe 's SOLR-4208 . Oh, maybe not, SOLR-4208 was committed... assuming Danny based his latest patch on the more recent trunk revision? I'm eyeing this with LUCENE-4499 in mind.
          Hide
          Danny Dvinov added a comment -

          Please feel free to make comments/suggestions!

          Show
          Danny Dvinov added a comment - Please feel free to make comments/suggestions!
          Hide
          Danny Dvinov added a comment -

          Changes include:

          • Making ExtendedDismaxQParser public to allow implementations that can extend it (and pulling it out into a separate file, ExtendedDismaxQParser.java).
          • Making ExtendedSolrQueryParser protected to allow to allow implementations that can extend it.
          • Introducing 3 protected methods on ExtendedDismaxQParser to allow implementations that extend ExtendedDismaxQParser to override some of the behavior:
            1. ExtendedSolrQueryParser getQueryParser to allow specifying the parser used to parse the query.
            2. boolean includeClause(Clause clause) to allow overriding whether given clause should be part of the {@link org.apache.lucene.search.Query}

            being parsed.
            3. String preProcessUserQuery(String queryString) to allow pre-processing the query string prior to parsing.

          Show
          Danny Dvinov added a comment - Changes include: Making ExtendedDismaxQParser public to allow implementations that can extend it (and pulling it out into a separate file, ExtendedDismaxQParser.java). Making ExtendedSolrQueryParser protected to allow to allow implementations that can extend it. Introducing 3 protected methods on ExtendedDismaxQParser to allow implementations that extend ExtendedDismaxQParser to override some of the behavior: 1. ExtendedSolrQueryParser getQueryParser to allow specifying the parser used to parse the query. 2. boolean includeClause(Clause clause) to allow overriding whether given clause should be part of the {@link org.apache.lucene.search.Query} being parsed. 3. String preProcessUserQuery(String queryString) to allow pre-processing the query string prior to parsing.
          Hide
          Danny Dvinov added a comment -

          Refactored the previous change to be more generic to allow pre-processing user query prior to parsing for whatever reasons one might have (as opposed to only for expanding wildcarded terms).

          Show
          Danny Dvinov added a comment - Refactored the previous change to be more generic to allow pre-processing user query prior to parsing for whatever reasons one might have (as opposed to only for expanding wildcarded terms).
          Hide
          Danny Dvinov added a comment -

          Made ExtendedDismaxQParser public for implementations that will be extending it.

          Show
          Danny Dvinov added a comment - Made ExtendedDismaxQParser public for implementations that will be extending it.

            People

            • Assignee:
              Unassigned
              Reporter:
              Danny Dvinov
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - 24h
                24h
                Remaining:
                Remaining Estimate - 24h
                24h
                Logged:
                Time Spent - Not Specified
                Not Specified

                  Development