From b4db26eaf9a07948e4da8d1197aadfb414699562 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Tue, 8 Mar 2016 18:37:20 +0100 Subject: [PATCH] Sort: Move up `order` field to SortBuilder Currently all SortBuilder implementations have their separate order field. This PR moves this up to SortBuilder, together with setter and getter and makes sure the default is set to SortOrder.ASC except for `_score` sorting where the default is SortOrder.DESC. --- .../search/sort/FieldSortBuilder.java | 17 +--- .../search/sort/GeoDistanceSortBuilder.java | 77 +++++++------------ .../search/sort/ScoreSortBuilder.java | 27 ++----- .../search/sort/ScriptSortBuilder.java | 19 +---- .../search/sort/SortBuilder.java | 24 +++++- .../builder/SearchSourceBuilderTests.java | 8 +- 6 files changed, 61 insertions(+), 111 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java b/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java index 67ceb75a29c..e805e21eff5 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java @@ -27,12 +27,10 @@ import java.io.IOException; /** * A sort builder to sort based on a document field. */ -public class FieldSortBuilder extends SortBuilder { +public class FieldSortBuilder extends SortBuilder { private final String fieldName; - private SortOrder order; - private Object missing; private String unmappedType; @@ -55,15 +53,6 @@ public class FieldSortBuilder extends SortBuilder { this.fieldName = fieldName; } - /** - * The order of sorting. Defaults to {@link SortOrder#ASC}. - */ - @Override - public FieldSortBuilder order(SortOrder order) { - this.order = order; - return this; - } - /** * Sets the value when a field is missing in a doc. Can also be set to _last or * _first to sort missing last or first respectively. @@ -118,9 +107,7 @@ public class FieldSortBuilder extends SortBuilder { @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(fieldName); - if (order != null) { - builder.field("order", order.toString()); - } + builder.field(ORDER_FIELD.getPreferredName(), order); if (missing != null) { builder.field("missing", missing); } 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 708152af1f0..b5a10e238b7 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java @@ -44,7 +44,7 @@ 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; @@ -57,14 +57,13 @@ public class GeoDistanceSortBuilder extends SortBuilder 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; - + // TODO switch to GeoValidationMethod enum private boolean coerce = DEFAULT_COERCE; private boolean ignoreMalformed = DEFAULT_IGNORE_MALFORMED; @@ -109,7 +108,7 @@ public class GeoDistanceSortBuilder extends SortBuilder } this.fieldName = fieldName; } - + /** * Copy constructor. * */ @@ -125,7 +124,7 @@ public class GeoDistanceSortBuilder extends SortBuilder this.coerce = original.coerce; this.ignoreMalformed = original.ignoreMalformed; } - + /** * Returns the geo point like field the distance based sort operates on. * */ @@ -153,7 +152,7 @@ public class GeoDistanceSortBuilder extends SortBuilder this.points.addAll(Arrays.asList(points)); return this; } - + /** * Returns the points to create the range distance facets from. */ @@ -163,7 +162,7 @@ public class GeoDistanceSortBuilder extends SortBuilder /** * The geohash of the geo point to create the range distance facets from. - * + * * Deprecated - please use points(GeoPoint... points) instead. */ @Deprecated @@ -173,7 +172,7 @@ public class GeoDistanceSortBuilder extends SortBuilder } return this; } - + /** * The geo distance type used to compute the distance. */ @@ -181,7 +180,7 @@ public class GeoDistanceSortBuilder extends SortBuilder this.geoDistance = geoDistance; return this; } - + /** * Returns the geo distance type used to compute the distance. */ @@ -204,20 +203,6 @@ public class GeoDistanceSortBuilder extends SortBuilder return this.unit; } - /** - * The order of sorting. Defaults to {@link SortOrder#ASC}. - */ - @Override - public GeoDistanceSortBuilder order(SortOrder order) { - this.order = order; - return this; - } - - /** Returns the order of sorting. */ - public SortOrder order() { - return this.order; - } - /** * Defines which distance to use for sorting in the case a document contains multiple geo points. * Possible values: min and max @@ -240,16 +225,16 @@ public class GeoDistanceSortBuilder extends SortBuilder * Sets the nested filter that the nested objects should match with in order to be taken into account * for sorting. */ - public GeoDistanceSortBuilder setNestedFilter(QueryBuilder nestedFilter) { + public GeoDistanceSortBuilder setNestedFilter(QueryBuilder nestedFilter) { 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. + * for sorting. **/ - public QueryBuilder getNestedFilter() { + public QueryBuilder getNestedFilter() { return this.nestedFilter; } @@ -261,7 +246,7 @@ 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. @@ -285,7 +270,7 @@ public class GeoDistanceSortBuilder extends SortBuilder } return this; } - + public boolean ignoreMalformed() { return this.ignoreMalformed; } @@ -302,11 +287,7 @@ public class GeoDistanceSortBuilder extends SortBuilder 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); - } + builder.field(ORDER_FIELD.getPreferredName(), order); if (sortMode != null) { builder.field("mode", sortMode); @@ -363,7 +344,7 @@ public class GeoDistanceSortBuilder extends SortBuilder public void writeTo(StreamOutput out) throws IOException { out.writeString(fieldName); out.writeGenericValue(points); - + geoDistance.writeTo(out); unit.writeTo(out); order.writeTo(out); @@ -382,10 +363,10 @@ public class GeoDistanceSortBuilder extends SortBuilder @Override public GeoDistanceSortBuilder readFrom(StreamInput in) throws IOException { String fieldName = in.readString(); - - ArrayList points = (ArrayList) in.readGenericValue(); + + 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)); @@ -409,9 +390,9 @@ public class GeoDistanceSortBuilder extends SortBuilder List geoPoints = new ArrayList<>(); DistanceUnit unit = DistanceUnit.DEFAULT; GeoDistance geoDistance = GeoDistance.DEFAULT; - boolean reverse = false; + SortOrder order = SortOrder.ASC; MultiValueMode sortMode = null; - QueryBuilder nestedFilter = null; + QueryBuilder nestedFilter = null; String nestedPath = null; boolean coerce = GeoDistanceSortBuilder.DEFAULT_COERCE; @@ -429,8 +410,8 @@ public class GeoDistanceSortBuilder extends SortBuilder } 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 + // 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 { @@ -441,9 +422,9 @@ public class GeoDistanceSortBuilder extends SortBuilder } } else if (token.isValue()) { if ("reverse".equals(currentName)) { - reverse = parser.booleanValue(); + order = parser.booleanValue() ? SortOrder.DESC : SortOrder.ASC; } else if ("order".equals(currentName)) { - reverse = "desc".equals(parser.text()); + order = SortOrder.fromString(parser.text()); } else if ("unit".equals(currentName)) { unit = DistanceUnit.fromString(parser.text()); } else if ("distance_type".equals(currentName) || "distanceType".equals(currentName)) { @@ -474,11 +455,7 @@ public class GeoDistanceSortBuilder extends SortBuilder 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); - } + result.order(order); if (sortMode != null) { result.sortMode(sortMode.name()); } diff --git a/core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java b/core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java index 5d1a0d82987..6b1bc054ee7 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java @@ -35,38 +35,24 @@ import java.util.Objects; /** * A sort builder allowing to sort by score. */ -public class ScoreSortBuilder extends SortBuilder implements NamedWriteable, +public class ScoreSortBuilder extends SortBuilder implements NamedWriteable, SortElementParserTemp { private static final String NAME = "_score"; static final ScoreSortBuilder PROTOTYPE = new ScoreSortBuilder(); public static final ParseField REVERSE_FIELD = new ParseField("reverse"); public static final ParseField ORDER_FIELD = new ParseField("order"); - private SortOrder order = SortOrder.DESC; - /** - * The order of sort scoring. By default, its {@link SortOrder#DESC}. - */ - @Override - public ScoreSortBuilder order(SortOrder order) { - Objects.requireNonNull(order, "sort order cannot be null."); - this.order = order; - return this; + public ScoreSortBuilder() { + // order defaults to desc when sorting on the _score + order(SortOrder.DESC); } - /** - * Get the order of sort scoring. By default, its {@link SortOrder#DESC}. - */ - public SortOrder order() { - return this.order; - } @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(NAME); - if (order == SortOrder.ASC) { - builder.field(REVERSE_FIELD.getPreferredName(), true); - } + builder.field(ORDER_FIELD.getPreferredName(), order); builder.endObject(); return builder; } @@ -124,7 +110,8 @@ public class ScoreSortBuilder extends SortBuilder implements NamedWriteable { private Script script; private final String type; - private SortOrder order; - private String sortMode; private QueryBuilder nestedFilter; @@ -53,15 +51,6 @@ public class ScriptSortBuilder extends SortBuilder { this.type = type; } - /** - * Sets the sort order. - */ - @Override - public ScriptSortBuilder order(SortOrder order) { - this.order = order; - return this; - } - /** * Defines which distance to use for sorting in the case a document contains multiple geo points. * Possible values: min and max @@ -75,7 +64,7 @@ public class ScriptSortBuilder extends SortBuilder { * Sets the nested filter that the nested objects should match with in order to be taken into account * for sorting. */ - public ScriptSortBuilder setNestedFilter(QueryBuilder nestedFilter) { + public ScriptSortBuilder setNestedFilter(QueryBuilder nestedFilter) { this.nestedFilter = nestedFilter; return this; } @@ -94,9 +83,7 @@ public class ScriptSortBuilder extends SortBuilder { builder.startObject("_script"); builder.field("script", script); builder.field("type", type); - if (order == SortOrder.DESC) { - builder.field("reverse", true); - } + builder.field(ORDER_FIELD.getPreferredName(), order); if (sortMode != null) { builder.field("mode", sortMode); } diff --git a/core/src/main/java/org/elasticsearch/search/sort/SortBuilder.java b/core/src/main/java/org/elasticsearch/search/sort/SortBuilder.java index 0935b76ece9..7852af4e97e 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/SortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/SortBuilder.java @@ -20,14 +20,20 @@ package org.elasticsearch.search.sort; import org.elasticsearch.ElasticsearchException; +import org.elasticsearch.common.ParseField; import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; +import java.util.Objects; + /** * */ -public abstract class SortBuilder implements ToXContent { +public abstract class SortBuilder> implements ToXContent { + + protected SortOrder order = SortOrder.ASC; + public static final ParseField ORDER_FIELD = new ParseField("order"); @Override public String toString() { @@ -42,7 +48,19 @@ public abstract class SortBuilder implements ToXContent { } /** - * The order of sorting. Defaults to {@link SortOrder#ASC}. + * Set the order of sorting. */ - public abstract SortBuilder order(SortOrder order); + @SuppressWarnings("unchecked") + public T order(SortOrder order) { + Objects.requireNonNull(order, "sort order cannot be null."); + this.order = order; + return (T) this; + } + + /** + * Return the {@link SortOrder} used for this {@link SortBuilder}. + */ + public SortOrder order() { + return this.order; + } } 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 6b29cabe3f6..2cb4554ed08 100644 --- a/core/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java @@ -49,15 +49,9 @@ import org.elasticsearch.env.Environment; import org.elasticsearch.env.EnvironmentModule; import org.elasticsearch.index.Index; import org.elasticsearch.index.query.AbstractQueryTestCase; -import org.elasticsearch.index.query.BoolQueryBuilder; import org.elasticsearch.index.query.EmptyQueryBuilder; -import org.elasticsearch.index.query.MatchAllQueryBuilder; import org.elasticsearch.index.query.QueryBuilders; import org.elasticsearch.index.query.QueryParseContext; -import org.elasticsearch.index.query.QueryRewriteContext; -import org.elasticsearch.index.query.TermQueryBuilder; -import org.elasticsearch.index.query.WrapperQueryBuilder; -import org.elasticsearch.index.query.functionscore.ScoreFunctionParser; import org.elasticsearch.indices.IndicesModule; import org.elasticsearch.indices.breaker.CircuitBreakerService; import org.elasticsearch.indices.breaker.NoneCircuitBreakerService; @@ -554,7 +548,7 @@ public class SearchSourceBuilderTests extends ESTestCase { SearchSourceBuilder searchSourceBuilder = SearchSourceBuilder.parseSearchSource(parser, createParseContext(parser), aggParsers); assertEquals(1, searchSourceBuilder.sorts().size()); - assertEquals("{\"foo\":{}}", searchSourceBuilder.sorts().get(0).toUtf8()); + assertEquals("{\"foo\":{\"order\":\"asc\"}}", searchSourceBuilder.sorts().get(0).toUtf8()); } }