From 2d25462d54eac909b040d27a8c33ac0b6f2147d5 Mon Sep 17 00:00:00 2001 From: gashutos Date: Sat, 1 Jul 2023 23:48:28 +0530 Subject: [PATCH] Avoid search_after short cutting in case of missing is specified Signed-off-by: gashutos --- .../test/search/90_search_after.yml | 44 +++++++++++++++++++ .../org/opensearch/search/SearchService.java | 4 +- .../opensearch/search/SearchServiceTests.java | 17 +++++++ 3 files changed, 64 insertions(+), 1 deletion(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search/90_search_after.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search/90_search_after.yml index 65c1527a68b96..55e1566656faf 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/search/90_search_after.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search/90_search_after.yml @@ -160,6 +160,28 @@ - match: {hits.hits.0._source.timestamp: "2019-10-21 00:30:04.828" } - match: {hits.hits.0.sort: [1571617804828] } + # search_after with the sort with missing + - do: + bulk: + refresh: true + index: test + body: | + {"index":{}} + {"timestamp": null} + - do: + search: + index: test + rest_total_hits_as_int: true + body: + "size": 5 + "sort": [ { "timestamp": { "order": "asc", "missing": "_last" } } ] + search_after: [ "2021-10-21 08:30:04.828" ] # making it out of min/max so only missing value hit is qualified + + - match: { hits.total: 3 } + - length: { hits.hits: 1 } + - match: { hits.hits.0._index: test } + - match: { hits.hits.0._source.timestamp: null } + --- "date_nanos": - skip: @@ -276,3 +298,25 @@ - match: {hits.hits.0._index: test } - match: {hits.hits.0._source.population: 15223372036854775800 } - match: {hits.hits.0.sort: [15223372036854775800] } + + # search_after with the sort with missing + - do: + bulk: + refresh: true + index: test + body: | + {"index":{}} + {"population": null} + - do: + search: + index: test + rest_total_hits_as_int: true + body: + "size": 5 + "sort": [ { "population": { "order": "asc", "missing": "_last" } } ] + search_after: [15223372036854775801] # making it out of min/max so only missing value hit is qualified + + - match: { hits.total: 3 } + - length: { hits.hits: 1 } + - match: { hits.hits.0._index: test } + - match: { hits.hits.0._source.population: null } diff --git a/server/src/main/java/org/opensearch/search/SearchService.java b/server/src/main/java/org/opensearch/search/SearchService.java index 7d67c6c3b45f4..9daad9112e473 100644 --- a/server/src/main/java/org/opensearch/search/SearchService.java +++ b/server/src/main/java/org/opensearch/search/SearchService.java @@ -1550,7 +1550,9 @@ private CanMatchResponse canMatch(ShardSearchRequest request, boolean checkRefre } public static boolean canMatchSearchAfter(FieldDoc searchAfter, MinAndMax minMax, FieldSortBuilder primarySortField) { - if (searchAfter != null && minMax != null && 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 (searchAfter != null && minMax != null && primarySortField != null && primarySortField.missing() == null) { final Object searchAfterPrimary = searchAfter.fields[0]; if (primarySortField.order() == SortOrder.DESC) { if (minMax.compareMin(searchAfterPrimary) > 0) { diff --git a/server/src/test/java/org/opensearch/search/SearchServiceTests.java b/server/src/test/java/org/opensearch/search/SearchServiceTests.java index 8f8789a3a0323..74ef289c4b75f 100644 --- a/server/src/test/java/org/opensearch/search/SearchServiceTests.java +++ b/server/src/test/java/org/opensearch/search/SearchServiceTests.java @@ -1748,4 +1748,21 @@ public void testCanMatchSearchAfterDescEqualMin() throws IOException { primarySort.order(SortOrder.DESC); assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort), 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), false); + primarySort.missing("_last"); + // Should be true with missing values + assertEquals(SearchService.canMatchSearchAfter(searchAfter, minMax, primarySort), true); + } }