From 67de5608b783724ff7bb7e8ca5e283c006964265 Mon Sep 17 00:00:00 2001 From: Chaitanya Gohel <104654647+gashutos@users.noreply.github.com> Date: Sun, 3 Sep 2023 00:25:09 +0530 Subject: [PATCH] Disable shard/segment level search_after short cutting if track_total_hits != false (#9683) Signed-off-by: gashutos Signed-off-by: Chaitanya Gohel <104654647+gashutos@users.noreply.github.com> --- CHANGELOG.md | 1 + .../org/opensearch/search/SearchService.java | 19 ++++++++++-- .../search/internal/ContextIndexSearcher.java | 7 ++++- .../opensearch/search/SearchServiceTests.java | 31 ++++++++++++++----- 4 files changed, 46 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b7c81df29917..9a23f471c1b54 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -118,6 +118,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Add support to clear archived index setting ([#9019](https://github.com/opensearch-project/OpenSearch/pull/9019)) - Fix range reads in respository-s3 ([9512](https://github.com/opensearch-project/OpenSearch/issues/9512)) - [Segment Replication] Fixed bug where replica shard temporarily serves stale data during an engine reset ([#9495](https://github.com/opensearch-project/OpenSearch/pull/9495)) +- Disable shard/segment level search_after short cutting if track_total_hits != false ([#9683](https://github.com/opensearch-project/OpenSearch/pull/9683)) - [Segment Replication] Fixed bug where bytes behind metric is not accurate ([#9686](https://github.com/opensearch-project/OpenSearch/pull/9686)) ### Security diff --git a/server/src/main/java/org/opensearch/search/SearchService.java b/server/src/main/java/org/opensearch/search/SearchService.java index 3f93049e1df44..a9e7dcdbc8a7c 100644 --- a/server/src/main/java/org/opensearch/search/SearchService.java +++ b/server/src/main/java/org/opensearch/search/SearchService.java @@ -147,6 +147,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.concurrent.ExecutionException; @@ -1545,17 +1546,29 @@ private CanMatchResponse canMatch(ShardSearchRequest request, boolean checkRefre canMatch = aliasFilterCanMatch; } final FieldDoc searchAfterFieldDoc = getSearchAfterFieldDoc(request, context); - canMatch = canMatch && canMatchSearchAfter(searchAfterFieldDoc, minMax, sortBuilder); + final Integer trackTotalHitsUpto = request.source() == null ? null : request.source().trackTotalHitsUpTo(); + canMatch = canMatch && canMatchSearchAfter(searchAfterFieldDoc, minMax, sortBuilder, trackTotalHitsUpto); return new CanMatchResponse(canMatch || hasRefreshPending, minMax); } } } - public static boolean canMatchSearchAfter(FieldDoc searchAfter, MinAndMax minMax, FieldSortBuilder primarySortField) { + public static boolean canMatchSearchAfter( + FieldDoc searchAfter, + MinAndMax minMax, + 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. - if (searchAfter != null && minMax != null && primarySortField != null && primarySortField.missing() == null) { + // 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)) { 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 b17d00a689165..6fb3a314efcf5 100644 --- a/server/src/main/java/org/opensearch/search/internal/ContextIndexSearcher.java +++ b/server/src/main/java/org/opensearch/search/internal/ContextIndexSearcher.java @@ -510,7 +510,12 @@ private boolean canMatchSearchAfter(LeafReaderContext ctx) throws IOException { ctx, primarySortField ); - return SearchService.canMatchSearchAfter(searchContext.searchAfter(), minMax, primarySortField); + return SearchService.canMatchSearchAfter( + searchContext.searchAfter(), + minMax, + primarySortField, + searchContext.trackTotalHitsUpTo() + ); } } return true; diff --git a/server/src/test/java/org/opensearch/search/SearchServiceTests.java b/server/src/test/java/org/opensearch/search/SearchServiceTests.java index b6dc9315003cc..390f032ef4a79 100644 --- a/server/src/test/java/org/opensearch/search/SearchServiceTests.java +++ b/server/src/test/java/org/opensearch/search/SearchServiceTests.java @@ -1755,7 +1755,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), false); + assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort, SearchContext.TRACK_TOTAL_HITS_DISABLED), false); } /** @@ -1768,7 +1768,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), true); + assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort, SearchContext.TRACK_TOTAL_HITS_DISABLED), true); } /** @@ -1781,7 +1781,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), true); + assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort, SearchContext.TRACK_TOTAL_HITS_DISABLED), true); } /** @@ -1794,7 +1794,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), true); + assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort, SearchContext.TRACK_TOTAL_HITS_DISABLED), true); } /** @@ -1807,7 +1807,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), false); + assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort, SearchContext.TRACK_TOTAL_HITS_DISABLED), false); } /** @@ -1820,7 +1820,7 @@ 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), true); + assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort, SearchContext.TRACK_TOTAL_HITS_DISABLED), true); } /** @@ -1834,9 +1834,24 @@ public void testCanMatchSearchAfterWithMissing() throws IOException { FieldSortBuilder primarySort = new FieldSortBuilder("test"); primarySort.order(SortOrder.DESC); // Should be false without missing values - assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort), false); + 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), true); + 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); } }