-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Propagate TooComplexToDeterminizeException in query_string regex queries #18883
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Propagate TooComplexToDeterminizeException in query_string regex queries #18883
Conversation
Signed-off-by: Peter Alfonsi <[email protected]>
Signed-off-by: Peter Alfonsi <[email protected]>
❌ Gradle check result for 89a1044: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Peter Alfonsi <[email protected]>
❌ Gradle check result for dd7f617: null Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Peter Alfonsi <[email protected]>
❕ Gradle check result for 3d0b614: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #18883 +/- ##
============================================
+ Coverage 72.78% 72.81% +0.02%
- Complexity 68681 68697 +16
============================================
Files 5582 5582
Lines 315495 315495
Branches 45784 45784
============================================
+ Hits 229625 229718 +93
+ Misses 67223 67185 -38
+ Partials 18647 18592 -55 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Not sure why codecov thinks line 802 of QueryStringQueryParser is uncovered, it would be run in any test that takes the lenient query path, for example QueryStringQueryBuilderTests.testPrefixNumeric() and others |
@jainankitk could you take a look at this one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @peteralfonsi for addressing this issue. LGTM!
…ies (opensearch-project#18883) Signed-off-by: Peter Alfonsi <[email protected]> Co-authored-by: Peter Alfonsi <[email protected]> Signed-off-by: sunqijun.jun <[email protected]>
…ies (opensearch-project#18883) Signed-off-by: Peter Alfonsi <[email protected]> Co-authored-by: Peter Alfonsi <[email protected]>
…ies (opensearch-project#18883) Signed-off-by: Peter Alfonsi <[email protected]> Co-authored-by: Peter Alfonsi <[email protected]>
Description
In
query_string
queries that use regex,TooComplexToDeterminizeException
was incorrectly swallowed iflenient
query behavior was on.lenient
is intended to "ignore data type mismatches between the query and the document field," butTooComplexToDeterminizeException
comes from the same place in the code despite not having to do with data type mismatches. This causedquery_string
queries to return 200 incorrectly even when the same regex on aregexp
query would return 400.A related question is why
lenient
was on in the first place withinQueryStringQueryBuilder
, given that the index setting defaults tofalse
and I didn't specify it in the query body. I will raise a separate issue for this as I'm not sure if the current behavior is intended or not and I want to get feedback from others. Either way though, the fix in this PR should apply.Testing: added coverage to the existing UT. Also manually tested the query from the issue:
This originally succeeded, but now correctly returns 400:
Related Issues
Resolves #18733
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.