From 9432d57409b76a3f4c5397120a8b1c298c2fafa5 Mon Sep 17 00:00:00 2001 From: Isabel Drost-Fromm Date: Mon, 7 Dec 2015 17:16:55 +0100 Subject: [PATCH] Make GeoDistanceSortBuilder serializable Adds to GeoDistanceSortBuilder: * equals * hashcode * writeto/readfrom * moves xcontent parsing logic over * adds roundtrip tests * fixes roundtrip test for xcontent by keeping points just as geopoints not geohashes internally * fixes xcontent parsing of ignore_malformed if coerce is set/unset * adds exception to sortMode setter to avoid setting invalid sort modes Relates to #15178 --- .../search/sort/GeoDistanceSortBuilder.java | 387 ++++++++++++++++-- .../search/sort/GeoDistanceSortParser.java | 28 +- .../search/sort/SortBuilders.java | 30 +- .../search/sort/SortElementParserTemp.java | 40 ++ .../elasticsearch/search/sort/SortOrder.java | 3 +- .../builder/SearchSourceBuilderTests.java | 4 +- .../search/sort/AbstractSortTestCase.java | 162 ++++++++ .../search/sort/GeoDistanceIT.java | 68 +-- .../search/sort/GeoDistanceSortBuilderIT.java | 38 +- .../sort/GeoDistanceSortBuilderTests.java | 252 ++++++++++++ .../search/sort/RandomSortDataGenerator.java | 113 +++++ 11 files changed, 1013 insertions(+), 112 deletions(-) create mode 100644 core/src/main/java/org/elasticsearch/search/sort/SortElementParserTemp.java create mode 100644 core/src/test/java/org/elasticsearch/search/sort/AbstractSortTestCase.java create mode 100644 core/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderTests.java create mode 100644 core/src/test/java/org/elasticsearch/search/sort/RandomSortDataGenerator.java diff --git a/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java b/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java index 8f502e53058..e37eed61c6d 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java @@ -22,41 +22,115 @@ package org.elasticsearch.search.sort; import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.geo.GeoDistance; import org.elasticsearch.common.geo.GeoPoint; +import org.elasticsearch.common.geo.GeoUtils; +import org.elasticsearch.common.io.stream.NamedWriteable; +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.unit.DistanceUnit; +import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.index.query.QueryBuilder; +import org.elasticsearch.index.query.QueryParseContext; +import org.elasticsearch.search.MultiValueMode; import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; import java.util.List; import java.util.Locale; +import java.util.Objects; /** * A geo distance based sorting on a geo point like field. */ -public class GeoDistanceSortBuilder extends SortBuilder { +public class GeoDistanceSortBuilder extends SortBuilder + implements ToXContent, NamedWriteable, SortElementParserTemp { + public static final String NAME = "_geo_distance"; + public static final boolean DEFAULT_COERCE = false; + public static final boolean DEFAULT_IGNORE_MALFORMED = false; - final String fieldName; + static final GeoDistanceSortBuilder PROTOTYPE = new GeoDistanceSortBuilder("", -1, -1); + + private final String fieldName; private final List points = new ArrayList<>(); - private final List geohashes = new ArrayList<>(); - private GeoDistance geoDistance; - private DistanceUnit unit; - private SortOrder order; - private String sortMode; + private GeoDistance geoDistance = GeoDistance.DEFAULT; + private DistanceUnit unit = DistanceUnit.DEFAULT; + private SortOrder order = SortOrder.ASC; + + // TODO there is an enum that covers that parameter which we should be using here + private String sortMode = null; + @SuppressWarnings("rawtypes") private QueryBuilder nestedFilter; private String nestedPath; - private Boolean coerce; - private Boolean ignoreMalformed; + + // TODO switch to GeoValidationMethod enum + private boolean coerce = DEFAULT_COERCE; + private boolean ignoreMalformed = DEFAULT_IGNORE_MALFORMED; /** * Constructs a new distance based sort on a geo point like field. * * @param fieldName The geo point like field name. + * @param points The points to create the range distance facets from. */ - public GeoDistanceSortBuilder(String fieldName) { + public GeoDistanceSortBuilder(String fieldName, GeoPoint... points) { this.fieldName = fieldName; + if (points.length == 0) { + throw new IllegalArgumentException("Geo distance sorting needs at least one point."); + } + this.points.addAll(Arrays.asList(points)); + } + + /** + * Constructs a new distance based sort on a geo point like field. + * + * @param fieldName The geo point like field name. + * @param lat Latitude of the point to create the range distance facets from. + * @param lon Longitude of the point to create the range distance facets from. + */ + public GeoDistanceSortBuilder(String fieldName, double lat, double lon) { + this(fieldName, new GeoPoint(lat, lon)); + } + + /** + * Constructs a new distance based sort on a geo point like field. + * + * @param fieldName The geo point like field name. + * @param geohashes The points to create the range distance facets from. + */ + public GeoDistanceSortBuilder(String fieldName, String ... geohashes) { + if (geohashes.length == 0) { + throw new IllegalArgumentException("Geo distance sorting needs at least one point."); + } + for (String geohash : geohashes) { + this.points.add(GeoPoint.fromGeohash(geohash)); + } + this.fieldName = fieldName; + } + + /** + * Copy constructor. + * */ + GeoDistanceSortBuilder(GeoDistanceSortBuilder original) { + this.fieldName = original.fieldName(); + this.points.addAll(original.points); + this.geoDistance = original.geoDistance; + this.unit = original.unit; + this.order = original.order; + this.sortMode = original.sortMode; + this.nestedFilter = original.nestedFilter; + this.nestedPath = original.nestedPath; + this.coerce = original.coerce; + this.ignoreMalformed = original.ignoreMalformed; + } + + /** + * Returns the geo point like field the distance based sort operates on. + * */ + public String fieldName() { + return this.fieldName; } /** @@ -79,15 +153,27 @@ public class GeoDistanceSortBuilder extends SortBuilder { this.points.addAll(Arrays.asList(points)); return this; } + + /** + * Returns the points to create the range distance facets from. + */ + public GeoPoint[] points() { + return this.points.toArray(new GeoPoint[this.points.size()]); + } /** * The geohash of the geo point to create the range distance facets from. + * + * Deprecated - please use points(GeoPoint... points) instead. */ + @Deprecated public GeoDistanceSortBuilder geohashes(String... geohashes) { - this.geohashes.addAll(Arrays.asList(geohashes)); + for (String geohash : geohashes) { + this.points.add(GeoPoint.fromGeohash(geohash)); + } return this; } - + /** * The geo distance type used to compute the distance. */ @@ -95,6 +181,13 @@ public class GeoDistanceSortBuilder extends SortBuilder { this.geoDistance = geoDistance; return this; } + + /** + * Returns the geo distance type used to compute the distance. + */ + public GeoDistance geoDistance() { + return this.geoDistance; + } /** * The distance unit to use. Defaults to {@link org.elasticsearch.common.unit.DistanceUnit#KILOMETERS} @@ -104,6 +197,13 @@ public class GeoDistanceSortBuilder extends SortBuilder { return this; } + /** + * Returns the distance unit to use. Defaults to {@link org.elasticsearch.common.unit.DistanceUnit#KILOMETERS} + */ + public DistanceUnit unit() { + return this.unit; + } + /** * The order of sorting. Defaults to {@link SortOrder#ASC}. */ @@ -113,11 +213,18 @@ public class GeoDistanceSortBuilder extends SortBuilder { return this; } + /** Returns the order of sorting. */ + public SortOrder order() { + return this.order; + } + /** * Not relevant. + * + * TODO should this throw an exception rather than silently ignore a parameter that is not used? */ @Override - public SortBuilder missing(Object missing) { + public GeoDistanceSortBuilder missing(Object missing) { return this; } @@ -126,10 +233,19 @@ public class GeoDistanceSortBuilder extends SortBuilder { * Possible values: min and max */ public GeoDistanceSortBuilder sortMode(String sortMode) { + MultiValueMode temp = MultiValueMode.fromString(sortMode); + if (temp == MultiValueMode.SUM) { + throw new IllegalArgumentException("sort_mode [sum] isn't supported for sorting by geo distance"); + } this.sortMode = sortMode; return this; } + /** Returns which distance to use for sorting in the case a document contains multiple geo points. */ + public String sortMode() { + return this.sortMode; + } + /** * Sets the nested filter that the nested objects should match with in order to be taken into account * for sorting. @@ -139,6 +255,14 @@ public class GeoDistanceSortBuilder extends SortBuilder { return this; } + /** + * Returns the nested filter that the nested objects should match with in order to be taken into account + * for sorting. + **/ + 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. @@ -147,42 +271,53 @@ public class GeoDistanceSortBuilder extends SortBuilder { 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. + */ + public String getNestedPath() { + return this.nestedPath; + } public GeoDistanceSortBuilder coerce(boolean coerce) { this.coerce = coerce; return this; } + public boolean coerce() { + return this.coerce; + } + public GeoDistanceSortBuilder ignoreMalformed(boolean ignoreMalformed) { - this.ignoreMalformed = ignoreMalformed; + if (coerce == false) { + this.ignoreMalformed = ignoreMalformed; + } return this; } + + public boolean ignoreMalformed() { + return this.ignoreMalformed; + } @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { - builder.startObject("_geo_distance"); - if (geohashes.size() == 0 && points.size() == 0) { - throw new ElasticsearchParseException("No points provided for _geo_distance sort."); - } + builder.startObject(NAME); builder.startArray(fieldName); for (GeoPoint point : points) { builder.value(point); } - for (String geohash : geohashes) { - builder.value(geohash); - } builder.endArray(); - if (unit != null) { - builder.field("unit", unit); - } - if (geoDistance != null) { - builder.field("distance_type", geoDistance.name().toLowerCase(Locale.ROOT)); - } + builder.field("unit", unit); + builder.field("distance_type", geoDistance.name().toLowerCase(Locale.ROOT)); if (order == SortOrder.DESC) { builder.field("reverse", true); + } else { + builder.field("reverse", false); } + if (sortMode != null) { builder.field("mode", sortMode); } @@ -193,14 +328,200 @@ public class GeoDistanceSortBuilder extends SortBuilder { if (nestedFilter != null) { builder.field("nested_filter", nestedFilter, params); } - if (coerce != null) { - builder.field("coerce", coerce); - } - if (ignoreMalformed != null) { - builder.field("ignore_malformed", ignoreMalformed); - } + builder.field("coerce", coerce); + builder.field("ignore_malformed", ignoreMalformed); builder.endObject(); return builder; } + + @Override + public String getWriteableName() { + return NAME; + } + + @Override + public boolean equals(Object object) { + if (this == object) { + return true; + } + + if (object == null || getClass() != object.getClass()) { + return false; + } + + GeoDistanceSortBuilder other = (GeoDistanceSortBuilder) object; + return Objects.equals(fieldName, other.fieldName) && + Objects.deepEquals(points, other.points) && + Objects.equals(geoDistance, other.geoDistance) && + 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(coerce, other.coerce) && + Objects.equals(ignoreMalformed, other.ignoreMalformed); + } + + @Override + public int hashCode() { + return Objects.hash(this.fieldName, this.points, this.geoDistance, + this.unit, this.sortMode, this.order, this.nestedFilter, this.nestedPath, this.coerce, this.ignoreMalformed); + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + out.writeString(fieldName); + out.writeGenericValue(points); + + geoDistance.writeTo(out); + unit.writeTo(out); + order.writeTo(out); + out.writeOptionalString(sortMode); + if (nestedFilter != null) { + out.writeBoolean(true); + out.writeQuery(nestedFilter); + } else { + out.writeBoolean(false); + } + out.writeOptionalString(nestedPath); + out.writeBoolean(coerce); + out.writeBoolean(ignoreMalformed); + } + + @Override + public GeoDistanceSortBuilder readFrom(StreamInput in) throws IOException { + String fieldName = in.readString(); + + ArrayList points = (ArrayList) in.readGenericValue(); + GeoDistanceSortBuilder result = new GeoDistanceSortBuilder(fieldName, points.toArray(new GeoPoint[points.size()])); + + result.geoDistance(GeoDistance.readGeoDistanceFrom(in)); + result.unit(DistanceUnit.readDistanceUnit(in)); + result.order(SortOrder.readOrderFrom(in)); + String sortMode = in.readOptionalString(); + if (sortMode != null) { + result.sortMode(sortMode); + } + if (in.readBoolean()) { + result.setNestedFilter(in.readQuery()); + } + result.setNestedPath(in.readOptionalString()); + result.coerce(in.readBoolean()); + result.ignoreMalformed(in.readBoolean()); + return result; + } + + @Override + public GeoDistanceSortBuilder fromXContent(QueryParseContext context, String elementName) throws IOException { + XContentParser parser = context.parser(); + String fieldName = null; + List geoPoints = new ArrayList<>(); + DistanceUnit unit = DistanceUnit.DEFAULT; + GeoDistance geoDistance = GeoDistance.DEFAULT; + boolean reverse = false; + MultiValueMode sortMode = null; + QueryBuilder nestedFilter = null; + String nestedPath = null; + + boolean coerce = GeoDistanceSortBuilder.DEFAULT_COERCE; + boolean ignoreMalformed = GeoDistanceSortBuilder.DEFAULT_IGNORE_MALFORMED; + + XContentParser.Token token; + String currentName = parser.currentName(); + while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { + if (token == XContentParser.Token.FIELD_NAME) { + currentName = parser.currentName(); + } else if (token == XContentParser.Token.START_ARRAY) { + parseGeoPoints(parser, geoPoints); + + fieldName = currentName; + } else if (token == XContentParser.Token.START_OBJECT) { + // the json in the format of -> field : { lat : 30, lon : 12 } + if ("nested_filter".equals(currentName) || "nestedFilter".equals(currentName)) { + // TODO Note to remember: while this is kept as a QueryBuilder internally, + // we need to make sure to call toFilter() on it once on the shard + // (e.g. in the new build() method) + nestedFilter = context.parseInnerQueryBuilder(); + } else { + fieldName = currentName; + GeoPoint point = new GeoPoint(); + GeoUtils.parseGeoPoint(parser, point); + geoPoints.add(point); + } + } else if (token.isValue()) { + if ("reverse".equals(currentName)) { + reverse = parser.booleanValue(); + } else if ("order".equals(currentName)) { + reverse = "desc".equals(parser.text()); + } else if ("unit".equals(currentName)) { + unit = DistanceUnit.fromString(parser.text()); + } else if ("distance_type".equals(currentName) || "distanceType".equals(currentName)) { + geoDistance = GeoDistance.fromString(parser.text()); + } else if ("coerce".equals(currentName) || "normalize".equals(currentName)) { + coerce = parser.booleanValue(); + if (coerce == true) { + ignoreMalformed = true; + } + } else if ("ignore_malformed".equals(currentName)) { + boolean ignore_malformed_value = parser.booleanValue(); + if (coerce == false) { + ignoreMalformed = ignore_malformed_value; + } + } else if ("sort_mode".equals(currentName) || "sortMode".equals(currentName) || "mode".equals(currentName)) { + sortMode = MultiValueMode.fromString(parser.text()); + } else if ("nested_path".equals(currentName) || "nestedPath".equals(currentName)) { + nestedPath = parser.text(); + } else { + GeoPoint point = new GeoPoint(); + point.resetFromString(parser.text()); + geoPoints.add(point); + fieldName = currentName; + } + } + } + + GeoDistanceSortBuilder result = new GeoDistanceSortBuilder(fieldName, geoPoints.toArray(new GeoPoint[geoPoints.size()])); + result.geoDistance(geoDistance); + result.unit(unit); + if (reverse) { + result.order(SortOrder.DESC); + } else { + result.order(SortOrder.ASC); + } + if (sortMode != null) { + result.sortMode(sortMode.name()); + } + result.setNestedFilter(nestedFilter); + result.setNestedPath(nestedPath); + result.coerce(coerce); + result.ignoreMalformed(ignoreMalformed); + return result; + + } + + static void parseGeoPoints(XContentParser parser, List geoPoints) throws IOException { + while (!parser.nextToken().equals(XContentParser.Token.END_ARRAY)) { + if (parser.currentToken() == XContentParser.Token.VALUE_NUMBER) { + // we might get here if the geo point is " number, number] " and the parser already moved over the opening bracket + // in this case we cannot use GeoUtils.parseGeoPoint(..) because this expects an opening bracket + double lon = parser.doubleValue(); + parser.nextToken(); + if (!parser.currentToken().equals(XContentParser.Token.VALUE_NUMBER)) { + throw new ElasticsearchParseException( + "geo point parsing: expected second number but got [{}] instead", + parser.currentToken()); + } + double lat = parser.doubleValue(); + GeoPoint point = new GeoPoint(); + point.reset(lat, lon); + geoPoints.add(point); + } else { + GeoPoint point = new GeoPoint(); + GeoUtils.parseGeoPoint(parser, point); + geoPoints.add(point); + } + + } + } } diff --git a/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortParser.java b/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortParser.java index 9fddf590ca4..248a051021e 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortParser.java +++ b/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortParser.java @@ -72,8 +72,8 @@ public class GeoDistanceSortParser implements SortParser { NestedInnerQueryParseSupport nestedHelper = null; final boolean indexCreatedBeforeV2_0 = context.indexShard().getIndexSettings().getIndexVersionCreated().before(Version.V_2_0_0); - boolean coerce = false; - boolean ignoreMalformed = false; + boolean coerce = GeoDistanceSortBuilder.DEFAULT_COERCE; + boolean ignoreMalformed = GeoDistanceSortBuilder.DEFAULT_IGNORE_MALFORMED; XContentParser.Token token; String currentName = parser.currentName(); @@ -81,7 +81,7 @@ public class GeoDistanceSortParser implements SortParser { if (token == XContentParser.Token.FIELD_NAME) { currentName = parser.currentName(); } else if (token == XContentParser.Token.START_ARRAY) { - parseGeoPoints(parser, geoPoints); + GeoDistanceSortBuilder.parseGeoPoints(parser, geoPoints); fieldName = currentName; } else if (token == XContentParser.Token.START_OBJECT) { @@ -213,26 +213,4 @@ public class GeoDistanceSortParser implements SortParser { return new SortField(fieldName, geoDistanceComparatorSource, reverse); } - private void parseGeoPoints(XContentParser parser, List geoPoints) throws IOException { - while (!parser.nextToken().equals(XContentParser.Token.END_ARRAY)) { - if (parser.currentToken() == XContentParser.Token.VALUE_NUMBER) { - // we might get here if the geo point is " number, number] " and the parser already moved over the opening bracket - // in this case we cannot use GeoUtils.parseGeoPoint(..) because this expects an opening bracket - double lon = parser.doubleValue(); - parser.nextToken(); - if (!parser.currentToken().equals(XContentParser.Token.VALUE_NUMBER)) { - throw new ElasticsearchParseException("geo point parsing: expected second number but got [{}] instead", parser.currentToken()); - } - double lat = parser.doubleValue(); - GeoPoint point = new GeoPoint(); - point.reset(lat, lon); - geoPoints.add(point); - } else { - GeoPoint point = new GeoPoint(); - GeoUtils.parseGeoPoint(parser, point); - geoPoints.add(point); - } - - } - } } diff --git a/core/src/main/java/org/elasticsearch/search/sort/SortBuilders.java b/core/src/main/java/org/elasticsearch/search/sort/SortBuilders.java index 9a843c43f74..f326fee3837 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/SortBuilders.java +++ b/core/src/main/java/org/elasticsearch/search/sort/SortBuilders.java @@ -19,8 +19,11 @@ package org.elasticsearch.search.sort; +import org.elasticsearch.common.geo.GeoPoint; import org.elasticsearch.script.Script; +import java.util.Arrays; + /** * A set of static factory methods for {@link SortBuilder}s. * @@ -58,8 +61,31 @@ public class SortBuilders { * A geo distance based sort. * * @param fieldName The geo point like field name. + * @param lat Latitude of the point to create the range distance facets from. + * @param lon Longitude of the point to create the range distance facets from. + * */ - public static GeoDistanceSortBuilder geoDistanceSort(String fieldName) { - return new GeoDistanceSortBuilder(fieldName); + public static GeoDistanceSortBuilder geoDistanceSort(String fieldName, double lat, double lon) { + return new GeoDistanceSortBuilder(fieldName, lat, lon); } + + /** + * Constructs a new distance based sort on a geo point like field. + * + * @param fieldName The geo point like field name. + * @param points The points to create the range distance facets from. + */ + public static GeoDistanceSortBuilder geoDistanceSort(String fieldName, GeoPoint... points) { + return new GeoDistanceSortBuilder(fieldName, points); + } + + /** + * Constructs a new distance based sort on a geo point like field. + * + * @param fieldName The geo point like field name. + * @param geohashes The points to create the range distance facets from. + */ + public static GeoDistanceSortBuilder geoDistanceSort(String fieldName, String ... geohashes) { + return new GeoDistanceSortBuilder(fieldName, geohashes); + } } diff --git a/core/src/main/java/org/elasticsearch/search/sort/SortElementParserTemp.java b/core/src/main/java/org/elasticsearch/search/sort/SortElementParserTemp.java new file mode 100644 index 00000000000..8893471b6c1 --- /dev/null +++ b/core/src/main/java/org/elasticsearch/search/sort/SortElementParserTemp.java @@ -0,0 +1,40 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.search.sort; + +import org.elasticsearch.common.xcontent.ToXContent; +import org.elasticsearch.index.query.QueryParseContext; + +import java.io.IOException; + +// TODO once sort refactoring is done this needs to be merged into SortBuilder +public interface SortElementParserTemp { + /** + * Creates a new SortBuilder from the json held by the {@link SortElementParserTemp} + * in {@link org.elasticsearch.common.xcontent.XContent} format + * + * @param context + * the input parse context. The state on the parser contained in + * this context will be changed as a side effect of this method + * call + * @return the new item + */ + T fromXContent(QueryParseContext context, String elementName) throws IOException; +} diff --git a/core/src/main/java/org/elasticsearch/search/sort/SortOrder.java b/core/src/main/java/org/elasticsearch/search/sort/SortOrder.java index 001924d1bdf..73e5ac55247 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/SortOrder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/SortOrder.java @@ -51,8 +51,7 @@ public enum SortOrder implements Writeable { } }; - public static final SortOrder DEFAULT = DESC; - private static final SortOrder PROTOTYPE = DEFAULT; + private static final SortOrder PROTOTYPE = ASC; @Override public SortOrder readFrom(StreamInput in) throws IOException { diff --git a/core/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java b/core/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java index 8521bd20395..786d5941827 100644 --- a/core/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java @@ -245,8 +245,8 @@ public class SearchSourceBuilderTests extends ESTestCase { builder.sort(SortBuilders.fieldSort(randomAsciiOfLengthBetween(5, 20)).order(randomFrom(SortOrder.values()))); break; case 1: - builder.sort(SortBuilders.geoDistanceSort(randomAsciiOfLengthBetween(5, 20)) - .geohashes(AbstractQueryTestCase.randomGeohash(1, 12)).order(randomFrom(SortOrder.values()))); + builder.sort(SortBuilders.geoDistanceSort(randomAsciiOfLengthBetween(5, 20), + AbstractQueryTestCase.randomGeohash(1, 12)).order(randomFrom(SortOrder.values()))); break; case 2: builder.sort(SortBuilders.scoreSort().order(randomFrom(SortOrder.values()))); diff --git a/core/src/test/java/org/elasticsearch/search/sort/AbstractSortTestCase.java b/core/src/test/java/org/elasticsearch/search/sort/AbstractSortTestCase.java new file mode 100644 index 00000000000..dfea1a9316b --- /dev/null +++ b/core/src/test/java/org/elasticsearch/search/sort/AbstractSortTestCase.java @@ -0,0 +1,162 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.search.sort; + +import org.elasticsearch.common.io.stream.BytesStreamOutput; +import org.elasticsearch.common.io.stream.NamedWriteable; +import org.elasticsearch.common.io.stream.NamedWriteableAwareStreamInput; +import org.elasticsearch.common.io.stream.NamedWriteableRegistry; +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.xcontent.ToXContent; +import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentFactory; +import org.elasticsearch.common.xcontent.XContentHelper; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.common.xcontent.XContentType; +import org.elasticsearch.index.query.QueryParseContext; +import org.elasticsearch.indices.query.IndicesQueriesRegistry; +import org.elasticsearch.search.SearchModule; +import org.elasticsearch.test.ESTestCase; +import org.junit.AfterClass; +import org.junit.BeforeClass; + +import java.io.IOException; + +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.not; + +public abstract class AbstractSortTestCase & ToXContent & SortElementParserTemp> extends ESTestCase { + + protected static NamedWriteableRegistry namedWriteableRegistry; + + private static final int NUMBER_OF_TESTBUILDERS = 20; + static IndicesQueriesRegistry indicesQueriesRegistry; + + @BeforeClass + public static void init() { + namedWriteableRegistry = new NamedWriteableRegistry(); + namedWriteableRegistry.registerPrototype(GeoDistanceSortBuilder.class, GeoDistanceSortBuilder.PROTOTYPE); + indicesQueriesRegistry = new SearchModule(Settings.EMPTY, namedWriteableRegistry).buildQueryParserRegistry(); + } + + @AfterClass + public static void afterClass() throws Exception { + namedWriteableRegistry = null; + } + + /** Returns random sort that is put under test */ + protected abstract T createTestItem(); + + /** Returns mutated version of original so the returned sort is different in terms of equals/hashcode */ + protected abstract T mutate(T original) throws IOException; + + /** + * Test that creates new sort from a random test sort and checks both for equality + */ + public void testFromXContent() throws IOException { + for (int runs = 0; runs < NUMBER_OF_TESTBUILDERS; runs++) { + T testItem = createTestItem(); + + XContentBuilder builder = XContentFactory.contentBuilder(randomFrom(XContentType.values())); + if (randomBoolean()) { + builder.prettyPrint(); + } + builder.startObject(); + testItem.toXContent(builder, ToXContent.EMPTY_PARAMS); + builder.endObject(); + + XContentParser itemParser = XContentHelper.createParser(builder.bytes()); + itemParser.nextToken(); + + /* + * filter out name of sort, or field name to sort on for element fieldSort + */ + itemParser.nextToken(); + String elementName = itemParser.currentName(); + itemParser.nextToken(); + + QueryParseContext context = new QueryParseContext(indicesQueriesRegistry); + context.reset(itemParser); + NamedWriteable parsedItem = testItem.fromXContent(context, elementName); + assertNotSame(testItem, parsedItem); + assertEquals(testItem, parsedItem); + assertEquals(testItem.hashCode(), parsedItem.hashCode()); + } + } + + /** + * Test serialization and deserialization of the test sort. + */ + public void testSerialization() throws IOException { + for (int runs = 0; runs < NUMBER_OF_TESTBUILDERS; runs++) { + T testsort = createTestItem(); + T deserializedsort = copyItem(testsort); + assertEquals(testsort, deserializedsort); + assertEquals(testsort.hashCode(), deserializedsort.hashCode()); + assertNotSame(testsort, deserializedsort); + } + } + + /** + * Test equality and hashCode properties + */ + public void testEqualsAndHashcode() throws IOException { + for (int runs = 0; runs < NUMBER_OF_TESTBUILDERS; runs++) { + T firstsort = createTestItem(); + assertFalse("sort is equal to null", firstsort.equals(null)); + assertFalse("sort is equal to incompatible type", firstsort.equals("")); + assertTrue("sort is not equal to self", firstsort.equals(firstsort)); + assertThat("same sort's hashcode returns different values if called multiple times", firstsort.hashCode(), + equalTo(firstsort.hashCode())); + assertThat("different sorts should not be equal", mutate(firstsort), not(equalTo(firstsort))); + assertThat("different sorts should have different hashcode", mutate(firstsort).hashCode(), not(equalTo(firstsort.hashCode()))); + + T secondsort = copyItem(firstsort); + assertTrue("sort is not equal to self", secondsort.equals(secondsort)); + assertTrue("sort is not equal to its copy", firstsort.equals(secondsort)); + assertTrue("equals is not symmetric", secondsort.equals(firstsort)); + assertThat("sort copy's hashcode is different from original hashcode", secondsort.hashCode(), equalTo(firstsort.hashCode())); + + T thirdsort = copyItem(secondsort); + assertTrue("sort is not equal to self", thirdsort.equals(thirdsort)); + assertTrue("sort is not equal to its copy", secondsort.equals(thirdsort)); + assertThat("sort copy's hashcode is different from original hashcode", secondsort.hashCode(), equalTo(thirdsort.hashCode())); + assertTrue("equals is not transitive", firstsort.equals(thirdsort)); + assertThat("sort copy's hashcode is different from original hashcode", firstsort.hashCode(), equalTo(thirdsort.hashCode())); + assertTrue("equals is not symmetric", thirdsort.equals(secondsort)); + assertTrue("equals is not symmetric", thirdsort.equals(firstsort)); + } + } + + protected T copyItem(T original) throws IOException { + try (BytesStreamOutput output = new BytesStreamOutput()) { + original.writeTo(output); + try (StreamInput in = new NamedWriteableAwareStreamInput(StreamInput.wrap(output.bytes()), namedWriteableRegistry)) { + @SuppressWarnings("unchecked") + T prototype = (T) namedWriteableRegistry.getPrototype(getPrototype(), original.getWriteableName()); + T copy = (T) prototype.readFrom(in); + return copy; + } + } + } + + protected abstract Class getPrototype(); +} diff --git a/core/src/test/java/org/elasticsearch/search/sort/GeoDistanceIT.java b/core/src/test/java/org/elasticsearch/search/sort/GeoDistanceIT.java index 158a0bb812b..ac8e988394a 100644 --- a/core/src/test/java/org/elasticsearch/search/sort/GeoDistanceIT.java +++ b/core/src/test/java/org/elasticsearch/search/sort/GeoDistanceIT.java @@ -33,7 +33,6 @@ import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.index.query.GeoDistanceQueryBuilder; import org.elasticsearch.index.query.QueryBuilders; import org.elasticsearch.plugins.Plugin; -import org.elasticsearch.rest.RestStatus; import org.elasticsearch.search.SearchHit; import org.elasticsearch.search.sort.SortBuilders; import org.elasticsearch.search.sort.SortOrder; @@ -53,7 +52,6 @@ import static org.elasticsearch.index.query.QueryBuilders.geoDistanceRangeQuery; import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery; import static org.elasticsearch.index.query.QueryBuilders.termQuery; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; -import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertFailures; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertFirstHit; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertOrderedSearchHits; @@ -62,7 +60,6 @@ import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSear import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.hasId; import static org.hamcrest.Matchers.anyOf; import static org.hamcrest.Matchers.closeTo; -import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; @@ -201,14 +198,14 @@ public class GeoDistanceIT extends ESIntegTestCase { // SORTING searchResponse = client().prepareSearch().setQuery(matchAllQuery()) - .addSort(SortBuilders.geoDistanceSort("location").point(40.7143528, -74.0059731).order(SortOrder.ASC)).execute() + .addSort(SortBuilders.geoDistanceSort("location", 40.7143528, -74.0059731).order(SortOrder.ASC)).execute() .actionGet(); assertHitCount(searchResponse, 7); assertOrderedSearchHits(searchResponse, "1", "3", "4", "5", "6", "2", "7"); searchResponse = client().prepareSearch().setQuery(matchAllQuery()) - .addSort(SortBuilders.geoDistanceSort("location").point(40.7143528, -74.0059731).order(SortOrder.DESC)).execute() + .addSort(SortBuilders.geoDistanceSort("location", 40.7143528, -74.0059731).order(SortOrder.DESC)).execute() .actionGet(); assertHitCount(searchResponse, 7); @@ -262,7 +259,7 @@ public class GeoDistanceIT extends ESIntegTestCase { // Order: Asc SearchResponse searchResponse = client().prepareSearch("test").setQuery(matchAllQuery()) - .addSort(SortBuilders.geoDistanceSort("locations").point(40.7143528, -74.0059731).order(SortOrder.ASC)).execute() + .addSort(SortBuilders.geoDistanceSort("locations", 40.7143528, -74.0059731).order(SortOrder.ASC)).execute() .actionGet(); assertHitCount(searchResponse, 5); @@ -275,7 +272,7 @@ public class GeoDistanceIT extends ESIntegTestCase { // Order: Asc, Mode: max searchResponse = client().prepareSearch("test").setQuery(matchAllQuery()) - .addSort(SortBuilders.geoDistanceSort("locations").point(40.7143528, -74.0059731).order(SortOrder.ASC).sortMode("max")) + .addSort(SortBuilders.geoDistanceSort("locations", 40.7143528, -74.0059731).order(SortOrder.ASC).sortMode("max")) .execute().actionGet(); assertHitCount(searchResponse, 5); @@ -288,7 +285,7 @@ public class GeoDistanceIT extends ESIntegTestCase { // Order: Desc searchResponse = client().prepareSearch("test").setQuery(matchAllQuery()) - .addSort(SortBuilders.geoDistanceSort("locations").point(40.7143528, -74.0059731).order(SortOrder.DESC)).execute() + .addSort(SortBuilders.geoDistanceSort("locations", 40.7143528, -74.0059731).order(SortOrder.DESC)).execute() .actionGet(); assertHitCount(searchResponse, 5); @@ -301,7 +298,7 @@ public class GeoDistanceIT extends ESIntegTestCase { // Order: Desc, Mode: min searchResponse = client().prepareSearch("test").setQuery(matchAllQuery()) - .addSort(SortBuilders.geoDistanceSort("locations").point(40.7143528, -74.0059731).order(SortOrder.DESC).sortMode("min")) + .addSort(SortBuilders.geoDistanceSort("locations", 40.7143528, -74.0059731).order(SortOrder.DESC).sortMode("min")) .execute().actionGet(); assertHitCount(searchResponse, 5); @@ -313,7 +310,7 @@ public class GeoDistanceIT extends ESIntegTestCase { assertThat(((Number) searchResponse.getHits().getAt(4).sortValues()[0]).doubleValue(), closeTo(0d, 10d)); searchResponse = client().prepareSearch("test").setQuery(matchAllQuery()) - .addSort(SortBuilders.geoDistanceSort("locations").point(40.7143528, -74.0059731).sortMode("avg").order(SortOrder.ASC)) + .addSort(SortBuilders.geoDistanceSort("locations", 40.7143528, -74.0059731).sortMode("avg").order(SortOrder.ASC)) .execute().actionGet(); assertHitCount(searchResponse, 5); @@ -325,7 +322,7 @@ public class GeoDistanceIT extends ESIntegTestCase { assertThat(((Number) searchResponse.getHits().getAt(4).sortValues()[0]).doubleValue(), closeTo(5301d, 10d)); searchResponse = client().prepareSearch("test").setQuery(matchAllQuery()) - .addSort(SortBuilders.geoDistanceSort("locations").point(40.7143528, -74.0059731).sortMode("avg").order(SortOrder.DESC)) + .addSort(SortBuilders.geoDistanceSort("locations", 40.7143528, -74.0059731).sortMode("avg").order(SortOrder.DESC)) .execute().actionGet(); assertHitCount(searchResponse, 5); @@ -336,10 +333,13 @@ public class GeoDistanceIT extends ESIntegTestCase { assertThat(((Number) searchResponse.getHits().getAt(3).sortValues()[0]).doubleValue(), closeTo(421.2d, 10d)); assertThat(((Number) searchResponse.getHits().getAt(4).sortValues()[0]).doubleValue(), closeTo(0d, 10d)); - assertFailures( + try { client().prepareSearch("test").setQuery(matchAllQuery()) - .addSort(SortBuilders.geoDistanceSort("locations").point(40.7143528, -74.0059731).sortMode("sum")), - RestStatus.BAD_REQUEST, containsString("sort_mode [sum] isn't supported for sorting by geo distance")); + .addSort(SortBuilders.geoDistanceSort("locations", 40.7143528, -74.0059731).sortMode("sum")); + fail("sum should not be supported for sorting by geo distance"); + } catch (IllegalArgumentException e) { + // expected + } } // Regression bug: @@ -371,7 +371,7 @@ public class GeoDistanceIT extends ESIntegTestCase { // Order: Asc SearchResponse searchResponse = client().prepareSearch("test").setQuery(matchAllQuery()) - .addSort(SortBuilders.geoDistanceSort("locations").point(40.7143528, -74.0059731).order(SortOrder.ASC)).execute() + .addSort(SortBuilders.geoDistanceSort("locations", 40.7143528, -74.0059731).order(SortOrder.ASC)).execute() .actionGet(); assertHitCount(searchResponse, 2); @@ -381,7 +381,7 @@ public class GeoDistanceIT extends ESIntegTestCase { // Order: Desc searchResponse = client().prepareSearch("test").setQuery(matchAllQuery()) - .addSort(SortBuilders.geoDistanceSort("locations").point(40.7143528, -74.0059731).order(SortOrder.DESC)).execute() + .addSort(SortBuilders.geoDistanceSort("locations", 40.7143528, -74.0059731).order(SortOrder.DESC)).execute() .actionGet(); // Doc with missing geo point is first, is consistent with 0.20.x @@ -444,7 +444,7 @@ public class GeoDistanceIT extends ESIntegTestCase { // Order: Asc SearchResponse searchResponse = client().prepareSearch("companies").setQuery(matchAllQuery()).addSort(SortBuilders - .geoDistanceSort("branches.location").point(40.7143528, -74.0059731).order(SortOrder.ASC).setNestedPath("branches")) + .geoDistanceSort("branches.location", 40.7143528, -74.0059731).order(SortOrder.ASC).setNestedPath("branches")) .execute().actionGet(); assertHitCount(searchResponse, 4); @@ -456,8 +456,8 @@ public class GeoDistanceIT extends ESIntegTestCase { // Order: Asc, Mode: max searchResponse = client() - .prepareSearch("companies").setQuery(matchAllQuery()).addSort(SortBuilders.geoDistanceSort("branches.location") - .point(40.7143528, -74.0059731).order(SortOrder.ASC).sortMode("max").setNestedPath("branches")) + .prepareSearch("companies").setQuery(matchAllQuery()).addSort(SortBuilders.geoDistanceSort("branches.location", + 40.7143528, -74.0059731).order(SortOrder.ASC).sortMode("max").setNestedPath("branches")) .execute().actionGet(); assertHitCount(searchResponse, 4); @@ -469,7 +469,7 @@ public class GeoDistanceIT extends ESIntegTestCase { // Order: Desc searchResponse = client().prepareSearch("companies").setQuery(matchAllQuery()).addSort(SortBuilders - .geoDistanceSort("branches.location").point(40.7143528, -74.0059731).order(SortOrder.DESC).setNestedPath("branches")) + .geoDistanceSort("branches.location", 40.7143528, -74.0059731).order(SortOrder.DESC).setNestedPath("branches")) .execute().actionGet(); assertHitCount(searchResponse, 4); @@ -481,8 +481,8 @@ public class GeoDistanceIT extends ESIntegTestCase { // Order: Desc, Mode: min searchResponse = client() - .prepareSearch("companies").setQuery(matchAllQuery()).addSort(SortBuilders.geoDistanceSort("branches.location") - .point(40.7143528, -74.0059731).order(SortOrder.DESC).sortMode("min").setNestedPath("branches")) + .prepareSearch("companies").setQuery(matchAllQuery()).addSort(SortBuilders.geoDistanceSort("branches.location", + 40.7143528, -74.0059731).order(SortOrder.DESC).sortMode("min").setNestedPath("branches")) .execute().actionGet(); assertHitCount(searchResponse, 4); @@ -493,8 +493,8 @@ public class GeoDistanceIT extends ESIntegTestCase { assertThat(((Number) searchResponse.getHits().getAt(3).sortValues()[0]).doubleValue(), closeTo(0d, 10d)); searchResponse = client() - .prepareSearch("companies").setQuery(matchAllQuery()).addSort(SortBuilders.geoDistanceSort("branches.location") - .point(40.7143528, -74.0059731).sortMode("avg").order(SortOrder.ASC).setNestedPath("branches")) + .prepareSearch("companies").setQuery(matchAllQuery()).addSort(SortBuilders.geoDistanceSort("branches.location", + 40.7143528, -74.0059731).sortMode("avg").order(SortOrder.ASC).setNestedPath("branches")) .execute().actionGet(); assertHitCount(searchResponse, 4); @@ -505,8 +505,8 @@ public class GeoDistanceIT extends ESIntegTestCase { assertThat(((Number) searchResponse.getHits().getAt(3).sortValues()[0]).doubleValue(), closeTo(5301.0d, 10d)); searchResponse = client().prepareSearch("companies") - .setQuery(matchAllQuery()).addSort(SortBuilders.geoDistanceSort("branches.location").setNestedPath("branches") - .point(40.7143528, -74.0059731).sortMode("avg").order(SortOrder.DESC).setNestedPath("branches")) + .setQuery(matchAllQuery()).addSort(SortBuilders.geoDistanceSort("branches.location", 40.7143528, -74.0059731) + .setNestedPath("branches").sortMode("avg").order(SortOrder.DESC).setNestedPath("branches")) .execute().actionGet(); assertHitCount(searchResponse, 4); @@ -517,8 +517,9 @@ public class GeoDistanceIT extends ESIntegTestCase { assertThat(((Number) searchResponse.getHits().getAt(3).sortValues()[0]).doubleValue(), closeTo(0d, 10d)); searchResponse = client().prepareSearch("companies").setQuery(matchAllQuery()) - .addSort(SortBuilders.geoDistanceSort("branches.location").setNestedFilter(termQuery("branches.name", "brooklyn")) - .point(40.7143528, -74.0059731).sortMode("avg").order(SortOrder.ASC).setNestedPath("branches")) + .addSort(SortBuilders.geoDistanceSort("branches.location", 40.7143528, -74.0059731) + .setNestedFilter(termQuery("branches.name", "brooklyn")) + .sortMode("avg").order(SortOrder.ASC).setNestedPath("branches")) .execute().actionGet(); assertHitCount(searchResponse, 4); assertFirstHit(searchResponse, hasId("4")); @@ -528,11 +529,14 @@ public class GeoDistanceIT extends ESIntegTestCase { assertThat(((Number) searchResponse.getHits().getAt(2).sortValues()[0]).doubleValue(), equalTo(Double.MAX_VALUE)); assertThat(((Number) searchResponse.getHits().getAt(3).sortValues()[0]).doubleValue(), equalTo(Double.MAX_VALUE)); - assertFailures( + try { client().prepareSearch("companies").setQuery(matchAllQuery()) - .addSort(SortBuilders.geoDistanceSort("branches.location").point(40.7143528, -74.0059731).sortMode("sum") - .setNestedPath("branches")), - RestStatus.BAD_REQUEST, containsString("sort_mode [sum] isn't supported for sorting by geo distance")); + .addSort(SortBuilders.geoDistanceSort("branches.location", 40.7143528, -74.0059731).sortMode("sum") + .setNestedPath("branches")); + fail("Sum should not be allowed as sort mode"); + } catch (IllegalArgumentException e) { + //expected + } } /** diff --git a/core/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderIT.java b/core/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderIT.java index 10053c40406..69f7431e371 100644 --- a/core/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderIT.java +++ b/core/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderIT.java @@ -95,7 +95,7 @@ public class GeoDistanceSortBuilderIT extends ESIntegTestCase { SearchResponse searchResponse = client().prepareSearch() .setQuery(matchAllQuery()) - .addSort(new GeoDistanceSortBuilder("location").points(q).sortMode("min").order(SortOrder.ASC).geoDistance(GeoDistance.PLANE).unit(DistanceUnit.KILOMETERS)) + .addSort(new GeoDistanceSortBuilder("location", q).sortMode("min").order(SortOrder.ASC).geoDistance(GeoDistance.PLANE).unit(DistanceUnit.KILOMETERS)) .execute().actionGet(); assertOrderedSearchHits(searchResponse, "d1", "d2"); assertThat((Double)searchResponse.getHits().getAt(0).getSortValues()[0], closeTo(GeoDistance.PLANE.calculate(2, 2, 3, 2, DistanceUnit.KILOMETERS), 0.01d)); @@ -103,7 +103,7 @@ public class GeoDistanceSortBuilderIT extends ESIntegTestCase { searchResponse = client().prepareSearch() .setQuery(matchAllQuery()) - .addSort(new GeoDistanceSortBuilder("location").points(q).sortMode("min").order(SortOrder.DESC).geoDistance(GeoDistance.PLANE).unit(DistanceUnit.KILOMETERS)) + .addSort(new GeoDistanceSortBuilder("location", q).sortMode("min").order(SortOrder.DESC).geoDistance(GeoDistance.PLANE).unit(DistanceUnit.KILOMETERS)) .execute().actionGet(); assertOrderedSearchHits(searchResponse, "d2", "d1"); assertThat((Double)searchResponse.getHits().getAt(0).getSortValues()[0], closeTo(GeoDistance.PLANE.calculate(2, 1, 5, 1, DistanceUnit.KILOMETERS), 0.01d)); @@ -111,7 +111,7 @@ public class GeoDistanceSortBuilderIT extends ESIntegTestCase { searchResponse = client().prepareSearch() .setQuery(matchAllQuery()) - .addSort(new GeoDistanceSortBuilder("location").points(q).sortMode("max").order(SortOrder.ASC).geoDistance(GeoDistance.PLANE).unit(DistanceUnit.KILOMETERS)) + .addSort(new GeoDistanceSortBuilder("location", q).sortMode("max").order(SortOrder.ASC).geoDistance(GeoDistance.PLANE).unit(DistanceUnit.KILOMETERS)) .execute().actionGet(); assertOrderedSearchHits(searchResponse, "d1", "d2"); assertThat((Double)searchResponse.getHits().getAt(0).getSortValues()[0], closeTo(GeoDistance.PLANE.calculate(2, 2, 4, 1, DistanceUnit.KILOMETERS), 0.01d)); @@ -119,7 +119,7 @@ public class GeoDistanceSortBuilderIT extends ESIntegTestCase { searchResponse = client().prepareSearch() .setQuery(matchAllQuery()) - .addSort(new GeoDistanceSortBuilder("location").points(q).sortMode("max").order(SortOrder.DESC).geoDistance(GeoDistance.PLANE).unit(DistanceUnit.KILOMETERS)) + .addSort(new GeoDistanceSortBuilder("location", q).sortMode("max").order(SortOrder.DESC).geoDistance(GeoDistance.PLANE).unit(DistanceUnit.KILOMETERS)) .execute().actionGet(); assertOrderedSearchHits(searchResponse, "d2", "d1"); assertThat((Double)searchResponse.getHits().getAt(0).getSortValues()[0], closeTo(GeoDistance.PLANE.calculate(2, 1, 6, 2, DistanceUnit.KILOMETERS), 0.01d)); @@ -139,6 +139,7 @@ public class GeoDistanceSortBuilderIT extends ESIntegTestCase { builder.endObject(); } + @SuppressWarnings("deprecation") public void testManyToManyGeoPointsWithDifferentFormats() throws ExecutionException, InterruptedException, IOException { /** q d1 d2 * |4 o| x | x @@ -171,13 +172,21 @@ public class GeoDistanceSortBuilderIT extends ESIntegTestCase { List qPoints = new ArrayList<>(); createQPoints(qHashes, qPoints); - GeoDistanceSortBuilder geoDistanceSortBuilder = new GeoDistanceSortBuilder("location"); + GeoDistanceSortBuilder geoDistanceSortBuilder = null; for (int i = 0; i < 4; i++) { int at = randomInt(3 - i); if (randomBoolean()) { - geoDistanceSortBuilder.geohashes(qHashes.get(at)); + if (geoDistanceSortBuilder == null) { + geoDistanceSortBuilder = new GeoDistanceSortBuilder("location", qHashes.get(at)); + } else { + geoDistanceSortBuilder.geohashes(qHashes.get(at)); + } } else { - geoDistanceSortBuilder.points(qPoints.get(at)); + if (geoDistanceSortBuilder == null) { + geoDistanceSortBuilder = new GeoDistanceSortBuilder("location", qPoints.get(at)); + } else { + geoDistanceSortBuilder.points(qPoints.get(at)); + } } qHashes.remove(at); qPoints.remove(at); @@ -210,8 +219,7 @@ public class GeoDistanceSortBuilderIT extends ESIntegTestCase { String hashPoint = "s037ms06g7h0"; - GeoDistanceSortBuilder geoDistanceSortBuilder = new GeoDistanceSortBuilder("location"); - geoDistanceSortBuilder.geohashes(hashPoint); + GeoDistanceSortBuilder geoDistanceSortBuilder = new GeoDistanceSortBuilder("location", hashPoint); SearchResponse searchResponse = client().prepareSearch() .setQuery(matchAllQuery()) @@ -219,8 +227,7 @@ public class GeoDistanceSortBuilderIT extends ESIntegTestCase { .execute().actionGet(); checkCorrectSortOrderForGeoSort(searchResponse); - geoDistanceSortBuilder = new GeoDistanceSortBuilder("location"); - geoDistanceSortBuilder.points(new GeoPoint(2, 2)); + geoDistanceSortBuilder = new GeoDistanceSortBuilder("location", new GeoPoint(2, 2)); searchResponse = client().prepareSearch() .setQuery(matchAllQuery()) @@ -228,8 +235,7 @@ public class GeoDistanceSortBuilderIT extends ESIntegTestCase { .execute().actionGet(); checkCorrectSortOrderForGeoSort(searchResponse); - geoDistanceSortBuilder = new GeoDistanceSortBuilder("location"); - geoDistanceSortBuilder.point(2, 2); + geoDistanceSortBuilder = new GeoDistanceSortBuilder("location", 2, 2); searchResponse = client().prepareSearch() .setQuery(matchAllQuery()) @@ -240,21 +246,21 @@ public class GeoDistanceSortBuilderIT extends ESIntegTestCase { searchResponse = client() .prepareSearch() .setSource( - new SearchSourceBuilder().sort(SortBuilders.geoDistanceSort("location").point(2.0, 2.0) + new SearchSourceBuilder().sort(SortBuilders.geoDistanceSort("location", 2.0, 2.0) .unit(DistanceUnit.KILOMETERS).geoDistance(GeoDistance.PLANE))).execute().actionGet(); checkCorrectSortOrderForGeoSort(searchResponse); searchResponse = client() .prepareSearch() .setSource( - new SearchSourceBuilder().sort(SortBuilders.geoDistanceSort("location").geohashes("s037ms06g7h0") + new SearchSourceBuilder().sort(SortBuilders.geoDistanceSort("location", "s037ms06g7h0") .unit(DistanceUnit.KILOMETERS).geoDistance(GeoDistance.PLANE))).execute().actionGet(); checkCorrectSortOrderForGeoSort(searchResponse); searchResponse = client() .prepareSearch() .setSource( - new SearchSourceBuilder().sort(SortBuilders.geoDistanceSort("location").point(2.0, 2.0) + new SearchSourceBuilder().sort(SortBuilders.geoDistanceSort("location", 2.0, 2.0) .unit(DistanceUnit.KILOMETERS).geoDistance(GeoDistance.PLANE))).execute().actionGet(); checkCorrectSortOrderForGeoSort(searchResponse); } diff --git a/core/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderTests.java b/core/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderTests.java new file mode 100644 index 00000000000..e957db58b38 --- /dev/null +++ b/core/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderTests.java @@ -0,0 +1,252 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.search.sort; + + +import org.elasticsearch.common.bytes.BytesArray; +import org.elasticsearch.common.geo.GeoDistance; +import org.elasticsearch.common.geo.GeoPoint; +import org.elasticsearch.common.unit.DistanceUnit; +import org.elasticsearch.common.xcontent.XContentHelper; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.index.query.QueryParseContext; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.test.geo.RandomGeoGenerator; + +import java.io.IOException; +import java.util.Arrays; + +public class GeoDistanceSortBuilderTests extends AbstractSortTestCase { + + @Override + protected GeoDistanceSortBuilder createTestItem() { + String fieldName = randomAsciiOfLengthBetween(1, 10); + GeoDistanceSortBuilder result = null; + + int id = randomIntBetween(0, 2); + switch(id) { + case 0: + int count = randomIntBetween(1, 10); + String[] geohashes = new String[count]; + for (int i = 0; i < count; i++) { + geohashes[i] = RandomGeoGenerator.randomPoint(getRandom()).geohash(); + } + + result = new GeoDistanceSortBuilder(fieldName, geohashes); + break; + case 1: + GeoPoint pt = RandomGeoGenerator.randomPoint(getRandom()); + result = new GeoDistanceSortBuilder(fieldName, pt.getLat(), pt.getLon()); + break; + case 2: + result = new GeoDistanceSortBuilder(fieldName, points(new GeoPoint[0])); + break; + default: + throw new IllegalStateException("one of three geo initialisation strategies must be used"); + + } + if (randomBoolean()) { + result.geoDistance(geoDistance(result.geoDistance())); + } + if (randomBoolean()) { + result.unit(unit(result.unit())); + } + if (randomBoolean()) { + result.order(RandomSortDataGenerator.order(result.order())); + } + if (randomBoolean()) { + result.sortMode(mode(result.sortMode())); + } + if (randomBoolean()) { + result.setNestedFilter(RandomSortDataGenerator.nestedFilter(result.getNestedFilter())); + } + if (randomBoolean()) { + result.setNestedPath(RandomSortDataGenerator.randomAscii(result.getNestedPath())); + } + if (randomBoolean()) { + result.coerce(! result.coerce()); + } + if (randomBoolean()) { + result.ignoreMalformed(! result.ignoreMalformed()); + } + + return result; + } + + private static String mode(String original) { + String[] modes = {"MIN", "MAX", "AVG"}; + String mode = ESTestCase.randomFrom(modes); + while (mode.equals(original)) { + mode = ESTestCase.randomFrom(modes); + } + return mode; + } + + private DistanceUnit unit(DistanceUnit original) { + int id = -1; + while (id == -1 || (original != null && original.ordinal() == id)) { + id = randomIntBetween(0, DistanceUnit.values().length - 1); + } + return DistanceUnit.values()[id]; + } + + private GeoPoint[] points(GeoPoint[] original) { + GeoPoint[] result = null; + while (result == null || Arrays.deepEquals(original, result)) { + int count = randomIntBetween(1, 10); + result = new GeoPoint[count]; + for (int i = 0; i < count; i++) { + result[i] = RandomGeoGenerator.randomPoint(getRandom()); + } + } + return result; + } + + private GeoDistance geoDistance(GeoDistance original) { + int id = -1; + while (id == -1 || (original != null && original.ordinal() == id)) { + id = randomIntBetween(0, GeoDistance.values().length - 1); + } + return GeoDistance.values()[id]; + } + + @Override + protected GeoDistanceSortBuilder mutate(GeoDistanceSortBuilder original) throws IOException { + GeoDistanceSortBuilder result = new GeoDistanceSortBuilder(original); + int parameter = randomIntBetween(0, 9); + switch (parameter) { + case 0: + while (Arrays.deepEquals(original.points(), result.points())) { + GeoPoint pt = RandomGeoGenerator.randomPoint(getRandom()); + result.point(pt.getLat(), pt.getLon()); + } + break; + case 1: + result.points(points(original.points())); + break; + case 2: + result.geoDistance(geoDistance(original.geoDistance())); + break; + case 3: + result.unit(unit(original.unit())); + break; + case 4: + result.order(RandomSortDataGenerator.order(original.order())); + break; + case 5: + result.sortMode(mode(original.sortMode())); + break; + case 6: + result.setNestedFilter(RandomSortDataGenerator.nestedFilter(original.getNestedFilter())); + break; + case 7: + result.setNestedPath(RandomSortDataGenerator.randomAscii(original.getNestedPath())); + break; + case 8: + result.coerce(! original.coerce()); + break; + case 9: + // ignore malformed will only be set if coerce is set to true + result.coerce(false); + result.ignoreMalformed(! original.ignoreMalformed()); + break; + } + return result; + + } + + @SuppressWarnings("unchecked") + @Override + protected Class getPrototype() { + return (Class) GeoDistanceSortBuilder.PROTOTYPE.getClass(); + } + + public void testSortModeSumIsRejectedInSetter() { + GeoDistanceSortBuilder builder = new GeoDistanceSortBuilder("testname", -1, -1); + GeoPoint point = RandomGeoGenerator.randomPoint(getRandom()); + builder.point(point.getLat(), point.getLon()); + try { + builder.sortMode("SUM"); + fail("sort mode sum should not be supported"); + } catch (IllegalArgumentException e) { + // all good + } + } + + public void testSortModeSumIsRejectedInJSON() throws IOException { + String json = "{\n" + + " \"testname\" : [ {\n" + + " \"lat\" : -6.046997540714173,\n" + + " \"lon\" : -51.94128329747579\n" + + " } ],\n" + + " \"unit\" : \"m\",\n" + + " \"distance_type\" : \"sloppy_arc\",\n" + + " \"reverse\" : true,\n" + + " \"mode\" : \"SUM\",\n" + + " \"coerce\" : false,\n" + + " \"ignore_malformed\" : false\n" + + "}"; + XContentParser itemParser = XContentHelper.createParser(new BytesArray(json)); + itemParser.nextToken(); + + QueryParseContext context = new QueryParseContext(indicesQueriesRegistry); + context.reset(itemParser); + + try { + GeoDistanceSortBuilder.PROTOTYPE.fromXContent(context, ""); + fail("sort mode sum should not be supported"); + } catch (IllegalArgumentException e) { + // all good + } + } + + public void testGeoDistanceSortCanBeParsedFromGeoHash() throws IOException { + String json = "{\n" + + " \"VDcvDuFjE\" : [ \"7umzzv8eychg\", \"dmdgmt5z13uw\", " + + " \"ezu09wxw6v4c\", \"kc7s3515p6k6\", \"jgeuvjwrmfzn\", \"kcpcfj7ruyf8\" ],\n" + + " \"unit\" : \"m\",\n" + + " \"distance_type\" : \"sloppy_arc\",\n" + + " \"reverse\" : true,\n" + + " \"mode\" : \"MAX\",\n" + + " \"nested_filter\" : {\n" + + " \"ids\" : {\n" + + " \"type\" : [ ],\n" + + " \"values\" : [ ],\n" + + " \"boost\" : 5.711116\n" + + " }\n" + + " },\n" + + " \"coerce\" : false,\n" + + " \"ignore_malformed\" : true\n" + + " }"; + XContentParser itemParser = XContentHelper.createParser(new BytesArray(json)); + itemParser.nextToken(); + + QueryParseContext context = new QueryParseContext(indicesQueriesRegistry); + context.reset(itemParser); + + GeoDistanceSortBuilder result = GeoDistanceSortBuilder.PROTOTYPE.fromXContent(context, json); + assertEquals("[-19.700583312660456, -2.8225036337971687, " + + "31.537466906011105, -74.63590376079082, " + + "43.71844606474042, -5.548660643398762, " + + "-37.20467280596495, 38.71751043945551, " + + "-69.44606635719538, 84.25200328230858, " + + "-39.03717711567879, 44.74099852144718]", Arrays.toString(result.points())); + } +} diff --git a/core/src/test/java/org/elasticsearch/search/sort/RandomSortDataGenerator.java b/core/src/test/java/org/elasticsearch/search/sort/RandomSortDataGenerator.java new file mode 100644 index 00000000000..fcd5284119c --- /dev/null +++ b/core/src/test/java/org/elasticsearch/search/sort/RandomSortDataGenerator.java @@ -0,0 +1,113 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.search.sort; + +import org.apache.lucene.util.BytesRef; +import org.elasticsearch.index.query.IdsQueryBuilder; +import org.elasticsearch.index.query.MatchAllQueryBuilder; +import org.elasticsearch.index.query.QueryBuilder; +import org.elasticsearch.index.query.TermQueryBuilder; +import org.elasticsearch.test.ESTestCase; + +public class RandomSortDataGenerator { + private RandomSortDataGenerator() { + // this is a helper class only, doesn't need a constructor + } + + public static QueryBuilder nestedFilter(QueryBuilder original) { + @SuppressWarnings("rawtypes") + QueryBuilder nested = null; + while (nested == null || nested.equals(original)) { + switch (ESTestCase.randomInt(2)) { + case 0: + nested = new MatchAllQueryBuilder(); + break; + case 1: + nested = new IdsQueryBuilder(); + break; + default: + case 2: + nested = new TermQueryBuilder(ESTestCase.randomAsciiOfLengthBetween(1, 10), ESTestCase.randomAsciiOfLengthBetween(1, 10)); + break; + } + nested.boost((float) ESTestCase.randomDoubleBetween(0, 10, false)); + } + return nested; + } + + public static String randomAscii(String original) { + String nestedPath = ESTestCase.randomAsciiOfLengthBetween(1, 10); + while (nestedPath.equals(original)) { + nestedPath = ESTestCase.randomAsciiOfLengthBetween(1, 10); + } + return nestedPath; + } + + public static String mode(String original) { + String[] modes = {"min", "max", "avg", "sum"}; + String mode = ESTestCase.randomFrom(modes); + while (mode.equals(original)) { + mode = ESTestCase.randomFrom(modes); + } + return mode; + } + + public static Object missing(Object original) { + Object missing = null; + Object otherMissing = null; + if (original instanceof BytesRef) { + otherMissing = ((BytesRef) original).utf8ToString(); + } else { + otherMissing = original; + } + + while (missing == null || missing.equals(otherMissing)) { + int missingId = ESTestCase.randomIntBetween(0, 3); + switch (missingId) { + case 0: + missing = ("_last"); + break; + case 1: + missing = ("_first"); + break; + case 2: + missing = ESTestCase.randomAsciiOfLength(10); + break; + case 3: + missing = ESTestCase.randomInt(); + break; + default: + throw new IllegalStateException("Unknown missing type."); + + } + } + return missing; + } + + public static SortOrder order(SortOrder original) { + SortOrder order = SortOrder.ASC; + if (order.equals(original)) { + return SortOrder.DESC; + } else { + return SortOrder.ASC; + } + } + +}