[BUG] Regression in 2.8.0: sort with `missing: _last` omits results

[myronmarston]

Describe the bug

My application is currently using AWS Elasticsearch 7.10 but we are working on upgrading it to Opensearch (ideally, to the latest release--2.8.0). In OpenSearch 2.8.0, when the _missing: _last option is used on a sort, it can omit documents that have a null value for the sort field.

I narrowed the issue down to a short script that reproduces (included below) and ran it on different versions of OpenSearch. It appears that OpenSearch's behavior matched Elasticsearch 7.10 from OpenSearch 1.0.0 through 2.7.0, but in 2.8.0 there's a regression and it has this bug.

To Reproduce

1. Put this in opensearch_regression:

#!/usr/bin/env bash

url_root=$1

echo 'Opensearch Version:'
curl -is $url_root | grep number

echo
echo 'Deleting example_index to have a clean slate...'
curl -X DELETE -H 'Content-type: application/json' $url_root/example_index?ignore_unavailable=true
echo

echo
echo 'Creating example_index...'
curl -X PUT -H 'Content-type: application/json' $url_root/example_index --data-binary @- << EOF
{"settings":{"index.number_of_shards":10},"mappings":{"dynamic":"strict","properties":{"id":{"type":"keyword"},"amount_cents":{"type":"integer"}},"_routing":{"required":true}}}
EOF
echo

echo
echo 'Indexing documents...'
curl -X POST -H 'Content-type: application/x-ndjson' $url_root/_bulk?refresh=true --data-binary @- << EOF
{"index":{"_index":"example_index","_id":"1","routing":"epmshv"}}
{"id":"1","amount_cents":100}
{"index":{"_index":"example_index","_id":"4","routing":"abowtm"}}
{"id":"4","amount_cents":250}
{"index":{"_index":"example_index","_id":"5","routing":"ewgphx"}}
{"id":"5","amount_cents":300}
{"index":{"_index":"example_index","_id":"3","routing":"weooyf"}}
{"id":"3","amount_cents":200}
{"index":{"_index":"example_index","_id":"2","routing":"pduylo"}}
{"id":"2","amount_cents":150}
{"index":{"_index":"example_index","_id":"with_null","routing":"jhsycu"}}
{"id":"with_null","amount_cents":null}
EOF
echo

echo
echo 'Searching...'
curl -X POST -H 'Content-type: application/json' $url_root/example_index/_search?filter_path=hits.hits._id --data-binary @- << EOF
{"size":5,"sort":[{"amount_cents":{"order":"desc","missing":"_last"}},{"id":{"order":"desc","missing":"_last"}}],"search_after":[150,"2"],"track_total_hits":false,"_source":false}
EOF
echo

2. Make the file executable (chmod +x opensearch_regression).
3. Run it against OpenSearch 2.7.0, passing it the URL for a 2.7.0 cluster. Here's the output when I run it on my machine:

$ opensearch_regression http://localhost:9234
Opensearch Version:
    "number" : "2.7.0",

Deleting example_index to have a clean slate...
{"acknowledged":true}

Creating example_index...
{"acknowledged":true,"shards_acknowledged":true,"index":"example_index"}

Indexing documents...
{"took":98,"errors":false,"items":[{"index":{"_index":"example_index","_id":"1","_version":1,"result":"created","forced_refresh":true,"_shards":{"total":2,"successful":1,"failed":0},"_seq_no":0,"_primary_term":1,"status":201}},{"index":{"_index":"example_index","_id":"4","_version":1,"result":"created","forced_refresh":true,"_shards":{"total":2,"successful":1,"failed":0},"_seq_no":0,"_primary_term":1,"status":201}},{"index":{"_index":"example_index","_id":"5","_version":1,"result":"created","forced_refresh":true,"_shards":{"total":2,"successful":1,"failed":0},"_seq_no":0,"_primary_term":1,"status":201}},{"index":{"_index":"example_index","_id":"3","_version":1,"result":"created","forced_refresh":true,"_shards":{"total":2,"successful":1,"failed":0},"_seq_no":0,"_primary_term":1,"status":201}},{"index":{"_index":"example_index","_id":"2","_version":1,"result":"created","forced_refresh":true,"_shards":{"total":2,"successful":1,"failed":0},"_seq_no":0,"_primary_term":1,"status":201}},{"index":{"_index":"example_index","_id":"with_null","_version":1,"result":"created","forced_refresh":true,"_shards":{"total":2,"successful":1,"failed":0},"_seq_no":1,"_primary_term":1,"status":201}}]}

Searching...
{"hits":{"hits":[{"_id":"1"},{"_id":"with_null"}]}}

Notice that the search results include both id 1 and id with_null.

4. Run it against OpenSearch 2.8.0, passing it the URL for a 2.8.0 cluster. Here's the output when I run it on my machine:

Opensearch Version:
    "number" : "2.8.0",

Deleting example_index to have a clean slate...
{"acknowledged":true}

Creating example_index...
{"acknowledged":true,"shards_acknowledged":true,"index":"example_index"}

Indexing documents...
{"took":63,"errors":false,"items":[{"index":{"_index":"example_index","_id":"1","_version":1,"result":"created","forced_refresh":true,"_shards":{"total":2,"successful":1,"failed":0},"_seq_no":0,"_primary_term":1,"status":201}},{"index":{"_index":"example_index","_id":"4","_version":1,"result":"created","forced_refresh":true,"_shards":{"total":2,"successful":1,"failed":0},"_seq_no":0,"_primary_term":1,"status":201}},{"index":{"_index":"example_index","_id":"5","_version":1,"result":"created","forced_refresh":true,"_shards":{"total":2,"successful":1,"failed":0},"_seq_no":0,"_primary_term":1,"status":201}},{"index":{"_index":"example_index","_id":"3","_version":1,"result":"created","forced_refresh":true,"_shards":{"total":2,"successful":1,"failed":0},"_seq_no":0,"_primary_term":1,"status":201}},{"index":{"_index":"example_index","_id":"2","_version":1,"result":"created","forced_refresh":true,"_shards":{"total":2,"successful":1,"failed":0},"_seq_no":0,"_primary_term":1,"status":201}},{"index":{"_index":"example_index","_id":"with_null","_version":1,"result":"created","forced_refresh":true,"_shards":{"total":2,"successful":1,"failed":0},"_seq_no":1,"_primary_term":1,"status":201}}]}

Searching...
{"hits":{"hits":[{"_id":"1"}]}}

Notice that the with_null result was omitted.

Expected behavior
As on Elasticsearch 7.10 and OpenSearch 1.0.0-2.7.0, I expect documents with null values for sort fields to be included in sorted search responses that use _missing: _last.

Plugins

Here's the Dockerfile I'm using to boot OpenSearch locally:

ARG VERSION
FROM opensearchproject/opensearch:${VERSION}
RUN /usr/share/opensearch/bin/opensearch-plugin remove opensearch-security
RUN /usr/share/opensearch/bin/opensearch-plugin install --batch mapper-size

It installs the mapper-size plugin and removes the opensearch-security plugin.

Screenshots
N/A

Host/Environment (please complete the following information):

• OS: MacOS Ventura
• Version: 13.4 (22F66)

Additional context

When I was narrowing this down to a reproducible script, I could not always consistently reproduce the issue on OpenSearch 2.8.0, depending on my index configuration. Specifically, these bits of index configuration seemed to matter:

• settings.index.number_of_shards: 10 -- when I removed this the bug wouldn't reproduce.
• _routing.required: true -- when I removed this (and the _routing values in the _bulk call) the bug wouldn't reproduce.

I suspect that the reproducibility of this bug depends on the example documents residing on specific shards relative to each other, and by changing either of those settings it changed what shards the example docs go on.

[dblock]

Thanks for digging this up and narrowing it down. I think the next steps could be to 1) write a YAML REST test that can reproduce the issue 2) bisect further across releases to see if this was introduced in a specific 2.x or between the last 1.x and the first 2.x. Also, does introducing a sleep between indexing and searching "fix" the issue?

[myronmarston]

Thanks for digging this up and narrowing it down. I think the next steps could be to 1) write a YAML REST test that can reproduce the issue

My script above consistently reproduces the issue but if you need it written as a YAML REST test I can try and make time to convert it if you can provide some examples. (I've never contributed to this project, or looked at the code before.). Or can the script I provided be enough to get the ball rolling?

2. bisect further across releases to see if this was introduced in a specific 2.x or between the last 1.x and the first 2.x.

I already did that. As I mentioned above:

It appears that OpenSearch's behavior matched Elasticsearch 7.10 from OpenSearch 1.0.0 through 2.7.0, but in 2.8.0 there's a regression and it has this bug.

I ran the script on a variety of 2.x versions and only 2.8.0 has this bug from what I found.

Also, does introducing a sleep between indexing and searching "fix" the issue?

I just added a 10 second sleep and it made no difference. It's also not clear to me how it could have an impact; the indexing call uses ?refresh=true to ensure the indexing is synchronous.

[dblock]

Thanks for digging this up and narrowing it down. I think the next steps could be to 1) write a YAML REST test that can reproduce the issue

My script above consistently reproduces the issue but if you need it written as a YAML REST test I can try and make time to convert it if you can provide some examples. (I've never contributed to this project, or looked at the code before.). Or can the script I provided be enough to get the ball rolling?

Yes, I am secretly (or not so secretly) hoping that you'll write the test, then you'll attempt a fix or at least narrow this down to a specific commit. Any fix, especially for a regression, won't get merged without the test, so we'll be half way there.

2. bisect further across releases to see if this was introduced in a specific 2.x or between the last 1.x and the first 2.x.

I already did that. As I mentioned above:

It appears that OpenSearch's behavior matched Elasticsearch 7.10 from OpenSearch 1.0.0 through 2.7.0, but in 2.8.0 there's a regression and it has this bug.

Ok good. Try to git bisect to a specific commit that broke this? Whomever made a code change that caused the regression will definitely jump on to (help) fix if you haven't found the issue by then.

I ran the script on a variety of 2.x versions and only 2.8.0 has this bug from what I found.

Also, does introducing a sleep between indexing and searching "fix" the issue?

I just added a 10 second sleep and it made no difference. It's also not clear to me how it could have an impact; the indexing call uses ?refresh=true to ensure the indexing is synchronous.

Ok, good.

[myronmarston]

Yes, I am secretly (or not so secretly) hoping that you'll write the test, then you'll attempt a fix or at least narrow this down to a specific commit. Any fix, especially for a regression, won't get merged without the test, so we'll be half way there.

I very much not secretly hoped that by investing multiple hours in narrowing down the bug to a SSCCE that would suffice :).

I don't have the bandwidth to invest in ramping up to contributing to this codebase at this time, unfortunately. I'm hoping someone can pick this up at some point, given it's a regresssion.

Ok good. Try to git bisect to a specific commit that broke this? Whomever made a code change that caused the regression will definitely jump on to (help) fix if you haven't found the issue by then.

I've been trying different OpenSearch versions using the docker image, and it's not clear to me how I could use git bisect while using them (for example, is a docker image published at every commit, or just on a new release?).

Assume I'm a total newcomer to this codebase (because I am).

[dblock]

First thank you very much @myronmarston for your help and time spent here, and for any time you want to continue investing here.

Generally, after checking out the code you can run ./gradlew run, which will start OpenSearch from source code. So git bisect on a 2.x branch, and a script that starts OpenSearch and executes your test.

I did it.

git checkout 2.7
git checkout 2.8
./gradlew run

In a different terminal I'm running your script, ./8212.sh

2.8

{"hits":{"hits":[{"_id":"1"}]}}

2.7

{"hits":{"hits":[{"_id":"1"},{"_id":"with_null"}]}}

Version 2.7 released May 2nd, I went back to 15db9a9.

osx ~/source/opensearch-project/OpenSearch/upstream-OpenSearch (2.x|BISECTING)$ git checkout 2.x
osx ~/source/opensearch-project/OpenSearch/upstream-OpenSearch (2.x|BISECTING)$ git bisect start
osx ~/source/opensearch-project/OpenSearch/upstream-OpenSearch (2.x|BISECTING)$ git bisect bad

osx ~/source/opensearch-project/OpenSearch/upstream-OpenSearch (2.x|BISECTING)$ git checkout 15db9a9e3393c4cbec4bffef017b880b98a7083c
HEAD is now at 15db9a9e339 Bumping 2.x to 2.8.0 (#7237)

osx ~/source/opensearch-project/OpenSearch/upstream-OpenSearch ((15db9a9e339...)|BISECTING)$ git bisect good
Bisecting: 113 revisions left to test after this (roughly 7 steps)
[db238adc84679ac8b9a641658ab9ec834aadee5f] Add release notes v2.8.0 (#7825) (#7841)

osx ~/source/opensearch-project/OpenSearch/upstream-OpenSearch ((db238adc846...)|BISECTING)$ git bisect bad
Bisecting: 56 revisions left to test after this (roughly 6 steps)
[75dbce2d8047874c9803d5390b043112e23e442e] [Remote Store] Add Lock Manager in Remote Segment Store to persist data (#6787) (#7517)

osx ~/source/opensearch-project/OpenSearch/upstream-OpenSearch ((75dbce2d804...)|BISECTING)$ git bisect good
Bisecting: 28 revisions left to test after this (roughly 5 steps)
[b02bfd1edcbb547e8b75ee4da125e1e6dcb70344] [Backport 2.x] Add support to run SegRep integ tests using remote store settings & Mute flaky test (#7692)

osx ~/source/opensearch-project/OpenSearch/upstream-OpenSearch ((b02bfd1edcb...)|BISECTING)$ git bisect good
Bisecting: 14 revisions left to test after this (roughly 4 steps)
[b563106d648d7750979f4a2169a2bda8ba4bd436] Remvoe flaky match conditions in script processor yaml test (#7737) (#7740)

osx ~/source/opensearch-project/OpenSearch/upstream-OpenSearch ((b563106d648...)|BISECTING)$ git bisect bad
Bisecting: 6 revisions left to test after this (roughly 3 steps)
[7fda531e8b55a96000489945570ab716980d285e] Backport of https://github.com/opensearch-project/OpenSearch/pull/7665 (#7710)

osx ~/source/opensearch-project/OpenSearch/upstream-OpenSearch ((7fda531e8b5...)|BISECTING)$ git bisect good
Bisecting: 3 revisions left to test after this (roughly 2 steps)
[06fc7de7e567b3534d8ce3224c7fccc327a1fda9] [Remote segments] Add backpressure in write path on segments lag between local and remote store (#7459) (#7720)

Bisecting: 1 revision left to test after this (roughly 1 step)
[6308fb078656e67bd026c7d0cd1e064b8912e2a2] search_after query optimization with shard/segment level short cutting (#7453) (#7727)

osx ~/source/opensearch-project/OpenSearch/upstream-OpenSearch ((6308fb07865...)|BISECTING)$ git bisect bad
Bisecting: 0 revisions left to test after this (roughly 0 steps)
[d133b6f3cdb60f0a00974ca21f795c284e8d5114] [Backport 2.x] Bump com.google.api:gax from 2.17.0 to 2.27.0 in /plugins/repository-gcs (#7707)

osx ~/source/opensearch-project/OpenSearch/upstream-OpenSearch ((d133b6f3cdb...)|BISECTING)$ git bisect good
6308fb078656e67bd026c7d0cd1e064b8912e2a2 is the first bad commit
commit 6308fb078656e67bd026c7d0cd1e064b8912e2a2
Author: Chaitanya Gohel <104654647+gashutos@users.noreply.github.com>
Date:   Wed May 24 14:50:38 2023 +0000

    search_after query optimization with shard/segment level short cutting (#7453) (#7727)
    
    * search_after query optimization with shard/segment level short cutting
    
    
    
    * Correcting logical && operator
    
    
    
    * Addressig review comments
    
    
    
    * Correcting NPE
    
    
    
    * Fixing failed integ
    
    
    
    * Addressing review comments and fixing some more integ
    
    
    
    * Addressing review comments and fixing some more integ
    
    
    
    * Refactoring a bit to aggressive null checks
    
    
    
    * Adding unit tests for MinAndMax methods
    
    
    
    ---------
    
    Signed-off-by: Chaitanya Gohel <gashutos@amazon.com>
    Signed-off-by: gashutos <gashutos@amazon.com>
    Signed-off-by: Chaitanya Gohel <104654647+gashutos@users.noreply.github.com>

 CHANGELOG.md                                       |  1 +
 .../opensearch/search/DefaultSearchContext.java    | 21 +-----
 .../java/org/opensearch/search/SearchService.java  | 36 +++++++++-
 .../search/internal/ContextIndexSearcher.java      | 70 ++++++++++++++----
 .../opensearch/search/sort/FieldSortBuilder.java   | 19 ++++-
 .../java/org/opensearch/search/sort/MinAndMax.java | 34 +++++++++
 .../org/opensearch/search/SearchServiceTests.java  | 82 ++++++++++++++++++++++
 .../org/opensearch/search/sort/MinAndMaxTests.java | 61 ++++++++++++++++
 8 files changed, 289 insertions(+), 35 deletions(-)
 create mode 100644 server/src/test/java/org/opensearch/search/sort/MinAndMaxTests.java

Looks like it broke in #7453, @gashutos care to take a look please?

[gashutos]

Taking a look.

[gashutos]

Yea, I just validated, this is very corner scenario where search_after is specified over a sort field where entire shard has missing value for that sort field. In that case, it is skipping that shard to search.
In above scenario, since we have settings.index.number_of_shards: 10, the bulk request contains 6 documents, so each shard is having 1 docuement and so document with null value gets assigned to a shard where only this document is present.

This looks very corner scenario, mostly not a real life scenario, I will send out PR to fix this in 2.9

[gashutos]

Validation :
changing _bulk payload to below too works since now each shard has 2 documents. (No shard has field value entirely null)

curl -X POST -H 'Content-type: application/x-ndjson' $url_root/_bulk?refresh=true --data-binary @- << EOF
{"index":{"_index":"example_index","_id":"1","routing":"epmshv"}}
{"id":"1","amount_cents":100}
{"index":{"_index":"example_index","_id":"4","routing":"abowtm"}}
{"id":"4","amount_cents":250}
{"index":{"_index":"example_index","_id":"5","routing":"ewgphx"}}
{"id":"5","amount_cents":300}
{"index":{"_index":"example_index","_id":"3","routing":"weooyf"}}
{"id":"3","amount_cents":200}
{"index":{"_index":"example_index","_id":"2","routing":"pduylo"}}
{"id":"2","amount_cents":150}
{"index":{"_index":"example_index","_id":"with_null","routing":"jhsycu"}}
{"id":"with_null","amount_cents":null}
{"index":{"_index":"example_index","_id":"6","routing":"epmshv"}}
{"id":"6","amount_cents":100}
{"index":{"_index":"example_index","_id":"7","routing":"abowtm"}}
{"id":"7","amount_cents":250}
{"index":{"_index":"example_index","_id":"8","routing":"ewgphx"}}
{"id":"8","amount_cents":300}
{"index":{"_index":"example_index","_id":"9","routing":"weooyf"}}
{"id":"9","amount_cents":200}
{"index":{"_index":"example_index","_id":"10","routing":"pduylo"}}
{"id":"10","amount_cents":150}
{"index":{"_index":"example_index","_id":"11","routing":"jhsycu"}}
{"id":"11","amount_cents":300}
EOF