Fix Plain Highlighter ordering for none (#74084) (#74913)

The plain highlighter should order the fragments by order of appearance by default, but sorts them by score internally. This fix changes the sorting comparator in case ordering by score is not selected (the default) and adds testing around this. Closes #58236
elastic · Jul 5, 2021 · 2a6b4e6 · 2a6b4e6
1 parent 36f5e7d
commit 2a6b4e6
Show file tree

Hide file tree

Showing 2 changed files with 50 additions and 4 deletions.
diff --git a/...usterTest/java/org/elasticsearch/search/fetch/subphase/highlight/HighlighterSearchIT.java b/...usterTest/java/org/elasticsearch/search/fetch/subphase/highlight/HighlighterSearchIT.java
@@ -698,15 +698,60 @@ public void testPlainHighlighter() throws Exception {
                 .setSource("field1", "this is a test", "field2", "The quick brown fox jumps over the lazy dog").get();
         refresh();
 
-        logger.info("--> highlighting and searching on field1");
         SearchSourceBuilder source = searchSource()
                 .query(termQuery("field1", "test"))
-                .highlighter(highlight().field("field1").order("score").preTags("<xxx>").postTags("</xxx>"));
+                .highlighter(highlight().highlighterType("plain").field("field1").order("score").preTags("<xxx>").postTags("</xxx>"));
 
         SearchResponse searchResponse = client().prepareSearch("test").setSource(source).get();
 
         assertHighlight(searchResponse, 0, "field1", 0, 1, equalTo("this is a <xxx>test</xxx>"));
+    }
+
+    public void testPlainHighlighterOrder() throws Exception {
+        ensureGreen();
+
+        client().prepareIndex("test", "type")
+                .setSource("field1", "The quick brown fox jumps over the lazy brown dog but to no suprise the dog doesn't care").get();
+        refresh();
+
+        {
+            // fragments should be in order of appearance by default
+            SearchSourceBuilder source = searchSource().query(matchQuery("field1", "brown dog"))
+                .highlighter(
+                    highlight().highlighterType("plain").field("field1").preTags("<xxx>").postTags("</xxx>").fragmentSize(25)
+                );
+
+            SearchResponse searchResponse = client().prepareSearch("test").setSource(source).get();
 
+            assertHighlight(searchResponse, 0, "field1", 0, 3, equalTo("The quick <xxx>brown</xxx> fox"));
+            assertHighlight(searchResponse, 0, "field1", 1, 3, equalTo(" jumps over the lazy <xxx>brown</xxx> <xxx>dog</xxx>"));
+            assertHighlight(searchResponse, 0, "field1", 2, 3, equalTo(" <xxx>dog</xxx> doesn't care"));
+
+            // lets be explicit about the order
+            source = searchSource().query(matchQuery("field1", "brown dog"))
+                .highlighter(
+                    highlight().highlighterType("plain").field("field1").order("none").preTags("<xxx>").postTags("</xxx>").fragmentSize(25)
+                );
+
+            searchResponse = client().prepareSearch("test").setSource(source).get();
+
+            assertHighlight(searchResponse, 0, "field1", 0, 3, equalTo("The quick <xxx>brown</xxx> fox"));
+            assertHighlight(searchResponse, 0, "field1", 1, 3, equalTo(" jumps over the lazy <xxx>brown</xxx> <xxx>dog</xxx>"));
+            assertHighlight(searchResponse, 0, "field1", 2, 3, equalTo(" <xxx>dog</xxx> doesn't care"));
+        }
+        {
+            // order by score
+            SearchSourceBuilder source = searchSource().query(matchQuery("field1", "brown dog"))
+                .highlighter(
+                    highlight().highlighterType("plain").order("score").field("field1").preTags("<xxx>").postTags("</xxx>").fragmentSize(25)
+                );
+
+            SearchResponse searchResponse = client().prepareSearch("test").setSource(source).get();
+
+            assertHighlight(searchResponse, 0, "field1", 0, 3, equalTo(" jumps over the lazy <xxx>brown</xxx> <xxx>dog</xxx>"));
+            assertHighlight(searchResponse, 0, "field1", 1, 3, equalTo("The quick <xxx>brown</xxx> fox"));
+            assertHighlight(searchResponse, 0, "field1", 2, 3, equalTo(" <xxx>dog</xxx> doesn't care"));
+        }
     }
 
     public void testFastVectorHighlighter() throws Exception {

diff --git a/server/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/PlainHighlighter.java b/server/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/PlainHighlighter.java
@@ -135,8 +135,9 @@ public HighlightField highlight(FieldHighlightContext fieldContext) throws IOExc
             }
         }
 
-        if (field.fieldOptions().scoreOrdered()) {
-            CollectionUtil.introSort(fragsList, (o1, o2) -> Math.round(o2.getScore() - o1.getScore()));
+        // fragments are ordered by score by default since we add them in best
+        if (field.fieldOptions().scoreOrdered() == false) {
+            CollectionUtil.introSort(fragsList, (o1, o2) -> o1.getFragNum() - o2.getFragNum());
         }
         String[] fragments;
         // number_of_fragments is set to 0 but we have a multivalued field