Skip to content
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

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
- [BWC and API enforcement] Reconsider the breaking changes check policy to detect breaking changes against released versions ([#13292](https://github.com/opensearch-project/OpenSearch/pull/13292))
- Switch to macos-13 runner for precommit and assemble github actions due to macos-latest is now arm64 ([#13412](https://github.com/opensearch-project/OpenSearch/pull/13412))
- [Revert] Prevent unnecessary fetch sub phase processor initialization during fetch phase execution ([#12503](https://github.com/opensearch-project/OpenSearch/pull/12503))
- Refactor implementations of query phase searcher, allow QueryCollectorContext to have zero collectors ([#13481](https://github.com/opensearch-project/OpenSearch/pull/13481))

### Deprecated

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,9 @@

import java.io.IOException;
import java.util.LinkedList;
import java.util.Objects;
import java.util.concurrent.ExecutionException;

import static org.opensearch.search.query.TopDocsCollectorContext.createTopDocsCollectorContext;

/**
* The implementation of the {@link QueryPhaseSearcher} which attempts to use concurrent
* search of Apache Lucene segments if it has been enabled.
Expand All @@ -46,24 +45,32 @@
ContextIndexSearcher searcher,
Query query,
LinkedList<QueryCollectorContext> collectors,
QueryCollectorContext queryCollectorContext,
boolean hasFilterCollector,
boolean hasTimeout
) throws IOException {
return searchWithCollectorManager(searchContext, searcher, query, collectors, hasFilterCollector, hasTimeout);
return searchWithCollectorManager(
searchContext,
searcher,
query,
collectors,
queryCollectorContext,
hasFilterCollector,
hasTimeout
);
}

private static boolean searchWithCollectorManager(
SearchContext searchContext,
ContextIndexSearcher searcher,
Query query,
LinkedList<QueryCollectorContext> collectorContexts,
QueryCollectorContext queryCollectorContext,
boolean hasFilterCollector,
boolean timeoutSet
) throws IOException {
// create the top docs collector last when the other collectors are known
final TopDocsCollectorContext topDocsFactory = createTopDocsCollectorContext(searchContext, hasFilterCollector);
// add the top docs collector, the first collector context in the chain
collectorContexts.addFirst(topDocsFactory);
// add the passed collector, the first collector context in the chain
collectorContexts.addFirst(Objects.requireNonNull(queryCollectorContext));

final QuerySearchResult queryResult = searchContext.queryResult();
final CollectorManager<?, ReduceableSearchResult> collectorManager;
Expand Down Expand Up @@ -95,7 +102,10 @@
queryResult.terminatedEarly(false);
}

return topDocsFactory.shouldRescore();
if (queryCollectorContext instanceof RescoringQueryCollectorContext) {
return ((RescoringQueryCollectorContext) queryCollectorContext).shouldRescore();
}
return false;

Check warning on line 108 in server/src/main/java/org/opensearch/search/query/ConcurrentQueryPhaseSearcher.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L108 was not covered by tests
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,29 @@
}
};

public static final QueryCollectorContext EMPTY_CONTEXT = new QueryCollectorContext("empty") {

@Override
Collector create(Collector in) throws IOException {
return EMPTY_COLLECTOR;

Check warning on line 84 in server/src/main/java/org/opensearch/search/query/QueryCollectorContext.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/search/query/QueryCollectorContext.java#L84

Added line #L84 was not covered by tests
}

@Override
CollectorManager<?, ReduceableSearchResult> createManager(CollectorManager<?, ReduceableSearchResult> in) throws IOException {
return new CollectorManager<Collector, ReduceableSearchResult>() {

Check warning on line 89 in server/src/main/java/org/opensearch/search/query/QueryCollectorContext.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/search/query/QueryCollectorContext.java#L89

Added line #L89 was not covered by tests
@Override
public Collector newCollector() throws IOException {
return EMPTY_COLLECTOR;

Check warning on line 92 in server/src/main/java/org/opensearch/search/query/QueryCollectorContext.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/search/query/QueryCollectorContext.java#L92

Added line #L92 was not covered by tests
}

@Override
public ReduceableSearchResult reduce(Collection<Collector> collectors) throws IOException {
return result -> {};

Check warning on line 97 in server/src/main/java/org/opensearch/search/query/QueryCollectorContext.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/search/query/QueryCollectorContext.java#L97

Added line #L97 was not covered by tests
}
};
}
};

private String profilerName;

QueryCollectorContext(String profilerName) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -335,13 +335,12 @@
ContextIndexSearcher searcher,
Query query,
LinkedList<QueryCollectorContext> collectors,
QueryCollectorContext queryCollectorContext,
boolean hasFilterCollector,
boolean timeoutSet
) throws IOException {
// create the top docs collector last when the other collectors are known
final TopDocsCollectorContext topDocsFactory = createTopDocsCollectorContext(searchContext, hasFilterCollector);
// add the top docs collector, the first collector context in the chain
collectors.addFirst(topDocsFactory);
// add passed collector, the first collector context in the chain
collectors.addFirst(Objects.requireNonNull(queryCollectorContext));

final Collector queryCollector;
if (searchContext.getProfilers() != null) {
Expand Down Expand Up @@ -370,7 +369,10 @@
for (QueryCollectorContext ctx : collectors) {
ctx.postProcess(queryResult);
}
return topDocsFactory.shouldRescore();
if (queryCollectorContext instanceof RescoringQueryCollectorContext) {
return ((RescoringQueryCollectorContext) queryCollectorContext).shouldRescore();
}
return false;

Check warning on line 375 in server/src/main/java/org/opensearch/search/query/QueryPhase.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L375 was not covered by tests
}

/**
Expand Down Expand Up @@ -440,7 +442,29 @@
boolean hasFilterCollector,
boolean hasTimeout
) throws IOException {
return QueryPhase.searchWithCollector(searchContext, searcher, query, collectors, hasFilterCollector, hasTimeout);
// create the top docs collector last when the other collectors are known
final TopDocsCollectorContext topDocsFactory = createTopDocsCollectorContext(searchContext, hasFilterCollector);
return searchWithCollector(searchContext, searcher, query, collectors, topDocsFactory, hasFilterCollector, hasTimeout);
}
martin-gaievski marked this conversation as resolved.
Show resolved Hide resolved

protected boolean searchWithCollector(
martin-gaievski marked this conversation as resolved.
Show resolved Hide resolved
SearchContext searchContext,
ContextIndexSearcher searcher,
Query query,
LinkedList<QueryCollectorContext> collectors,
QueryCollectorContext queryCollectorContext,
boolean hasFilterCollector,
boolean hasTimeout
) throws IOException {
return QueryPhase.searchWithCollector(
searchContext,
searcher,
query,
collectors,
queryCollectorContext,
hasFilterCollector,
hasTimeout
);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*/

package org.opensearch.search.query;

import org.opensearch.common.annotation.PublicApi;

/**
* Abstraction that allows indication of whether results should be rescored or not based on
* custom logic of exact {@link QueryCollectorContext} implementation.
*
* @opensearch.api
*/
@PublicApi(since = "2.15.0")
public interface RescoringQueryCollectorContext {

/**
* Indicates if results from the query context should be rescored
* @return true if results must be rescored, false otherwise
*/
boolean shouldRescore();
}
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@
*
* @opensearch.internal
*/
public abstract class TopDocsCollectorContext extends QueryCollectorContext {
public abstract class TopDocsCollectorContext extends QueryCollectorContext implements RescoringQueryCollectorContext {
protected final int numHits;

TopDocsCollectorContext(String profilerName, int numHits) {
Expand Down
Loading