From 720f47e87f065dba680b5eb71abb67c56f8283c0 Mon Sep 17 00:00:00 2001 From: Isabel Drost-Fromm Date: Mon, 8 Feb 2016 15:26:08 +0100 Subject: [PATCH] Second round of comments --- .../indices/query/IndicesQueriesRegistry.java | 9 ++ .../search/sort/FieldSortBuilder.java | 97 +++++++++++-------- ...ameterParser.java => SortBuilderTemp.java} | 11 ++- .../elasticsearch/search/sort/SortOrder.java | 2 +- .../AbstractSearchSourceItemTestCase.java | 19 ++-- 5 files changed, 87 insertions(+), 51 deletions(-) rename core/src/main/java/org/elasticsearch/search/sort/{ParameterParser.java => SortBuilderTemp.java} (75%) diff --git a/core/src/main/java/org/elasticsearch/indices/query/IndicesQueriesRegistry.java b/core/src/main/java/org/elasticsearch/indices/query/IndicesQueriesRegistry.java index a9e90884a68..c72810212eb 100644 --- a/core/src/main/java/org/elasticsearch/indices/query/IndicesQueriesRegistry.java +++ b/core/src/main/java/org/elasticsearch/indices/query/IndicesQueriesRegistry.java @@ -24,9 +24,11 @@ import java.util.Map; import org.elasticsearch.common.component.AbstractComponent; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.query.QueryParser; +import org.elasticsearch.search.sort.SortBuilderTemp; public class IndicesQueriesRegistry extends AbstractComponent { private Map> queryParsers; + private Map> sortParsers; public IndicesQueriesRegistry(Settings settings, Map> queryParsers) { super(settings); @@ -39,4 +41,11 @@ public class IndicesQueriesRegistry extends AbstractComponent { public Map> queryParsers() { return queryParsers; } + + /** + * Returns all registered sort parsers + */ + public Map> sortParsers() { + return sortParsers; + } } 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 ff9192c039f..5440e996b50 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java @@ -21,12 +21,9 @@ package org.elasticsearch.search.sort; import org.apache.lucene.util.BytesRef; import org.elasticsearch.common.ParseField; -import org.elasticsearch.common.ParsingException; -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.lucene.BytesRefs; -import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.index.query.QueryBuilder; @@ -38,17 +35,17 @@ import java.util.Objects; /** * A sort builder to sort based on a document field. */ -public class FieldSortBuilder extends SortBuilder { +public class FieldSortBuilder extends SortBuilder implements SortBuilderTemp { + public static final String NAME = "field_sort"; static final FieldSortBuilder PROTOTYPE = new FieldSortBuilder(""); public static final String NAME = "field_sort"; public static final ParseField NESTED_PATH = new ParseField("nested_path"); public static final ParseField NESTED_FILTER = new ParseField("nested_filter"); public static final ParseField MISSING = new ParseField("missing"); public static final ParseField ORDER = new ParseField("order"); - public static final ParseField SORT_MODE = new ParseField("mode"); + public static final ParseField SORT_MODE = new ParseField("mode"); public static final ParseField UNMAPPED_TYPE = new ParseField("unmapped_type"); - private final String fieldName; private Object missing; @@ -71,10 +68,12 @@ public class FieldSortBuilder extends SortBuilder { this.setNestedFilter(template.getNestedFilter()); this.setNestedPath(template.getNestedPath()); } + /** * Constructs a new sort based on a document field. * - * @param fieldName The field name. + * @param fieldName + * The field name. */ public FieldSortBuilder(String fieldName) { if (fieldName == null) { @@ -82,7 +81,7 @@ public class FieldSortBuilder extends SortBuilder { } this.fieldName = fieldName; } - + /** Returns the document field this sort should be based on. */ public String getFieldName() { return this.fieldName; @@ -111,24 +110,29 @@ public class FieldSortBuilder extends SortBuilder { /** * Set the type to use in case the current field is not mapped in an index. - * Specifying a type tells Elasticsearch what type the sort values should have, which is important - * for cross-index search, if there are sort fields that exist on some indices only. - * If the unmapped type is null then query execution will fail if one or more indices - * don't have a mapping for the current field. + * Specifying a type tells Elasticsearch what type the sort values should + * have, which is important for cross-index search, if there are sort fields + * that exist on some indices only. If the unmapped type is null + * then query execution will fail if one or more indices don't have a + * mapping for the current field. */ public FieldSortBuilder unmappedType(String type) { this.unmappedType = type; return this; } - /** Returns the type to use in case the current field is not mapped in an index. */ + /** + * Returns the type to use in case the current field is not mapped in an + * index. + */ public String unmappedType() { return this.unmappedType; } /** - * Defines what values to pick in the case a document contains multiple values for the targeted sort field. - * Possible values: min, max, sum and avg + * Defines what values to pick in the case a document contains multiple + * values for the targeted sort field. Possible values: min, max, sum and + * avg * * TODO would love to see an enum here *

@@ -139,37 +143,48 @@ public class FieldSortBuilder extends SortBuilder { return this; } - /** Returns what values to pick in the case a document contains multiple values for the targeted sort field. */ + /** + * Returns what values to pick in the case a document contains multiple + * values for the targeted sort field. + */ 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. + * Sets the nested filter that the nested objects should match with in order + * to be taken into account for sorting. * - * TODO should the above getters and setters be deprecated/ changed in favour of real getters and setters? + * TODO should the above getters and setters be deprecated/ changed in + * favour of real getters and setters? */ public FieldSortBuilder 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. */ + /** + * 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. + * 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. */ public FieldSortBuilder setNestedPath(String nestedPath) { this.nestedPath = nestedPath; return this; } - /** Returns the nested path if sorting occurs in a field that is inside a nested object. */ + /** + * Returns the nested path if sorting occurs in a field that is inside a + * nested object. + */ public String getNestedPath() { return this.nestedPath; } @@ -207,24 +222,20 @@ public class FieldSortBuilder extends SortBuilder { return true; } - if (other== null || getClass() != other.getClass()) { + if (other == null || getClass() != other.getClass()) { return false; } FieldSortBuilder builder = (FieldSortBuilder) other; - return (Objects.equals(this.fieldName, builder.fieldName) && - Objects.equals(this.nestedFilter, builder.nestedFilter) && - Objects.equals(this.nestedPath, builder.nestedPath) && - Objects.equals(this.missing, builder.missing) && - Objects.equals(this.order, builder.order) && - Objects.equals(this.sortMode, builder.sortMode) && - Objects.equals(this.unmappedType, builder.unmappedType)); + return (Objects.equals(this.fieldName, builder.fieldName) && Objects.equals(this.nestedFilter, builder.nestedFilter) + && Objects.equals(this.nestedPath, builder.nestedPath) && Objects.equals(this.missing, builder.missing) + && Objects.equals(this.order, builder.order) && Objects.equals(this.sortMode, builder.sortMode) + && Objects.equals(this.unmappedType, builder.unmappedType)); } - + @Override public int hashCode() { - return Objects.hash(this.fieldName, this.nestedFilter, this.nestedPath, - this.missing, this.order, this.sortMode, this.unmappedType); + return Objects.hash(this.fieldName, this.nestedFilter, this.nestedPath, this.missing, this.order, this.sortMode, this.unmappedType); } @Override @@ -243,14 +254,14 @@ public class FieldSortBuilder extends SortBuilder { } out.writeOptionalString(this.nestedPath); out.writeGenericValue(this.missing); - + if (this.order != null) { out.writeBoolean(true); this.order.writeTo(out); } else { out.writeBoolean(false); } - + out.writeOptionalString(this.sortMode); out.writeOptionalString(this.unmappedType); } @@ -285,7 +296,7 @@ public class FieldSortBuilder extends SortBuilder { SortOrder order = null; String sortMode = null; String unmappedType = null; - + String currentFieldName = null; XContentParser.Token token; fieldName = elementName; @@ -345,4 +356,14 @@ public class FieldSortBuilder extends SortBuilder { return builder; } + @Override + public String getName() { + return "field_sort_builder"; + } + + @Override + public SortBuilderTemp getBuilderPrototype() { + return PROTOTYPE; + } + } diff --git a/core/src/main/java/org/elasticsearch/search/sort/ParameterParser.java b/core/src/main/java/org/elasticsearch/search/sort/SortBuilderTemp.java similarity index 75% rename from core/src/main/java/org/elasticsearch/search/sort/ParameterParser.java rename to core/src/main/java/org/elasticsearch/search/sort/SortBuilderTemp.java index 74f0628fcc6..15a12ec90f2 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/ParameterParser.java +++ b/core/src/main/java/org/elasticsearch/search/sort/SortBuilderTemp.java @@ -19,14 +19,15 @@ package org.elasticsearch.search.sort; +import org.elasticsearch.common.io.stream.NamedWriteable; import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.index.query.QueryParseContext; import java.io.IOException; -public interface ParameterParser { +public interface SortBuilderTemp extends NamedWriteable, ToXContent { /** - * Creates a new item from the json held by the {@link ParameterParser} + * Creates a new item from the json held by the {@link SortBuilderTemp} * in {@link org.elasticsearch.common.xcontent.XContent} format * * @param context @@ -35,5 +36,9 @@ public interface ParameterParser { * call * @return the new item */ - T fromXContent(QueryParseContext context, String elementName) throws IOException; + NamedWriteable fromXContent(QueryParseContext context, String elementName) throws IOException; + + String getName(); + + SortBuilderTemp getBuilderPrototype(); } 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 73e5ac55247..9c351880293 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/SortOrder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/SortOrder.java @@ -51,7 +51,7 @@ public enum SortOrder implements Writeable { } }; - private static final SortOrder PROTOTYPE = ASC; + private static final SortOrder PROTOTYPE = DESC; @Override public SortOrder readFrom(StreamInput in) throws IOException { diff --git a/core/src/test/java/org/elasticsearch/search/sort/AbstractSearchSourceItemTestCase.java b/core/src/test/java/org/elasticsearch/search/sort/AbstractSearchSourceItemTestCase.java index 896bce1843f..3e86a227986 100644 --- a/core/src/test/java/org/elasticsearch/search/sort/AbstractSearchSourceItemTestCase.java +++ b/core/src/test/java/org/elasticsearch/search/sort/AbstractSearchSourceItemTestCase.java @@ -42,8 +42,7 @@ import java.io.IOException; import static org.hamcrest.Matchers.*; -//TODO maybe merge with AbstractsortBuilderTestCase once #14933 is in? -public abstract class AbstractSearchSourceItemTestCase & ToXContent & ParameterParser> extends ESTestCase { +public abstract class AbstractSearchSourceItemTestCase> extends ESTestCase { protected static NamedWriteableRegistry namedWriteableRegistry; @@ -146,17 +145,19 @@ public abstract class AbstractSearchSourceItemTestCase sort) throws IOException { try (BytesStreamOutput output = new BytesStreamOutput()) { - original.writeTo(output); + sort.writeTo(output); try (StreamInput in = new NamedWriteableAwareStreamInput(StreamInput.wrap(output.bytes()), namedWriteableRegistry)) { + SortBuilderTemp prototype = sortParser(sort.getName()).getBuilderPrototype(); @SuppressWarnings("unchecked") - T prototype = (T) namedWriteableRegistry.getPrototype(getPrototype(), original.getWriteableName()); - T copy = (T) prototype.readFrom(in); - return copy; + T secondQuery = (T) prototype.readFrom(in); + return secondQuery; } } } - - protected abstract Class getPrototype(); + + private SortBuilderTemp sortParser(String queryId) { + return indicesQueriesRegistry.sortParsers().get(queryId); + } }