Skip to content

Commit ad942ca

Browse files
committed
Reintroduce QueryPhaseListener with performance optimizations
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>
1 parent be8bbe1 commit ad942ca

File tree

7 files changed

+979
-2
lines changed

7 files changed

+979
-2
lines changed

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
2727
- [Workload Management] Modify logging message in WorkloadGroupService ([#18712](https://github.com/opensearch-project/OpenSearch/pull/18712))
2828
- Add BooleanQuery rewrite moving constant-scoring must clauses to filter clauses ([#18510](https://github.com/opensearch-project/OpenSearch/issues/18510))
2929
- Add functionality for plugins to inject QueryCollectorContext during QueryPhase ([#18637](https://github.com/opensearch-project/OpenSearch/pull/18637))
30+
- Add QueryPhaseListener interface for pre/post collection hooks with performance optimization ([#17593](https://github.com/opensearch-project/OpenSearch/issues/17593))
3031
- Add support for non-timing info in profiler ([#18460](https://github.com/opensearch-project/OpenSearch/issues/18460))
3132
- [Rule-based auto tagging] Bug fix and improvements ([#18726](https://github.com/opensearch-project/OpenSearch/pull/18726))
3233
- Extend Approximation Framework to other numeric types ([#18530](https://github.com/opensearch-project/OpenSearch/issues/18530))
Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
/*
2+
* SPDX-License-Identifier: Apache-2.0
3+
*
4+
* The OpenSearch Contributors require contributions made to
5+
* this file be licensed under the Apache-2.0 license or a
6+
* compatible open source license.
7+
*/
8+
9+
package org.opensearch.search.query;
10+
11+
import org.apache.lucene.search.Collector;
12+
import org.apache.lucene.search.CollectorManager;
13+
import org.apache.lucene.search.Query;
14+
import org.opensearch.common.annotation.InternalApi;
15+
import org.opensearch.search.internal.ContextIndexSearcher;
16+
import org.opensearch.search.internal.SearchContext;
17+
18+
import java.io.IOException;
19+
import java.util.LinkedList;
20+
import java.util.List;
21+
import java.util.Optional;
22+
23+
import static org.opensearch.search.query.TopDocsCollectorContext.createTopDocsCollectorContext;
24+
25+
/**
26+
* Abstract base class for QueryPhaseSearcher implementations that provides
27+
* extension hook execution logic using the template pattern.
28+
*
29+
* @opensearch.internal
30+
*/
31+
@InternalApi
32+
public abstract class AbstractQueryPhaseSearcher implements QueryPhaseSearcher {
33+
34+
@Override
35+
public boolean searchWith(
36+
SearchContext searchContext,
37+
ContextIndexSearcher searcher,
38+
Query query,
39+
LinkedList<QueryCollectorContext> collectors,
40+
boolean hasFilterCollector,
41+
boolean hasTimeout
42+
) throws IOException {
43+
// Check if we have listeners using simple check
44+
if (!hasQueryPhaseListeners()) {
45+
// Fast path: no listeners, direct delegation
46+
return doSearchWith(searchContext, searcher, query, collectors, hasFilterCollector, hasTimeout);
47+
}
48+
49+
List<QueryPhaseListener> listeners = queryPhaseListeners();
50+
51+
// Optimize for single listener case
52+
if (listeners.size() == 1) {
53+
QueryPhaseListener listener = listeners.get(0);
54+
listener.beforeCollection(searchContext);
55+
boolean shouldRescore = doSearchWith(searchContext, searcher, query, collectors, hasFilterCollector, hasTimeout);
56+
listener.afterCollection(searchContext);
57+
return shouldRescore;
58+
}
59+
60+
for (QueryPhaseListener listener : listeners) {
61+
listener.beforeCollection(searchContext);
62+
}
63+
64+
boolean shouldRescore = doSearchWith(searchContext, searcher, query, collectors, hasFilterCollector, hasTimeout);
65+
66+
for (QueryPhaseListener listener : listeners) {
67+
listener.afterCollection(searchContext);
68+
}
69+
70+
return shouldRescore;
71+
}
72+
73+
/**
74+
* Template method for actual search implementation.
75+
* Subclasses must implement this to define their specific search behavior.
76+
*/
77+
protected abstract boolean doSearchWith(
78+
SearchContext searchContext,
79+
ContextIndexSearcher searcher,
80+
Query query,
81+
LinkedList<QueryCollectorContext> collectors,
82+
boolean hasFilterCollector,
83+
boolean hasTimeout
84+
) throws IOException;
85+
86+
/**
87+
* Common method to create QueryCollectorContext.
88+
*/
89+
protected QueryCollectorContext getQueryCollectorContext(SearchContext searchContext, boolean hasFilterCollector) throws IOException {
90+
// create the top docs collector last when the other collectors are known
91+
final Optional<QueryCollectorContext> queryCollectorContextOpt = QueryCollectorContextSpecRegistry.getQueryCollectorContextSpec(
92+
searchContext,
93+
new QueryCollectorArguments.Builder().hasFilterCollector(hasFilterCollector).build()
94+
).map(queryCollectorContextSpec -> new QueryCollectorContext(queryCollectorContextSpec.getContextName()) {
95+
@Override
96+
Collector create(Collector in) throws IOException {
97+
return queryCollectorContextSpec.create(in);
98+
}
99+
100+
@Override
101+
CollectorManager<?, ReduceableSearchResult> createManager(CollectorManager<?, ReduceableSearchResult> in) throws IOException {
102+
return queryCollectorContextSpec.createManager(in);
103+
}
104+
105+
@Override
106+
void postProcess(QuerySearchResult result) throws IOException {
107+
queryCollectorContextSpec.postProcess(result);
108+
}
109+
});
110+
if (queryCollectorContextOpt.isPresent()) {
111+
return queryCollectorContextOpt.get();
112+
} else {
113+
return createTopDocsCollectorContext(searchContext, hasFilterCollector);
114+
}
115+
}
116+
}

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

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -411,7 +411,7 @@ public static class TimeExceededException extends RuntimeException {
411411
*
412412
* @opensearch.internal
413413
*/
414-
public static class DefaultQueryPhaseSearcher implements QueryPhaseSearcher {
414+
public static class DefaultQueryPhaseSearcher extends AbstractQueryPhaseSearcher {
415415
private final AggregationProcessor aggregationProcessor;
416416

417417
/**
@@ -429,6 +429,22 @@ public boolean searchWith(
429429
LinkedList<QueryCollectorContext> collectors,
430430
boolean hasFilterCollector,
431431
boolean hasTimeout
432+
) throws IOException {
433+
if (!hasQueryPhaseListeners()) {
434+
// Fast path when no listeners
435+
return searchWithCollector(searchContext, searcher, query, collectors, hasFilterCollector, hasTimeout);
436+
}
437+
return super.searchWith(searchContext, searcher, query, collectors, hasFilterCollector, hasTimeout);
438+
}
439+
440+
@Override
441+
protected boolean doSearchWith(
442+
SearchContext searchContext,
443+
ContextIndexSearcher searcher,
444+
Query query,
445+
LinkedList<QueryCollectorContext> collectors,
446+
boolean hasFilterCollector,
447+
boolean hasTimeout
432448
) throws IOException {
433449
return searchWithCollector(searchContext, searcher, query, collectors, hasFilterCollector, hasTimeout);
434450
}
@@ -450,7 +466,7 @@ protected boolean searchWithCollector(
450466
return searchWithCollector(searchContext, searcher, query, collectors, queryCollectorContext, hasFilterCollector, hasTimeout);
451467
}
452468

453-
private QueryCollectorContext getQueryCollectorContext(SearchContext searchContext, boolean hasFilterCollector) throws IOException {
469+
protected QueryCollectorContext getQueryCollectorContext(SearchContext searchContext, boolean hasFilterCollector) throws IOException {
454470
// create the top docs collector last when the other collectors are known
455471
final Optional<QueryCollectorContext> queryCollectorContextOpt = QueryCollectorContextSpecRegistry.getQueryCollectorContextSpec(
456472
searchContext,
Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
/*
2+
* SPDX-License-Identifier: Apache-2.0
3+
*
4+
* The OpenSearch Contributors require contributions made to
5+
* this file be licensed under the Apache-2.0 license or a
6+
* compatible open source license.
7+
*/
8+
9+
package org.opensearch.search.query;
10+
11+
import org.opensearch.common.annotation.ExperimentalApi;
12+
import org.opensearch.search.internal.SearchContext;
13+
14+
/**
15+
* Listener interface for search query phase operations.
16+
* Allows plugins to hook into the query phase before and after document collection.
17+
*
18+
* @opensearch.experimental
19+
*/
20+
@ExperimentalApi
21+
public interface QueryPhaseListener {
22+
/**
23+
* Called before document collection begins.
24+
* This can be used to set up any state needed during collection.
25+
*
26+
* @param searchContext the search context for the current search request
27+
*/
28+
void beforeCollection(SearchContext searchContext);
29+
30+
/**
31+
* Called after document collection completes.
32+
* This can be used to perform cleanup or post-processing.
33+
*
34+
* @param searchContext the search context for the current search request
35+
*/
36+
void afterCollection(SearchContext searchContext);
37+
}

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,9 @@
1717
import org.opensearch.search.internal.SearchContext;
1818

1919
import java.io.IOException;
20+
import java.util.Collections;
2021
import java.util.LinkedList;
22+
import java.util.List;
2123

2224
/**
2325
* The extension point which allows to plug in custom search implementation to be
@@ -53,4 +55,22 @@ boolean searchWith(
5355
default AggregationProcessor aggregationProcessor(SearchContext searchContext) {
5456
return new DefaultAggregationProcessor();
5557
}
58+
59+
/**
60+
* Get the list of query phase listeners that should be executed before and after score collection.
61+
* @return list of query phase listeners, empty list if none
62+
*/
63+
default List<QueryPhaseListener> queryPhaseListeners() {
64+
return Collections.emptyList();
65+
}
66+
67+
/**
68+
* Check if this searcher has any query phase listeners registered.
69+
* This method allows for fast-path optimization to skip listener logic entirely.
70+
* @return true if there are listeners, false otherwise
71+
*/
72+
default boolean hasQueryPhaseListeners() {
73+
// Returning false by default allows JVM to optimize for common case
74+
return false;
75+
}
5676
}

0 commit comments

Comments
 (0)