Extend testing of build method in ScriptSortBuilder (#26520)

Improve testing around the ScriptSortBuilder#build method, adding checks for
correct transfers of the sort mode and nested sorts.

Also changing the behaviour around the nested_path, nested_filter vs. nested
parameter in a similar way as in #26490 and deprecating the setters/getters for
the old syntax.

Closes #17286

core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java

@@ -408,7 +408,7 @@ public class FieldSortBuilder extends SortBuilder<FieldSortBuilder> {
     }
 
     @Override
-    public SortBuilder rewrite(QueryRewriteContext ctx) throws IOException {
+    public FieldSortBuilder rewrite(QueryRewriteContext ctx) throws IOException {
         if (nestedFilter == null) {
             return this;
         }

core/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java

@@ -176,15 +176,24 @@ public class ScriptSortBuilder extends SortBuilder<ScriptSortBuilder> {
     /**
      * Sets the nested filter that the nested objects should match with in order to be taken into account
      * for sorting.
+     *
+     * @deprecated set nested sort with {@link #setNestedSort(NestedSortBuilder)} and retrieve with {@link #getNestedSort()}
      */
+    @Deprecated
     public ScriptSortBuilder setNestedFilter(QueryBuilder nestedFilter) {
+        if (this.nestedSort != null) {
+            throw new IllegalArgumentException("Setting both nested_path/nested_filter and nested not allowed");
+        }
         this.nestedFilter = nestedFilter;
         return this;
     }
 
     /**
      * Gets the nested filter.
+     *
+     * @deprecated set nested sort with {@link #setNestedSort(NestedSortBuilder)} and retrieve with {@link #getNestedSort()}
      */
+    @Deprecated
     public QueryBuilder getNestedFilter() {
         return this.nestedFilter;
     }
@@ -192,24 +201,45 @@ public class ScriptSortBuilder extends SortBuilder<ScriptSortBuilder> {
     /**
      * Sets the nested path if sorting occurs on a field that is inside a nested object. For sorting by script this
      * needs to be specified.
+     *
+     * @deprecated set nested sort with {@link #setNestedSort(NestedSortBuilder)} and retrieve with {@link #getNestedSort()}
      */
+    @Deprecated
     public ScriptSortBuilder setNestedPath(String nestedPath) {
+        if (this.nestedSort != null) {
+            throw new IllegalArgumentException("Setting both nested_path/nested_filter and nested not allowed");
+        }
         this.nestedPath = nestedPath;
         return this;
     }
 
     /**
      * Gets the nested path.
+     *
+     * @deprecated set nested sort with {@link #setNestedSort(NestedSortBuilder)} and retrieve with {@link #getNestedSort()}
      */
+    @Deprecated
     public String getNestedPath() {
         return this.nestedPath;
     }
 
+    /**
+     * Returns the {@link NestedSortBuilder}
+     */
     public NestedSortBuilder getNestedSort() {
         return this.nestedSort;
     }
 
+    /**
+     * Sets the {@link NestedSortBuilder} to be used for fields that are inside a nested
+     * object. The {@link NestedSortBuilder} takes a `path` argument and an optional
+     * nested filter that the nested objects should match with in
+     * order to be taken into account for sorting.
+     */
     public ScriptSortBuilder setNestedSort(final NestedSortBuilder nestedSort) {
+        if (this.nestedFilter != null || this.nestedPath != null) {
+            throw new IllegalArgumentException("Setting both nested_path/nested_filter and nested not allowed");
+        }
         this.nestedSort = nestedSort;
         return this;
     }
@@ -420,7 +450,7 @@ public class ScriptSortBuilder extends SortBuilder<ScriptSortBuilder> {
     }
 
     @Override
-    public SortBuilder rewrite(QueryRewriteContext ctx) throws IOException {
+    public ScriptSortBuilder rewrite(QueryRewriteContext ctx) throws IOException {
         if (nestedFilter == null) {
             return this;
         }

core/src/test/java/org/elasticsearch/search/sort/AbstractSortTestCase.java

@@ -76,13 +76,14 @@ public abstract class AbstractSortTestCase<T extends SortBuilder<T>> extends EST
 
     private static NamedXContentRegistry xContentRegistry;
     private static ScriptService scriptService;
+    protected static String MOCK_SCRIPT_NAME = "dummy";
 
     @BeforeClass
     public static void init() {
         Settings baseSettings = Settings.builder()
                 .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir().toString())
                 .build();
-        Map<String, Function<Map<String, Object>, Object>> scripts = Collections.singletonMap("dummy", p -> null);
+        Map<String, Function<Map<String, Object>, Object>> scripts = Collections.singletonMap(MOCK_SCRIPT_NAME, p -> null);
         ScriptEngine engine = new MockScriptEngine(MockScriptEngine.NAME, scripts);
         scriptService = new ScriptService(baseSettings, Collections.singletonMap(engine.getType(), engine), ScriptModule.CORE_CONTEXTS);
 

core/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java

@@ -365,7 +365,6 @@ public class FieldSortBuilderTests extends AbstractSortTestCase<FieldSortBuilder
         iae = expectThrows(IllegalArgumentException.class,
                 () -> sortBuilder.setNestedSort(new NestedSortBuilder("otherPath")).setNestedFilter(QueryBuilders.matchAllQuery()));
         assertEquals("Setting both nested_path/nested_filter and nested not allowed", iae.getMessage());
-
     }
 
     @Override

core/src/test/java/org/elasticsearch/search/sort/ScriptSortBuilderTests.java

@@ -20,12 +20,23 @@
 package org.elasticsearch.search.sort;
 
 
+import org.apache.lucene.index.Term;
+import org.apache.lucene.search.MatchAllDocsQuery;
 import org.apache.lucene.search.SortField;
+import org.apache.lucene.search.TermQuery;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.common.xcontent.json.JsonXContent;
+import org.elasticsearch.index.fielddata.IndexFieldData.XFieldComparatorSource;
+import org.elasticsearch.index.fielddata.IndexFieldData.XFieldComparatorSource.Nested;
+import org.elasticsearch.index.fielddata.fieldcomparator.BytesRefFieldComparatorSource;
+import org.elasticsearch.index.fielddata.fieldcomparator.DoubleValuesComparatorSource;
+import org.elasticsearch.index.mapper.TypeFieldMapper;
+import org.elasticsearch.index.query.QueryBuilders;
+import org.elasticsearch.index.query.QueryShardContext;
 import org.elasticsearch.script.Script;
 import org.elasticsearch.script.ScriptType;
 import org.elasticsearch.search.DocValueFormat;
+import org.elasticsearch.search.MultiValueMode;
 import org.elasticsearch.search.sort.ScriptSortBuilder.ScriptSortType;
 
 import java.io.IOException;
@@ -34,6 +45,7 @@ import java.util.HashSet;
 import java.util.Set;
 
 import static org.elasticsearch.search.sort.NestedSortBuilderTests.createRandomNestedSort;
+import static org.hamcrest.Matchers.instanceOf;
 
 public class ScriptSortBuilderTests extends AbstractSortTestCase<ScriptSortBuilder> {
 
@@ -44,7 +56,7 @@ public class ScriptSortBuilderTests extends AbstractSortTestCase<ScriptSortBuild
 
     public static ScriptSortBuilder randomScriptSortBuilder() {
         ScriptSortType type = randomBoolean() ? ScriptSortType.NUMBER : ScriptSortType.STRING;
-        ScriptSortBuilder builder = new ScriptSortBuilder(mockScript("dummy"),
+        ScriptSortBuilder builder = new ScriptSortBuilder(mockScript(MOCK_SCRIPT_NAME),
                 type);
         if (randomBoolean()) {
                 builder.order(randomFrom(SortOrder.values()));
@@ -237,12 +249,112 @@ public class ScriptSortBuilderTests extends AbstractSortTestCase<ScriptSortBuild
      * script sort of type {@link ScriptSortType} does not work with {@link SortMode#AVG}, {@link SortMode#MEDIAN} or {@link SortMode#SUM}
      */
     public void testBadSortMode() throws IOException {
-        ScriptSortBuilder builder = new ScriptSortBuilder(mockScript("something"), ScriptSortType.STRING);
+        ScriptSortBuilder builder = new ScriptSortBuilder(mockScript(MOCK_SCRIPT_NAME), ScriptSortType.STRING);
         String sortMode = randomFrom(new String[] { "avg", "median", "sum" });
         Exception e = expectThrows(IllegalArgumentException.class, () -> builder.sortMode(SortMode.fromString(sortMode)));
         assertEquals("script sort of type [string] doesn't support mode [" + sortMode + "]", e.getMessage());
     }
 
+    /**
+     * Test that the sort builder mode gets transfered correctly to the SortField
+     */
+    public void testMultiValueMode() throws IOException {
+        QueryShardContext shardContextMock = createMockShardContext();
+        for (SortMode mode : SortMode.values()) {
+            ScriptSortBuilder sortBuilder = new ScriptSortBuilder(mockScript(MOCK_SCRIPT_NAME), ScriptSortType.NUMBER);
+            sortBuilder.sortMode(mode);
+            SortField sortField = sortBuilder.build(shardContextMock).field;
+            assertThat(sortField.getComparatorSource(), instanceOf(XFieldComparatorSource.class));
+            XFieldComparatorSource comparatorSource = (XFieldComparatorSource) sortField.getComparatorSource();
+            assertEquals(MultiValueMode.fromString(mode.toString()), comparatorSource.sortMode());
+        }
+
+        // check that without mode set, order ASC sets mode to MIN, DESC to MAX
+        ScriptSortBuilder sortBuilder = new ScriptSortBuilder(mockScript(MOCK_SCRIPT_NAME), ScriptSortType.NUMBER);
+        sortBuilder.order(SortOrder.ASC);
+        SortField sortField = sortBuilder.build(shardContextMock).field;
+        assertThat(sortField.getComparatorSource(), instanceOf(XFieldComparatorSource.class));
+        XFieldComparatorSource comparatorSource = (XFieldComparatorSource) sortField.getComparatorSource();
+        assertEquals(MultiValueMode.MIN, comparatorSource.sortMode());
+
+        sortBuilder = new ScriptSortBuilder(mockScript(MOCK_SCRIPT_NAME), ScriptSortType.NUMBER);
+        sortBuilder.order(SortOrder.DESC);
+        sortField = sortBuilder.build(shardContextMock).field;
+        assertThat(sortField.getComparatorSource(), instanceOf(XFieldComparatorSource.class));
+        comparatorSource = (XFieldComparatorSource) sortField.getComparatorSource();
+        assertEquals(MultiValueMode.MAX, comparatorSource.sortMode());
+    }
+
+    /**
+     * Test that the correct comparator sort is returned, based on the script type
+     */
+    public void testBuildCorrectComparatorType() throws IOException {
+        ScriptSortBuilder sortBuilder = new ScriptSortBuilder(mockScript(MOCK_SCRIPT_NAME), ScriptSortType.STRING);
+        SortField sortField = sortBuilder.build(createMockShardContext()).field;
+        assertThat(sortField.getComparatorSource(), instanceOf(BytesRefFieldComparatorSource.class));
+
+        sortBuilder = new ScriptSortBuilder(mockScript(MOCK_SCRIPT_NAME), ScriptSortType.NUMBER);
+        sortField = sortBuilder.build(createMockShardContext()).field;
+        assertThat(sortField.getComparatorSource(), instanceOf(DoubleValuesComparatorSource.class));
+    }
+
+    /**
+     * Test that the sort builder nested object gets created in the SortField
+     */
+    public void testBuildNested() throws IOException {
+        QueryShardContext shardContextMock = createMockShardContext();
+
+        ScriptSortBuilder sortBuilder = new ScriptSortBuilder(mockScript(MOCK_SCRIPT_NAME), ScriptSortType.NUMBER)
+                .setNestedSort(new NestedSortBuilder("path").setFilter(QueryBuilders.matchAllQuery()));
+        SortField sortField = sortBuilder.build(shardContextMock).field;
+        assertThat(sortField.getComparatorSource(), instanceOf(XFieldComparatorSource.class));
+        XFieldComparatorSource comparatorSource = (XFieldComparatorSource) sortField.getComparatorSource();
+        Nested nested = comparatorSource.nested();
+        assertNotNull(nested);
+        assertEquals(new MatchAllDocsQuery(), nested.getInnerQuery());
+
+        sortBuilder = new ScriptSortBuilder(mockScript(MOCK_SCRIPT_NAME), ScriptSortType.NUMBER).setNestedPath("path");
+        sortField = sortBuilder.build(shardContextMock).field;
+        assertThat(sortField.getComparatorSource(), instanceOf(XFieldComparatorSource.class));
+        comparatorSource = (XFieldComparatorSource) sortField.getComparatorSource();
+        nested = comparatorSource.nested();
+        assertNotNull(nested);
+        assertEquals(new TermQuery(new Term(TypeFieldMapper.NAME, "__path")), nested.getInnerQuery());
+
+        sortBuilder = new ScriptSortBuilder(mockScript(MOCK_SCRIPT_NAME), ScriptSortType.NUMBER).setNestedPath("path")
+                .setNestedFilter(QueryBuilders.matchAllQuery());
+        sortField = sortBuilder.build(shardContextMock).field;
+        assertThat(sortField.getComparatorSource(), instanceOf(XFieldComparatorSource.class));
+        comparatorSource = (XFieldComparatorSource) sortField.getComparatorSource();
+        nested = comparatorSource.nested();
+        assertNotNull(nested);
+        assertEquals(new MatchAllDocsQuery(), nested.getInnerQuery());
+
+        // if nested path is missing, we omit nested element in the comparator
+        sortBuilder = new ScriptSortBuilder(mockScript(MOCK_SCRIPT_NAME), ScriptSortType.NUMBER)
+                .setNestedFilter(QueryBuilders.matchAllQuery());
+        sortField = sortBuilder.build(shardContextMock).field;
+        assertThat(sortField.getComparatorSource(), instanceOf(XFieldComparatorSource.class));
+        comparatorSource = (XFieldComparatorSource) sortField.getComparatorSource();
+        assertNull(comparatorSource.nested());
+    }
+
+    /**
+     * Test we can either set nested sort via path/filter or via nested sort builder, not both
+     */
+    public void testNestedSortBothThrows() throws IOException {
+        ScriptSortBuilder sortBuilder = new ScriptSortBuilder(mockScript(MOCK_SCRIPT_NAME), ScriptSortType.NUMBER);
+        IllegalArgumentException iae = expectThrows(IllegalArgumentException.class,
+                () -> sortBuilder.setNestedPath("nestedPath").setNestedSort(new NestedSortBuilder("otherPath")));
+        assertEquals("Setting both nested_path/nested_filter and nested not allowed", iae.getMessage());
+        iae = expectThrows(IllegalArgumentException.class,
+                () -> sortBuilder.setNestedSort(new NestedSortBuilder("otherPath")).setNestedPath("nestedPath"));
+        assertEquals("Setting both nested_path/nested_filter and nested not allowed", iae.getMessage());
+        iae = expectThrows(IllegalArgumentException.class,
+                () -> sortBuilder.setNestedSort(new NestedSortBuilder("otherPath")).setNestedFilter(QueryBuilders.matchAllQuery()));
+        assertEquals("Setting both nested_path/nested_filter and nested not allowed", iae.getMessage());
+    }
+
     @Override
     protected ScriptSortBuilder fromXContent(XContentParser parser, String fieldName) throws IOException {
         return ScriptSortBuilder.fromXContent(parser, fieldName);