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

[Backport 2.x] Disable shard/segment level search_after short cutting if track_total_hits != false (#9683) #9716

Merged
merged 1 commit into from
Sep 5, 2023
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 @@ -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
Expand Down
19 changes: 16 additions & 3 deletions server/src/main/java/org/opensearch/search/SearchService.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1755,7 +1755,7 @@ public void testCanMatchSearchAfterAscGreaterThanMax() throws IOException {
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, SearchContext.TRACK_TOTAL_HITS_DISABLED), false);
}

/**
Expand All @@ -1768,7 +1768,7 @@ public void testCanMatchSearchAfterAscLessThanMax() throws IOException {
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, SearchContext.TRACK_TOTAL_HITS_DISABLED), true);
}

/**
Expand All @@ -1781,7 +1781,7 @@ public void testCanMatchSearchAfterAscEqualMax() throws IOException {
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, SearchContext.TRACK_TOTAL_HITS_DISABLED), true);
}

/**
Expand All @@ -1794,7 +1794,7 @@ public void testCanMatchSearchAfterDescGreaterThanMin() throws IOException {
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, SearchContext.TRACK_TOTAL_HITS_DISABLED), true);
}

/**
Expand All @@ -1807,7 +1807,7 @@ public void testCanMatchSearchAfterDescLessThanMin() throws IOException {
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, SearchContext.TRACK_TOTAL_HITS_DISABLED), false);
}

/**
Expand All @@ -1820,7 +1820,7 @@ public void testCanMatchSearchAfterDescEqualMin() throws IOException {
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, SearchContext.TRACK_TOTAL_HITS_DISABLED), true);
}

/**
Expand All @@ -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<Long>(0L, 9L);
FieldSortBuilder primarySort = new FieldSortBuilder("test");
primarySort.order(SortOrder.DESC);
assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort, 1000), true);
}
}