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.

Sign up for GitHub

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

Optimize the canMatch phase on the data node #14511

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+242 −77
Open

Optimize the canMatch phase on the data node #14511

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
15d55bc
optimize the canMatch phase on the data node
bugmakerrrrrr Jun 23, 2024
27a1324
refactor and add test
bugmakerrrrrr Jun 28, 2024
8c91a12
apply review comments
bugmakerrrrrr Aug 6, 2024
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
- Add ThreadContextPermission for stashAndMergeHeaders and stashWithOrigin ([#15039](https://github.com/opensearch-project/OpenSearch/pull/15039))
- [Concurrent Segment Search] Support composite aggregations with scripting ([#15072](https://github.com/opensearch-project/OpenSearch/pull/15072))
- Add `rangeQuery` and `regexpQuery` for `constant_keyword` field type ([#14711](https://github.com/opensearch-project/OpenSearch/pull/14711))
- Optimize the canMatch phase on the data node ([#14511](https://github.com/opensearch-project/OpenSearch/pull/14511))

### Dependencies
- Bump `netty` from 4.1.111.Final to 4.1.112.Final ([#15081](https://github.com/opensearch-project/OpenSearch/pull/15081))
Expand Down
75 changes: 39 additions & 36 deletions server/src/main/java/org/opensearch/search/SearchService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,6 @@
import org.opensearch.index.query.InnerHitContextBuilder;
import org.opensearch.index.query.MatchAllQueryBuilder;
import org.opensearch.index.query.MatchNoneQueryBuilder;
import org.opensearch.index.query.QueryBuilder;
import org.opensearch.index.query.QueryRewriteContext;
import org.opensearch.index.query.QueryShardContext;
import org.opensearch.index.query.Rewriteable;
Expand Down Expand Up @@ -1597,8 +1596,7 @@ public CanMatchResponse canMatch(ShardSearchRequest request) throws IOException
private CanMatchResponse canMatch(ShardSearchRequest request, boolean checkRefreshPending) throws IOException {
assert request.searchType() == SearchType.QUERY_THEN_FETCH : "unexpected search type: " + request.searchType();
final ReaderContext readerContext = request.readerId() != null ? findReaderContext(request.readerId(), request) : null;
final Releasable markAsUsed = readerContext != null ? readerContext.markAsUsed(getKeepAlive(request)) : () -> {};
try (Releasable ignored = markAsUsed) {
try (Releasable ignored = readerContext != null ? readerContext.markAsUsed(getKeepAlive(request)) : () -> {}) {
final IndexService indexService;
final Engine.Searcher canMatchSearcher;
final boolean hasRefreshPending;
Expand All @@ -1621,22 +1619,35 @@ private CanMatchResponse canMatch(ShardSearchRequest request, boolean checkRefre
request.getClusterAlias()
);
Rewriteable.rewrite(request.getRewriteable(), context, false);
final boolean aliasFilterCanMatch = request.getAliasFilter().getQueryBuilder() instanceof MatchNoneQueryBuilder == false;
FieldSortBuilder sortBuilder = FieldSortBuilder.getPrimaryFieldSortOrNull(request.source());
MinAndMax<?> minMax = sortBuilder != null ? FieldSortBuilder.getMinMaxOrNull(context, sortBuilder) : null;
boolean canMatch;
if (canRewriteToMatchNone(request.source())) {
QueryBuilder queryBuilder = request.source().query();
canMatch = aliasFilterCanMatch && queryBuilder instanceof MatchNoneQueryBuilder == false;
} else {
// null query means match_all
canMatch = aliasFilterCanMatch;

if (hasRefreshPending == false) {
final boolean aliasFilterCannotMatch = request.getAliasFilter().getQueryBuilder() instanceof MatchNoneQueryBuilder;
if (aliasFilterCannotMatch
|| (canRewriteToMatchNone(request.source()) && request.source().query() instanceof MatchNoneQueryBuilder)) {
return new CanMatchResponse(false, null);
}
}
final FieldDoc searchAfterFieldDoc = getSearchAfterFieldDoc(request, context);
final Integer trackTotalHitsUpto = request.source() == null ? null : request.source().trackTotalHitsUpTo();
canMatch = canMatch && canMatchSearchAfter(searchAfterFieldDoc, minMax, sortBuilder, trackTotalHitsUpto);

return new CanMatchResponse(canMatch || hasRefreshPending, minMax);
final FieldSortBuilder sortBuilder = FieldSortBuilder.getPrimaryFieldSortOrNull(request.source());
final MinAndMax<?> minMax = sortBuilder != null ? FieldSortBuilder.getMinMaxOrNull(context, sortBuilder) : null;
if (hasRefreshPending || minMax == null) {
return new CanMatchResponse(true, minMax);
}

boolean canMatch = true;
// Skipping search on shard/segment entirely can cause mismatch on total_tracking_hits, hence skip only if
// track_total_hits is false.
// Check for sort.missing == null, since in case of missing values sort queries, if segment/shard's min/max
// is out of search_after range, it still should be printed and hence we should not skip segment/shard.
final Integer trackTotalHitsUpto = request.source() == null ? null : request.source().trackTotalHitsUpTo();
if (Objects.equals(trackTotalHitsUpto, SearchContext.TRACK_TOTAL_HITS_DISABLED) && sortBuilder.missing() == null) {
final Object primarySearchAfterField = SearchAfterBuilder.getPrimarySearchAfterFieldOrNull(request.source());
if (primarySearchAfterField != null) {
final FieldDoc searchAfterFieldDoc = getPrimarySearchAfterFieldDoc(sortBuilder, primarySearchAfterField, context);
canMatch = canMatchSearchAfter(searchAfterFieldDoc, minMax, sortBuilder, trackTotalHitsUpto);
}
}
return new CanMatchResponse(canMatch, canMatch ? minMax : null);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, can we just not send over minMax? Should get ignored if canMatch is false anyway?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that setting minMax as null is equivalent to not sending this value.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we probably should not even care what we are sending for minMax if canMatch is false? So below should also work?

new CanMatchResponse(canMatch, minMax);

}
}
}
Expand All @@ -1647,15 +1658,9 @@ public static boolean canMatchSearchAfter(
FieldSortBuilder primarySortField,
Integer trackTotalHitsUpto
) {
// Check for sort.missing == null, since in case of missing values sort queries, if segment/shard's min/max
// is out of search_after range, it still should be printed and hence we should not skip segment/shard.
// Skipping search on shard/segment entirely can cause mismatch on total_tracking_hits, hence skip only if
// track_total_hits is false.
if (searchAfter != null
&& minMax != null
&& primarySortField != null
&& primarySortField.missing() == null
&& Objects.equals(trackTotalHitsUpto, SearchContext.TRACK_TOTAL_HITS_DISABLED)) {
assert primarySortField != null && primarySortField.missing() == null;
assert Objects.equals(trackTotalHitsUpto, SearchContext.TRACK_TOTAL_HITS_DISABLED);
if (searchAfter != null && minMax != null) {
final Object searchAfterPrimary = searchAfter.fields[0];
if (primarySortField.order() == SortOrder.DESC) {
if (minMax.compareMin(searchAfterPrimary) > 0) {
Expand All @@ -1672,16 +1677,14 @@ public static boolean canMatchSearchAfter(
return true;
}

private static FieldDoc getSearchAfterFieldDoc(ShardSearchRequest request, QueryShardContext context) throws IOException {
if (context != null && request != null && request.source() != null && request.source().sorts() != null) {
final List<SortBuilder<?>> sorts = request.source().sorts();
final Object[] searchAfter = request.source().searchAfter();
final Optional<SortAndFormats> sortOpt = SortBuilder.buildSort(sorts, context);
if (sortOpt.isPresent() && !CollectionUtils.isEmpty(searchAfter)) {
return SearchAfterBuilder.buildFieldDoc(sortOpt.get(), searchAfter);
}
}
return null;
private static FieldDoc getPrimarySearchAfterFieldDoc(
FieldSortBuilder primarySortBuilder,
Object primarySearchAfter,
QueryShardContext context
) throws IOException {
final Optional<SortAndFormats> sortOpt = SortBuilder.buildSort(List.of(primarySortBuilder), context);
return sortOpt.map(sortAndFormats -> SearchAfterBuilder.buildFieldDoc(sortAndFormats, new Object[] { primarySearchAfter }))
.orElse(null);
Comment on lines +1680 to +1687
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unable to see any optimization from this refactoring. Seems more like restructuring and not sure if the readability is improving.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous implementation would construct all sortFields if user request contains multi SortBuilders and search after fields, but what we need is just the primary sort search after field.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to qualitatively quantify the impact? Maybe some approximation of the worst case query earlier that would be improved with this change

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this is quite simple, for example, if I have following query:

{
  "query": {
    "match_all": {}
  },
  "track_total_hits": false,
  "sort": [
    {
      "grade": "desc"
    },
    {
      "age": "asc"
    },
    {
      "id": "asc"
    }
  ],
  "search_after": [
    10,
    20,
    12345
  ]
}

Previously, we have to evaluate and build these all three sort fields, now we just need to build the primary sort field. The idea behind this change is same as FieldSortBuilder.getPrimaryFieldSortOrNull.

}

/**
Expand Down
11 changes: 9 additions & 2 deletions server/src/main/java/org/opensearch/search/internal/ContextIndexSearcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,10 +513,17 @@ private boolean canMatch(LeafReaderContext ctx) throws IOException {
}

private boolean canMatchSearchAfter(LeafReaderContext ctx) throws IOException {
if (searchContext.searchAfter() != null && searchContext.request() != null && searchContext.request().source() != null) {
// Skipping search on shard/segment entirely can cause mismatch on total_tracking_hits, hence skip only if
// track_total_hits is false.
if (searchContext.searchAfter() != null
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jainankitk The potential improvements are mostly related to short circuit. For example, we can check the conditions upfront here and avoid to build sort field or read min max values unnecessary.

&& searchContext.request() != null
&& searchContext.request().source() != null
&& searchContext.trackTotalHitsUpTo() == SearchContext.TRACK_TOTAL_HITS_DISABLED) {
// Only applied on primary sort field and primary search_after.
FieldSortBuilder primarySortField = FieldSortBuilder.getPrimaryFieldSortOrNull(searchContext.request().source());
if (primarySortField != null) {
// Check for sort.missing == null, since in case of missing values sort queries, if segment/shard's min/max
// is out of search_after range, it still should be printed and hence we should not skip segment/shard.
if (primarySortField != null && primarySortField.missing() == null) {
MinAndMax<?> minMax = FieldSortBuilder.getMinMaxOrNullForSegment(
this.searchContext.getQueryShardContext(),
ctx,
Expand Down
9 changes: 9 additions & 0 deletions server/src/main/java/org/opensearch/search/searchafter/SearchAfterBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,13 @@
import org.opensearch.core.common.io.stream.StreamOutput;
import org.opensearch.core.common.io.stream.Writeable;
import org.opensearch.core.common.text.Text;
import org.opensearch.core.common.util.CollectionUtils;
import org.opensearch.core.xcontent.ToXContentObject;
import org.opensearch.core.xcontent.XContentBuilder;
import org.opensearch.core.xcontent.XContentParser;
import org.opensearch.index.fielddata.IndexFieldData;
import org.opensearch.search.DocValueFormat;
import org.opensearch.search.builder.SearchSourceBuilder;
import org.opensearch.search.sort.SortAndFormats;

import java.io.IOException;
Expand Down Expand Up @@ -148,6 +150,13 @@
return new FieldDoc(Integer.MAX_VALUE, 0, fieldValues);
}

public static Object getPrimarySearchAfterFieldOrNull(SearchSourceBuilder source) {
if (source == null || CollectionUtils.isEmpty(source.searchAfter())) {
return null;

Check warning on line 155 in server/src/main/java/org/opensearch/search/searchafter/SearchAfterBuilder.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/search/searchafter/SearchAfterBuilder.java#L155 

Added line #L155 was not covered by tests
}
return source.searchAfter()[0];
}

/**
* Returns the inner {@link SortField.Type} expected for this sort field.
*/
Expand Down
3 changes: 2 additions & 1 deletion server/src/main/java/org/opensearch/search/sort/FieldSortBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import org.opensearch.core.common.ParsingException;
import org.opensearch.core.common.io.stream.StreamInput;
import org.opensearch.core.common.io.stream.StreamOutput;
import org.opensearch.core.common.util.CollectionUtils;
import org.opensearch.core.xcontent.ObjectParser;
import org.opensearch.core.xcontent.ObjectParser.ValueType;
import org.opensearch.core.xcontent.XContent;
Expand Down Expand Up @@ -599,7 +600,7 @@ public static boolean hasPrimaryFieldSort(SearchSourceBuilder source) {
* is an instance of this class, null otherwise.
*/
public static FieldSortBuilder getPrimaryFieldSortOrNull(SearchSourceBuilder source) {
if (source == null || source.sorts() == null || source.sorts().isEmpty()) {
if (source == null || CollectionUtils.isEmpty(source.sorts())) {
return null;
}
return source.sorts().get(0) instanceof FieldSortBuilder ? (FieldSortBuilder) source.sorts().get(0) : null;
Expand Down
Loading
Loading