diff --git a/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java b/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java index 5c95f3deb30..4a9fd3f8a36 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java @@ -408,7 +408,7 @@ public class FieldSortBuilder extends SortBuilder { } @Override - public SortBuilder rewrite(QueryRewriteContext ctx) throws IOException { + public FieldSortBuilder rewrite(QueryRewriteContext ctx) throws IOException { if (nestedFilter == null) { return this; } diff --git a/core/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java b/core/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java index e9f28f4617e..bb433063dd1 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java @@ -176,15 +176,24 @@ public class ScriptSortBuilder extends SortBuilder { /** * 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 { /** * 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 { } @Override - public SortBuilder rewrite(QueryRewriteContext ctx) throws IOException { + public ScriptSortBuilder rewrite(QueryRewriteContext ctx) throws IOException { if (nestedFilter == null) { return this; } diff --git a/core/src/test/java/org/elasticsearch/search/sort/AbstractSortTestCase.java b/core/src/test/java/org/elasticsearch/search/sort/AbstractSortTestCase.java index 683e949d5cd..d05ddf4ee64 100644 --- a/core/src/test/java/org/elasticsearch/search/sort/AbstractSortTestCase.java +++ b/core/src/test/java/org/elasticsearch/search/sort/AbstractSortTestCase.java @@ -76,13 +76,14 @@ public abstract class AbstractSortTestCase> 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, Object>> scripts = Collections.singletonMap("dummy", p -> null); + Map, 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); diff --git a/core/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java b/core/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java index 7dd3f0f6818..0ea9e04e1cb 100644 --- a/core/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java @@ -365,7 +365,6 @@ public class FieldSortBuilderTests extends AbstractSortTestCase sortBuilder.setNestedSort(new NestedSortBuilder("otherPath")).setNestedFilter(QueryBuilders.matchAllQuery())); assertEquals("Setting both nested_path/nested_filter and nested not allowed", iae.getMessage()); - } @Override diff --git a/core/src/test/java/org/elasticsearch/search/sort/ScriptSortBuilderTests.java b/core/src/test/java/org/elasticsearch/search/sort/ScriptSortBuilderTests.java index a38fbe77cf6..9158c14eb7c 100644 --- a/core/src/test/java/org/elasticsearch/search/sort/ScriptSortBuilderTests.java +++ b/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 { @@ -44,7 +56,7 @@ public class ScriptSortBuilderTests extends AbstractSortTestCase 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);