Query refactoring: QueryBuilder to extend Writeable

`QueryBuilder`s need to become streamable over the wire, so we can use them as our own intermediate query representation and send it over the wire. Using `Writeable` rather than `Streamable` allows us to make some fields `final` and delete default constructors needed only for serialization purposes.

Taken the chance also to revise the internals of `IdsQueryBuilder` (modified some internal data type and added deduplication of ids to reduce bits to serialize). Expanded also `IdsQueryBuilderTest`, injected random types and improved comparison.

Moved all query builder tests to leverage lucene's equals and hashcode to compare result of `toQuery` with expected lucene query

Also refactored spanterm and term query tests, more core could be shared.

Also changed `RangeQueryBuilderTest` to remove the need for query rewriting and added some more coverage (we don't test when search context is null only anymore, search context gets now randomly set)

Closes elastic#11191

Author: javanna

src/main/java/org/elasticsearch/index/query/BaseTermQueryBuilder.java

@@ -22,19 +22,18 @@
 import org.apache.lucene.util.BytesRef;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
-import org.elasticsearch.common.io.stream.Streamable;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 
 import java.io.IOException;
 import java.util.Objects;
 
-public abstract class BaseTermQueryBuilder<QB extends BoostableQueryBuilder<QB>> extends QueryBuilder implements Streamable, BoostableQueryBuilder<QB> {
-
+public abstract class BaseTermQueryBuilder<QB extends BaseTermQueryBuilder<QB>> extends QueryBuilder<QB> implements BoostableQueryBuilder<QB> {
+    
     /** Name of field to match against. */
-    protected String fieldName;
+    protected final String fieldName;
 
     /** Value to find matches for. */
-    protected Object value;
+    protected final Object value;
 
     /** Query boost. */
     protected float boost = 1.0f;
@@ -116,10 +115,6 @@ public BaseTermQueryBuilder(String fieldName, Object value) {
         this.value = convertToBytesRefIfString(value);
     }
 
-    BaseTermQueryBuilder() {
-        // for serialization only
-    }
-
     /** Returns the field name used in this query. */
     public String fieldName() {
         return this.fieldName;
@@ -206,24 +201,23 @@ public boolean equals(Object obj) {
         if (obj == null || getClass() != obj.getClass()) {
             return false;
         }
-        @SuppressWarnings("rawtypes")
         BaseTermQueryBuilder other = (BaseTermQueryBuilder) obj;
         return Objects.equals(fieldName, other.fieldName) &&
                Objects.equals(value, other.value) &&
                Objects.equals(boost, other.boost) &&
                Objects.equals(queryName, other.queryName);
     }
 
-    /** Read the given parameters. */
     @Override
-    public void readFrom(StreamInput in) throws IOException {
-        fieldName = in.readString();
-        value = in.readGenericValue();
-        boost = in.readFloat();
-        queryName = in.readOptionalString();
+    public QB readFrom(StreamInput in) throws IOException {
+        QB emptyBuilder = createBuilder(in.readString(), in.readGenericValue());
+        emptyBuilder.boost = in.readFloat();
+        emptyBuilder.queryName = in.readOptionalString();
+        return emptyBuilder;
     }
 
-    /** Writes the given parameters. */
+    protected abstract QB createBuilder(String fieldName, Object value);
+
     @Override
     public void writeTo(StreamOutput out) throws IOException {
         out.writeString(fieldName);

src/main/java/org/elasticsearch/index/query/IdsQueryBuilder.java

@@ -19,59 +19,53 @@
 
 package org.elasticsearch.index.query;
 
-import com.google.common.collect.Iterables;
-
+import com.google.common.collect.Sets;
 import org.apache.lucene.queries.TermsQuery;
 import org.apache.lucene.search.Query;
+import org.elasticsearch.cluster.metadata.MetaData;
+import org.elasticsearch.common.Nullable;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
-import org.elasticsearch.common.io.stream.Streamable;
 import org.elasticsearch.common.lucene.search.Queries;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.index.mapper.Uid;
 import org.elasticsearch.index.mapper.internal.UidFieldMapper;
 
 import java.io.IOException;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Collection;
-import java.util.List;
-import java.util.Objects;
+import java.util.*;
 
 /**
  * A query that will return only documents matching specific ids (and a type).
  */
-public class IdsQueryBuilder extends QueryBuilder implements Streamable, BoostableQueryBuilder<IdsQueryBuilder> {
+public class IdsQueryBuilder extends QueryBuilder<IdsQueryBuilder> implements BoostableQueryBuilder<IdsQueryBuilder> {
 
-    private List<String> types = new ArrayList<>();
+    private final Set<String> ids = Sets.newHashSet();
 
-    private List<String> ids = new ArrayList<>();
+    private final String[] types;
 
     private float boost = 1.0f;
 
     private String queryName;
 
-    public IdsQueryBuilder() {
-        //for serialization only
-    }
-
-    public IdsQueryBuilder(String... types) {
-        this.types = (types == null || types.length == 0) ? new ArrayList<String>() : Arrays.asList(types);
+    /**
+     * Creates a new IdsQueryBuilder by optionally providing the types of the documents to look for
+     */
+    public IdsQueryBuilder(@Nullable String... types) {
+        this.types = types;
     }
 
     /**
-     * Get the types used in this query
-     * @return the types
+     * Returns the types used in this query
      */
-    public Collection<String> types() {
+    public String[] types() {
         return this.types;
     }
 
     /**
      * Adds ids to the query.
      */
     public IdsQueryBuilder addIds(String... ids) {
-        this.ids.addAll(Arrays.asList(ids));
+        Collections.addAll(this.ids, ids);
         return this;
     }
 
@@ -83,9 +77,9 @@ public IdsQueryBuilder ids(String... ids) {
     }
 
     /**
-     * Gets the ids for the query.
+     * Returns the ids for the query.
      */
-    public Collection<String> ids() {
+    public Set<String> ids() {
         return this.ids;
     }
 
@@ -125,22 +119,18 @@ public String queryName() {
     protected void doXContent(XContentBuilder builder, Params params) throws IOException {
         builder.startObject(IdsQueryParser.NAME);
         if (types != null) {
-            if (types.size() == 1) {
-                builder.field("type", types.get(0));
+            if (types.length == 1) {
+                builder.field("type", types[0]);
             } else {
-                builder.startArray("types");
-                for (String type : types) {
-                    builder.value(type);
-                }
-                builder.endArray();
+                builder.array("types", types);
             }
         }
         builder.startArray("values");
         for (String value : ids) {
             builder.value(value);
         }
         builder.endArray();
-        if (boost != -1) {
+        if (boost != 1.0f) {
             builder.field("boost", boost);
         }
         if (queryName != null) {
@@ -155,18 +145,21 @@ protected String parserName() {
     }
 
     public Query toQuery(QueryParseContext parseContext) throws IOException, QueryParsingException {
+        Query query;
         if (this.ids.isEmpty()) {
-            return Queries.newMatchNoDocsQuery();
-        }
+             query = Queries.newMatchNoDocsQuery();
+        } else {
+            Collection<String> typesForQuery;
+            if (types == null || types.length == 0) {
+                typesForQuery = parseContext.queryTypes();
+            } else if (types.length == 1 && MetaData.ALL.equals(types[0])) {
+                typesForQuery = parseContext.mapperService().types();
+            } else {
+                typesForQuery = Sets.newHashSet(types);
+            }
 
-        Collection<String> typesForQuery = this.types;
-        if (typesForQuery == null || typesForQuery.isEmpty()) {
-            typesForQuery = parseContext.queryTypes();
-        } else if (typesForQuery.size() == 1 && Iterables.getFirst(typesForQuery, null).equals("_all")) {
-            typesForQuery = parseContext.mapperService().types();
+            query = new TermsQuery(UidFieldMapper.NAME, Uid.createUidsForTypesAndIds(typesForQuery, ids));
         }
-
-        TermsQuery query = new TermsQuery(UidFieldMapper.NAME, Uid.createUidsForTypesAndIds(typesForQuery, ids));
         query.setBoost(boost);
         if (queryName != null) {
             parseContext.addNamedQuery(queryName, query);
@@ -181,24 +174,25 @@ public QueryValidationException validate() {
     }
 
     @Override
-    public void readFrom(StreamInput in) throws IOException {
-        this.types = in.readStringList();
-        this.ids = in.readStringList();
-        queryName = in.readOptionalString();
-        boost = in.readFloat();
+    public IdsQueryBuilder readFrom(StreamInput in) throws IOException {
+        IdsQueryBuilder idsQueryBuilder = new IdsQueryBuilder(in.readStringArray());
+        idsQueryBuilder.addIds(in.readStringArray());
+        idsQueryBuilder.queryName = in.readOptionalString();
+        idsQueryBuilder.boost = in.readFloat();
+        return idsQueryBuilder;
     }
 
     @Override
     public void writeTo(StreamOutput out) throws IOException {
-        out.writeStringList(this.types);
-        out.writeStringList(this.ids);
+        out.writeStringArray(this.types);
+        out.writeStringArray(this.ids.toArray(new String[this.ids.size()]));
         out.writeOptionalString(queryName);
         out.writeFloat(boost);
     }
 
     @Override
     public int hashCode() {
-        return Objects.hash(ids, types, boost, queryName);
+        return Objects.hash(ids, Arrays.hashCode(types), boost, queryName);
     }
 
     @Override
@@ -211,7 +205,7 @@ public boolean equals(Object obj) {
         }
         IdsQueryBuilder other = (IdsQueryBuilder) obj;
         return Objects.equals(ids, other.ids) &&
-               Objects.equals(types, other.types) &&
+               Arrays.equals(types, other.types) &&
                Objects.equals(boost, other.boost) &&
                Objects.equals(queryName, other.queryName);
     }

src/main/java/org/elasticsearch/index/query/IdsQueryParser.java

@@ -20,7 +20,6 @@
 package org.elasticsearch.index.query;
 
 import com.google.common.collect.ImmutableList;
-
 import org.elasticsearch.common.inject.Inject;
 import org.elasticsearch.common.xcontent.XContentParser;
 

src/main/java/org/elasticsearch/index/query/MatchAllQueryBuilder.java

@@ -23,7 +23,6 @@
 import org.apache.lucene.search.Query;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
-import org.elasticsearch.common.io.stream.Streamable;
 import org.elasticsearch.common.lucene.search.Queries;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 
@@ -32,7 +31,7 @@
 /**
  * A query that matches on all documents.
  */
-public class MatchAllQueryBuilder extends QueryBuilder implements Streamable, BoostableQueryBuilder<MatchAllQueryBuilder> {
+public class MatchAllQueryBuilder extends QueryBuilder<MatchAllQueryBuilder> implements BoostableQueryBuilder<MatchAllQueryBuilder> {
 
     private float boost = 1.0f;
 
@@ -67,7 +66,6 @@ public Query toQuery(QueryParseContext parseContext) {
         if (this.boost == 1.0f) {
             return Queries.newMatchAllQuery();
         }
-
         MatchAllDocsQuery query = new MatchAllDocsQuery();
         query.setBoost(boost);
         return query;
@@ -91,8 +89,10 @@ public int hashCode() {
     }
 
     @Override
-    public void readFrom(StreamInput in) throws IOException {
-        this.boost = in.readFloat();
+    public MatchAllQueryBuilder readFrom(StreamInput in) throws IOException {
+        MatchAllQueryBuilder matchAllQueryBuilder = new MatchAllQueryBuilder();
+        matchAllQueryBuilder.boost = in.readFloat();
+        return matchAllQueryBuilder;
     }
 
     @Override

src/main/java/org/elasticsearch/index/query/MultiTermQueryBuilder.java

@@ -18,6 +18,6 @@
  */
 package org.elasticsearch.index.query;
 
-public abstract class MultiTermQueryBuilder extends QueryBuilder {
+public abstract class MultiTermQueryBuilder<QB extends MultiTermQueryBuilder<QB>> extends QueryBuilder<QB> {
 
 }

src/main/java/org/elasticsearch/index/query/QueryBuilder.java

@@ -23,6 +23,9 @@
 import org.elasticsearch.action.support.ToXContentToBytes;
 import org.apache.lucene.util.BytesRef;
 import org.elasticsearch.common.lucene.BytesRefs;
+import org.elasticsearch.common.io.stream.StreamInput;
+import org.elasticsearch.common.io.stream.StreamOutput;
+import org.elasticsearch.common.io.stream.Writeable;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentType;
 
@@ -32,7 +35,7 @@
  * Base class for all classes producing lucene queries.
  * Supports conversion to BytesReference and creation of lucene Query objects.
  */
-public abstract class QueryBuilder extends ToXContentToBytes {
+public abstract class QueryBuilder<QB> extends ToXContentToBytes implements Writeable<QB> {
 
     protected QueryBuilder() {
         super(XContentType.JSON);
@@ -103,4 +106,15 @@ protected static Object convertToStringIfBytesRef(Object obj) {
         }
         return obj;
     }
+
+    //norelease remove this once all builders implement readFrom themselves
+    @Override
+    public QB readFrom(StreamInput in) throws IOException {
+        return null;
+    }
+
+    //norelease remove this once all builders implement writeTo themselves
+    @Override
+    public void writeTo(StreamOutput out) throws IOException {
+    }
 }