-
Notifications
You must be signed in to change notification settings - Fork 2.3k
[Approximation Framework] Extend approximation to single clause boolean queries #18693
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
Conversation
Signed-off-by: Sawan Srivastava <[email protected]>
… in ApproximateBooleanQuery Signed-off-by: Sawan Srivastava <[email protected]>
Signed-off-by: Sawan Srivastava <[email protected]>
…text in ApproximateBooleanQuery Signed-off-by: Sawan Srivastava <[email protected]>
…y check Signed-off-by: Sawan Srivastava <[email protected]>
Signed-off-by: Sawan Srivastava <[email protected]>
|
❌ Gradle check result for 5e709c3: 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: Sawan Srivastava <[email protected]>
…(for now) Signed-off-by: Sawan Srivastava <[email protected]>
|
❌ Gradle check result for 0056e3d: 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: Sawan Srivastava <[email protected]>
|
❌ Gradle check result for 2b9514b: 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: Sawan Srivastava <[email protected]>
|
❌ Gradle check result for 61fbf45: 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? |
…ritten Signed-off-by: Sawan Srivastava <[email protected]>
|
❌ Gradle check result for 77c7068: 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: Sawan Srivastava <[email protected]>
Signed-off-by: Sawan Srivastava <[email protected]>
|
❌ Gradle check result for be6a051: 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: Sawan Srivastava <[email protected]>
|
❌ Gradle check result for 5a8e024: 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: Sawan Srivastava <[email protected]>
|
❌ Gradle check result for 6c53755: 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: Sawan Srivastava <[email protected]>
|
❌ Gradle check result for b1eb7e6: 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: Sawan Srivastava <[email protected]>
|
❌ Gradle check result for 6399b32: 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: Sawan Srivastava <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #18693 +/- ##
============================================
+ Coverage 72.74% 72.81% +0.07%
- Complexity 68400 68452 +52
============================================
Files 5568 5569 +1
Lines 314401 314452 +51
Branches 45598 45610 +12
============================================
+ Hits 228696 228966 +270
+ Misses 67055 66843 -212
+ Partials 18650 18643 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I'm seeing good latency reduction for range queries (in line with ApproximatePointRangeQuery). Before approximation: After approximation: Query: |
| } | ||
|
|
||
| // TODO: Figure out why multi-clause breaks testPhrasePrefix() in HighlighterWithAnalyzersTests.java | ||
| return ((BooleanQuery) query).clauses().size() == 1 |
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.
Is it guaranteed that we will only get BooleanQuery instances here?
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.
Yeah, the result after calling booleanQueryBuilder.build() is always a BooleanQuery.
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.
The query here might be MATCH_ALL_DOCS since you call fixNegativeQueryIfNeeded before this check here - so it is not the pure output of BooleanQueryBuilder's build method.
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.
I see, we can probably add a defensive check before casting. Thanks.
| private final int size; | ||
| private final List<BooleanClause> clauses; | ||
| private ApproximateBooleanQuery booleanQuery; | ||
| public boolean isUnwrapped = false; |
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.
Redundant variable
| } | ||
|
|
||
| // For single clause boolean queries, check if the clause can be approximated | ||
| if (clauses.size() == 1 && clauses.get(0).occur() != BooleanClause.Occur.MUST_NOT) { |
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.
Do we need clauses.size() == 1 again since it is guaranteed by BoolQueryBuilder?
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.
No, but this check will be necessary when multi-clause boolean query approximation will be implemented.
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.
Better to add it then IMO.
| appxResolved.setContext(context); | ||
| } | ||
| try { | ||
| resolvedQuery = resolvedQuery.rewrite(context.searcher()); |
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.
Calling rewrite() inside setContext() breaks typical Lucene patterns where rewrite happens before context setting. Why is additional rewriting needed here?
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.
I believe I was running into issues with MultiTermQueries within single clause boolean queries not rewriting properly. Forcing the resolved query through a rewrite seemed to fix the issue.
Description
Implements ApproximateBooleanQuery which flattens and rewrites to single clause queries at the OpenSearch level and applies approximation if possible.
Related Issues
Resolves #18692
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.