From dd33dd21314fa8ef48ed062faa31c1d259764820 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Mon, 2 Aug 2021 15:26:38 +0200 Subject: [PATCH 1/3] nested path filter draft --- rest-api-spec/build.gradle | 2 +- .../search/sort/10_nested_path_filter.yml | 53 +++++++++++++++++++ .../search/sort/GeoDistanceSortBuilder.java | 26 +++++++-- .../search/sort/ScriptSortBuilder.java | 15 ++++++ .../search/sort/SortBuilder.java | 7 +++ 5 files changed, 98 insertions(+), 5 deletions(-) create mode 100644 rest-api-spec/src/yamlRestCompatTest/resources/rest-api-spec/test/v7compat/search/sort/10_nested_path_filter.yml diff --git a/rest-api-spec/build.gradle b/rest-api-spec/build.gradle index 85d4adddae9d2..3691e08cf0a20 100644 --- a/rest-api-spec/build.gradle +++ b/rest-api-spec/build.gradle @@ -89,7 +89,7 @@ tasks.named("yamlRestCompatTest").configure { OS.current() != OS.WINDOWS } systemProperty 'tests.rest.blacklist', ([ - 'search.aggregation/200_top_hits_metric/top_hits aggregation with sequence numbers', +// 'search.aggregation/200_top_hits_metric/top_hits aggregation with sequence numbers', 'search/310_match_bool_prefix/multi_match multiple fields with cutoff_frequency throws exception', //cutoff_frequency 'search/340_type_query/type query', // type_query - probably should behave like match_all ] + v7compatibilityNotSupportedTests()) diff --git a/rest-api-spec/src/yamlRestCompatTest/resources/rest-api-spec/test/v7compat/search/sort/10_nested_path_filter.yml b/rest-api-spec/src/yamlRestCompatTest/resources/rest-api-spec/test/v7compat/search/sort/10_nested_path_filter.yml new file mode 100644 index 0000000000000..09d95fdde414d --- /dev/null +++ b/rest-api-spec/src/yamlRestCompatTest/resources/rest-api-spec/test/v7compat/search/sort/10_nested_path_filter.yml @@ -0,0 +1,53 @@ +--- +setup: +- skip: + features: + - "headers" + - "allowed_warnings_regex" +- do: + indices.create: + index: "my-index" + body: + settings: + number_of_shards: 1 + number_of_replicas: 0 + mappings: + properties: + offer: + type: "nested" +- do: + index: + index: "my-index" + id: 1 + refresh: true + body: + offer: + price: 10 + color: blue + + headers: + Content-Type: "application/vnd.elasticsearch+json;compatible-with=7" + Accept: "application/vnd.elasticsearch+json;compatible-with=7" + allowed_warnings_regex: + - "\\[types removal\\].*" + +--- +"Sort with nested_path throws exception": +- do: + search: + rest_total_hits_as_int: true + index: "my-index" + body: + sort: + - offer.price: + mode: avg + order: asc + nested_path: offer + nested_filter: + term: + offer.color: blue + headers: + Content-Type: "application/vnd.elasticsearch+json;compatible-with=7" + Accept: "application/vnd.elasticsearch+json;compatible-with=7" + allowed_warnings_regex: + - "\\[types removal\\].*" 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 5704a83d1c791..9ac6644c793de 100644 --- a/server/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java @@ -19,6 +19,7 @@ import org.apache.lucene.util.BitSet; import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.Version; +import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.xcontent.ParseField; import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.geo.GeoDistance; @@ -31,6 +32,7 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentParser.Token; +import org.elasticsearch.core.RestApiVersion; import org.elasticsearch.index.fielddata.FieldData; import org.elasticsearch.index.fielddata.IndexFieldData; import org.elasticsearch.index.fielddata.IndexFieldData.XFieldComparatorSource.Nested; @@ -63,6 +65,7 @@ * A geo distance based sorting on a geo point like field. */ public class GeoDistanceSortBuilder extends SortBuilder { + private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(GeoDistanceSortBuilder.class); public static final String NAME = "_geo_distance"; public static final String ALTERNATIVE_NAME = "_geoDistance"; @@ -448,11 +451,26 @@ public static GeoDistanceSortBuilder fromXContent(XContentParser parser, String point.resetFromString(parser.text()); geoPoints.add(point); fieldName = currentName; - } else if (fieldName.equals(currentName)){ + } else if (fieldName.equals(currentName)) { throw new ParsingException( - parser.getTokenLocation(), - "Only geohashes of type string supported for field [{}]", - currentName); + parser.getTokenLocation(), + "Only geohashes of type string supported for field [{}]", + currentName); + } else if (parser.getRestApiVersion() == RestApiVersion.V_7 && + NESTED_PATH_FIELD.match(currentName, parser.getDeprecationHandler())) { + deprecationLogger.compatibleApiWarning("nested_path", + "[nested_path] has been removed in favour of the [nested] parameter"); + throw new ParsingException( + parser.getTokenLocation(), + "[nested_path] has been removed in favour of the [nested] parameter", + currentName); + } else if (parser.getRestApiVersion() == RestApiVersion.V_7 && + NESTED_FILTER_FIELD.match(currentName, parser.getDeprecationHandler())) { + deprecationLogger.compatibleApiWarning("nested_filter", "[nested_filter] has been removed in favour of the [nested] parameter"); + throw new ParsingException( + parser.getTokenLocation(), + "[nested_filter] has been removed in favour of the [nested] parameter", + currentName); } else { throw new ParsingException( parser.getTokenLocation(), 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 828e532499189..79e6546c60ff5 100644 --- a/server/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java @@ -15,6 +15,7 @@ import org.apache.lucene.util.BytesRef; import org.apache.lucene.util.BytesRefBuilder; import org.elasticsearch.Version; +import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.xcontent.ParseField; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; @@ -204,6 +205,20 @@ public XContentBuilder toXContent(XContentBuilder builder, Params builderParams) PARSER.declareString((b, v) -> b.order(SortOrder.fromString(v)), ORDER_FIELD); PARSER.declareString((b, v) -> b.sortMode(SortMode.fromString(v)), SORTMODE_FIELD); PARSER.declareObject(ScriptSortBuilder::setNestedSort, (p, c) -> NestedSortBuilder.fromXContent(p), NESTED_FIELD); + + PARSER.declareObject((b,v)->{}, (p, c) -> { + throw new ParsingException( + p.getTokenLocation(), + "[nested_path] has been removed in favour of the [nested] parameter", + c); + }, NESTED_PATH_FIELD); + + PARSER.declareObject((b,v)->{}, (p, c) -> { + throw new ParsingException( + p.getTokenLocation(), + "[nested_filter] has been removed in favour of the [nested] parameter", + c); + }, NESTED_FILTER_FIELD); } /** 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 173b277697e6f..7964de6ecd29c 100644 --- a/server/src/main/java/org/elasticsearch/search/sort/SortBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/sort/SortBuilder.java @@ -20,6 +20,7 @@ import org.elasticsearch.common.util.BigArrays; import org.elasticsearch.common.xcontent.ToXContentObject; import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.core.RestApiVersion; import org.elasticsearch.index.fielddata.IndexFieldData.XFieldComparatorSource.Nested; import org.elasticsearch.index.mapper.NestedObjectMapper; import org.elasticsearch.index.mapper.ObjectMapper; @@ -45,6 +46,12 @@ 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") + .withAllDeprecated() + .forRestApiVersion(RestApiVersion.equalTo(RestApiVersion.V_7)); + public static final ParseField NESTED_PATH_FIELD = new ParseField("nested_path") + .withAllDeprecated() + .forRestApiVersion(RestApiVersion.equalTo(RestApiVersion.V_7)); private static final Map> PARSERS = Map.of( ScriptSortBuilder.NAME, ScriptSortBuilder::fromXContent, From ffbaadcf6a0cf2e1e10bb7d1a13914193d7efe92 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Tue, 3 Aug 2021 16:56:13 +0200 Subject: [PATCH 2/3] test fix --- rest-api-spec/build.gradle | 2 +- .../search/sort/10_nested_path_filter.yml | 25 +++++++++++++++---- .../search/sort/FieldSortBuilder.java | 14 +++++++++++ .../search/sort/GeoDistanceSortBuilder.java | 3 ++- 4 files changed, 37 insertions(+), 7 deletions(-) diff --git a/rest-api-spec/build.gradle b/rest-api-spec/build.gradle index 3691e08cf0a20..4400b270d456d 100644 --- a/rest-api-spec/build.gradle +++ b/rest-api-spec/build.gradle @@ -81,6 +81,7 @@ def v7compatibilityNotSupportedTests = { 'field_caps/30_filter/Field caps with index filter', //behaviour change after #63692 4digits dates are parsed as epoch and in quotes as year 'indices.forcemerge/10_basic/Check deprecation warning when incompatible only_expunge_deletes and max_num_segments values are both set', //#44761 bug fix + 'search.aggregation/200_top_hits_metric/top_hits aggregation with sequence numbers', // #42809 the use nested path and filter sort throws an exception ] } tasks.named("yamlRestCompatTest").configure { @@ -89,7 +90,6 @@ tasks.named("yamlRestCompatTest").configure { OS.current() != OS.WINDOWS } systemProperty 'tests.rest.blacklist', ([ -// 'search.aggregation/200_top_hits_metric/top_hits aggregation with sequence numbers', 'search/310_match_bool_prefix/multi_match multiple fields with cutoff_frequency throws exception', //cutoff_frequency 'search/340_type_query/type query', // type_query - probably should behave like match_all ] + v7compatibilityNotSupportedTests()) diff --git a/rest-api-spec/src/yamlRestCompatTest/resources/rest-api-spec/test/v7compat/search/sort/10_nested_path_filter.yml b/rest-api-spec/src/yamlRestCompatTest/resources/rest-api-spec/test/v7compat/search/sort/10_nested_path_filter.yml index 09d95fdde414d..20becb83b9398 100644 --- a/rest-api-spec/src/yamlRestCompatTest/resources/rest-api-spec/test/v7compat/search/sort/10_nested_path_filter.yml +++ b/rest-api-spec/src/yamlRestCompatTest/resources/rest-api-spec/test/v7compat/search/sort/10_nested_path_filter.yml @@ -34,6 +34,7 @@ setup: --- "Sort with nested_path throws exception": - do: + catch: /\[nested_path\] has been removed in favour of the \[nested\] parameter/ search: rest_total_hits_as_int: true index: "my-index" @@ -43,11 +44,25 @@ setup: mode: avg order: asc nested_path: offer - nested_filter: - term: - offer.color: blue headers: Content-Type: "application/vnd.elasticsearch+json;compatible-with=7" Accept: "application/vnd.elasticsearch+json;compatible-with=7" - allowed_warnings_regex: - - "\\[types removal\\].*" + +--- +"Sort with nested_filter throws exception": + - do: + catch: /\[nested_filter\] has been removed in favour of the \[nested\] parameter/ + search: + rest_total_hits_as_int: true + index: "my-index" + body: + sort: + - offer.price: + mode: avg + order: asc + nested_filter: + term: + offer.color: blue + headers: + Content-Type: "application/vnd.elasticsearch+json;compatible-with=7" + Accept: "application/vnd.elasticsearch+json;compatible-with=7" 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 ea585ec19c05f..b6bba8b9c2ef2 100644 --- a/server/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java @@ -16,6 +16,7 @@ import org.apache.lucene.search.SortField; import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.Version; +import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.xcontent.ParseField; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; @@ -692,6 +693,19 @@ public static FieldSortBuilder fromXContent(XContentParser parser, String fieldN PARSER.declareObject(FieldSortBuilder::setNestedSort, (p, c) -> NestedSortBuilder.fromXContent(p), NESTED_FIELD); PARSER.declareString(FieldSortBuilder::setNumericType, NUMERIC_TYPE); PARSER.declareString(FieldSortBuilder::setFormat, FORMAT); + PARSER.declareField((b,v)->{}, (p, c) -> { + throw new ParsingException( + p.getTokenLocation(), + "[nested_path] has been removed in favour of the [nested] parameter", + c); + }, NESTED_PATH_FIELD, ValueType.STRING); + + PARSER.declareObject((b,v)->{}, (p, c) -> { + throw new ParsingException( + p.getTokenLocation(), + "[nested_filter] has been removed in favour of the [nested] parameter", + c); + }, NESTED_FILTER_FIELD); } @Override 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 9ac6644c793de..25c1e16d67fb0 100644 --- a/server/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java @@ -466,7 +466,8 @@ public static GeoDistanceSortBuilder fromXContent(XContentParser parser, String currentName); } else if (parser.getRestApiVersion() == RestApiVersion.V_7 && NESTED_FILTER_FIELD.match(currentName, parser.getDeprecationHandler())) { - deprecationLogger.compatibleApiWarning("nested_filter", "[nested_filter] has been removed in favour of the [nested] parameter"); + deprecationLogger.compatibleApiWarning("nested_filter", + "[nested_filter] has been removed in favour of the [nested] parameter"); throw new ParsingException( parser.getTokenLocation(), "[nested_filter] has been removed in favour of the [nested] parameter", From 165e3852e82f8f946a000c580e47006c7718c387 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Wed, 4 Aug 2021 10:39:28 +0200 Subject: [PATCH 3/3] geo sort test --- .../search/sort/10_nested_path_filter.yml | 85 ++++++++++++++++++- .../search/sort/GeoDistanceSortBuilder.java | 46 +++++----- 2 files changed, 106 insertions(+), 25 deletions(-) diff --git a/rest-api-spec/src/yamlRestCompatTest/resources/rest-api-spec/test/v7compat/search/sort/10_nested_path_filter.yml b/rest-api-spec/src/yamlRestCompatTest/resources/rest-api-spec/test/v7compat/search/sort/10_nested_path_filter.yml index 20becb83b9398..536ad86378e69 100644 --- a/rest-api-spec/src/yamlRestCompatTest/resources/rest-api-spec/test/v7compat/search/sort/10_nested_path_filter.yml +++ b/rest-api-spec/src/yamlRestCompatTest/resources/rest-api-spec/test/v7compat/search/sort/10_nested_path_filter.yml @@ -28,8 +28,46 @@ setup: headers: Content-Type: "application/vnd.elasticsearch+json;compatible-with=7" Accept: "application/vnd.elasticsearch+json;compatible-with=7" - allowed_warnings_regex: - - "\\[types removal\\].*" + +- do: + indices.create: + index: "my-locations" + body: + settings: + number_of_shards: 1 + number_of_replicas: 0 + mappings: + properties: + pin: + properties: + location: + type: geo_point + offer: + type: "nested" + headers: + Content-Type: "application/vnd.elasticsearch+json;compatible-with=7" + Accept: "application/vnd.elasticsearch+json;compatible-with=7" + +- do: + index: + index: "my-locations" + id: 1 + refresh: true + body: + offer: + price: 10 + color: blue + pin: + location: + lat: 40.12 + lon: -71.34 + headers: + Content-Type: "application/vnd.elasticsearch+json;compatible-with=7" + Accept: "application/vnd.elasticsearch+json;compatible-with=7" + + + + --- "Sort with nested_path throws exception": @@ -66,3 +104,46 @@ setup: headers: Content-Type: "application/vnd.elasticsearch+json;compatible-with=7" Accept: "application/vnd.elasticsearch+json;compatible-with=7" + + +--- +"Geo search with nested_filter throws exception": + - do: + catch: /\[nested_filter\] has been removed in favour of the \[nested\] parameter/ + search: + rest_total_hits_as_int: true + index: "my-locations" + body: + query: + match_all: {} + sort: + _geo_distance: + pin.location: + - -70 + - 40 + nested_filter: + term: + offer.color: blue + headers: + Content-Type: "application/vnd.elasticsearch+json;compatible-with=7" + Accept: "application/vnd.elasticsearch+json;compatible-with=7" + +--- +"Geo search with nested_path throws exception": + - do: + catch: /\[nested_path\] has been removed in favour of the \[nested\] parameter/ + search: + rest_total_hits_as_int: true + index: "my-locations" + body: + query: + match_all: {} + sort: + _geo_distance: + pin.location: + - -70 + - 40 + nested_path: "offer" + headers: + Content-Type: "application/vnd.elasticsearch+json;compatible-with=7" + Accept: "application/vnd.elasticsearch+json;compatible-with=7" 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 25c1e16d67fb0..75fb86841a376 100644 --- a/server/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java @@ -409,7 +409,15 @@ public static GeoDistanceSortBuilder fromXContent(XContentParser parser, String fieldName = currentName; } else if (token == XContentParser.Token.START_OBJECT) { - if (NESTED_FIELD.match(currentName, parser.getDeprecationHandler())) { + if (parser.getRestApiVersion() == RestApiVersion.V_7 && + NESTED_FILTER_FIELD.match(currentName, parser.getDeprecationHandler())) { + deprecationLogger.compatibleApiWarning("nested_filter", + "[nested_filter] has been removed in favour of the [nested] parameter"); + throw new ParsingException( + parser.getTokenLocation(), + "[nested_filter] has been removed in favour of the [nested] parameter", + currentName); + } else if (NESTED_FIELD.match(currentName, parser.getDeprecationHandler())) { nestedSort = NestedSortBuilder.fromXContent(parser); } else { // the json in the format of -> field : { lat : 30, lon : 12 } @@ -426,7 +434,15 @@ public static GeoDistanceSortBuilder fromXContent(XContentParser parser, String geoPoints.add(point); } } else if (token.isValue()) { - if (ORDER_FIELD.match(currentName, parser.getDeprecationHandler())) { + if (parser.getRestApiVersion() == RestApiVersion.V_7 && + NESTED_PATH_FIELD.match(currentName, parser.getDeprecationHandler())) { + deprecationLogger.compatibleApiWarning("nested_path", + "[nested_path] has been removed in favour of the [nested] parameter"); + throw new ParsingException( + parser.getTokenLocation(), + "[nested_path] has been removed in favour of the [nested] parameter", + currentName); + } else if (ORDER_FIELD.match(currentName, parser.getDeprecationHandler())) { order = SortOrder.fromString(parser.text()); } else if (UNIT_FIELD.match(currentName, parser.getDeprecationHandler())) { unit = DistanceUnit.fromString(parser.text()); @@ -438,13 +454,13 @@ public static GeoDistanceSortBuilder fromXContent(XContentParser parser, String sortMode = SortMode.fromString(parser.text()); } else if (IGNORE_UNMAPPED.match(currentName, parser.getDeprecationHandler())) { ignoreUnmapped = parser.booleanValue(); - } else if (token == Token.VALUE_STRING){ + } else if (token == Token.VALUE_STRING) { if (fieldName != null && fieldName.equals(currentName) == false) { throw new ParsingException( - parser.getTokenLocation(), - "Trying to reset fieldName to [{}], already set to [{}].", - currentName, - fieldName); + parser.getTokenLocation(), + "Trying to reset fieldName to [{}], already set to [{}].", + currentName, + fieldName); } GeoPoint point = new GeoPoint(); @@ -456,22 +472,6 @@ public static GeoDistanceSortBuilder fromXContent(XContentParser parser, String parser.getTokenLocation(), "Only geohashes of type string supported for field [{}]", currentName); - } else if (parser.getRestApiVersion() == RestApiVersion.V_7 && - NESTED_PATH_FIELD.match(currentName, parser.getDeprecationHandler())) { - deprecationLogger.compatibleApiWarning("nested_path", - "[nested_path] has been removed in favour of the [nested] parameter"); - throw new ParsingException( - parser.getTokenLocation(), - "[nested_path] has been removed in favour of the [nested] parameter", - currentName); - } else if (parser.getRestApiVersion() == RestApiVersion.V_7 && - NESTED_FILTER_FIELD.match(currentName, parser.getDeprecationHandler())) { - deprecationLogger.compatibleApiWarning("nested_filter", - "[nested_filter] has been removed in favour of the [nested] parameter"); - throw new ParsingException( - parser.getTokenLocation(), - "[nested_filter] has been removed in favour of the [nested] parameter", - currentName); } else { throw new ParsingException( parser.getTokenLocation(),