From 15d55bc5ce880b996850a1f8f97ce131696608f1 Mon Sep 17 00:00:00 2001 From: panguixin Date: Sun, 23 Jun 2024 15:48:50 +0800 Subject: [PATCH 1/3] optimize the canMatch phase on the data node Signed-off-by: panguixin --- .../org/opensearch/search/SearchService.java | 60 +++++++++++-------- .../searchafter/SearchAfterBuilder.java | 9 +++ .../search/sort/FieldSortBuilder.java | 3 +- 3 files changed, 45 insertions(+), 27 deletions(-) diff --git a/server/src/main/java/org/opensearch/search/SearchService.java b/server/src/main/java/org/opensearch/search/SearchService.java index a53a7198c366f..06321d1a7dcc5 100644 --- a/server/src/main/java/org/opensearch/search/SearchService.java +++ b/server/src/main/java/org/opensearch/search/SearchService.java @@ -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; @@ -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; @@ -1621,22 +1619,34 @@ private CanMatchResponse canMatch(ShardSearchRequest request, boolean checkRefre request.getClusterAlias() ); Rewriteable.rewrite(request.getRewriteable(), context, false); + + if (hasRefreshPending) { + final FieldSortBuilder sortBuilder = FieldSortBuilder.getPrimaryFieldSortOrNull(request.source()); + final MinAndMax minMax = sortBuilder != null ? FieldSortBuilder.getMinMaxOrNull(context, sortBuilder) : null; + return new CanMatchResponse(true, minMax); + } + 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 (aliasFilterCanMatch == false) { + return new CanMatchResponse(false, null); + } + + // null query means match_all + boolean canMatch = canRewriteToMatchNone(request.source()) == false + || request.source().query() instanceof MatchNoneQueryBuilder == false; + if (canMatch == false) { + 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; + final Object primarySearchAfterField = SearchAfterBuilder.getPrimarySearchAfterFieldOrNull(request.source()); + if (minMax != null && primarySearchAfterField != null) { + final FieldDoc searchAfterFieldDoc = getPrimarySearchAfterFieldDoc(sortBuilder, primarySearchAfterField, context); + final Integer trackTotalHitsUpto = request.source() == null ? null : request.source().trackTotalHitsUpTo(); + canMatch = canMatchSearchAfter(searchAfterFieldDoc, minMax, sortBuilder, trackTotalHitsUpto); + } + return new CanMatchResponse(canMatch, minMax); } } } @@ -1672,16 +1682,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> sorts = request.source().sorts(); - final Object[] searchAfter = request.source().searchAfter(); - final Optional 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 sortOpt = SortBuilder.buildSort(List.of(primarySortBuilder), context); + return sortOpt.map(sortAndFormats -> SearchAfterBuilder.buildFieldDoc(sortAndFormats, new Object[] { primarySearchAfter })) + .orElse(null); } /** diff --git a/server/src/main/java/org/opensearch/search/searchafter/SearchAfterBuilder.java b/server/src/main/java/org/opensearch/search/searchafter/SearchAfterBuilder.java index a45b2bd40c03d..3c648e00e2af5 100644 --- a/server/src/main/java/org/opensearch/search/searchafter/SearchAfterBuilder.java +++ b/server/src/main/java/org/opensearch/search/searchafter/SearchAfterBuilder.java @@ -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; @@ -148,6 +150,13 @@ public static FieldDoc buildFieldDoc(SortAndFormats sort, Object[] values) { return new FieldDoc(Integer.MAX_VALUE, 0, fieldValues); } + public static Object getPrimarySearchAfterFieldOrNull(SearchSourceBuilder source) { + if (source == null || CollectionUtils.isEmpty(source.searchAfter())) { + return null; + } + return source.searchAfter()[0]; + } + /** * Returns the inner {@link SortField.Type} expected for this sort field. */ diff --git a/server/src/main/java/org/opensearch/search/sort/FieldSortBuilder.java b/server/src/main/java/org/opensearch/search/sort/FieldSortBuilder.java index 5cecda1346b90..8f0a8acba39bf 100644 --- a/server/src/main/java/org/opensearch/search/sort/FieldSortBuilder.java +++ b/server/src/main/java/org/opensearch/search/sort/FieldSortBuilder.java @@ -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; @@ -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; From 27a132406350b9a6899f9eb359c3a4957b3a41d1 Mon Sep 17 00:00:00 2001 From: panguixin Date: Sat, 29 Jun 2024 00:28:14 +0800 Subject: [PATCH 2/3] refactor and add test Signed-off-by: panguixin --- .../org/opensearch/search/SearchService.java | 43 ++-- .../search/internal/ContextIndexSearcher.java | 11 +- .../opensearch/search/SearchServiceTests.java | 220 +++++++++++++++--- 3 files changed, 211 insertions(+), 63 deletions(-) diff --git a/server/src/main/java/org/opensearch/search/SearchService.java b/server/src/main/java/org/opensearch/search/SearchService.java index 06321d1a7dcc5..845cd259afd98 100644 --- a/server/src/main/java/org/opensearch/search/SearchService.java +++ b/server/src/main/java/org/opensearch/search/SearchService.java @@ -1627,26 +1627,29 @@ private CanMatchResponse canMatch(ShardSearchRequest request, boolean checkRefre } final boolean aliasFilterCanMatch = request.getAliasFilter().getQueryBuilder() instanceof MatchNoneQueryBuilder == false; - if (aliasFilterCanMatch == false) { - return new CanMatchResponse(false, null); - } - - // null query means match_all - boolean canMatch = canRewriteToMatchNone(request.source()) == false - || request.source().query() instanceof MatchNoneQueryBuilder == false; - if (canMatch == false) { + if (aliasFilterCanMatch == false + || (canRewriteToMatchNone(request.source()) && request.source().query() instanceof MatchNoneQueryBuilder)) { return new CanMatchResponse(false, null); } + boolean canMatch = true; final FieldSortBuilder sortBuilder = FieldSortBuilder.getPrimaryFieldSortOrNull(request.source()); final MinAndMax minMax = sortBuilder != null ? FieldSortBuilder.getMinMaxOrNull(context, sortBuilder) : null; - final Object primarySearchAfterField = SearchAfterBuilder.getPrimarySearchAfterFieldOrNull(request.source()); - if (minMax != null && primarySearchAfterField != null) { - final FieldDoc searchAfterFieldDoc = getPrimarySearchAfterFieldDoc(sortBuilder, primarySearchAfterField, context); - final Integer trackTotalHitsUpto = request.source() == null ? null : request.source().trackTotalHitsUpTo(); - canMatch = canMatchSearchAfter(searchAfterFieldDoc, minMax, sortBuilder, trackTotalHitsUpto); + final Integer trackTotalHitsUpto = request.source() == null ? null : request.source().trackTotalHitsUpTo(); + // 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. + if (Objects.equals(trackTotalHitsUpto, SearchContext.TRACK_TOTAL_HITS_DISABLED) + && minMax != null + && 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, minMax); + return new CanMatchResponse(canMatch, canMatch ? minMax : null); } } } @@ -1657,15 +1660,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) { diff --git a/server/src/main/java/org/opensearch/search/internal/ContextIndexSearcher.java b/server/src/main/java/org/opensearch/search/internal/ContextIndexSearcher.java index ec3ed2332d0b8..32619a2726bfc 100644 --- a/server/src/main/java/org/opensearch/search/internal/ContextIndexSearcher.java +++ b/server/src/main/java/org/opensearch/search/internal/ContextIndexSearcher.java @@ -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 + && searchContext.request() != null + && searchContext.request().source() != null + && Objects.equals(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, diff --git a/server/src/test/java/org/opensearch/search/SearchServiceTests.java b/server/src/test/java/org/opensearch/search/SearchServiceTests.java index 1caa2c99fc3b8..c3aa98e83dfe3 100644 --- a/server/src/test/java/org/opensearch/search/SearchServiceTests.java +++ b/server/src/test/java/org/opensearch/search/SearchServiceTests.java @@ -39,6 +39,7 @@ import org.apache.lucene.store.AlreadyClosedException; import org.opensearch.OpenSearchException; import org.opensearch.action.OriginalIndices; +import org.opensearch.action.bulk.BulkRequestBuilder; import org.opensearch.action.index.IndexResponse; import org.opensearch.action.search.ClearScrollRequest; import org.opensearch.action.search.DeletePitResponse; @@ -54,6 +55,7 @@ import org.opensearch.action.support.IndicesOptions; import org.opensearch.action.support.PlainActionFuture; import org.opensearch.action.support.WriteRequest; +import org.opensearch.common.CheckedTriFunction; import org.opensearch.common.UUIDs; import org.opensearch.common.settings.Settings; import org.opensearch.common.settings.SettingsException; @@ -66,6 +68,7 @@ import org.opensearch.core.index.Index; import org.opensearch.core.index.shard.ShardId; import org.opensearch.core.rest.RestStatus; +import org.opensearch.core.xcontent.MediaTypeRegistry; import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.index.IndexModule; import org.opensearch.index.IndexNotFoundException; @@ -108,6 +111,8 @@ import org.opensearch.search.query.QuerySearchResult; import org.opensearch.search.sort.FieldSortBuilder; import org.opensearch.search.sort.MinAndMax; +import org.opensearch.search.sort.ScoreSortBuilder; +import org.opensearch.search.sort.SortBuilders; import org.opensearch.search.sort.SortOrder; import org.opensearch.search.suggest.SuggestBuilder; import org.opensearch.test.OpenSearchSingleNodeTestCase; @@ -116,6 +121,7 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.LinkedList; @@ -1134,6 +1140,176 @@ public void onFailure(Exception e) { latch.await(); } + public void testCanMatchWithPendingRefresh() throws Exception { + IndexService indexService = createIndex( + "test", + Settings.builder().put(IndexSettings.INDEX_SEARCH_IDLE_AFTER.getKey(), TimeValue.ZERO).build() + ); + assertFalse(indexService.getIndexSettings().isExplicitRefresh()); + ensureGreen(); + final IndexShard shard = indexService.getShard(0); + + final BulkRequestBuilder bulkRequestBuilder = client().prepareBulk(); + final int numDocs = randomIntBetween(10, 20); + final long[] nums = new long[numDocs]; + for (int i = 0; i < numDocs; i++) { + nums[i] = randomIntBetween(0, 100); + bulkRequestBuilder.add( + client().prepareIndex("test") + .setId(String.valueOf(i)) + .setSource(String.format(Locale.ROOT, "{\"num\": %d}", nums[i]), MediaTypeRegistry.JSON) + ); + } + bulkRequestBuilder.get(); + client().admin().indices().prepareRefresh().get(); + assertFalse(shard.hasRefreshPending()); + + client().prepareIndex("test").setId(String.valueOf(numDocs)).setSource("{\"foo\" : \"bar\"}", MediaTypeRegistry.JSON).get(); + assertFalse(shard.scheduledRefresh()); + assertTrue(shard.hasRefreshPending()); + + final SearchService service = getInstanceFromNode(SearchService.class); + final SearchRequest searchRequest = new SearchRequest().allowPartialSearchResults(true); + searchRequest.source(new SearchSourceBuilder().query(new MatchNoneQueryBuilder())); + searchRequest.source().sort("num"); + + SearchService.CanMatchResponse canMatchResponse = service.canMatch( + new ShardSearchRequest( + OriginalIndices.NONE, + searchRequest, + shard.shardId(), + 1, + new AliasFilter(null, Strings.EMPTY_ARRAY), + 1f, + -1, + null, + null + ) + ); + assertTrue(canMatchResponse.canMatch()); + Arrays.sort(nums); + assertEquals(nums[0], canMatchResponse.estimatedMinAndMax().getMin()); + assertEquals(nums[numDocs - 1], canMatchResponse.estimatedMinAndMax().getMax()); + } + + public void testCanMatchWithSort() throws Exception { + IndexService indexService = createIndex("test"); + ensureGreen(); + + final BulkRequestBuilder bulkRequestBuilder = client().prepareBulk(); + final int numDocs = randomIntBetween(10, 20); + final long[] nums1 = new long[numDocs]; + final long[] nums2 = new long[numDocs]; + for (int i = 0; i < numDocs; i++) { + nums1[i] = randomIntBetween(0, 100); + nums2[i] = randomIntBetween(100, 200); + bulkRequestBuilder.add( + client().prepareIndex("test") + .setId(String.valueOf(i)) + .setSource(String.format(Locale.ROOT, "{\"num1\": %d, \"num2\": %d}", nums1[i], nums2[i]), MediaTypeRegistry.JSON) + ); + } + bulkRequestBuilder.get(); + client().admin().indices().prepareRefresh().get(); + Arrays.sort(nums1); + Arrays.sort(nums2); + + final IndexShard shard = indexService.getShard(0); + final SearchService service = getInstanceFromNode(SearchService.class); + final ShardSearchRequest shardRequest = new ShardSearchRequest( + OriginalIndices.NONE, + new SearchRequest().allowPartialSearchResults(true), + shard.shardId(), + 1, + new AliasFilter(null, Strings.EMPTY_ARRAY), + 1f, + -1, + null, + null + ); + assertFalse(shard.hasRefreshPending()); + + // the primary sort is score sort + shardRequest.source(new SearchSourceBuilder().query(new MatchAllQueryBuilder())); + shardRequest.source().sort(ScoreSortBuilder.NAME); + if (randomBoolean()) { + shardRequest.source().sort("num1"); + } + SearchService.CanMatchResponse canMatchResponse = service.canMatch(shardRequest); + assertTrue(canMatchResponse.canMatch()); + assertNull(canMatchResponse.estimatedMinAndMax()); + + // the primary sort is field sort + shardRequest.source(new SearchSourceBuilder().query(new MatchAllQueryBuilder())); + shardRequest.source().sort("num2"); + if (randomBoolean()) { + shardRequest.source().sort("num1"); + } + canMatchResponse = service.canMatch(shardRequest); + assertTrue(canMatchResponse.canMatch()); + assertEquals(nums2[0], canMatchResponse.estimatedMinAndMax().getMin()); + assertEquals(nums2[numDocs - 1], canMatchResponse.estimatedMinAndMax().getMax()); + + // the primary searchAfter is greater than max or less than min + final CheckedTriFunction outOfRangeTester = (missingNull, traceTotalHits, reverse) -> { + shardRequest.source(new SearchSourceBuilder().query(new MatchAllQueryBuilder()).trackTotalHits(traceTotalHits)); + shardRequest.source() + .sort(SortBuilders.fieldSort("num1").missing(missingNull ? null : 1).order(reverse ? SortOrder.DESC : SortOrder.ASC)); + List searchAfterFields = new ArrayList<>(); + searchAfterFields.add(reverse ? nums1[0] - 1 : nums1[numDocs - 1] + 1); + if (randomBoolean()) { + shardRequest.source().sort("num2"); + searchAfterFields.add(randomLong()); + } + shardRequest.source().searchAfter(searchAfterFields.toArray()); + SearchService.CanMatchResponse response = service.canMatch(shardRequest); + if (missingNull && traceTotalHits == false) { + assertFalse(response.canMatch()); + assertNull(response.estimatedMinAndMax()); + } else { + assertTrue(response.canMatch()); + assertEquals(nums1[0], response.estimatedMinAndMax().getMin()); + assertEquals(nums1[numDocs - 1], response.estimatedMinAndMax().getMax()); + } + return null; + }; + for (boolean missingNull : new boolean[] { true, false }) { + for (boolean traceTotalHit : new boolean[] { true, false }) { + for (boolean reverse : new boolean[] { true, false }) { + outOfRangeTester.apply(missingNull, traceTotalHit, reverse); + } + } + } + + // the primary searchAfter is within the range of min and max + final CheckedTriFunction withinRangeTester = (missingNull, traceTotalHits, reverse) -> { + shardRequest.source(new SearchSourceBuilder().query(new MatchAllQueryBuilder()).trackTotalHits(traceTotalHits)); + shardRequest.source() + .sort(SortBuilders.fieldSort("num2").missing(missingNull ? null : 1).order(reverse ? SortOrder.DESC : SortOrder.ASC)); + List searchAfterFields = new ArrayList<>(); + searchAfterFields.add(randomLongBetween(nums2[0], nums2[numDocs - 1])); + if (randomBoolean()) { + shardRequest.source().sort("num1"); + searchAfterFields.add(randomLong()); + } + shardRequest.source().searchAfter(searchAfterFields.toArray()); + SearchService.CanMatchResponse response = service.canMatch(shardRequest); + + assertTrue(response.canMatch()); + assertEquals(nums2[0], response.estimatedMinAndMax().getMin()); + assertEquals(nums2[numDocs - 1], response.estimatedMinAndMax().getMax()); + + return null; + }; + for (boolean missingNull : new boolean[] { true, false }) { + for (boolean traceTotalHit : new boolean[] { true, false }) { + for (boolean reverse : new boolean[] { true, false }) { + withinRangeTester.apply(missingNull, traceTotalHit, reverse); + } + } + } + } + public void testCanRewriteToMatchNone() { assertFalse( SearchService.canRewriteToMatchNone( @@ -1877,7 +2053,7 @@ public void testCanMatchSearchAfterAscGreaterThanMax() throws IOException { MinAndMax minMax = new MinAndMax(0L, 9L); FieldSortBuilder primarySort = new FieldSortBuilder("test"); primarySort.order(SortOrder.ASC); - assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort, SearchContext.TRACK_TOTAL_HITS_DISABLED), false); + assertFalse(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort, SearchContext.TRACK_TOTAL_HITS_DISABLED)); } /** @@ -1890,7 +2066,7 @@ public void testCanMatchSearchAfterAscLessThanMax() throws IOException { MinAndMax minMax = new MinAndMax(0L, 9L); FieldSortBuilder primarySort = new FieldSortBuilder("test"); primarySort.order(SortOrder.ASC); - assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort, SearchContext.TRACK_TOTAL_HITS_DISABLED), true); + assertTrue(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort, SearchContext.TRACK_TOTAL_HITS_DISABLED)); } /** @@ -1903,7 +2079,7 @@ public void testCanMatchSearchAfterAscEqualMax() throws IOException { MinAndMax minMax = new MinAndMax(0L, 9L); FieldSortBuilder primarySort = new FieldSortBuilder("test"); primarySort.order(SortOrder.ASC); - assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort, SearchContext.TRACK_TOTAL_HITS_DISABLED), true); + assertTrue(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort, SearchContext.TRACK_TOTAL_HITS_DISABLED)); } /** @@ -1916,7 +2092,7 @@ public void testCanMatchSearchAfterDescGreaterThanMin() throws IOException { MinAndMax minMax = new MinAndMax(0L, 9L); FieldSortBuilder primarySort = new FieldSortBuilder("test"); primarySort.order(SortOrder.DESC); - assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort, SearchContext.TRACK_TOTAL_HITS_DISABLED), true); + assertTrue(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort, SearchContext.TRACK_TOTAL_HITS_DISABLED)); } /** @@ -1929,7 +2105,7 @@ public void testCanMatchSearchAfterDescLessThanMin() throws IOException { MinAndMax minMax = new MinAndMax(0L, 9L); FieldSortBuilder primarySort = new FieldSortBuilder("test"); primarySort.order(SortOrder.DESC); - assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort, SearchContext.TRACK_TOTAL_HITS_DISABLED), false); + assertFalse(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort, SearchContext.TRACK_TOTAL_HITS_DISABLED)); } /** @@ -1942,38 +2118,6 @@ public void testCanMatchSearchAfterDescEqualMin() throws IOException { MinAndMax minMax = new MinAndMax(0L, 9L); FieldSortBuilder primarySort = new FieldSortBuilder("test"); primarySort.order(SortOrder.DESC); - assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort, SearchContext.TRACK_TOTAL_HITS_DISABLED), true); - } - - /** - * Test canMatchSearchAfter with missing value, even if min/max is out of range - * Min = 0L, Max = 9L, search_after = -1L - * Expected result is canMatch = true - */ - public void testCanMatchSearchAfterWithMissing() throws IOException { - FieldDoc searchAfter = new FieldDoc(0, 0, new Long[] { -1L }); - MinAndMax minMax = new MinAndMax(0L, 9L); - FieldSortBuilder primarySort = new FieldSortBuilder("test"); - primarySort.order(SortOrder.DESC); - // Should be false without missing values - assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort, SearchContext.TRACK_TOTAL_HITS_DISABLED), false); - primarySort.missing("_last"); - // Should be true with missing values - assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort, SearchContext.TRACK_TOTAL_HITS_DISABLED), true); - } - - /** - * Test for DESC order search_after query with track_total_hits=true. - * Min = 0L, Max = 9L, search_after = -1L - * With above min/max and search_after, it should not match, but since - * track_total_hits = true, - * Expected result is canMatch = true - */ - public void testCanMatchSearchAfterDescLessThanMinWithTrackTotalhits() throws IOException { - FieldDoc searchAfter = new FieldDoc(0, 0, new Long[] { -1L }); - MinAndMax minMax = new MinAndMax(0L, 9L); - FieldSortBuilder primarySort = new FieldSortBuilder("test"); - primarySort.order(SortOrder.DESC); - assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort, 1000), true); + assertTrue(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort, SearchContext.TRACK_TOTAL_HITS_DISABLED)); } } From 8c91a1274f52e136e9008d978dade031615cf5ad Mon Sep 17 00:00:00 2001 From: panguixin Date: Tue, 6 Aug 2024 20:48:56 +0800 Subject: [PATCH 3/3] apply review comments Signed-off-by: panguixin --- CHANGELOG.md | 1 + .../org/opensearch/search/SearchService.java | 26 +++++++++---------- .../search/internal/ContextIndexSearcher.java | 2 +- 3 files changed, 14 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c7d7aa9bf780..e42c72902e835 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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)) diff --git a/server/src/main/java/org/opensearch/search/SearchService.java b/server/src/main/java/org/opensearch/search/SearchService.java index 845cd259afd98..41f29b875566f 100644 --- a/server/src/main/java/org/opensearch/search/SearchService.java +++ b/server/src/main/java/org/opensearch/search/SearchService.java @@ -1620,29 +1620,27 @@ private CanMatchResponse canMatch(ShardSearchRequest request, boolean checkRefre ); Rewriteable.rewrite(request.getRewriteable(), context, false); - if (hasRefreshPending) { - final FieldSortBuilder sortBuilder = FieldSortBuilder.getPrimaryFieldSortOrNull(request.source()); - final MinAndMax minMax = sortBuilder != null ? FieldSortBuilder.getMinMaxOrNull(context, sortBuilder) : null; - return new CanMatchResponse(true, minMax); + 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 boolean aliasFilterCanMatch = request.getAliasFilter().getQueryBuilder() instanceof MatchNoneQueryBuilder == false; - if (aliasFilterCanMatch == false - || (canRewriteToMatchNone(request.source()) && request.source().query() instanceof MatchNoneQueryBuilder)) { - return new CanMatchResponse(false, null); + 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; - final FieldSortBuilder sortBuilder = FieldSortBuilder.getPrimaryFieldSortOrNull(request.source()); - final MinAndMax minMax = sortBuilder != null ? FieldSortBuilder.getMinMaxOrNull(context, sortBuilder) : null; - final Integer trackTotalHitsUpto = request.source() == null ? null : request.source().trackTotalHitsUpTo(); // 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. - if (Objects.equals(trackTotalHitsUpto, SearchContext.TRACK_TOTAL_HITS_DISABLED) - && minMax != null - && sortBuilder.missing() == null) { + 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); diff --git a/server/src/main/java/org/opensearch/search/internal/ContextIndexSearcher.java b/server/src/main/java/org/opensearch/search/internal/ContextIndexSearcher.java index 32619a2726bfc..3f02e4ffbea50 100644 --- a/server/src/main/java/org/opensearch/search/internal/ContextIndexSearcher.java +++ b/server/src/main/java/org/opensearch/search/internal/ContextIndexSearcher.java @@ -518,7 +518,7 @@ private boolean canMatchSearchAfter(LeafReaderContext ctx) throws IOException { if (searchContext.searchAfter() != null && searchContext.request() != null && searchContext.request().source() != null - && Objects.equals(searchContext.trackTotalHitsUpTo(), SearchContext.TRACK_TOTAL_HITS_DISABLED)) { + && searchContext.trackTotalHitsUpTo() == SearchContext.TRACK_TOTAL_HITS_DISABLED) { // Only applied on primary sort field and primary search_after. FieldSortBuilder primarySortField = FieldSortBuilder.getPrimaryFieldSortOrNull(searchContext.request().source()); // Check for sort.missing == null, since in case of missing values sort queries, if segment/shard's min/max