Scroll search with ASC sort order on missing fields does not terminate

[gokl]

Hi,

doing an ASC sorted scroll search on a field that only exists in the mapping but not in the document yields to a non terminating scroll. It always returns the documents that don't have the field on which we sorted.

POST test/test/1
{
    "data":"test"
}

PUT test/test/_mapping
{
   "test": {
      "properties": {
         "data": {
            "type": "string"
         },
         "bad": {
            "type": "string"
         }
      }
   }
}

POST test/test/_search?scroll=1m
{
    "sort": [
       {
          "bad": {
             "missing" : "_last",
             "order": "asc"
          }
       }
    ]
}

GET _search/scroll?scroll=1m&scroll_id=FILL_IN_SCROLL_ID
GET _search/scroll?scroll=1m&scroll_id=FILL_IN_SCROLL_ID

Both _search/scroll requests should not return documents because the initial search already returned the document with id 1, but they do return the document.

However, using either a DESC sort order or "missing":"_first" makes the scroll work/terminate as expected.

[clintongormley]

Nice catch! thanks @gokl

[clintongormley]

@martijnvg could you have a look at this please?

[jpountz]

This is related to #9155. When sorting missing values last on a string field, we artificially replace null with an hypothetic maximum term so that the coordinating node knows how to compare this document to documents coming from other shards. But we forgot to do the opposite operation in setTopValue which is used for paging. So the comparator is feeded with this maximum term, which compares lower than a document without a value given that this maximum term IS a value (we should provide the comparator with null). Here is an untested patch that should fix the issue:

diff --git a/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/BytesRefFieldComparatorSource.java b/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/BytesRefFieldComparatorSource.java
index 7e64720..66fa306 100644
--- a/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/BytesRefFieldComparatorSource.java
+++ b/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/BytesRefFieldComparatorSource.java
@@ -97,11 +97,21 @@ public class BytesRefFieldComparatorSource extends IndexFieldData.XFieldComparat
                     // TopDocs.merge deal with it (it knows how to)
                     BytesRef value = super.value(slot);
                     if (value == null) {
+                        assert sortMissingFirst(missingValue) || sortMissingLast(missingValue);
                         value = missingBytes;
                     }
                     return value;
                 }

+                public void setTopValue(BytesRef topValue) {
+                    // symetric of value(int): if we need to feed the comparator with <tt>null</tt>
+                    // if we overrode the value with MAX_TERM in value(int)
+                    if (topValue == missingBytes && (sortMissingFirst(missingValue) || sortMissingLast(missingValue))) {
+                        topValue = null;
+                    }
+                    super.setTopValue(topValue);
+                }
+                
             };
         }

But I hope we can come with a cleaner solution on 2.x by doing #9155

[martijnvg]

@jpountz This proposed fix fixes the reported bug, so +1 for adding this initially to master, 1.x and 1.4 branches and then work on a better fix in master.