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
        95 kB
        Mark Miller
      2. LUCENE-996.patch
        95 kB
        Mark Miller
      3. LUCENE-996.patch
        3 kB
        Yonik Seeley
      4. lucene-996.patch
        39 kB
        Andrew Schurman
      5. lucene-996.patch
        71 kB
        Andrew Schurman

        Issue Links

          Activity

          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Resolved Resolved
          426d 9h 4m 1 Mark Miller 13/Nov/08 00:36
          Resolved Resolved Reopened Reopened
          3m 12s 1 Mark Miller 13/Nov/08 00:39
          Reopened Reopened Resolved Resolved
          708d 20h 2m 1 Yonik Seeley 22/Oct/10 21:41
          Resolved Resolved Closed Closed
          930d 14h 2m 1 Uwe Schindler 10/May/13 11:43
          Uwe Schindler made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Mark Thomas made changes -
          Workflow Default workflow, editable Closed status [ 12563709 ] jira [ 12585266 ]
          Mark Thomas made changes -
          Workflow jira [ 12412806 ] Default workflow, editable Closed status [ 12563709 ]
          Yonik Seeley made changes -
          Status Reopened [ 4 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          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).
          Yonik Seeley made changes -
          Attachment LUCENE-996.patch [ 12457836 ]
          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?
          Luis Alves made changes -
          Link This issue is part of LUCENE-1823 [ LUCENE-1823 ]
          Mark Miller made changes -
          Fix Version/s 3.1 [ 12314025 ]
          Fix Version/s 3.0 [ 12312889 ]
          Mark Miller made changes -
          Fix Version/s 3.0 [ 12312889 ]
          Fix Version/s 2.9 [ 12312682 ]
          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.
          Mark Miller made changes -
          Attachment LUCENE-996.patch [ 12394014 ]
          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.
          Mark Miller made changes -
          Attachment LUCENE-996.patch [ 12393832 ]
          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.
          Mark Miller made changes -
          Link This issue depends on LUCENE-1424 [ LUCENE-1424 ]
          Mark Miller made changes -
          Status Resolved [ 5 ] Reopened [ 4 ]
          Resolution Fixed [ 1 ]
          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.
          Mark Miller made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Fix Version/s 2.9 [ 12312682 ]
          Resolution Fixed [ 1 ]
          Mark Miller made changes -
          Link This issue depends on LUCENE-1424 [ LUCENE-1424 ]
          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.
          Andrew Schurman made changes -
          Attachment lucene-996.patch [ 12365892 ]
          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.
          Andrew Schurman made changes -
          Link This issue blocks SOLR-355 [ SOLR-355 ]
          Andrew Schurman made changes -
          Field Original Value New Value
          Attachment lucene-996.patch [ 12365743 ]
          Hide
          Andrew Schurman added a comment -

          Potential fix for revision 574260.

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

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development