Revert "#17593 Add Extension Points For Pre and Post Collection of Scores in QueryPhase" - #18913
Merged
owaiskazi19 merged 1 commit intoAug 5, 2025
Conversation
martin-gaievski
requested review from
a team,
Bukhtawar,
CEHENKLE,
Rishikesh1159,
VachaShah,
anasalkouz,
andrross,
ashking94,
cwperks,
dbwiddis,
gbbafna,
jed326,
kotwanikunal,
mch2,
msfroh,
owaiskazi19,
reta,
sachinpkale,
saratvemulapalli,
shwetathareja and
sohami
as code owners
August 5, 2025 02:17
Member
|
@atris FYI |
Member
|
@martin-gaievski can you sign the commit to pass DCO? |
…t Collection of Scores in …" This reverts commit eda3a48. Signed-off-by: Martin Gaievski <gaievski@amazon.com>
martin-gaievski
force-pushed
the
revert-18814-add_querytime_extensions
branch
from
August 5, 2025 02:21
6bba140 to
7b10f60
Compare
Member
Author
yes, done. btw this is what gh workflow creates, we may need to look into possible improvements for CI |
owaiskazi19
approved these changes
Aug 5, 2025
This was referenced Aug 5, 2025
Contributor
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #18913 +/- ##
============================================
+ Coverage 72.75% 72.85% +0.10%
- Complexity 68822 68862 +40
============================================
Files 5600 5599 -1
Lines 316247 316236 -11
Branches 45866 45864 -2
============================================
+ Hits 230085 230408 +323
+ Misses 67543 67139 -404
- Partials 18619 18689 +70 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
atris
added a commit
to atris/OpenSearch
that referenced
this pull request
Aug 8, 2025
This commit reintroduces the QueryPhaseListener functionality that was reverted in opensearch-project#18913 due to performance regressions, now with optimizations that eliminate the overhead. Changes: - Add QueryPhaseListener interface for plugins to hook into query phase - Add AbstractQueryPhaseSearcher implementing template pattern for listeners - Modify QueryPhaseSearcher interface to support listener registration - Add comprehensive tests for listener functionality Performance optimizations: - Fast-path in DefaultQueryPhaseSearcher bypasses template pattern when no listeners - Direct hasQueryPhaseListeners() check avoids virtual method calls - Single listener optimization avoids iterator creation - Zero overhead for the common case (no listeners registered) The original implementation caused 10-15% regression in neural-search because it forced all searches through the template pattern even without listeners. This optimized version maintains the extension functionality while eliminating overhead for plugins that extend DefaultQueryPhaseSearcher without using listeners. Fixes opensearch-project#17593 Resolves performance regression from opensearch-project#18913 Signed-off-by: Atri Sharma <atri.jiit@gmail.com>
atris
added a commit
to atris/OpenSearch
that referenced
this pull request
Aug 8, 2025
This commit reintroduces the QueryPhaseListener functionality that was reverted in opensearch-project#18913 due to performance regressions, now with optimizations that eliminate the overhead. Changes: - Add QueryPhaseListener interface for plugins to hook into query phase - Add AbstractQueryPhaseSearcher implementing template pattern for listeners - Modify QueryPhaseSearcher interface to support listener registration - Add comprehensive tests for listener functionality Performance optimizations: - Fast-path in DefaultQueryPhaseSearcher bypasses template pattern when no listeners - Direct hasQueryPhaseListeners() check avoids virtual method calls - Single listener optimization avoids iterator creation - Zero overhead for the common case (no listeners registered) The original implementation caused 10-15% regression in neural-search because it forced all searches through the template pattern even without listeners. This optimized version maintains the extension functionality while eliminating overhead for plugins that extend DefaultQueryPhaseSearcher without using listeners. Fixes opensearch-project#17593 Resolves performance regression from opensearch-project#18913 Signed-off-by: Atri Sharma <atri.jiit@gmail.com>
atris
added a commit
to atris/OpenSearch
that referenced
this pull request
Aug 8, 2025
This commit reintroduces the QueryPhaseListener functionality that was reverted in opensearch-project#18913 due to performance regressions, now with optimizations that eliminate the overhead. Changes: - Add QueryPhaseListener interface for plugins to hook into query phase - Add AbstractQueryPhaseSearcher implementing template pattern for listeners - Modify QueryPhaseSearcher interface to support listener registration - Add comprehensive tests for listener functionality Performance optimizations: - Fast-path in DefaultQueryPhaseSearcher bypasses template pattern when no listeners - Direct hasQueryPhaseListeners() check avoids virtual method calls - Single listener optimization avoids iterator creation - Zero overhead for the common case (no listeners registered) The original implementation caused 10-15% regression in neural-search because it forced all searches through the template pattern even without listeners. This optimized version maintains the extension functionality while eliminating overhead for plugins that extend DefaultQueryPhaseSearcher without using listeners. Fixes opensearch-project#17593 Resolves performance regression from opensearch-project#18913 Signed-off-by: Atri Sharma <atri.jiit@gmail.com>
vinaykpud
pushed a commit
to vinaykpud/OpenSearch
that referenced
this pull request
Sep 26, 2025
…t Collection of Scores in …" (opensearch-project#18913) This reverts commit eda3a48. Signed-off-by: Martin Gaievski <gaievski@amazon.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reverts #18814.
We adopted this change in neural-search for hybrid query and found that it increases latency and drop throughput 10-15%. As there is no urgency in this feature except for request from neural-search contributors it's better to revert the change from core and do deep dive to figure out what is causes the problem.
Ref: opensearch-project/neural-search#1493