Lucene - Core
  1. Lucene - Core
  2. LUCENE-996

Parsing mixed inclusive/exclusive range queries

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 4.0-ALPHA
    • Component/s: core/queryparser
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      The current query parser doesn't handle parsing a range query (i.e. ConstantScoreRangeQuery) with mixed inclusive/exclusive bounds.

      1. LUCENE-996.patch
        3 kB
        Yonik Seeley
      2. LUCENE-996.patch
        95 kB
        Mark Miller
      3. LUCENE-996.patch
        95 kB
        Mark Miller
      4. lucene-996.patch
        71 kB
        Andrew Schurman
      5. lucene-996.patch
        39 kB
        Andrew Schurman

        Issue Links

          Activity

          Hide
          Yonik Seeley added a comment -

          Committed.

          Show
          Yonik Seeley added a comment - Committed.
          Hide
          Yonik Seeley added a comment -

          OK, here's the full patch.
          Rather than try to maintain back compat for overriders that only works half of the time and silently fails the other half, I took a different approach that will make their compile fail (since lucene4 isn't a drop in replacement for any previous impl).

          Show
          Yonik Seeley added a comment - OK, here's the full patch. Rather than try to maintain back compat for overriders that only works half of the time and silently fails the other half, I took a different approach that will make their compile fail (since lucene4 isn't a drop in replacement for any previous impl).
          Hide
          Yonik Seeley added a comment -

          I didn't realize there was already a patch for this!
          Anyway, I just implemented my own changes to the grammar (attached).

          Show
          Yonik Seeley added a comment - I didn't realize there was already a patch for this! Anyway, I just implemented my own changes to the grammar (attached).
          Hide
          Luis Alves added a comment -

          Issue LUCENE-1823 includes syntax for >=, <=, = :

          • Instead of "foo:[a TO z}" the query would be "foo >a AND foo <=z"
          • instead of "foo:{a TO z]" the query would be "foo >= a AND foo <z"

          Is that good enough?

          Show
          Luis Alves added a comment - Issue LUCENE-1823 includes syntax for >=, <=, = : Instead of "foo:[a TO z}" the query would be "foo >a AND foo <=z" instead of "foo:{a TO z]" the query would be "foo >= a AND foo <z" Is that good enough?
          Hide
          Mark Miller added a comment -

          Because this requires changing a callback or two in the queryparser, its probably easier to put it into 3 than 2.9.

          Show
          Mark Miller added a comment - Because this requires changing a callback or two in the queryparser, its probably easier to put it into 3 than 2.9.
          Hide
          Mark Miller added a comment -

          Missed that the new tests didn't patch, so I've merged and fixed them. Looks pretty good now except that a contrib test fails - those that overrode the deprecated protected Query getRangeQuery(String field, String part1, String part2, boolean inclusive) are being left out in the cold.

          Once that is addressed, I think this patch looks pretty good.

          Show
          Mark Miller added a comment - Missed that the new tests didn't patch, so I've merged and fixed them. Looks pretty good now except that a contrib test fails - those that overrode the deprecated protected Query getRangeQuery(String field, String part1, String part2, boolean inclusive) are being left out in the cold. Once that is addressed, I think this patch looks pretty good.
          Hide
          Mark Miller added a comment -

          Bit of a pain catching everything that was slightly off, but I think we want this, especially since RangeQuery accepts separate start/end point inclusive/exclusive now.

          Patch brings everything up to trunk, but definitely still needs a strong look over. I'll come back to it in a few days.

          Show
          Mark Miller added a comment - Bit of a pain catching everything that was slightly off, but I think we want this, especially since RangeQuery accepts separate start/end point inclusive/exclusive now. Patch brings everything up to trunk, but definitely still needs a strong look over. I'll come back to it in a few days.
          Hide
          Mark Miller added a comment -

          Needs the queryparser piece still. That patch looks a little noisy, but I'll take a crack at it.

          Show
          Mark Miller added a comment - Needs the queryparser piece still. That patch looks a little noisy, but I'll take a crack at it.
          Hide
          Hoss Man added a comment -

          there's nothing wrong with requiring people who want to take advantage of new syntax to use new code.

          Show
          Hoss Man added a comment - there's nothing wrong with requiring people who want to take advantage of new syntax to use new code.
          Hide
          Andrew Schurman added a comment -

          Your absolutely right, I had a problem with the Solr src extending this method. I don't think there is a nice way to cover all the bases here, but I've updated the code help alleviate the problem.

          I believe the only thing that isn't covered is parsing foo:{a TO bat] with useOldRangeQueries=false. Classes that extend will still get a query back, but it might not be what they expect.

          Show
          Andrew Schurman added a comment - Your absolutely right, I had a problem with the Solr src extending this method. I don't think there is a nice way to cover all the bases here, but I've updated the code help alleviate the problem. I believe the only thing that isn't covered is parsing foo:{a TO bat] with useOldRangeQueries=false. Classes that extend will still get a query back, but it might not be what they expect.
          Hide
          Hoss Man added a comment -

          so this changes the query syntax such that foo:

          {a TO z] and foo:[a TO z}

          are now legal ... the querysyntax docs should be modified to mention this in the patch as well.

          one hitch: this seems to break backwards compatibility for anyone who has previously subclassed QueryParser and overridden the getRangeQuery(String, String, String, boolean) method ... if someone defines that method in their query parser, it will now never be called – even if they don't take advantage of the new syntax.

          off the top of my head, one way to remain backwards compatible is to have a deprecated getRangeQuery(String, String, String, boolean) method which does the same thing it currently does, and have the new getRangeQuery(String, String, String, boolean, boolean) method call it if the booleans have the same value ... if they don't have the same value then do the new stuff. document that people subclassing QueryParser that want to override RangeQueries only need to override the double boolean method.

          Show
          Hoss Man added a comment - so this changes the query syntax such that foo: {a TO z] and foo:[a TO z} are now legal ... the querysyntax docs should be modified to mention this in the patch as well. one hitch: this seems to break backwards compatibility for anyone who has previously subclassed QueryParser and overridden the getRangeQuery(String, String, String, boolean) method ... if someone defines that method in their query parser, it will now never be called – even if they don't take advantage of the new syntax. off the top of my head, one way to remain backwards compatible is to have a deprecated getRangeQuery(String, String, String, boolean) method which does the same thing it currently does, and have the new getRangeQuery(String, String, String, boolean, boolean) method call it if the booleans have the same value ... if they don't have the same value then do the new stuff. document that people subclassing QueryParser that want to override RangeQueries only need to override the double boolean method.
          Hide
          Andrew Schurman added a comment -

          Potential fix for revision 574260.

          Show
          Andrew Schurman added a comment - Potential fix for revision 574260.

            People

            • Assignee:
              Unassigned
              Reporter:
              Andrew Schurman
            • Votes:
              1 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development