Skip to content

Commit

Permalink
Disable shard/segment level search_after short cutting if track_total…
Browse files Browse the repository at this point in the history 
…_hits != false

Signed-off-by: gashutos <gashutos@amazon.com>
  • Loading branch information
@gashutos
gashutos committed Sep 1, 2023
1 parent 04c90c7 commit dd6967b
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 12 deletions.
21 changes: 18 additions & 3 deletions server/src/main/java/org/opensearch/search/SearchService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1558,17 +1558,28 @@ private CanMatchResponse canMatch(ShardSearchRequest request, boolean checkRefre
canMatch = aliasFilterCanMatch;
}
final FieldDoc searchAfterFieldDoc = getSearchAfterFieldDoc(request, context);
canMatch = canMatch && canMatchSearchAfter(searchAfterFieldDoc, minMax, sortBuilder);
canMatch = canMatch && canMatchSearchAfter(searchAfterFieldDoc, minMax, sortBuilder, request.source());

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,
SearchSourceBuilder source
) {
// 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
&& isTrackTotalHitsDisabled(source)) {
final Object searchAfterPrimary = searchAfter.fields[0];
if (primarySortField.order() == SortOrder.DESC) {
if (minMax.compareMin(searchAfterPrimary) > 0) {
Expand Down Expand Up @@ -1597,6 +1608,10 @@ private static FieldDoc getSearchAfterFieldDoc(ShardSearchRequest request, Query
return null;
}

private static boolean isTrackTotalHitsDisabled(SearchSourceBuilder source) {
return source.trackTotalHitsUpTo() != null && source.trackTotalHitsUpTo() == SearchContext.TRACK_TOTAL_HITS_DISABLED;
}

/**
* Returns true iff the given search source builder can be early terminated by rewriting to a match none query. Or in other words
* if the execution of the search request can be early terminated without executing it. This is for instance not possible if
Expand Down
7 changes: 6 additions & 1 deletion server/src/main/java/org/opensearch/search/internal/ContextIndexSearcher.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -511,7 +511,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.request().source()
);
}
}
return true;
Expand Down
46 changes: 38 additions & 8 deletions server/src/test/java/org/opensearch/search/SearchServiceTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1752,11 +1752,13 @@ public void validatePitStats(String index, long expectedPitCurrent, long expecte
* Expected result is canMatch = false
*/
public void testCanMatchSearchAfterAscGreaterThanMax() throws IOException {
SearchSourceBuilder source = new SearchSourceBuilder();
source.trackTotalHitsUpTo(SearchContext.TRACK_TOTAL_HITS_DISABLED);
FieldDoc searchAfter = new FieldDoc(0, 0, new Long[] { 10L });
MinAndMax<?> minMax = new MinAndMax<Long>(0L, 9L);
FieldSortBuilder primarySort = new FieldSortBuilder("test");
primarySort.order(SortOrder.ASC);
assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort), false);
assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort, source), false);
}

/**
Expand All @@ -1765,11 +1767,13 @@ public void testCanMatchSearchAfterAscGreaterThanMax() throws IOException {
* Expected result is canMatch = true
*/
public void testCanMatchSearchAfterAscLessThanMax() throws IOException {
SearchSourceBuilder source = new SearchSourceBuilder();
source.trackTotalHitsUpTo(SearchContext.TRACK_TOTAL_HITS_DISABLED);
FieldDoc searchAfter = new FieldDoc(0, 0, new Long[] { 7L });
MinAndMax<?> minMax = new MinAndMax<Long>(0L, 9L);
FieldSortBuilder primarySort = new FieldSortBuilder("test");
primarySort.order(SortOrder.ASC);
assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort), true);
assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort, source), true);
}

/**
Expand All @@ -1778,11 +1782,13 @@ public void testCanMatchSearchAfterAscLessThanMax() throws IOException {
* Expected result is canMatch = true
*/
public void testCanMatchSearchAfterAscEqualMax() throws IOException {
SearchSourceBuilder source = new SearchSourceBuilder();
source.trackTotalHitsUpTo(SearchContext.TRACK_TOTAL_HITS_DISABLED);
FieldDoc searchAfter = new FieldDoc(0, 0, new Long[] { 9L });
MinAndMax<?> minMax = new MinAndMax<Long>(0L, 9L);
FieldSortBuilder primarySort = new FieldSortBuilder("test");
primarySort.order(SortOrder.ASC);
assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort), true);
assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort, source), true);
}

/**
Expand All @@ -1791,11 +1797,13 @@ public void testCanMatchSearchAfterAscEqualMax() throws IOException {
* Expected result is canMatch = true
*/
public void testCanMatchSearchAfterDescGreaterThanMin() throws IOException {
SearchSourceBuilder source = new SearchSourceBuilder();
source.trackTotalHitsUpTo(SearchContext.TRACK_TOTAL_HITS_DISABLED);
FieldDoc searchAfter = new FieldDoc(0, 0, new Long[] { 10L });
MinAndMax<?> minMax = new MinAndMax<Long>(0L, 9L);
FieldSortBuilder primarySort = new FieldSortBuilder("test");
primarySort.order(SortOrder.DESC);
assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort), true);
assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort, source), true);
}

/**
Expand All @@ -1804,11 +1812,13 @@ public void testCanMatchSearchAfterDescGreaterThanMin() throws IOException {
* Expected result is canMatch = false
*/
public void testCanMatchSearchAfterDescLessThanMin() throws IOException {
SearchSourceBuilder source = new SearchSourceBuilder();
source.trackTotalHitsUpTo(SearchContext.TRACK_TOTAL_HITS_DISABLED);
FieldDoc searchAfter = new FieldDoc(0, 0, new Long[] { -1L });
MinAndMax<?> minMax = new MinAndMax<Long>(0L, 9L);
FieldSortBuilder primarySort = new FieldSortBuilder("test");
primarySort.order(SortOrder.DESC);
assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort), false);
assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort, source), false);
}

/**
Expand All @@ -1817,11 +1827,13 @@ public void testCanMatchSearchAfterDescLessThanMin() throws IOException {
* Expected result is canMatch = true
*/
public void testCanMatchSearchAfterDescEqualMin() throws IOException {
SearchSourceBuilder source = new SearchSourceBuilder();
source.trackTotalHitsUpTo(SearchContext.TRACK_TOTAL_HITS_DISABLED);
FieldDoc searchAfter = new FieldDoc(0, 0, new Long[] { 0L });
MinAndMax<?> minMax = new MinAndMax<Long>(0L, 9L);
FieldSortBuilder primarySort = new FieldSortBuilder("test");
primarySort.order(SortOrder.DESC);
assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort), true);
assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort, source), true);
}

/**
Expand All @@ -1830,14 +1842,32 @@ public void testCanMatchSearchAfterDescEqualMin() throws IOException {
* Expected result is canMatch = true
*/
public void testCanMatchSearchAfterWithMissing() throws IOException {
SearchSourceBuilder source = new SearchSourceBuilder();
source.trackTotalHitsUpTo(SearchContext.TRACK_TOTAL_HITS_DISABLED);
FieldDoc searchAfter = new FieldDoc(0, 0, new Long[] { -1L });
MinAndMax<?> minMax = new MinAndMax<Long>(0L, 9L);
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, source), false);
primarySort.missing("_last");
// Should be true with missing values
assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort), true);
assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort, source), true);
}

/**
* Test for DESC order search_after query with track_total_hits=true.
* Min = 0L, Max = 9L, search_after = -1L
* With above min/max & search_after, it should not match, but since
* track_total_hits = true,
* Expected result is canMatch = true
*/
public void testCanMatchSearchAfterDescLessThanMin() throws IOException {
SearchSourceBuilder source = new SearchSourceBuilder();
FieldDoc searchAfter = new FieldDoc(0, 0, new Long[] { -1L });
MinAndMax<?> minMax = new MinAndMax<Long>(0L, 9L);
FieldSortBuilder primarySort = new FieldSortBuilder("test");
primarySort.order(SortOrder.DESC);
assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort, source), false);
}
}

0 comments on commit dd6967b

Please sign in to comment.