From e886fd349779f88837375dc991b823aeccf77066 Mon Sep 17 00:00:00 2001 From: erayarslan Date: Sun, 15 Sep 2019 18:39:04 +0300 Subject: [PATCH 1/2] max_children exist only in top level nested sort --- .../search/sort/FieldSortBuilder.java | 17 ++++++++--- .../search/sort/GeoDistanceSortBuilder.java | 7 ++--- .../search/sort/ScriptSortBuilder.java | 6 ++-- .../search/sort/FieldSortIT.java | 30 +++++++++++++++++++ 4 files changed, 47 insertions(+), 13 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java b/server/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java index 949a5a3ff441c..ede0616ccfe66 100644 --- a/server/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java @@ -332,10 +332,7 @@ public SortFieldAndFormat build(QueryShardContext context) throws IOException { Nested nested = null; if (isUnmapped == false) { if (nestedSort != null) { - if (nestedSort.getNestedSort() != null && nestedSort.getMaxChildren() != Integer.MAX_VALUE) { - throw new QueryShardException(context, - "max_children is only supported on last level of nested sort"); - } + validateMaxChildrenExistOnlyInTopLevelNestedSort(context, nestedSort); nested = resolveNested(context, nestedSort); } else { validateMissingNestedPath(context, fieldName); @@ -362,6 +359,18 @@ public SortFieldAndFormat build(QueryShardContext context) throws IOException { return new SortFieldAndFormat(field, fieldType.docValueFormat(null, null)); } + /** + * Throws an exception if max children is not located at top level nested sort. + */ + static void validateMaxChildrenExistOnlyInTopLevelNestedSort(QueryShardContext context, NestedSortBuilder nestedSort) { + for (NestedSortBuilder child = nestedSort.getNestedSort(); child != null; child = child.getNestedSort()) { + if (child.getMaxChildren() != Integer.MAX_VALUE) { + throw new QueryShardException(context, + "max_children is only supported on top level of nested sort"); + } + } + } + /** * Throws an exception if the provided field requires a nested context. */ diff --git a/server/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java b/server/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java index 630b93b4f34b5..12b378cfd6c0c 100644 --- a/server/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java @@ -52,7 +52,6 @@ import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.index.query.QueryRewriteContext; import org.elasticsearch.index.query.QueryShardContext; -import org.elasticsearch.index.query.QueryShardException; import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.MultiValueMode; @@ -64,6 +63,7 @@ import java.util.Objects; import static org.elasticsearch.search.sort.FieldSortBuilder.validateMissingNestedPath; +import static org.elasticsearch.search.sort.FieldSortBuilder.validateMaxChildrenExistOnlyInTopLevelNestedSort; import static org.elasticsearch.search.sort.NestedSortBuilder.NESTED_FIELD; /** @@ -536,10 +536,7 @@ public SortFieldAndFormat build(QueryShardContext context) throws IOException { Nested nested = null; if (nestedSort != null) { - if (nestedSort.getNestedSort() != null && nestedSort.getMaxChildren() != Integer.MAX_VALUE) { - throw new QueryShardException(context, - "max_children is only supported on last level of nested sort"); - } + validateMaxChildrenExistOnlyInTopLevelNestedSort(context, nestedSort); nested = resolveNested(context, nestedSort); } else { validateMissingNestedPath(context, fieldName); diff --git a/server/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java b/server/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java index 17fed4d9ac19c..d3fa3bb0a1fe9 100644 --- a/server/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java @@ -58,6 +58,7 @@ import java.util.Objects; import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg; +import static org.elasticsearch.search.sort.FieldSortBuilder.validateMaxChildrenExistOnlyInTopLevelNestedSort; import static org.elasticsearch.search.sort.NestedSortBuilder.NESTED_FIELD; /** @@ -242,10 +243,7 @@ public SortFieldAndFormat build(QueryShardContext context) throws IOException { Nested nested = null; if (nestedSort != null) { - if (nestedSort.getNestedSort() != null && nestedSort.getMaxChildren() != Integer.MAX_VALUE) { - throw new QueryShardException(context, - "max_children is only supported on last level of nested sort"); - } + validateMaxChildrenExistOnlyInTopLevelNestedSort(context, nestedSort); nested = resolveNested(context, nestedSort); } diff --git a/server/src/test/java/org/elasticsearch/search/sort/FieldSortIT.java b/server/src/test/java/org/elasticsearch/search/sort/FieldSortIT.java index 0a7c30101d3b3..250731938f959 100644 --- a/server/src/test/java/org/elasticsearch/search/sort/FieldSortIT.java +++ b/server/src/test/java/org/elasticsearch/search/sort/FieldSortIT.java @@ -1421,6 +1421,20 @@ public void testNestedSort() throws IOException, InterruptedException, Execution .endObject() .endObject() .endObject() + .startObject("bar") + .field("type", "nested") + .startObject("properties") + .startObject("foo") + .field("type", "text") + .field("fielddata", true) + .startObject("fields") + .startObject("sub") + .field("type", "keyword") + .endObject() + .endObject() + .endObject() + .endObject() + .endObject() .endObject() .endObject() .endObject() @@ -1473,6 +1487,22 @@ public void testNestedSort() throws IOException, InterruptedException, Execution assertThat(hits[0].getSortValues()[0], is("bar")); assertThat(hits[1].getSortValues()[0], is("abc")); + try { + client().prepareSearch() + .setQuery(matchAllQuery()) + .addSort(SortBuilders + .fieldSort("nested.bar.foo") + .setNestedSort(new NestedSortBuilder("nested") + .setNestedSort(new NestedSortBuilder("nested.bar") + .setMaxChildren(1))) + .order(SortOrder.DESC)) + .get(); + } catch (SearchPhaseExecutionException e) { + for (ShardSearchFailure shardSearchFailure : e.shardFailures()) { + assertThat(shardSearchFailure.toString(), containsString("[max_children is only supported on top level of nested sort]")); + } + } + // We sort on nested sub field searchResponse = client().prepareSearch() .setQuery(matchAllQuery()) From 61370d0cf86a9c23707498bd2c8a5b1244beefe6 Mon Sep 17 00:00:00 2001 From: Eray Date: Wed, 18 Sep 2019 13:29:50 +0300 Subject: [PATCH 2/2] integration test style changed: try/catch -> expectThrows --- .../search/sort/FieldSortIT.java | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/search/sort/FieldSortIT.java b/server/src/test/java/org/elasticsearch/search/sort/FieldSortIT.java index 250731938f959..42fde64e3ed09 100644 --- a/server/src/test/java/org/elasticsearch/search/sort/FieldSortIT.java +++ b/server/src/test/java/org/elasticsearch/search/sort/FieldSortIT.java @@ -1487,20 +1487,20 @@ public void testNestedSort() throws IOException, InterruptedException, Execution assertThat(hits[0].getSortValues()[0], is("bar")); assertThat(hits[1].getSortValues()[0], is("abc")); - try { - client().prepareSearch() - .setQuery(matchAllQuery()) - .addSort(SortBuilders - .fieldSort("nested.bar.foo") - .setNestedSort(new NestedSortBuilder("nested") - .setNestedSort(new NestedSortBuilder("nested.bar") - .setMaxChildren(1))) - .order(SortOrder.DESC)) - .get(); - } catch (SearchPhaseExecutionException e) { - for (ShardSearchFailure shardSearchFailure : e.shardFailures()) { - assertThat(shardSearchFailure.toString(), containsString("[max_children is only supported on top level of nested sort]")); - } + { + SearchPhaseExecutionException exc = expectThrows(SearchPhaseExecutionException.class, + () -> client().prepareSearch() + .setQuery(matchAllQuery()) + .addSort(SortBuilders + .fieldSort("nested.bar.foo") + .setNestedSort(new NestedSortBuilder("nested") + .setNestedSort(new NestedSortBuilder("nested.bar") + .setMaxChildren(1))) + .order(SortOrder.DESC)) + .get() + ); + assertThat(exc.toString(), + containsString("max_children is only supported on top level of nested sort")); } // We sort on nested sub field