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

Disable shard/segment level search_after short cutting if track_total_hits != false #9683

Merged
merged 2 commits into from
Sep 2, 2023
Merged

Disable shard/segment level search_after short cutting if track_total_hits != false #9683

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
21a3679
Disable shard/segment level search_after short cutting if track_total…
gashutos Sep 1, 2023
eb0a9d5
Merge branch 'main' into main
gashutos Sep 1, 2023
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 @@ -188,6 +188,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
- Fix condition to remove index create block ([#9437](https://github.com/opensearch-project/OpenSearch/pull/9437))
- Add support to clear archived index setting ([#9019](https://github.com/opensearch-project/OpenSearch/pull/9019))
- [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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,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 @@ -1558,17 +1559,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)) {
Copy link

Choose a reason for hiding this comment

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

@gashutos
Hello, I have a question. In these two scenarios：trackTotalHitsUpto=null and trackTotalHitsUpto < DEFAULT_TRACK_TOTAL_HITS_UP_TO，Is it possible to go through the optimization of shard/segment level shortcutting？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

trackTotalHitsUpto=null
This should trigger optimization.
trackTotalHitsUpto < DEFAULT_TRACK_TOTAL_HITS_UP_TO
This should not.

However, We have made a fix in Lucene itself for this optimizations.
apache/lucene#12334

With this lucene optimization, even if we have this OpenSearch shortcutting disabled, it wont spend much time in actual lucene logic and it would be up par with shortcutting.

Copy link

Choose a reason for hiding this comment

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

@gashutos Tks，there may be another problem for query with "search_after": [null, 1000000]; searchAfterPrimary equal null, minMax.compareMin will throw NPE, this behaviour will not same as before

example:

curl -XGET "127.0.0.1:9200/test/_search?pretty&track_total_hits=false" -H'Content-Type:application/json' -d'
{              
   "query": {                                
     "match_all": {}
   },                                        
   "sort":[{"name":{"order":"asc"}}, {"Id":{"order":"desc"}}],
   "search_after": [null, 1000000]
 }'
{
  "error" : {
    "root_cause" : [
      {
        "type" : "null_pointer_exception",
        "reason" : null
      }
    ],
    "type" : "search_phase_execution_exception",
    "reason" : "all shards failed",
    "phase" : "can_match",
    "grouped" : true,
    "failed_shards" : [
      {
        "shard" : 112820878,
        "index" : "search_qingzhi",
        "node" : "127.0.0.1,51651,1711676797164",
        "reason" : {
          "type" : "null_pointer_exception",
          "reason" : null
        }
      }
    ],
    "caused_by" : {
      "type" : "null_pointer_exception",
      "reason" : null,
      "caused_by" : {
        "type" : "null_pointer_exception",
        "reason" : null
      }
    }
  },
  "status" : 500
}

final Object searchAfterPrimary = searchAfter.fields[0];
if (primarySortField.order() == SortOrder.DESC) {
if (minMax.compareMin(searchAfterPrimary) > 0) {
Expand Down
7 changes: 6 additions & 1 deletion 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 @@ -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.trackTotalHitsUpTo()
);
}
}
return true;
Expand Down
31 changes: 23 additions & 8 deletions server/src/test/java/org/opensearch/search/SearchServiceTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1756,7 +1756,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 @@ -1769,7 +1769,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 @@ -1782,7 +1782,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 @@ -1795,7 +1795,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 @@ -1808,7 +1808,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 @@ -1821,7 +1821,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 @@ -1835,9 +1835,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);
}
}