From 216828ec222cf1325acd1fa17cc6f1547232a9a5 Mon Sep 17 00:00:00 2001 From: jimczi Date: Mon, 3 Jun 2019 17:54:22 +0200 Subject: [PATCH 1/6] Remove deprecated sort options: nested_path and nested_filter This commit removes the nested_path and nested_filter options deprecated in 6x. This change also checks that the sort field has a [nested] option if it is under a nested object and throws an exception if it's not the case. Closes #27098 --- .../migration/migrate_8_0/search.asciidoc | 13 +- docs/reference/search/request/sort.asciidoc | 14 +- .../index/search/NestedHelper.java | 2 +- .../search/sort/FieldSortBuilder.java | 219 ++++++------------ .../search/sort/GeoDistanceSortBuilder.java | 125 ++-------- .../search/sort/ScriptSortBuilder.java | 117 ++-------- .../search/sort/SortBuilder.java | 7 +- .../search/nested/SimpleNestedIT.java | 63 ++--- .../search/sort/FieldSortBuilderTests.java | 73 +----- .../search/sort/FieldSortIT.java | 15 +- .../search/sort/GeoDistanceIT.java | 26 ++- .../sort/GeoDistanceSortBuilderTests.java | 90 ++----- .../search/sort/ScriptSortBuilderTests.java | 36 +-- .../search/sort/SortBuilderTests.java | 10 - 14 files changed, 233 insertions(+), 577 deletions(-) diff --git a/docs/reference/migration/migrate_8_0/search.asciidoc b/docs/reference/migration/migrate_8_0/search.asciidoc index 6fba2970f593e..9955457e02753 100644 --- a/docs/reference/migration/migrate_8_0/search.asciidoc +++ b/docs/reference/migration/migrate_8_0/search.asciidoc @@ -12,11 +12,16 @@ The `/{index}/{type}/_termvectors`, `/{index}/{type}/{id}/_termvectors` and `/{i [float] ==== Removal of queries -The `common` query was deprecated in 7.x and has been removed in 8.0. -The same functionality can be achieved by the `match` query if the total number of hits is not tracked. +* The `common` query was deprecated in 7.x and has been removed in 8.0. + The same functionality can be achieved by the `match` query if the total number of hits is not tracked. [float] ===== Removal of query parameters -The `cutoff_frequency` parameter was deprecated in 7.x and has been removed in 8.0 from `match` and `multi_match` queries. -The same functionality can be achieved without any configuration provided that the total number of hits is not tracked. +* The `cutoff_frequency` parameter was deprecated in 7.x and has been removed in 8.0 from `match` and `multi_match` queries. + The same functionality can be achieved without any configuration provided that the total number of hits is not tracked. + +[float] +===== Removal of sort parameters + +* The `nested_filter` and `nested_path` options, deprecated in 6.x, has been removed in favor of the `nested` context. \ No newline at end of file diff --git a/docs/reference/search/request/sort.asciidoc b/docs/reference/search/request/sort.asciidoc index c12ec3a679a72..5ccfef46440d4 100644 --- a/docs/reference/search/request/sort.asciidoc +++ b/docs/reference/search/request/sort.asciidoc @@ -252,7 +252,7 @@ field support has a `nested` sort option with the following properties: A filter that the inner objects inside the nested path should match with in order for its field values to be taken into account by sorting. Common case is to repeat the query / filter inside the - nested filter or query. By default no `nested_filter` is active. + nested filter or query. By default no `filter` is active. `max_children`:: The maximum number of children to consider per root document when picking the sort value. Defaults to unlimited. @@ -260,12 +260,8 @@ field support has a `nested` sort option with the following properties: Same as top-level `nested` but applies to another nested path within the current nested object. -[WARNING] -.Nested sort options before Elasticsearch 6.1 -============================================ - -The `nested_path` and `nested_filter` options have been deprecated in -favor of the options documented above. +NOTE: Elasticsearch will throw an error if a nested field is defined in a sort without +a `nested` context. ============================================ @@ -300,7 +296,7 @@ POST /_search // CONSOLE In the below example `parent` and `child` fields are of type `nested`. -The `nested_path` needs to be specified at each level; otherwise, Elasticsearch doesn't know on what nested level sort values need to be captured. +The `nested.path` needs to be specified at each level; otherwise, Elasticsearch doesn't know on what nested level sort values need to be captured. [source,js] -------------------------------------------------- @@ -374,7 +370,7 @@ GET /_search // CONSOLE NOTE: If a nested inner object doesn't match with -the `nested_filter` then a missing value is used. +the `nested.filter` then a missing value is used. ==== Ignoring Unmapped Fields diff --git a/server/src/main/java/org/elasticsearch/index/search/NestedHelper.java b/server/src/main/java/org/elasticsearch/index/search/NestedHelper.java index 1c17fa0cb935f..ab2ae044bdbaf 100644 --- a/server/src/main/java/org/elasticsearch/index/search/NestedHelper.java +++ b/server/src/main/java/org/elasticsearch/index/search/NestedHelper.java @@ -192,7 +192,7 @@ boolean mightMatchNonNestedDocs(String field, String nestedPath) { return true; // the field is not a sub field of the nested path } - private static String parentObject(String field) { + public static String parentObject(String field) { int lastDot = field.lastIndexOf('.'); if (lastDot == -1) { return null; 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 6598d32bc2ca8..0ef0a6dc3da8b 100644 --- a/server/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java @@ -19,13 +19,11 @@ package org.elasticsearch.search.sort; -import org.apache.logging.log4j.LogManager; import org.apache.lucene.search.SortField; import org.elasticsearch.Version; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; -import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.xcontent.ObjectParser; import org.elasticsearch.common.xcontent.ObjectParser.ValueType; import org.elasticsearch.common.xcontent.XContentBuilder; @@ -36,6 +34,7 @@ import org.elasticsearch.index.fielddata.IndexNumericFieldData.NumericType; import org.elasticsearch.index.fielddata.plain.SortedNumericDVIndexFieldData; import org.elasticsearch.index.mapper.MappedFieldType; +import org.elasticsearch.index.mapper.ObjectMapper; import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.index.query.QueryRewriteContext; import org.elasticsearch.index.query.QueryShardContext; @@ -47,13 +46,13 @@ import java.util.Locale; import java.util.Objects; +import static org.elasticsearch.index.search.NestedHelper.parentObject; import static org.elasticsearch.search.sort.NestedSortBuilder.NESTED_FIELD; /** * A sort builder to sort based on a document field. */ public class FieldSortBuilder extends SortBuilder { - private static final DeprecationLogger deprecationLogger = new DeprecationLogger(LogManager.getLogger(FieldSortBuilder.class)); public static final String NAME = "field_sort"; public static final ParseField MISSING = new ParseField("missing"); @@ -80,10 +79,6 @@ public class FieldSortBuilder extends SortBuilder { private SortMode sortMode; - private QueryBuilder nestedFilter; - - private String nestedPath; - private NestedSortBuilder nestedSort; /** Copy constructor. */ @@ -95,8 +90,6 @@ public FieldSortBuilder(FieldSortBuilder template) { if (template.sortMode != null) { this.sortMode(template.sortMode()); } - this.setNestedFilter(template.getNestedFilter()); - this.setNestedPath(template.getNestedPath()); if (template.getNestedSort() != null) { this.setNestedSort(template.getNestedSort()); } @@ -121,8 +114,12 @@ public FieldSortBuilder(String fieldName) { */ public FieldSortBuilder(StreamInput in) throws IOException { fieldName = in.readString(); - nestedFilter = in.readOptionalNamedWriteable(QueryBuilder.class); - nestedPath = in.readOptionalString(); + if (in.getVersion().before(Version.V_8_0_0)) { + if (in.readOptionalNamedWriteable(QueryBuilder.class) != null || in.readOptionalString() != null) { + throw new IOException("the [sort] options [nested_path] and [nested_filter] are removed in 8.x, " + + "please use [nested] instead"); + } + } missing = in.readGenericValue(); order = in.readOptionalWriteable(SortOrder::readFromStream); sortMode = in.readOptionalWriteable(SortMode::readFromStream); @@ -136,8 +133,10 @@ public FieldSortBuilder(StreamInput in) throws IOException { @Override public void writeTo(StreamOutput out) throws IOException { out.writeString(fieldName); - out.writeOptionalNamedWriteable(nestedFilter); - out.writeOptionalString(nestedPath); + if (out.getVersion().before(Version.V_8_0_0)) { + out.writeOptionalNamedWriteable(null); + out.writeOptionalString(null); + } out.writeGenericValue(missing); out.writeOptionalWriteable(order); out.writeOptionalWriteable(sortMode); @@ -210,58 +209,6 @@ public SortMode sortMode() { return this.sortMode; } - /** - * 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 FieldSortBuilder 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; - } - - /** - * Returns 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 QueryBuilder getNestedFilter() { - return this.nestedFilter; - } - - /** - * Sets the nested path if sorting occurs on a field that is inside a nested - * object. By default when sorting on a field inside a nested object, the - * nearest upper nested object is selected as nested path. - * - * @deprecated set nested sort with {@link #setNestedSort(NestedSortBuilder)} and retrieve with {@link #getNestedSort()} - */ - @Deprecated - public FieldSortBuilder 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; - } - - /** - * Returns the nested path if sorting occurs in a field that is inside a - * nested object. - * @deprecated set nested sort with {@link #setNestedSort(NestedSortBuilder)} and retrieve with {@link #getNestedSort()} - */ - @Deprecated - public String getNestedPath() { - return this.nestedPath; - } - /** * Returns the {@link NestedSortBuilder} */ @@ -276,9 +223,6 @@ public NestedSortBuilder getNestedSort() { * order to be taken into account for sorting. */ public FieldSortBuilder 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; } @@ -330,12 +274,6 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws if (sortMode != null) { builder.field(SORT_MODE.getPreferredName(), sortMode); } - if (nestedFilter != null) { - builder.field(NESTED_FILTER_FIELD.getPreferredName(), nestedFilter, params); - } - if (nestedPath != null) { - builder.field(NESTED_PATH_FIELD.getPreferredName(), nestedPath); - } if (nestedSort != null) { builder.field(NESTED_FIELD.getPreferredName(), nestedSort); } @@ -367,65 +305,73 @@ private static NumericType resolveNumericType(String value) { @Override public SortFieldAndFormat build(QueryShardContext context) throws IOException { if (DOC_FIELD_NAME.equals(fieldName)) { - if (order == SortOrder.DESC) { - return SORT_DOC_REVERSE; + return order == SortOrder.DESC ? SORT_DOC_REVERSE : SORT_DOC; + } + + boolean isUnmapped = false; + MappedFieldType fieldType = context.fieldMapper(fieldName); + if (fieldType == null) { + isUnmapped = true; + if (unmappedType != null) { + fieldType = context.getMapperService().unmappedFieldType(unmappedType); } else { - return SORT_DOC; - } - } else { - boolean isUnmapped = false; - MappedFieldType fieldType = context.fieldMapper(fieldName); - if (fieldType == null) { - isUnmapped = true; - if (unmappedType != null) { - fieldType = context.getMapperService().unmappedFieldType(unmappedType); - } else { - throw new QueryShardException(context, "No mapping found for [" + fieldName + "] in order to sort on"); - } + throw new QueryShardException(context, "No mapping found for [" + fieldName + "] in order to sort on"); } + } - MultiValueMode localSortMode = null; - if (sortMode != null) { - localSortMode = MultiValueMode.fromString(sortMode.toString()); - } + MultiValueMode localSortMode = null; + if (sortMode != null) { + localSortMode = MultiValueMode.fromString(sortMode.toString()); + } - boolean reverse = (order == SortOrder.DESC); - if (localSortMode == null) { - localSortMode = reverse ? MultiValueMode.MAX : MultiValueMode.MIN; - } + boolean reverse = (order == SortOrder.DESC); + if (localSortMode == null) { + localSortMode = reverse ? MultiValueMode.MAX : MultiValueMode.MIN; + } - 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"); - } - // new nested sorts takes priority - nested = resolveNested(context, nestedSort); - } else { - nested = resolveNested(context, nestedPath, nestedFilter); + 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"); } + // new nested sorts takes priority + nested = resolveNested(context, nestedSort); + } else { + verifyNestedField(context, fieldName); } + } - IndexFieldData fieldData = context.getForField(fieldType); - if (fieldData instanceof IndexNumericFieldData == false - && (sortMode == SortMode.SUM || sortMode == SortMode.AVG || sortMode == SortMode.MEDIAN)) { - throw new QueryShardException(context, "we only support AVG, MEDIAN and SUM on number based fields"); + IndexFieldData fieldData = context.getForField(fieldType); + if (fieldData instanceof IndexNumericFieldData == false + && (sortMode == SortMode.SUM || sortMode == SortMode.AVG || sortMode == SortMode.MEDIAN)) { + throw new QueryShardException(context, "we only support AVG, MEDIAN and SUM on number based fields"); + } + final SortField field; + if (numericType != null) { + if (fieldData instanceof IndexNumericFieldData == false) { + throw new QueryShardException(context, + "[numeric_type] option cannot be set on a non-numeric field, got " + fieldType.typeName()); } - final SortField field; - if (numericType != null) { - if (fieldData instanceof IndexNumericFieldData == false) { + SortedNumericDVIndexFieldData numericFieldData = (SortedNumericDVIndexFieldData) fieldData; + NumericType resolvedType = resolveNumericType(numericType); + field = numericFieldData.sortField(resolvedType, missing, localSortMode, nested, reverse); + } else { + field = fieldData.sortField(missing, localSortMode, nested, reverse); + } + return new SortFieldAndFormat(field, fieldType.docValueFormat(null, null)); + } + + static void verifyNestedField(QueryShardContext context, String field) { + for (String parent = parentObject(field); parent != null; parent = parentObject(parent)) { + ObjectMapper mapper = context.getObjectMapper(parent); + if (mapper != null && mapper.nested().isNested()) { + if (mapper.nested().isIncludeInRoot() == false) { throw new QueryShardException(context, - "[numeric_type] option cannot be set on a non-numeric field, got " + fieldType.typeName()); + "it is mandatory to set the [nested] context on the nested sort field: [" + field + "]."); } - SortedNumericDVIndexFieldData numericFieldData = (SortedNumericDVIndexFieldData) fieldData; - NumericType resolvedType = resolveNumericType(numericType); - field = numericFieldData.sortField(resolvedType, missing, localSortMode, nested, reverse); - } else { - field = fieldData.sortField(missing, localSortMode, nested, reverse); } - return new SortFieldAndFormat(field, fieldType.docValueFormat(null, null)); } } @@ -440,8 +386,7 @@ public boolean equals(Object other) { } FieldSortBuilder builder = (FieldSortBuilder) other; - return (Objects.equals(this.fieldName, builder.fieldName) && Objects.equals(this.nestedFilter, builder.nestedFilter) - && Objects.equals(this.nestedPath, builder.nestedPath) && Objects.equals(this.missing, builder.missing) + return (Objects.equals(this.fieldName, builder.fieldName) && Objects.equals(this.missing, builder.missing) && Objects.equals(this.order, builder.order) && Objects.equals(this.sortMode, builder.sortMode) && Objects.equals(this.unmappedType, builder.unmappedType) && Objects.equals(this.nestedSort, builder.nestedSort)) && Objects.equals(this.numericType, builder.numericType); @@ -449,7 +394,7 @@ public boolean equals(Object other) { @Override public int hashCode() { - return Objects.hash(this.fieldName, this.nestedFilter, this.nestedPath, this.nestedSort, this.missing, this.order, this.sortMode, + return Objects.hash(this.fieldName, this.nestedSort, this.missing, this.order, this.sortMode, this.unmappedType, this.numericType); } @@ -475,38 +420,22 @@ public static FieldSortBuilder fromXContent(XContentParser parser, String fieldN static { PARSER.declareField(FieldSortBuilder::missing, p -> p.objectText(), MISSING, ValueType.VALUE); - PARSER.declareString((fieldSortBuilder, nestedPath) -> { - deprecationLogger.deprecated("[nested_path] has been deprecated in favor of the [nested] parameter"); - fieldSortBuilder.setNestedPath(nestedPath); - }, NESTED_PATH_FIELD); PARSER.declareString(FieldSortBuilder::unmappedType , UNMAPPED_TYPE); PARSER.declareString((b, v) -> b.order(SortOrder.fromString(v)) , ORDER_FIELD); PARSER.declareString((b, v) -> b.sortMode(SortMode.fromString(v)), SORT_MODE); - PARSER.declareObject(FieldSortBuilder::setNestedFilter, (p, c) -> { - deprecationLogger.deprecated("[nested_filter] has been deprecated in favour for the [nested] parameter"); - return SortBuilder.parseNestedFilter(p); - }, NESTED_FILTER_FIELD); PARSER.declareObject(FieldSortBuilder::setNestedSort, (p, c) -> NestedSortBuilder.fromXContent(p), NESTED_FIELD); PARSER.declareString((b, v) -> b.setNumericType(v), NUMERIC_TYPE); } @Override public FieldSortBuilder rewrite(QueryRewriteContext ctx) throws IOException { - if (nestedFilter == null && nestedSort == null) { + if (nestedSort == null) { return this; } - if (nestedFilter != null) { - QueryBuilder rewrite = nestedFilter.rewrite(ctx); - if (nestedFilter == rewrite) { - return this; - } - return new FieldSortBuilder(this).setNestedFilter(rewrite); - } else { - NestedSortBuilder rewrite = nestedSort.rewrite(ctx); - if (nestedSort == rewrite) { - return this; - } - return new FieldSortBuilder(this).setNestedSort(rewrite); + NestedSortBuilder rewrite = nestedSort.rewrite(ctx); + if (nestedSort == rewrite) { + return this; } + return new FieldSortBuilder(this).setNestedSort(rewrite); } } 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 73877b3faa309..d67f03dd5f252 100644 --- a/server/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java @@ -19,7 +19,6 @@ package org.elasticsearch.search.sort; -import org.apache.logging.log4j.LogManager; import org.apache.lucene.document.LatLonDocValuesField; import org.apache.lucene.index.LeafReaderContext; import org.apache.lucene.index.NumericDocValues; @@ -28,6 +27,7 @@ import org.apache.lucene.search.SortField; import org.apache.lucene.util.BitSet; import org.elasticsearch.ElasticsearchParseException; +import org.elasticsearch.Version; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.geo.GeoDistance; @@ -35,7 +35,6 @@ import org.elasticsearch.common.geo.GeoUtils; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; -import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.unit.DistanceUnit; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; @@ -64,14 +63,12 @@ import java.util.Locale; import java.util.Objects; -import static org.elasticsearch.index.query.AbstractQueryBuilder.parseInnerQueryBuilder; import static org.elasticsearch.search.sort.NestedSortBuilder.NESTED_FIELD; /** * A geo distance based sorting on a geo point like field. */ public class GeoDistanceSortBuilder extends SortBuilder { - private static final DeprecationLogger deprecationLogger = new DeprecationLogger(LogManager.getLogger(GeoDistanceSortBuilder.class)); public static final String NAME = "_geo_distance"; public static final String ALTERNATIVE_NAME = "_geoDistance"; @@ -90,8 +87,6 @@ public class GeoDistanceSortBuilder extends SortBuilder private DistanceUnit unit = DistanceUnit.DEFAULT; private SortMode sortMode = null; - private QueryBuilder nestedFilter; - private String nestedPath; private NestedSortBuilder nestedSort; @@ -150,8 +145,6 @@ public GeoDistanceSortBuilder(String fieldName, String ... geohashes) { this.unit = original.unit; this.order = original.order; this.sortMode = original.sortMode; - this.nestedFilter = original.nestedFilter; - this.nestedPath = original.nestedPath; this.validation = original.validation; this.nestedSort = original.nestedSort; this.ignoreUnmapped = original.ignoreUnmapped; @@ -168,8 +161,13 @@ public GeoDistanceSortBuilder(StreamInput in) throws IOException { unit = DistanceUnit.readFromStream(in); order = SortOrder.readFromStream(in); sortMode = in.readOptionalWriteable(SortMode::readFromStream); - nestedFilter = in.readOptionalNamedWriteable(QueryBuilder.class); - nestedPath = in.readOptionalString(); + if (in.getVersion().before(Version.V_8_0_0)) { + if (in.readOptionalNamedWriteable(QueryBuilder.class) != null || in.readOptionalString() != null) { + throw new IOException("the [sort] options [nested_path] and [nested_filter] are removed in 8.x, " + + "please use [nested] instead"); + } + + } nestedSort = in.readOptionalWriteable(NestedSortBuilder::new); validation = GeoValidationMethod.readFromStream(in); ignoreUnmapped = in.readBoolean(); @@ -183,8 +181,10 @@ public void writeTo(StreamOutput out) throws IOException { unit.writeTo(out); order.writeTo(out); out.writeOptionalWriteable(sortMode); - out.writeOptionalNamedWriteable(nestedFilter); - out.writeOptionalString(nestedPath); + if (out.getVersion().before(Version.V_8_0_0)) { + out.writeOptionalNamedWriteable(null); + out.writeOptionalString(null); + } out.writeOptionalWriteable(nestedSort); validation.writeTo(out); out.writeBoolean(ignoreUnmapped); @@ -288,59 +288,6 @@ public SortMode sortMode() { return this.sortMode; } - /** - * 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 GeoDistanceSortBuilder 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; - } - - /** - * Returns 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 QueryBuilder getNestedFilter() { - return this.nestedFilter; - } - - /** - * Sets the nested path if sorting occurs on a field that is inside a nested object. By default when sorting on a - * field inside a nested object, the nearest upper nested object is selected as nested path. - * @deprecated set nested sort with {@link #setNestedSort(NestedSortBuilder)} - * and retrieve with {@link #getNestedSort()} - **/ - @Deprecated - public GeoDistanceSortBuilder 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; - } - - /** - * Returns the nested path if sorting occurs on a field that is inside a nested object. By default when sorting on a - * field inside a nested object, the nearest upper nested object is selected as 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} */ @@ -355,9 +302,6 @@ public NestedSortBuilder getNestedSort() { * order to be taken into account for sorting. */ public GeoDistanceSortBuilder 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; } @@ -393,12 +337,6 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.field(SORTMODE_FIELD.getPreferredName(), sortMode); } - if (nestedPath != null) { - builder.field(NESTED_PATH_FIELD.getPreferredName(), nestedPath); - } - if (nestedFilter != null) { - builder.field(NESTED_FILTER_FIELD.getPreferredName(), nestedFilter, params); - } if (nestedSort != null) { builder.field(NESTED_FIELD.getPreferredName(), nestedSort); } @@ -432,8 +370,6 @@ public boolean equals(Object object) { Objects.equals(unit, other.unit) && Objects.equals(sortMode, other.sortMode) && Objects.equals(order, other.order) && - Objects.equals(nestedFilter, other.nestedFilter) && - Objects.equals(nestedPath, other.nestedPath) && Objects.equals(validation, other.validation) && Objects.equals(nestedSort, other.nestedSort) && ignoreUnmapped == other.ignoreUnmapped; @@ -442,8 +378,7 @@ public boolean equals(Object object) { @Override public int hashCode() { return Objects.hash(this.fieldName, this.points, this.geoDistance, - this.unit, this.sortMode, this.order, this.nestedFilter, - this.nestedPath, this.validation, this.nestedSort, this.ignoreUnmapped); + this.unit, this.sortMode, this.order, this.validation, this.nestedSort, this.ignoreUnmapped); } /** @@ -479,10 +414,7 @@ public static GeoDistanceSortBuilder fromXContent(XContentParser parser, String fieldName = currentName; } else if (token == XContentParser.Token.START_OBJECT) { - if (NESTED_FILTER_FIELD.match(currentName, parser.getDeprecationHandler())) { - deprecationLogger.deprecated("[nested_filter] has been deprecated in favour of the [nested] parameter"); - nestedFilter = parseInnerQueryBuilder(parser); - } else if (NESTED_FIELD.match(currentName, parser.getDeprecationHandler())) { + if (NESTED_FIELD.match(currentName, parser.getDeprecationHandler())) { nestedSort = NestedSortBuilder.fromXContent(parser); } else { // the json in the format of -> field : { lat : 30, lon : 12 } @@ -509,9 +441,6 @@ public static GeoDistanceSortBuilder fromXContent(XContentParser parser, String validation = GeoValidationMethod.fromString(parser.text()); } else if (SORTMODE_FIELD.match(currentName, parser.getDeprecationHandler())) { sortMode = SortMode.fromString(parser.text()); - } else if (NESTED_PATH_FIELD.match(currentName, parser.getDeprecationHandler())) { - deprecationLogger.deprecated("[nested_path] has been deprecated in favour of the [nested] parameter"); - nestedPath = parser.text(); } else if (IGNORE_UNMAPPED.match(currentName, parser.getDeprecationHandler())) { ignoreUnmapped = parser.booleanValue(); } else if (token == Token.VALUE_STRING){ @@ -549,10 +478,6 @@ public static GeoDistanceSortBuilder fromXContent(XContentParser parser, String if (sortMode != null) { result.sortMode(sortMode); } - if (nestedFilter != null) { - result.setNestedFilter(nestedFilter); - } - result.setNestedPath(nestedPath); if (nestedSort != null) { result.setNestedSort(nestedSort); } @@ -610,7 +535,7 @@ public SortFieldAndFormat build(QueryShardContext context) throws IOException { } final IndexGeoPointFieldData geoIndexFieldData = context.getForField(fieldType); - final Nested nested; + Nested nested = null; if (nestedSort != null) { if (nestedSort.getNestedSort() != null && nestedSort.getMaxChildren() != Integer.MAX_VALUE) { throw new QueryShardException(context, @@ -618,8 +543,6 @@ public SortFieldAndFormat build(QueryShardContext context) throws IOException { } // new nested sorts takes priority nested = resolveNested(context, nestedSort); - } else { - nested = resolveNested(context, nestedPath, nestedFilter); } if (geoIndexFieldData.getClass() == LatLonPointDVIndexFieldData.class // only works with 5.x geo_point @@ -698,21 +621,13 @@ static void parseGeoPoints(XContentParser parser, List geoPoints) thro @Override public GeoDistanceSortBuilder rewrite(QueryRewriteContext ctx) throws IOException { - if (nestedFilter == null && nestedSort == null) { + if (nestedSort == null) { return this; } - if (nestedFilter != null) { - QueryBuilder rewrite = nestedFilter.rewrite(ctx); - if (nestedFilter == rewrite) { - return this; - } - return new GeoDistanceSortBuilder(this).setNestedFilter(rewrite); - } else { - NestedSortBuilder rewrite = nestedSort.rewrite(ctx); - if (nestedSort == rewrite) { - return this; - } - return new GeoDistanceSortBuilder(this).setNestedSort(rewrite); + NestedSortBuilder rewrite = nestedSort.rewrite(ctx); + if (nestedSort == rewrite) { + return this; } + return new GeoDistanceSortBuilder(this).setNestedSort(rewrite); } } 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 21de011e276b7..2d8063db0eec8 100644 --- a/server/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java @@ -19,18 +19,17 @@ package org.elasticsearch.search.sort; -import org.apache.logging.log4j.LogManager; import org.apache.lucene.index.BinaryDocValues; import org.apache.lucene.index.LeafReaderContext; import org.apache.lucene.search.Scorable; import org.apache.lucene.search.SortField; import org.apache.lucene.util.BytesRef; import org.apache.lucene.util.BytesRefBuilder; +import org.elasticsearch.Version; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; -import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.xcontent.ConstructingObjectParser; import org.elasticsearch.common.xcontent.ObjectParser.ValueType; import org.elasticsearch.common.xcontent.XContentBuilder; @@ -65,7 +64,6 @@ * Script sort builder allows to sort based on a custom script expression. */ public class ScriptSortBuilder extends SortBuilder { - private static final DeprecationLogger deprecationLogger = new DeprecationLogger(LogManager.getLogger(ScriptSortBuilder.class)); public static final String NAME = "_script"; public static final ParseField TYPE_FIELD = new ParseField("type"); @@ -78,10 +76,6 @@ public class ScriptSortBuilder extends SortBuilder { private SortMode sortMode; - private QueryBuilder nestedFilter; - - private String nestedPath; - private NestedSortBuilder nestedSort; /** @@ -105,8 +99,6 @@ public ScriptSortBuilder(Script script, ScriptSortType type) { this.type = original.type; this.order = original.order; this.sortMode = original.sortMode; - this.nestedFilter = original.nestedFilter; - this.nestedPath = original.nestedPath; this.nestedSort = original.nestedSort; } @@ -118,8 +110,12 @@ public ScriptSortBuilder(StreamInput in) throws IOException { type = ScriptSortType.readFromStream(in); order = SortOrder.readFromStream(in); sortMode = in.readOptionalWriteable(SortMode::readFromStream); - nestedPath = in.readOptionalString(); - nestedFilter = in.readOptionalNamedWriteable(QueryBuilder.class); + if (in.getVersion().before(Version.V_8_0_0)) { + if (in.readOptionalNamedWriteable(QueryBuilder.class) != null || in.readOptionalString() != null) { + throw new IOException("the [sort] options [nested_path] and [nested_filter] are removed in 8.x, " + + "please use [nested] instead"); + } + } nestedSort = in.readOptionalWriteable(NestedSortBuilder::new); } @@ -129,8 +125,10 @@ public void writeTo(StreamOutput out) throws IOException { type.writeTo(out); order.writeTo(out); out.writeOptionalWriteable(sortMode); - out.writeOptionalString(nestedPath); - out.writeOptionalNamedWriteable(nestedFilter); + if (out.getVersion().before(Version.V_8_0_0)) { + out.writeOptionalString(null); + out.writeOptionalNamedWriteable(null); + } out.writeOptionalWriteable(nestedSort); } @@ -169,56 +167,6 @@ public SortMode sortMode() { return this.sortMode; } - /** - * 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; - } - - /** - * 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} */ @@ -233,9 +181,6 @@ public NestedSortBuilder getNestedSort() { * 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; } @@ -250,12 +195,6 @@ public XContentBuilder toXContent(XContentBuilder builder, Params builderParams) if (sortMode != null) { builder.field(SORTMODE_FIELD.getPreferredName(), sortMode); } - if (nestedPath != null) { - builder.field(NESTED_PATH_FIELD.getPreferredName(), nestedPath); - } - if (nestedFilter != null) { - builder.field(NESTED_FILTER_FIELD.getPreferredName(), nestedFilter, builderParams); - } if (nestedSort != null) { builder.field(NESTED_FIELD.getPreferredName(), nestedSort); } @@ -273,14 +212,6 @@ public XContentBuilder toXContent(XContentBuilder builder, Params builderParams) PARSER.declareField(constructorArg(), p -> ScriptSortType.fromString(p.text()), TYPE_FIELD, ValueType.STRING); PARSER.declareString((b, v) -> b.order(SortOrder.fromString(v)), ORDER_FIELD); PARSER.declareString((b, v) -> b.sortMode(SortMode.fromString(v)), SORTMODE_FIELD); - PARSER.declareString((fieldSortBuilder, nestedPath) -> { - deprecationLogger.deprecated("[nested_path] has been deprecated in favor of the [nested] parameter"); - fieldSortBuilder.setNestedPath(nestedPath); - }, NESTED_PATH_FIELD); - PARSER.declareObject(ScriptSortBuilder::setNestedFilter, (p, c) -> { - deprecationLogger.deprecated("[nested_filter] has been deprecated in favour for the [nested] parameter"); - return SortBuilder.parseNestedFilter(p); - }, NESTED_FILTER_FIELD); PARSER.declareObject(ScriptSortBuilder::setNestedSort, (p, c) -> NestedSortBuilder.fromXContent(p), NESTED_FIELD); } @@ -309,7 +240,7 @@ public SortFieldAndFormat build(QueryShardContext context) throws IOException { valueMode = reverse ? MultiValueMode.MAX : MultiValueMode.MIN; } - final Nested nested; + Nested nested = null; if (nestedSort != null) { if (nestedSort.getNestedSort() != null && nestedSort.getMaxChildren() != Integer.MAX_VALUE) { throw new QueryShardException(context, @@ -317,8 +248,6 @@ public SortFieldAndFormat build(QueryShardContext context) throws IOException { } // new nested sorts takes priority nested = resolveNested(context, nestedSort); - } else { - nested = resolveNested(context, nestedPath, nestedFilter); } final IndexFieldData.XFieldComparatorSource fieldComparatorSource; @@ -399,14 +328,12 @@ public boolean equals(Object object) { Objects.equals(type, other.type) && Objects.equals(order, other.order) && Objects.equals(sortMode, other.sortMode) && - Objects.equals(nestedFilter, other.nestedFilter) && - Objects.equals(nestedPath, other.nestedPath) && Objects.equals(nestedSort, other.nestedSort); } @Override public int hashCode() { - return Objects.hash(script, type, order, sortMode, nestedFilter, nestedPath, nestedSort); + return Objects.hash(script, type, order, sortMode, nestedSort); } @Override @@ -452,21 +379,13 @@ public String toString() { @Override public ScriptSortBuilder rewrite(QueryRewriteContext ctx) throws IOException { - if (nestedFilter == null && nestedSort == null) { + if (nestedSort == null) { return this; } - if (nestedFilter != null) { - QueryBuilder rewrite = nestedFilter.rewrite(ctx); - if (nestedFilter == rewrite) { - return this; - } - return new ScriptSortBuilder(this).setNestedFilter(rewrite); - } else { - NestedSortBuilder rewrite = nestedSort.rewrite(ctx); - if (nestedSort == rewrite) { - return this; - } - return new ScriptSortBuilder(this).setNestedSort(rewrite); + NestedSortBuilder rewrite = nestedSort.rewrite(ctx); + if (nestedSort == rewrite) { + return this; } + return new ScriptSortBuilder(this).setNestedSort(rewrite); } } diff --git a/server/src/main/java/org/elasticsearch/search/sort/SortBuilder.java b/server/src/main/java/org/elasticsearch/search/sort/SortBuilder.java index 4d793de18443d..1dbd08f9780cc 100644 --- a/server/src/main/java/org/elasticsearch/search/sort/SortBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/sort/SortBuilder.java @@ -46,6 +46,7 @@ import java.util.Optional; import static org.elasticsearch.index.query.AbstractQueryBuilder.parseInnerQueryBuilder; +import static org.elasticsearch.search.sort.NestedSortBuilder.FILTER_FIELD; public abstract class SortBuilder> implements NamedWriteable, ToXContentObject, Rewriteable> { @@ -53,8 +54,6 @@ public abstract class SortBuilder> implements NamedWrit // parse fields common to more than one SortBuilder public static final ParseField ORDER_FIELD = new ParseField("order"); - public static final ParseField NESTED_FILTER_FIELD = new ParseField("nested_filter"); - public static final ParseField NESTED_PATH_FIELD = new ParseField("nested_path"); private static final Map> PARSERS = Map.of( ScriptSortBuilder.NAME, ScriptSortBuilder::fromXContent, @@ -207,7 +206,7 @@ private static Query resolveNestedQuery(QueryShardContext context, NestedSortBui if (nestedObjectMapper == null) { throw new QueryShardException(context, "[nested] failed to find nested object under path [" + nestedPath + "]"); } - if (!nestedObjectMapper.nested().isNested()) { + if (nestedObjectMapper.nested().isNested() == false) { throw new QueryShardException(context, "[nested] nested object under path [" + nestedPath + "] is not of nested type"); } ObjectMapper objectMapper = context.nestedScope().getObjectMapper(); @@ -256,7 +255,7 @@ protected static QueryBuilder parseNestedFilter(XContentParser parser) { try { return parseInnerQueryBuilder(parser); } catch (Exception e) { - throw new ParsingException(parser.getTokenLocation(), "Expected " + NESTED_FILTER_FIELD.getPreferredName() + " element.", e); + throw new ParsingException(parser.getTokenLocation(), "Expected " + FILTER_FIELD.getPreferredName() + " element.", e); } } diff --git a/server/src/test/java/org/elasticsearch/search/nested/SimpleNestedIT.java b/server/src/test/java/org/elasticsearch/search/nested/SimpleNestedIT.java index 1937bb2a847fd..c9af6bb3e2e04 100644 --- a/server/src/test/java/org/elasticsearch/search/nested/SimpleNestedIT.java +++ b/server/src/test/java/org/elasticsearch/search/nested/SimpleNestedIT.java @@ -385,7 +385,8 @@ public void testSimpleNestedSorting() throws Exception { SearchResponse searchResponse = client().prepareSearch("test") .setQuery(QueryBuilders.matchAllQuery()) - .addSort(SortBuilders.fieldSort("nested1.field1").order(SortOrder.ASC).setNestedPath("nested1")) + .addSort(SortBuilders.fieldSort("nested1.field1").order(SortOrder.ASC) + .setNestedSort(new NestedSortBuilder("nested1"))) .get(); assertHitCount(searchResponse, 3); @@ -399,7 +400,8 @@ public void testSimpleNestedSorting() throws Exception { searchResponse = client().prepareSearch("test") .setQuery(QueryBuilders.matchAllQuery()) - .addSort(SortBuilders.fieldSort("nested1.field1").order(SortOrder.DESC).setNestedPath("nested1")) + .addSort(SortBuilders.fieldSort("nested1.field1").order(SortOrder.DESC) + .setNestedSort(new NestedSortBuilder("nested1"))) .get(); assertHitCount(searchResponse, 3); @@ -476,8 +478,10 @@ public void testSimpleNestedSortingWithNestedFilterMissing() throws Exception { SearchRequestBuilder searchRequestBuilder = client().prepareSearch("test") .setQuery(QueryBuilders.matchAllQuery()) - .addSort(SortBuilders.fieldSort("nested1.field1").setNestedPath("nested1") - .setNestedFilter(termQuery("nested1.field2", true)).missing(10).order(SortOrder.ASC)); + .addSort(SortBuilders.fieldSort("nested1.field1") + .setNestedSort(new NestedSortBuilder("nested1") + .setFilter(termQuery("nested1.field2", true))) + .missing(10).order(SortOrder.ASC)); if (randomBoolean()) { searchRequestBuilder.setScroll("10m"); @@ -494,8 +498,10 @@ public void testSimpleNestedSortingWithNestedFilterMissing() throws Exception { assertThat(searchResponse.getHits().getHits()[2].getSortValues()[0].toString(), equalTo("10")); searchRequestBuilder = client().prepareSearch("test").setQuery(QueryBuilders.matchAllQuery()) - .addSort(SortBuilders.fieldSort("nested1.field1").setNestedPath("nested1") - .setNestedFilter(termQuery("nested1.field2", true)).missing(10).order(SortOrder.DESC)); + .addSort(SortBuilders.fieldSort("nested1.field1") + .setNestedSort(new NestedSortBuilder("nested1") + .setFilter(termQuery("nested1.field2", true))) + .missing(10).order(SortOrder.DESC)); if (randomBoolean()) { searchRequestBuilder.setScroll("10m"); @@ -953,7 +959,7 @@ public void testSortNestedWithNestedFilter() throws Exception { .setQuery(matchAllQuery()) .addSort( SortBuilders.fieldSort("parent.child.child_values") - .setNestedPath("parent.child") + .setNestedSort(new NestedSortBuilder("parent.child")) .order(SortOrder.ASC) ) .get(); @@ -967,12 +973,13 @@ public void testSortNestedWithNestedFilter() throws Exception { assertThat(searchResponse.getHits().getHits()[2].getSortValues()[0].toString(), equalTo("-1")); // With nested filter + NestedSortBuilder nestedSort = new NestedSortBuilder("parent.child"); + nestedSort.setFilter(QueryBuilders.termQuery("parent.child.filter", true)); searchResponse = client().prepareSearch() .setQuery(matchAllQuery()) .addSort( SortBuilders.fieldSort("parent.child.child_values") - .setNestedPath("parent.child") - .setNestedFilter(QueryBuilders.termQuery("parent.child.filter", true)) + .setNestedSort(nestedSort) .order(SortOrder.ASC) ) .get(); @@ -990,8 +997,7 @@ public void testSortNestedWithNestedFilter() throws Exception { .setQuery(matchAllQuery()) .addSort( SortBuilders.fieldSort("parent.child.child_values") - .setNestedPath("parent.child") - .setNestedFilter(QueryBuilders.termQuery("parent.child.filter", true)) + .setNestedSort(nestedSort) .order(SortOrder.ASC) ) .get(); @@ -1009,8 +1015,7 @@ public void testSortNestedWithNestedFilter() throws Exception { .setQuery(matchAllQuery()) .addSort( SortBuilders.fieldSort("parent.parent_values") - .setNestedPath("parent.child") - .setNestedFilter(QueryBuilders.termQuery("parent.filter", false)) + .setNestedSort(nestedSort) .order(SortOrder.ASC) ) .get(); @@ -1051,8 +1056,8 @@ public void testSortNestedWithNestedFilter() throws Exception { .setQuery(matchAllQuery()) .addSort( SortBuilders.fieldSort("parent.child.child_obj.value") - .setNestedPath("parent.child") - .setNestedFilter(QueryBuilders.termQuery("parent.child.filter", true)) + .setNestedSort(new NestedSortBuilder("parent.child") + .setFilter(QueryBuilders.termQuery("parent.child.filter", true))) .order(SortOrder.ASC) ) .get(); @@ -1071,7 +1076,7 @@ public void testSortNestedWithNestedFilter() throws Exception { .setQuery(matchAllQuery()) .addSort( SortBuilders.fieldSort("parent.child.child_values") - .setNestedPath("parent.child") + .setNestedSort(new NestedSortBuilder("parent.child")) .sortMode(SortMode.SUM) .order(SortOrder.ASC) ) @@ -1091,7 +1096,7 @@ public void testSortNestedWithNestedFilter() throws Exception { .setQuery(matchAllQuery()) .addSort( SortBuilders.fieldSort("parent.child.child_values") - .setNestedPath("parent.child") + .setNestedSort(new NestedSortBuilder("parent.child")) .sortMode(SortMode.SUM) .order(SortOrder.DESC) ) @@ -1111,8 +1116,9 @@ public void testSortNestedWithNestedFilter() throws Exception { .setQuery(matchAllQuery()) .addSort( SortBuilders.fieldSort("parent.child.child_values") - .setNestedPath("parent.child") - .setNestedFilter(QueryBuilders.termQuery("parent.child.filter", true)) + .setNestedSort(new NestedSortBuilder("parent.child") + .setFilter(QueryBuilders.termQuery("parent.child.filter", true)) + ) .sortMode(SortMode.SUM) .order(SortOrder.ASC) ) @@ -1132,7 +1138,7 @@ public void testSortNestedWithNestedFilter() throws Exception { .setQuery(matchAllQuery()) .addSort( SortBuilders.fieldSort("parent.child.child_values") - .setNestedPath("parent.child") + .setNestedSort(new NestedSortBuilder("parent.child")) .sortMode(SortMode.AVG) .order(SortOrder.ASC) ) @@ -1151,7 +1157,7 @@ public void testSortNestedWithNestedFilter() throws Exception { .setQuery(matchAllQuery()) .addSort( SortBuilders.fieldSort("parent.child.child_values") - .setNestedPath("parent.child") + .setNestedSort(new NestedSortBuilder("parent.child")) .sortMode(SortMode.AVG) .order(SortOrder.DESC) ) @@ -1171,8 +1177,9 @@ public void testSortNestedWithNestedFilter() throws Exception { .setQuery(matchAllQuery()) .addSort( SortBuilders.fieldSort("parent.child.child_values") - .setNestedPath("parent.child") - .setNestedFilter(QueryBuilders.termQuery("parent.child.filter", true)) + .setNestedSort(new NestedSortBuilder("parent.child") + .setFilter(QueryBuilders.termQuery("parent.child.filter", true)) + ) .sortMode(SortMode.AVG) .order(SortOrder.ASC) ) @@ -1307,13 +1314,15 @@ public void testNestedSortingWithNestedFilterAsFilter() throws Exception { SearchResponse searchResponse = client().prepareSearch("test") .addSort(SortBuilders.fieldSort("users.first") - .setNestedPath("users") + .setNestedSort(new NestedSortBuilder("users")) .order(SortOrder.ASC)) .addSort(SortBuilders.fieldSort("users.first") .order(SortOrder.ASC) - .setNestedPath("users") - .setNestedFilter(nestedQuery("users.workstations", termQuery("users.workstations.stationid", "s5"), - ScoreMode.Avg))) + .setNestedSort(new NestedSortBuilder("users") + .setFilter(nestedQuery("users.workstations", termQuery("users.workstations.stationid", "s5"), + ScoreMode.Avg)) + ) + ) .get(); assertNoFailures(searchResponse); assertHitCount(searchResponse, 2); diff --git a/server/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java b/server/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java index ee9c8f8ed1105..1623bc2bb378d 100644 --- a/server/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java @@ -45,7 +45,6 @@ import org.elasticsearch.search.MultiValueMode; import java.io.IOException; -import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -90,17 +89,7 @@ public FieldSortBuilder randomFieldSortBuilder() { builder.sortMode(randomFrom(SortMode.values())); } if (randomBoolean()) { - if (randomBoolean()) { - builder.setNestedSort(createRandomNestedSort(3)); - } else { - // the following are alternative ways to setNestedSort for nested sorting - if (randomBoolean()) { - builder.setNestedFilter(randomNestedFilter()); - } - if (randomBoolean()) { - builder.setNestedPath(randomAlphaOfLengthBetween(1, 10)); - } - } + builder.setNestedSort(createRandomNestedSort(3)); } if (randomBoolean()) { builder.setNumericType(randomFrom(random(), "long", "double", "date", "date_nanos")); @@ -114,16 +103,8 @@ protected FieldSortBuilder mutate(FieldSortBuilder original) throws IOException int parameter = randomIntBetween(0, 5); switch (parameter) { case 0: - if (original.getNestedPath() == null && original.getNestedFilter() == null) { - mutated.setNestedSort( - randomValueOtherThan(original.getNestedSort(), () -> NestedSortBuilderTests.createRandomNestedSort(3))); - } else { - if (randomBoolean()) { - mutated.setNestedPath(randomValueOtherThan(original.getNestedPath(), () -> randomAlphaOfLengthBetween(1, 10))); - } else { - mutated.setNestedFilter(randomValueOtherThan(original.getNestedFilter(), () -> randomNestedFilter())); - } - } + mutated.setNestedSort( + randomValueOtherThan(original.getNestedSort(), () -> NestedSortBuilderTests.createRandomNestedSort(3))); break; case 1: mutated.sortMode(randomValueOtherThan(original.sortMode(), () -> randomFrom(SortMode.values()))); @@ -285,7 +266,8 @@ public void testBuildNested() throws IOException { assertNotNull(nested); assertEquals(new TermQuery(new Term(MAPPED_STRING_FIELDNAME, "value")), nested.getInnerQuery()); - sortBuilder = new FieldSortBuilder("fieldName").setNestedPath("path"); + NestedSortBuilder nestedSort = new NestedSortBuilder("path"); + sortBuilder = new FieldSortBuilder("fieldName").setNestedSort(nestedSort); sortField = sortBuilder.build(shardContextMock).field; assertThat(sortField.getComparatorSource(), instanceOf(XFieldComparatorSource.class)); comparatorSource = (XFieldComparatorSource) sortField.getComparatorSource(); @@ -293,19 +275,14 @@ public void testBuildNested() throws IOException { assertNotNull(nested); assertEquals(new TermQuery(new Term(TypeFieldMapper.NAME, "__path")), nested.getInnerQuery()); - sortBuilder = new FieldSortBuilder("fieldName").setNestedPath("path") - .setNestedFilter(QueryBuilders.termQuery(MAPPED_STRING_FIELDNAME, "value")); + nestedSort.setFilter(QueryBuilders.termQuery(MAPPED_STRING_FIELDNAME, "value")); + sortBuilder = new FieldSortBuilder("fieldName").setNestedSort(nestedSort); 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(MAPPED_STRING_FIELDNAME, "value")), nested.getInnerQuery()); - - // if nested path is missing, we omit any filter and return a SortedNumericSortField - sortBuilder = new FieldSortBuilder("fieldName").setNestedFilter(QueryBuilders.termQuery(MAPPED_STRING_FIELDNAME, "value")); - sortField = sortBuilder.build(shardContextMock).field; - assertThat(sortField, instanceOf(SortedNumericSortField.class)); } public void testUnknownOptionFails() throws IOException { @@ -364,22 +341,6 @@ public void testModeNonNumericField() throws IOException { assertEquals(expectedError, e.getMessage()); } - /** - * Test we can either set nested sort via path/filter or via nested sort builder, not both - */ - public void testNestedSortBothThrows() throws IOException { - FieldSortBuilder sortBuilder = new FieldSortBuilder(MAPPED_STRING_FIELDNAME); - 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()); - } - /** * Test the nested Filter gets rewritten */ @@ -391,10 +352,12 @@ public QueryBuilder doRewrite(QueryRewriteContext queryShardContext) throws IOEx return new MatchNoneQueryBuilder(); } }; - sortBuilder.setNestedPath("path").setNestedFilter(rangeQuery); + NestedSortBuilder nestedSort = new NestedSortBuilder("path"); + nestedSort.setFilter(rangeQuery); + sortBuilder.setNestedSort(nestedSort); FieldSortBuilder rewritten = sortBuilder .rewrite(createMockShardContext()); - assertNotSame(rangeQuery, rewritten.getNestedFilter()); + assertNotSame(rangeQuery, rewritten.getNestedSort().getFilter()); } /** @@ -414,20 +377,6 @@ public QueryBuilder doRewrite(QueryRewriteContext queryShardContext) throws IOEx assertNotSame(rangeQuery, rewritten.getNestedSort().getFilter()); } - @Override - protected void assertWarnings(FieldSortBuilder testItem) { - List expectedWarnings = new ArrayList<>(); - if (testItem.getNestedFilter() != null) { - expectedWarnings.add("[nested_filter] has been deprecated in favour for the [nested] parameter"); - } - if (testItem.getNestedPath() != null) { - expectedWarnings.add("[nested_path] has been deprecated in favor of the [nested] parameter"); - } - if (expectedWarnings.isEmpty() == false) { - assertWarnings(expectedWarnings.toArray(new String[expectedWarnings.size()])); - } - } - @Override protected FieldSortBuilder fromXContent(XContentParser parser, String fieldName) throws IOException { return FieldSortBuilder.fromXContent(parser, fieldName); 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 d3f21867ab1d1..400dc30afa11f 100644 --- a/server/src/test/java/org/elasticsearch/search/sort/FieldSortIT.java +++ b/server/src/test/java/org/elasticsearch/search/sort/FieldSortIT.java @@ -1443,9 +1443,11 @@ public void testNestedSort() throws IOException, InterruptedException, Execution refresh(); // We sort on nested field + SearchResponse searchResponse = client().prepareSearch() .setQuery(matchAllQuery()) - .addSort(SortBuilders.fieldSort("nested.foo").setNestedPath("nested").order(SortOrder.DESC)) + .addSort(SortBuilders.fieldSort("nested.foo") + .setNestedSort(new NestedSortBuilder("nested")).order(SortOrder.DESC)) .get(); assertNoFailures(searchResponse); SearchHit[] hits = searchResponse.getHits().getHits(); @@ -1474,7 +1476,8 @@ public void testNestedSort() throws IOException, InterruptedException, Execution // We sort on nested sub field searchResponse = client().prepareSearch() .setQuery(matchAllQuery()) - .addSort(SortBuilders.fieldSort("nested.foo.sub").setNestedPath("nested").order(SortOrder.DESC)) + .addSort(SortBuilders.fieldSort("nested.foo.sub") + .setNestedSort(new NestedSortBuilder("nested")).order(SortOrder.DESC)) .get(); assertNoFailures(searchResponse); hits = searchResponse.getHits().getHits(); @@ -1483,6 +1486,14 @@ public void testNestedSort() throws IOException, InterruptedException, Execution assertThat(hits[1].getSortValues().length, is(1)); assertThat(hits[0].getSortValues()[0], is("cba bca")); assertThat(hits[1].getSortValues()[0], is("bar bar")); + + // missing nested path + SearchPhaseExecutionException exc = expectThrows(SearchPhaseExecutionException.class, + () -> client().prepareSearch() + .setQuery(matchAllQuery()).addSort(SortBuilders.fieldSort("nested.foo")) + .get() + ); + assertThat(exc.toString(), containsString("it is mandatory to set the [nested] context")); } public void testSortDuelBetweenSingleShardAndMultiShardIndex() throws Exception { diff --git a/server/src/test/java/org/elasticsearch/search/sort/GeoDistanceIT.java b/server/src/test/java/org/elasticsearch/search/sort/GeoDistanceIT.java index 87d84484aca0a..f3899785c499c 100644 --- a/server/src/test/java/org/elasticsearch/search/sort/GeoDistanceIT.java +++ b/server/src/test/java/org/elasticsearch/search/sort/GeoDistanceIT.java @@ -272,7 +272,8 @@ public void testDistanceSortingNestedFields() throws Exception { // Order: Asc SearchResponse searchResponse = client().prepareSearch("companies").setQuery(matchAllQuery()).addSort(SortBuilders - .geoDistanceSort("branches.location", 40.7143528, -74.0059731).order(SortOrder.ASC).setNestedPath("branches")) + .geoDistanceSort("branches.location", 40.7143528, -74.0059731).order(SortOrder.ASC) + .setNestedSort(new NestedSortBuilder("branches"))) .get(); assertHitCount(searchResponse, 4); @@ -285,7 +286,8 @@ public void testDistanceSortingNestedFields() throws Exception { // Order: Asc, Mode: max searchResponse = client() .prepareSearch("companies").setQuery(matchAllQuery()).addSort(SortBuilders.geoDistanceSort("branches.location", - 40.7143528, -74.0059731).order(SortOrder.ASC).sortMode(SortMode.MAX).setNestedPath("branches")) + 40.7143528, -74.0059731).order(SortOrder.ASC).sortMode(SortMode.MAX) + .setNestedSort(new NestedSortBuilder("branches"))) .get(); assertHitCount(searchResponse, 4); @@ -297,7 +299,8 @@ public void testDistanceSortingNestedFields() throws Exception { // Order: Desc searchResponse = client().prepareSearch("companies").setQuery(matchAllQuery()).addSort(SortBuilders - .geoDistanceSort("branches.location", 40.7143528, -74.0059731).order(SortOrder.DESC).setNestedPath("branches")) + .geoDistanceSort("branches.location", 40.7143528, -74.0059731).order(SortOrder.DESC) + .setNestedSort(new NestedSortBuilder("branches"))) .get(); assertHitCount(searchResponse, 4); @@ -310,7 +313,8 @@ public void testDistanceSortingNestedFields() throws Exception { // Order: Desc, Mode: min searchResponse = client() .prepareSearch("companies").setQuery(matchAllQuery()).addSort(SortBuilders.geoDistanceSort("branches.location", - 40.7143528, -74.0059731).order(SortOrder.DESC).sortMode(SortMode.MIN).setNestedPath("branches")) + 40.7143528, -74.0059731).order(SortOrder.DESC).sortMode(SortMode.MIN) + .setNestedSort(new NestedSortBuilder("branches"))) .get(); assertHitCount(searchResponse, 4); @@ -322,7 +326,8 @@ public void testDistanceSortingNestedFields() throws Exception { searchResponse = client() .prepareSearch("companies").setQuery(matchAllQuery()).addSort(SortBuilders.geoDistanceSort("branches.location", - 40.7143528, -74.0059731).sortMode(SortMode.AVG).order(SortOrder.ASC).setNestedPath("branches")) + 40.7143528, -74.0059731).sortMode(SortMode.AVG).order(SortOrder.ASC) + .setNestedSort(new NestedSortBuilder("branches"))) .get(); assertHitCount(searchResponse, 4); @@ -334,7 +339,8 @@ public void testDistanceSortingNestedFields() throws Exception { searchResponse = client().prepareSearch("companies") .setQuery(matchAllQuery()).addSort(SortBuilders.geoDistanceSort("branches.location", 40.7143528, -74.0059731) - .setNestedPath("branches").sortMode(SortMode.AVG).order(SortOrder.DESC).setNestedPath("branches")) + .setNestedSort(new NestedSortBuilder("branches")) + .sortMode(SortMode.AVG).order(SortOrder.DESC)) .get(); assertHitCount(searchResponse, 4); @@ -346,8 +352,10 @@ public void testDistanceSortingNestedFields() throws Exception { searchResponse = client().prepareSearch("companies").setQuery(matchAllQuery()) .addSort(SortBuilders.geoDistanceSort("branches.location", 40.7143528, -74.0059731) - .setNestedFilter(termQuery("branches.name", "brooklyn")) - .sortMode(SortMode.AVG).order(SortOrder.ASC).setNestedPath("branches")) + .setNestedSort(new NestedSortBuilder("branches") + .setFilter(termQuery("branches.name", "brooklyn")) + ) + .sortMode(SortMode.AVG).order(SortOrder.ASC)) .get(); assertHitCount(searchResponse, 4); assertFirstHit(searchResponse, hasId("4")); @@ -360,7 +368,7 @@ public void testDistanceSortingNestedFields() throws Exception { try { client().prepareSearch("companies").setQuery(matchAllQuery()) .addSort(SortBuilders.geoDistanceSort("branches.location", 40.7143528, -74.0059731).sortMode(SortMode.SUM) - .setNestedPath("branches")); + .setNestedSort(new NestedSortBuilder("branches"))); fail("Sum should not be allowed as sort mode"); } catch (IllegalArgumentException e) { //expected diff --git a/server/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderTests.java b/server/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderTests.java index c90e6ef0dde39..73be5837faa90 100644 --- a/server/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderTests.java @@ -50,9 +50,7 @@ import org.elasticsearch.test.geo.RandomGeoGenerator; import java.io.IOException; -import java.util.ArrayList; import java.util.Arrays; -import java.util.List; import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; import static org.hamcrest.Matchers.instanceOf; @@ -106,20 +104,10 @@ public static GeoDistanceSortBuilder randomGeoDistanceSortBuilder() { result.validation(randomValueOtherThan(result.validation(), () -> randomFrom(GeoValidationMethod.values()))); } if (randomBoolean()) { - if (randomBoolean()) { - // don't fully randomize here, GeoDistanceSort is picky about the filters that are allowed - NestedSortBuilder nestedSort = new NestedSortBuilder(randomAlphaOfLengthBetween(3, 10)); - nestedSort.setFilter(new MatchAllQueryBuilder()); - result.setNestedSort(nestedSort); - } else { - // the following are alternative ways to setNestedSort for nested sorting - if (randomBoolean()) { - result.setNestedFilter(new MatchAllQueryBuilder()); - } - if (randomBoolean()) { - result.setNestedPath(randomAlphaOfLengthBetween(1, 10)); - } - } + // don't fully randomize here, GeoDistanceSort is picky about the filters that are allowed + NestedSortBuilder nestedSort = new NestedSortBuilder(randomAlphaOfLengthBetween(3, 10)); + nestedSort.setFilter(new MatchAllQueryBuilder()); + result.setNestedSort(nestedSort); } if (randomBoolean()) { result.ignoreUnmapped(result.ignoreUnmapped() == false); @@ -183,16 +171,8 @@ protected GeoDistanceSortBuilder mutate(GeoDistanceSortBuilder original) throws () -> randomFrom(SortMode.values()))); break; case 6: - if (original.getNestedPath() == null && original.getNestedFilter() == null) { - result.setNestedSort( - randomValueOtherThan(original.getNestedSort(), () -> NestedSortBuilderTests.createRandomNestedSort(3))); - } else { - if (randomBoolean()) { - result.setNestedPath(randomValueOtherThan(original.getNestedPath(), () -> randomAlphaOfLengthBetween(1, 10))); - } else { - result.setNestedFilter(randomValueOtherThan(original.getNestedFilter(), () -> randomNestedFilter())); - } - } + result.setNestedSort( + randomValueOtherThan(original.getNestedSort(), () -> NestedSortBuilderTests.createRandomNestedSort(3))); break; case 7: result.validation(randomValueOtherThan(result.validation(), () -> randomFrom(GeoValidationMethod.values()))); @@ -387,21 +367,6 @@ private GeoDistanceSortBuilder parse(XContentBuilder sortBuilder) throws Excepti } } - @Override - protected void assertWarnings(GeoDistanceSortBuilder testItem) { - List expectedWarnings = new ArrayList<>(); - if (testItem.getNestedFilter() != null) { - expectedWarnings.add("[nested_filter] has been deprecated in favour of the [nested] parameter"); - } - if (testItem.getNestedPath() != null) { - expectedWarnings.add("[nested_path] has been deprecated in favour of the [nested] parameter"); - } - if (expectedWarnings.isEmpty() == false) { - assertWarnings(expectedWarnings.toArray(new String[expectedWarnings.size()])); - } - } - - @Override protected GeoDistanceSortBuilder fromXContent(XContentParser parser, String fieldName) throws IOException { return GeoDistanceSortBuilder.fromXContent(parser, fieldName); @@ -433,7 +398,7 @@ public void testCommonCaseIsOptimized() throws IOException { assertEquals(SortField.class, sort.field.getClass()); // descending means the max value should be considered rather than min builder = new GeoDistanceSortBuilder("random_field_name", new GeoPoint(3.5, 2.1)); - builder.setNestedPath("some_nested_path"); + builder.setNestedSort(new NestedSortBuilder("some_nested_path")); sort = builder.build(context); assertEquals(SortField.class, sort.field.getClass()); // can't use LatLon optimized sorting with nested fields @@ -523,7 +488,8 @@ public void testBuildNested() throws IOException { assertNotNull(nested); assertEquals(new MatchAllDocsQuery(), nested.getInnerQuery()); - sortBuilder = new GeoDistanceSortBuilder("fieldName", 1.0, 1.0).setNestedPath("path"); + sortBuilder = new GeoDistanceSortBuilder("fieldName", 1.0, 1.0) + .setNestedSort(new NestedSortBuilder("path")); sortField = sortBuilder.build(shardContextMock).field; assertThat(sortField.getComparatorSource(), instanceOf(XFieldComparatorSource.class)); comparatorSource = (XFieldComparatorSource) sortField.getComparatorSource(); @@ -531,20 +497,16 @@ public void testBuildNested() throws IOException { assertNotNull(nested); assertEquals(new TermQuery(new Term(TypeFieldMapper.NAME, "__path")), nested.getInnerQuery()); - sortBuilder = new GeoDistanceSortBuilder("fieldName", 1.0, 1.0).setNestedPath("path") - .setNestedFilter(QueryBuilders.matchAllQuery()); + sortBuilder = new GeoDistanceSortBuilder("fieldName", 1.0, 1.0) + .setNestedSort(new NestedSortBuilder("path") + .setFilter(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 any filter and return a regular SortField - // (LatLonSortField) - sortBuilder = new GeoDistanceSortBuilder("fieldName", 1.0, 1.0).setNestedFilter(QueryBuilders.termQuery("fieldName", "value")); - sortField = sortBuilder.build(shardContextMock).field; - assertThat(sortField, instanceOf(SortField.class)); } /** @@ -579,22 +541,6 @@ public void testBuildInvalidPoints() throws IOException { } } - /** - * Test we can either set nested sort via path/filter or via nested sort builder, not both - */ - public void testNestedSortBothThrows() throws IOException { - GeoDistanceSortBuilder sortBuilder = new GeoDistanceSortBuilder("fieldName", 0.0, 0.0); - 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()); - } - /** * Test the nested Filter gets rewritten */ @@ -606,10 +552,12 @@ public QueryBuilder doRewrite(QueryRewriteContext queryShardContext) throws IOEx return new MatchNoneQueryBuilder(); } }; - sortBuilder.setNestedPath("path").setNestedFilter(rangeQuery); - GeoDistanceSortBuilder rewritten = (GeoDistanceSortBuilder) sortBuilder - .rewrite(createMockShardContext()); - assertNotSame(rangeQuery, rewritten.getNestedFilter()); + sortBuilder.setNestedSort( + new NestedSortBuilder("path") + .setFilter(rangeQuery) + ); + GeoDistanceSortBuilder rewritten = sortBuilder.rewrite(createMockShardContext()); + assertNotSame(rangeQuery, rewritten.getNestedSort().getFilter()); } /** diff --git a/server/src/test/java/org/elasticsearch/search/sort/ScriptSortBuilderTests.java b/server/src/test/java/org/elasticsearch/search/sort/ScriptSortBuilderTests.java index 3017a2e0c067d..1d3b71bbf40db 100644 --- a/server/src/test/java/org/elasticsearch/search/sort/ScriptSortBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/search/sort/ScriptSortBuilderTests.java @@ -324,7 +324,8 @@ public void testBuildNested() throws IOException { assertNotNull(nested); assertEquals(new MatchAllDocsQuery(), nested.getInnerQuery()); - sortBuilder = new ScriptSortBuilder(mockScript(MOCK_SCRIPT_NAME), ScriptSortType.NUMBER).setNestedPath("path"); + sortBuilder = new ScriptSortBuilder(mockScript(MOCK_SCRIPT_NAME), ScriptSortType.NUMBER) + .setNestedSort(new NestedSortBuilder("path")); sortField = sortBuilder.build(shardContextMock).field; assertThat(sortField.getComparatorSource(), instanceOf(XFieldComparatorSource.class)); comparatorSource = (XFieldComparatorSource) sortField.getComparatorSource(); @@ -332,40 +333,17 @@ public void testBuildNested() throws IOException { 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()); + sortBuilder = new ScriptSortBuilder(mockScript(MOCK_SCRIPT_NAME), ScriptSortType.NUMBER) + .setNestedSort(new NestedSortBuilder("path") + .setFilter(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()); - } - /** * Test the nested Filter gets rewritten */ @@ -377,10 +355,10 @@ public QueryBuilder doRewrite(QueryRewriteContext queryShardContext) throws IOEx return new MatchNoneQueryBuilder(); } }; - sortBuilder.setNestedPath("path").setNestedFilter(rangeQuery); + sortBuilder.setNestedSort(new NestedSortBuilder("path").setFilter(rangeQuery)); ScriptSortBuilder rewritten = sortBuilder .rewrite(createMockShardContext()); - assertNotSame(rangeQuery, rewritten.getNestedFilter()); + assertNotSame(rangeQuery, rewritten.getNestedSort().getFilter()); } /** diff --git a/server/src/test/java/org/elasticsearch/search/sort/SortBuilderTests.java b/server/src/test/java/org/elasticsearch/search/sort/SortBuilderTests.java index 5f5ea5e869450..ac2cb402f706a 100644 --- a/server/src/test/java/org/elasticsearch/search/sort/SortBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/search/sort/SortBuilderTests.java @@ -143,16 +143,6 @@ public void testRandomSortBuilders() throws IOException { xContentBuilder.field("sort"); } for (SortBuilder builder : testBuilders) { - if (builder instanceof GeoDistanceSortBuilder) { - GeoDistanceSortBuilder gdsb = (GeoDistanceSortBuilder) builder; - if (gdsb.getNestedFilter() != null) { - expectedWarningHeaders.add("[nested_filter] has been deprecated in favour of the [nested] parameter"); - } - if (gdsb.getNestedPath() != null) { - expectedWarningHeaders.add("[nested_path] has been deprecated in favour of the [nested] parameter"); - } - } - if (builder instanceof ScoreSortBuilder || builder instanceof FieldSortBuilder) { switch (randomIntBetween(0, 2)) { case 0: From 30f03f0f56e92899caca6900bf53eeeeab39a01e Mon Sep 17 00:00:00 2001 From: jimczi Date: Mon, 3 Jun 2019 22:56:27 +0200 Subject: [PATCH 2/6] handle sort already in a nested context --- .../search/sort/FieldSortBuilder.java | 24 ++++++++++++++----- .../search/sort/GeoDistanceSortBuilder.java | 6 ++--- .../search/sort/ScriptSortBuilder.java | 1 - 3 files changed, 21 insertions(+), 10 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 0ef0a6dc3da8b..949a5a3ff441c 100644 --- a/server/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java @@ -336,10 +336,9 @@ public SortFieldAndFormat build(QueryShardContext context) throws IOException { throw new QueryShardException(context, "max_children is only supported on last level of nested sort"); } - // new nested sorts takes priority nested = resolveNested(context, nestedSort); } else { - verifyNestedField(context, fieldName); + validateMissingNestedPath(context, fieldName); } } @@ -363,11 +362,24 @@ public SortFieldAndFormat build(QueryShardContext context) throws IOException { return new SortFieldAndFormat(field, fieldType.docValueFormat(null, null)); } - static void verifyNestedField(QueryShardContext context, String field) { + /** + * Throws an exception if the provided field requires a nested context. + */ + static void validateMissingNestedPath(QueryShardContext context, String field) { + ObjectMapper contextMapper = context.nestedScope().getObjectMapper(); + if (contextMapper != null && contextMapper.nested().isNested() == false) { + // already in nested context + return; + } for (String parent = parentObject(field); parent != null; parent = parentObject(parent)) { - ObjectMapper mapper = context.getObjectMapper(parent); - if (mapper != null && mapper.nested().isNested()) { - if (mapper.nested().isIncludeInRoot() == false) { + ObjectMapper parentMapper = context.getObjectMapper(parent); + if (parentMapper != null && parentMapper.nested().isNested()) { + if (contextMapper != null && contextMapper.fullPath().equals(parentMapper.fullPath())) { + // we are in a nested context that matches the path of the provided field so the nested path + // is not required + return ; + } + if (parentMapper.nested().isIncludeInRoot() == false) { throw new QueryShardException(context, "it is mandatory to set the [nested] context on the nested sort field: [" + field + "]."); } 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 d67f03dd5f252..630b93b4f34b5 100644 --- a/server/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java @@ -63,6 +63,7 @@ import java.util.Locale; import java.util.Objects; +import static org.elasticsearch.search.sort.FieldSortBuilder.validateMissingNestedPath; import static org.elasticsearch.search.sort.NestedSortBuilder.NESTED_FIELD; /** @@ -398,8 +399,6 @@ public static GeoDistanceSortBuilder fromXContent(XContentParser parser, String GeoDistance geoDistance = GeoDistance.ARC; SortOrder order = SortOrder.ASC; SortMode sortMode = null; - QueryBuilder nestedFilter = null; - String nestedPath = null; NestedSortBuilder nestedSort = null; GeoValidationMethod validation = null; boolean ignoreUnmapped = false; @@ -541,8 +540,9 @@ public SortFieldAndFormat build(QueryShardContext context) throws IOException { throw new QueryShardException(context, "max_children is only supported on last level of nested sort"); } - // new nested sorts takes priority nested = resolveNested(context, nestedSort); + } else { + validateMissingNestedPath(context, fieldName); } if (geoIndexFieldData.getClass() == LatLonPointDVIndexFieldData.class // only works with 5.x geo_point 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 2d8063db0eec8..17fed4d9ac19c 100644 --- a/server/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java @@ -246,7 +246,6 @@ public SortFieldAndFormat build(QueryShardContext context) throws IOException { throw new QueryShardException(context, "max_children is only supported on last level of nested sort"); } - // new nested sorts takes priority nested = resolveNested(context, nestedSort); } From 889662bdd2bd3a6a47cbe756454f6b85ee6de715 Mon Sep 17 00:00:00 2001 From: jimczi Date: Wed, 12 Jun 2019 11:29:49 +0200 Subject: [PATCH 3/6] fix tests that use wrong nested sort --- .../test/search.aggregation/200_top_hits_metric.yml | 9 ++++++--- .../org/elasticsearch/search/nested/SimpleNestedIT.java | 1 + .../org/elasticsearch/search/scroll/DuelScrollIT.java | 9 +++++++-- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/200_top_hits_metric.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/200_top_hits_metric.yml index cde56fa41e3d9..76274e9034d62 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/200_top_hits_metric.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/200_top_hits_metric.yml @@ -92,16 +92,19 @@ setup: aggs: users: top_hits: - sort: "users.last.keyword" + sort: + users.last.keyword: + nested: + path: users seq_no_primary_term: true - match: { hits.total: 2 } - length: { aggregations.groups.buckets.0.users.hits.hits: 2 } - - match: { aggregations.groups.buckets.0.users.hits.hits.0._id: "1" } + - match: { aggregations.groups.buckets.0.users.hits.hits.0._id: "2" } - match: { aggregations.groups.buckets.0.users.hits.hits.0._index: my-index } - gte: { aggregations.groups.buckets.0.users.hits.hits.0._seq_no: 0 } - gte: { aggregations.groups.buckets.0.users.hits.hits.0._primary_term: 1 } - - match: { aggregations.groups.buckets.0.users.hits.hits.1._id: "2" } + - match: { aggregations.groups.buckets.0.users.hits.hits.1._id: "1" } - match: { aggregations.groups.buckets.0.users.hits.hits.1._index: my-index } - gte: { aggregations.groups.buckets.0.users.hits.hits.1._seq_no: 0 } - gte: { aggregations.groups.buckets.0.users.hits.hits.1._primary_term: 1 } diff --git a/server/src/test/java/org/elasticsearch/search/nested/SimpleNestedIT.java b/server/src/test/java/org/elasticsearch/search/nested/SimpleNestedIT.java index c9af6bb3e2e04..d80fa2c1e4cca 100644 --- a/server/src/test/java/org/elasticsearch/search/nested/SimpleNestedIT.java +++ b/server/src/test/java/org/elasticsearch/search/nested/SimpleNestedIT.java @@ -1011,6 +1011,7 @@ public void testSortNestedWithNestedFilter() throws Exception { assertThat(searchResponse.getHits().getHits()[2].getId(), equalTo("3")); assertThat(searchResponse.getHits().getHits()[2].getSortValues()[0].toString(), equalTo("3")); + nestedSort.setFilter(QueryBuilders.termQuery("parent.filter", false)); searchResponse = client().prepareSearch() .setQuery(matchAllQuery()) .addSort( diff --git a/server/src/test/java/org/elasticsearch/search/scroll/DuelScrollIT.java b/server/src/test/java/org/elasticsearch/search/scroll/DuelScrollIT.java index 04349e846c78f..fb42d244f78ff 100644 --- a/server/src/test/java/org/elasticsearch/search/scroll/DuelScrollIT.java +++ b/server/src/test/java/org/elasticsearch/search/scroll/DuelScrollIT.java @@ -30,6 +30,7 @@ import org.elasticsearch.index.query.QueryBuilders; import org.elasticsearch.search.SearchHit; import org.elasticsearch.search.SearchHits; +import org.elasticsearch.search.sort.NestedSortBuilder; import org.elasticsearch.search.sort.SortBuilder; import org.elasticsearch.search.sort.SortBuilders; import org.elasticsearch.search.sort.SortOrder; @@ -163,9 +164,13 @@ private TestContext create(SearchType... searchTypes) throws Exception { } } else { if (randomBoolean()) { - sort = SortBuilders.fieldSort("nested.field3").missing(1); + sort = SortBuilders.fieldSort("nested.field3") + .setNestedSort(new NestedSortBuilder("nested")) + .missing(1); } else { - sort = SortBuilders.fieldSort("nested.field4").missing("1"); + sort = SortBuilders.fieldSort("nested.field4") + .setNestedSort(new NestedSortBuilder("nested")) + .missing("1"); } } sort.order(randomBoolean() ? SortOrder.ASC : SortOrder.DESC); From 44671ee92f21d30a7f0b445de7a4c29013fcded3 Mon Sep 17 00:00:00 2001 From: jimczi Date: Wed, 12 Jun 2019 14:32:21 +0200 Subject: [PATCH 4/6] address review comments --- .../java/org/elasticsearch/search/sort/SortBuilder.java | 6 ------ 1 file changed, 6 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/sort/SortBuilder.java b/server/src/main/java/org/elasticsearch/search/sort/SortBuilder.java index c57aa6c509abc..881df666802d7 100644 --- a/server/src/main/java/org/elasticsearch/search/sort/SortBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/sort/SortBuilder.java @@ -170,12 +170,6 @@ public static Optional buildSort(List> sortBuilde return Optional.empty(); } - protected static Nested resolveNested(QueryShardContext context, String nestedPath, QueryBuilder nestedFilter) throws IOException { - NestedSortBuilder nestedSortBuilder = new NestedSortBuilder(nestedPath); - nestedSortBuilder.setFilter(nestedFilter); - return resolveNested(context, nestedSortBuilder); - } - protected static Nested resolveNested(QueryShardContext context, NestedSortBuilder nestedSort) throws IOException { final Query childQuery = resolveNestedQuery(context, nestedSort, null); if (childQuery == null) { From 7dd5f1df51057d57c3fb5e68896f8146eaf6ef35 Mon Sep 17 00:00:00 2001 From: jimczi Date: Wed, 12 Jun 2019 17:45:50 +0200 Subject: [PATCH 5/6] fix docs --- docs/reference/migration/migrate_8_0/search.asciidoc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/reference/migration/migrate_8_0/search.asciidoc b/docs/reference/migration/migrate_8_0/search.asciidoc index 9955457e02753..97796a10fca22 100644 --- a/docs/reference/migration/migrate_8_0/search.asciidoc +++ b/docs/reference/migration/migrate_8_0/search.asciidoc @@ -12,16 +12,16 @@ The `/{index}/{type}/_termvectors`, `/{index}/{type}/{id}/_termvectors` and `/{i [float] ==== Removal of queries -* The `common` query was deprecated in 7.x and has been removed in 8.0. - The same functionality can be achieved by the `match` query if the total number of hits is not tracked. +The `common` query was deprecated in 7.x and has been removed in 8.0. +The same functionality can be achieved by the `match` query if the total number of hits is not tracked. [float] ===== Removal of query parameters -* The `cutoff_frequency` parameter was deprecated in 7.x and has been removed in 8.0 from `match` and `multi_match` queries. - The same functionality can be achieved without any configuration provided that the total number of hits is not tracked. +The `cutoff_frequency` parameter was deprecated in 7.x and has been removed in 8.0 from `match` and `multi_match` queries. +The same functionality can be achieved without any configuration provided that the total number of hits is not tracked. [float] ===== Removal of sort parameters -* The `nested_filter` and `nested_path` options, deprecated in 6.x, has been removed in favor of the `nested` context. \ No newline at end of file +The `nested_filter` and `nested_path` options, deprecated in 6.x, have been removed in favor of the `nested` context. From d21672aa6e042be64779c12d75f48fb1774f9c61 Mon Sep 17 00:00:00 2001 From: jimczi Date: Thu, 27 Jun 2019 12:50:22 +0200 Subject: [PATCH 6/6] fix docs --- docs/reference/search/request/sort.asciidoc | 2 -- 1 file changed, 2 deletions(-) diff --git a/docs/reference/search/request/sort.asciidoc b/docs/reference/search/request/sort.asciidoc index 5ccfef46440d4..ccbc3da6e063b 100644 --- a/docs/reference/search/request/sort.asciidoc +++ b/docs/reference/search/request/sort.asciidoc @@ -263,8 +263,6 @@ field support has a `nested` sort option with the following properties: NOTE: Elasticsearch will throw an error if a nested field is defined in a sort without a `nested` context. -============================================ - ===== Nested sorting examples In the below example `offer` is a field of type `nested`.