-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Remove MultiCollectorWrapper and use MultiCollector in Lucene instead #19595
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Binlong Gao <[email protected]>
Signed-off-by: Binlong Gao <[email protected]>
❌ Gradle check result for db44cf9: 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? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #19595 +/- ##
============================================
- Coverage 73.06% 72.96% -0.11%
+ Complexity 70630 70542 -88
============================================
Files 5723 5722 -1
Lines 323513 323503 -10
Branches 46852 46851 -1
============================================
- Hits 236384 236032 -352
- Misses 68013 68400 +387
+ Partials 19116 19071 -45 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
server/src/main/java/org/opensearch/search/query/TopDocsCollectorContext.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/search/query/TopDocsCollectorContext.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Binlong Gao <[email protected]>
❕ Gradle check result for 6fd02e5: 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. |
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.
LGTM, just a minor code refactoring near the lines of code you are touching upon!
for (final Collector sub : (((MultiCollectorWrapper) collector).getCollectors())) { | ||
if (collector instanceof MultiCollector m) { | ||
for (final Collector sub : (m.getCollectors())) { | ||
if (sub instanceof TopDocsCollector<?>) { |
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.
Not directly related to your code change, but can you also do Pattern Matching for instanceof in consecutive lines of code as well in the same method.
Description
As this comment said, once MultiCollector.getCollectors() method in Lucene is declared as public, we can use that method directly rather than creating a new similar class, this PR replaces MultiCollectorWrapper with MultiCollector in Lucene, and removes MultiCollectorWrapper as it was internally used and won't be used anymore.
Related Issues
No issue.
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.