Refactor implementations of query phase searcher, add empty QueryCollectorContext #13481

martin-gaievski · 2024-05-01T00:58:39Z

Description

Current implementations of query phase searcher (default and concurrent) alway adds TopDocsCollector collector to the collector context. In some scenario it's not the preferred behavior, for instance if in a plugin we're adding custom collector that collects doc scores in a different fashion. With current implementation there will be two collectors in the context, this will create a performance overhead.

With this change we're not changing core functionality or implementation of query phase searcher. Instead we're adding overloaded searchWith method that child query phase search can override and pass empty QueryPhaseSearcher.

Related Issues

#13170

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)
Public documentation issue/PR created

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.

github-actions · 2024-05-01T01:00:54Z

❌ Gradle check result for 6c522fa: 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?

martin-gaievski · 2024-05-01T01:28:52Z

this change need to be backported to 2.x, need help from maintainers, I don't have permissions to add a label for backport

martin-gaievski · 2024-05-01T01:31:20Z

@reta can you please review this change, this is related to #13481? I've tested the PR with corresponding changes at the plugin level, I can created PR for neural-search repo once core change is merged.

github-actions · 2024-05-01T02:06:50Z

✅ Gradle check result for e290171: SUCCESS

codecov · 2024-05-01T02:10:46Z

Codecov Report

Attention: Patch coverage is 55.55556% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 71.55%. Comparing base (b15cb0c) to head (d13f4a2).
Report is 252 commits behind head on main.

Files	Patch %	Lines
...opensearch/search/query/QueryCollectorContext.java	20.00%	4 Missing ⚠️
...rch/search/query/ConcurrentQueryPhaseSearcher.java	60.00%	1 Missing and 1 partial ⚠️
...n/java/org/opensearch/search/query/QueryPhase.java	71.42%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #13481      +/-   ##
============================================
+ Coverage     71.42%   71.55%   +0.13%     
- Complexity    59978    61008    +1030     
============================================
  Files          4985     5050      +65     
  Lines        282275   286815    +4540     
  Branches      40946    41554     +608     
============================================
+ Hits         201603   205220    +3617     
- Misses        63999    64647     +648     
- Partials      16673    16948     +275

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

server/src/main/java/org/opensearch/search/query/ConcurrentQueryPhaseSearcher.java

server/src/main/java/org/opensearch/search/query/QueryPhase.java

github-actions · 2024-05-01T23:48:29Z

❌ Gradle check result for 4381743: 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?

github-actions · 2024-05-02T00:07:15Z

❌ Gradle check result for 8744c3e: 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?

github-actions · 2024-05-02T01:23:52Z

❕ Gradle check result for af14d56: UNSTABLE

TEST FAILURES:

      1 org.opensearch.repositories.azure.AzureBlobContainerRetriesTests.testReadRangeBlobWithRetries

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

github-actions · 2024-05-02T01:28:03Z

❌ Gradle check result for 837c4a0: 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?

martin-gaievski · 2024-05-02T16:01:18Z

@reta can you please help with merging this PR, Jay has approved the PR

reta · 2024-05-02T16:13:33Z

@reta can you please help with merging this PR, Jay has approved the PR

Fixing tests with #13510

reta · 2024-05-02T17:24:46Z

@martin-gaievski could you please rebase? thank you!

… searchWith method Signed-off-by: Martin Gaievski <[email protected]>

Signed-off-by: Martin Gaievski <[email protected]>

github-actions · 2024-05-02T18:26:41Z

✅ Gradle check result for d13f4a2: SUCCESS

…ectorContext (#13481) * Refactor implementations of query hpase searcher by adding overloaded searchWith method Signed-off-by: Martin Gaievski <[email protected]> * Switched to Empty context add rescoring interface Signed-off-by: Martin Gaievski <[email protected]> * Changed if by simple null check for querycontext argument Signed-off-by: Martin Gaievski <[email protected]> * Added Override annotation for searchWith method Signed-off-by: Martin Gaievski <[email protected]> * Remove old override method from Concurrent Query Phase Searcher Signed-off-by: Martin Gaievski <[email protected]> --------- Signed-off-by: Martin Gaievski <[email protected]> (cherry picked from commit 5ff50a7) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…ectorContext (#13481) (#13512) * Refactor implementations of query hpase searcher by adding overloaded searchWith method * Switched to Empty context add rescoring interface * Changed if by simple null check for querycontext argument * Added Override annotation for searchWith method * Remove old override method from Concurrent Query Phase Searcher --------- (cherry picked from commit 5ff50a7) Signed-off-by: Martin Gaievski <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…ectorContext (opensearch-project#13481) * Refactor implementations of query hpase searcher by adding overloaded searchWith method Signed-off-by: Martin Gaievski <[email protected]> * Switched to Empty context add rescoring interface Signed-off-by: Martin Gaievski <[email protected]> * Changed if by simple null check for querycontext argument Signed-off-by: Martin Gaievski <[email protected]> * Added Override annotation for searchWith method Signed-off-by: Martin Gaievski <[email protected]> * Remove old override method from Concurrent Query Phase Searcher Signed-off-by: Martin Gaievski <[email protected]> --------- Signed-off-by: Martin Gaievski <[email protected]>

martin-gaievski force-pushed the override_search_with_method_for_query_phase_searcher branch from 6c522fa to e290171 Compare May 1, 2024 00:59

martin-gaievski marked this pull request as ready for review May 1, 2024 01:31

martin-gaievski requested review from anasalkouz, andrross, Bukhtawar, CEHENKLE, dblock, dbwiddis, dreamer-89, gbbafna, kotwanikunal, mch2, msfroh, nknize, owaiskazi19, reta, Rishikesh1159, sachinpkale, saratvemulapalli, shwetathareja, sohami, tlfeng and VachaShah as code owners May 1, 2024 01:31

reta reviewed May 1, 2024

View reviewed changes

server/src/main/java/org/opensearch/search/query/ConcurrentQueryPhaseSearcher.java Outdated Show resolved Hide resolved

reta reviewed May 1, 2024

View reviewed changes

server/src/main/java/org/opensearch/search/query/ConcurrentQueryPhaseSearcher.java Outdated Show resolved Hide resolved

jed326 reviewed May 1, 2024

View reviewed changes

martin-gaievski force-pushed the override_search_with_method_for_query_phase_searcher branch from d694e2d to 4381743 Compare May 1, 2024 23:11

martin-gaievski requested a review from jed326 May 1, 2024 23:22

martin-gaievski force-pushed the override_search_with_method_for_query_phase_searcher branch from 8744c3e to af14d56 Compare May 2, 2024 00:29

jed326 approved these changes May 2, 2024

View reviewed changes

martin-gaievski added 5 commits May 2, 2024 10:31

Refactor implementations of query hpase searcher by adding overloaded…

fd33866

… searchWith method Signed-off-by: Martin Gaievski <[email protected]>

Switched to Empty context add rescoring interface

396bbfe

Signed-off-by: Martin Gaievski <[email protected]>

Changed if by simple null check for querycontext argument

a3268d5

Signed-off-by: Martin Gaievski <[email protected]>

Added Override annotation for searchWith method

1f1702d

Signed-off-by: Martin Gaievski <[email protected]>

Remove old override method from Concurrent Query Phase Searcher

d13f4a2

Signed-off-by: Martin Gaievski <[email protected]>

martin-gaievski force-pushed the override_search_with_method_for_query_phase_searcher branch from 837c4a0 to d13f4a2 Compare May 2, 2024 17:31

reta approved these changes May 2, 2024

View reviewed changes

reta merged commit 5ff50a7 into opensearch-project:main May 2, 2024
27 of 28 checks passed

opensearch-trigger-bot bot mentioned this pull request May 2, 2024

[Backport 2.x] Refactor implementations of query phase searcher, add empty QueryCollectorContext #13512

Merged

martin-gaievski mentioned this pull request May 3, 2024

Pass empty QueryCollectorContext in case of hybrid query to improve latencies by 20% opensearch-project/neural-search#731

Merged

4 tasks

jed326 mentioned this pull request Jun 4, 2024

[RFC] Introduce pluggable QueryCollectorContexts and CollectorManagers #13978

Open

BrewTestBot mentioned this pull request Jun 26, 2024

opensearch 2.15.0 Homebrew/homebrew-core#175637

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor implementations of query phase searcher, add empty QueryCollectorContext #13481

Refactor implementations of query phase searcher, add empty QueryCollectorContext #13481

martin-gaievski commented May 1, 2024 •

edited

Loading

github-actions bot commented May 1, 2024

martin-gaievski commented May 1, 2024 •

edited

Loading

martin-gaievski commented May 1, 2024

github-actions bot commented May 1, 2024

codecov bot commented May 1, 2024 •

edited

Loading

github-actions bot commented May 1, 2024

github-actions bot commented May 2, 2024

github-actions bot commented May 2, 2024

github-actions bot commented May 2, 2024

martin-gaievski commented May 2, 2024

reta commented May 2, 2024 •

edited

Loading

reta commented May 2, 2024

github-actions bot commented May 2, 2024

Refactor implementations of query phase searcher, add empty QueryCollectorContext #13481

Refactor implementations of query phase searcher, add empty QueryCollectorContext #13481

Conversation

martin-gaievski commented May 1, 2024 • edited Loading

Description

Related Issues

Check List

github-actions bot commented May 1, 2024

martin-gaievski commented May 1, 2024 • edited Loading

martin-gaievski commented May 1, 2024

github-actions bot commented May 1, 2024

codecov bot commented May 1, 2024 • edited Loading

Codecov Report

github-actions bot commented May 1, 2024

github-actions bot commented May 2, 2024

github-actions bot commented May 2, 2024

github-actions bot commented May 2, 2024

martin-gaievski commented May 2, 2024

reta commented May 2, 2024 • edited Loading

reta commented May 2, 2024

github-actions bot commented May 2, 2024

martin-gaievski commented May 1, 2024 •

edited

Loading

martin-gaievski commented May 1, 2024 •

edited

Loading

codecov bot commented May 1, 2024 •

edited

Loading

reta commented May 2, 2024 •

edited

Loading