You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "benchaplin (via GitHub)" <gi...@apache.org> on 2024/04/25 00:15:00 UTC
[PR] Add test for parsing brackets in range queries [lucene]
benchaplin opened a new pull request, #13323:
URL: https://github.com/apache/lucene/pull/13323
### Description
I've added some unit tests to protect/display the QueryParser's ability to parse brackets in range query terms when in quotes.
[This issue](https://github.com/apache/lucene/issues/13234) raised a question about the QueryParser's ability to handle brackets in a range query's terms, even when escaped, for example:
```java
queryParser.parse( "[ a\\[i\\] TO b\\[i\\] ]" );
/*
org.apache.lucene.queryparser.classic.ParseException: Cannot parse '[a\[i\] TO b\[i\]]': Encountered " "]" "] "" at line 1, column 6.
Was expecting:
"TO" ...
*/
```
You can get around this exception by wrapping each range query term in quotes:
```java
queryParser.parse( "[\"a[i]\" TO \"b[i]\"]" );
```
This is because of [this JavaCC grammar rule](https://github.com/apache/lucene/blob/9af3ef8952597ead37a5a9ce5373ed3bbd40238e/lucene/queryparser/src/java/org/apache/lucene/queryparser/classic/QueryParser.jj#L345). `<RANGE_GOOP>` cannot handle a closing bracket, even an escaped one, while `<RANGE_QUOTED>` can.
I've added some unit tests to protect this functionality.
---
Any thoughts on whether `queryParser.parse( "[ a\\[i\\] TO b\\[i\\] ]" );` should work? I did play around with updating the grammar, it should be possible but I need to think more about how it might break backwards compatibility.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add test for parsing brackets in range queries [lucene]
Posted by "dweiss (via GitHub)" <gi...@apache.org>.
dweiss commented on PR #13323:
URL: https://github.com/apache/lucene/pull/13323#issuecomment-2078091483
> Any thoughts on whether queryParser.parse( "[ a\\[i\\] TO b\\[i\\] ]" ); should work? I did play around with updating the grammar, it should be possible but I need to think more about how it might break backwards compatibility.
Looking at the javacc file it seems range terms are parsed differently compared to normal tokens. This is strange but I bet there was a reason for it - now long forgotten. I think, logically, it should accept the same term notation that is used elsewhere. It will break backward compatibility but maybe consistency is worth this cost (for the major release)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add test for parsing brackets in range queries [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13323:
URL: https://github.com/apache/lucene/pull/13323#issuecomment-2111363928
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add test for parsing brackets in range queries [lucene]
Posted by "benchaplin (via GitHub)" <gi...@apache.org>.
benchaplin commented on PR #13323:
URL: https://github.com/apache/lucene/pull/13323#issuecomment-2083680863
I was thinking a fix like this could work:
```java
| <RANGE_GOOP: (~[ " ", "]", "}" ] | "\\ " | "\\]" | "\\}" )+ >
```
simply allowing the range term to continue parsing through an escaped space or closing bracket.
"Breaking" changes:
- This would permit some queries that previously threw exceptions (like the one described in the original issue)
- I'm trying to think of a query that was permitted and now would break (all existing unit tests pass). My first thought was a range query ending with an escaped bracket, however those are disallowed by the `Term can not end with escape character` rule. Any thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add test for parsing brackets in range queries [lucene]
Posted by "dweiss (via GitHub)" <gi...@apache.org>.
dweiss commented on PR #13323:
URL: https://github.com/apache/lucene/pull/13323#issuecomment-2133921546
Apologies for the delay, @benchaplin - I've merged this test into the main branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add test for parsing brackets in range queries [lucene]
Posted by "dweiss (via GitHub)" <gi...@apache.org>.
dweiss merged PR #13323:
URL: https://github.com/apache/lucene/pull/13323
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add test for parsing brackets in range queries [lucene]
Posted by "dweiss (via GitHub)" <gi...@apache.org>.
dweiss commented on PR #13323:
URL: https://github.com/apache/lucene/pull/13323#issuecomment-2084510167
The joys of escaping... Never easy (hello, Windows command-line users...). You may want to add a randomized test that constructs those terms using a mix of characters and "allowed" escape sequences - a corner case will pop up quickly, I think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add test for parsing brackets in range queries [lucene]
Posted by "benchaplin (via GitHub)" <gi...@apache.org>.
benchaplin commented on PR #13323:
URL: https://github.com/apache/lucene/pull/13323#issuecomment-2127990572
Thanks for the advice @dweiss, I will play around with some test cases for this fix. In the meantime, I think we can merge this and I will open another PR with the grammar changes if I can get something working.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org