Merge pull request #11621 from cbuescher/feature/query-refactoring-boostingquery

Query refactoring: BoostingQueryBuilder and Parser

core/src/main/java/org/elasticsearch/index/query/BoostingQueryBuilder.java

@@ -19,9 +19,14 @@
 
 package org.elasticsearch.index.query;
 
+import org.apache.lucene.search.Query;
+import org.apache.lucene.queries.BoostingQuery;
+import org.elasticsearch.common.io.stream.StreamInput;
+import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 
 import java.io.IOException;
+import java.util.Objects;
 
 /**
  * The BoostingQuery class can be used to effectively demote results that match a given query.
@@ -35,7 +40,7 @@ import java.io.IOException;
  * multiplied by the supplied "boost" parameter, so this should be less than 1 to achieve a
  * demoting effect
  */
-public class BoostingQueryBuilder extends QueryBuilder implements BoostableQueryBuilder<BoostingQueryBuilder> {
+public class BoostingQueryBuilder extends QueryBuilder<BoostingQueryBuilder> implements BoostableQueryBuilder<BoostingQueryBuilder> {
 
     public static final String NAME = "boosting";
 
@@ -45,34 +50,74 @@ public class BoostingQueryBuilder extends QueryBuilder implements BoostableQuery
 
     private float negativeBoost = -1;
 
-    private float boost = -1;
+    private float boost = 1.0f;
 
     static final BoostingQueryBuilder PROTOTYPE = new BoostingQueryBuilder();
 
     public BoostingQueryBuilder() {
     }
 
+    /**
+     * Add the positive query for this boosting query.
+     */
     public BoostingQueryBuilder positive(QueryBuilder positiveQuery) {
         this.positiveQuery = positiveQuery;
         return this;
     }
 
+    /**
+     * Get the positive query for this boosting query.
+     */
+    public QueryBuilder positive() {
+        return this.positiveQuery;
+    }
+
+    /**
+     * Add the negative query for this boosting query.
+     */
     public BoostingQueryBuilder negative(QueryBuilder negativeQuery) {
         this.negativeQuery = negativeQuery;
         return this;
     }
 
+    /**
+     * Get the negative query for this boosting query.
+     */
+    public QueryBuilder negative() {
+        return this.negativeQuery;
+    }
+
+    /**
+     * Set the negative boost factor.
+     */
     public BoostingQueryBuilder negativeBoost(float negativeBoost) {
         this.negativeBoost = negativeBoost;
         return this;
     }
 
+    /**
+     * Get the negative boost factor.
+     */
+    public float negativeBoost() {
+        return this.negativeBoost;
+    }
+
+    /**
+     * Set the boost factor.
+     */
     @Override
     public BoostingQueryBuilder boost(float boost) {
         this.boost = boost;
         return this;
     }
 
+    /**
+     * Get the boost factor.
+     */
+    public float boost() {
+        return this.boost;
+    }
+
     @Override
     protected void doXContent(XContentBuilder builder, Params params) throws IOException {
         if (positiveQuery == null) {
@@ -81,25 +126,87 @@ public class BoostingQueryBuilder extends QueryBuilder implements BoostableQuery
         if (negativeQuery == null) {
             throw new IllegalArgumentException("boosting query requires negative query to be set");
         }
-        if (negativeBoost == -1) {
-            throw new IllegalArgumentException("boosting query requires negativeBoost to be set");
-        }
         builder.startObject(NAME);
         builder.field("positive");
         positiveQuery.toXContent(builder, params);
         builder.field("negative");
         negativeQuery.toXContent(builder, params);
-
         builder.field("negative_boost", negativeBoost);
-
-        if (boost != -1) {
         builder.field("boost", boost);
-        }
         builder.endObject();
     }
 
+    @Override
+    public QueryValidationException validate() {
+        QueryValidationException validationException = null;
+        if (negativeBoost < 0) {
+            validationException = QueryValidationException
+                    .addValidationError("[boosting] query requires negativeBoost to be set to positive value", validationException);
+        }
+        return validationException;
+    };
+
     @Override
     public String queryId() {
         return NAME;
     }
+
+    @Override
+    public Query toQuery(QueryParseContext parseContext) throws QueryParsingException, IOException {
+
+        // make upstream queries ignore this query by returning `null`
+        // if either inner query builder is null or returns null-Query
+        if (positiveQuery == null || negativeQuery == null) {
+            return null;
+        }
+        Query positive = positiveQuery.toQuery(parseContext);
+        Query negative = negativeQuery.toQuery(parseContext);
+        if (positive == null || negative == null) {
+            return null;
+        }
+
+        BoostingQuery boostingQuery = new BoostingQuery(positive, negative, negativeBoost);
+        boostingQuery.setBoost(boost);
+        return boostingQuery;
+    }
+
+    @Override
+    public int hashCode() {
+        return Objects.hash(this.boost, this.negativeBoost, this.positiveQuery, this.negativeQuery);
+    }
+
+    @Override
+    public boolean equals(Object obj) {
+        if (this == obj) {
+            return true;
+        }
+        if (obj == null || getClass() != obj.getClass()) {
+            return false;
+        }
+        BoostingQueryBuilder other = (BoostingQueryBuilder) obj;
+        return Objects.equals(this.boost, other.boost) &&
+                Objects.equals(this.negativeBoost, other.negativeBoost) &&
+                Objects.equals(this.positiveQuery, other.positiveQuery) &&
+                Objects.equals(this.negativeQuery, other.negativeQuery);
+    }
+
+    @Override
+    public BoostingQueryBuilder readFrom(StreamInput in) throws IOException {
+        QueryBuilder positiveQuery = in.readNamedWriteable();
+        QueryBuilder negativeQuery = in.readNamedWriteable();
+        BoostingQueryBuilder boostingQuery = new BoostingQueryBuilder();
+        boostingQuery.positive(positiveQuery);
+        boostingQuery.negative(negativeQuery);
+        boostingQuery.boost = in.readFloat();
+        boostingQuery.negativeBoost = in.readFloat();
+        return boostingQuery;
+    }
+
+    @Override
+    public void writeTo(StreamOutput out) throws IOException {
+        out.writeNamedWriteable(this.positiveQuery);
+        out.writeNamedWriteable(this.negativeQuery);
+        out.writeFloat(this.boost);
+        out.writeFloat(this.negativeBoost);
+    }
 }

core/src/main/java/org/elasticsearch/index/query/BoostingQueryParser.java

@@ -19,8 +19,6 @@
 
 package org.elasticsearch.index.query;
 
-import org.apache.lucene.queries.BoostingQuery;
-import org.apache.lucene.search.Query;
 import org.elasticsearch.common.inject.Inject;
 import org.elasticsearch.common.xcontent.XContentParser;
 
@@ -29,7 +27,7 @@ import java.io.IOException;
 /**
  *
  */
-public class BoostingQueryParser extends BaseQueryParserTemp {
+public class BoostingQueryParser extends BaseQueryParser {
 
     @Inject
     public BoostingQueryParser() {
@@ -41,14 +39,14 @@ public class BoostingQueryParser extends BaseQueryParserTemp {
     }
 
     @Override
-    public Query parse(QueryParseContext parseContext) throws IOException, QueryParsingException {
+    public QueryBuilder fromXContent(QueryParseContext parseContext) throws IOException, QueryParsingException {
         XContentParser parser = parseContext.parser();
 
-        Query positiveQuery = null;
+        QueryBuilder positiveQuery = null;
         boolean positiveQueryFound = false;
-        Query negativeQuery = null;
+        QueryBuilder negativeQuery = null;
         boolean negativeQueryFound = false;
-        float boost = -1;
+        float boost = 1.0f;
         float negativeBoost = -1;
 
         String currentFieldName = null;
@@ -58,10 +56,10 @@ public class BoostingQueryParser extends BaseQueryParserTemp {
                 currentFieldName = parser.currentName();
             } else if (token == XContentParser.Token.START_OBJECT) {
                 if ("positive".equals(currentFieldName)) {
-                    positiveQuery = parseContext.parseInnerQuery();
+                    positiveQuery = parseContext.parseInnerQueryBuilder();
                     positiveQueryFound = true;
                 } else if ("negative".equals(currentFieldName)) {
-                    negativeQuery = parseContext.parseInnerQuery();
+                    negativeQuery = parseContext.parseInnerQueryBuilder();
                     negativeQueryFound = true;
                 } else {
                     throw new QueryParsingException(parseContext, "[boosting] query does not support [" + currentFieldName + "]");
@@ -83,19 +81,16 @@ public class BoostingQueryParser extends BaseQueryParserTemp {
         if (negativeQuery == null && !negativeQueryFound) {
             throw new QueryParsingException(parseContext, "[boosting] query requires 'negative' query to be set'");
         }
-        if (negativeBoost == -1) {
+        if (negativeBoost < 0) {
-            throw new QueryParsingException(parseContext, "[boosting] query requires 'negative_boost' to be set'");
+            throw new QueryParsingException(parseContext, "[boosting] query requires 'negative_boost' to be set to be a positive value'");
         }
 
-        // parsers returned null
+        BoostingQueryBuilder boostingQuery = new BoostingQueryBuilder();
-        if (positiveQuery == null || negativeQuery == null) {
+        boostingQuery.positive(positiveQuery);
-            return null;
+        boostingQuery.negative(negativeQuery);
-        }
+        boostingQuery.negativeBoost(negativeBoost);
-
+        boostingQuery.boost(boost);
-        BoostingQuery boostingQuery = new BoostingQuery(positiveQuery, negativeQuery, negativeBoost);
+        boostingQuery.validate();
-        if (boost != -1) {
-            boostingQuery.setBoost(boost);
-        }
         return boostingQuery;
     }
 

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

@@ -60,9 +60,12 @@ public abstract class QueryBuilder<QB extends QueryBuilder> extends ToXContentTo
     }
 
     /**
-     * Converts this QueryBuilder to a lucene {@link Query}
+     * Converts this QueryBuilder to a lucene {@link Query}.
+     * Returns <tt>null</tt> if this query should be ignored in the context of
+     * parent queries.
+     *
      * @param parseContext additional information needed to construct the queries
-     * @return the {@link Query}
+     * @return the {@link Query} or <tt>null</tt> if this query should be ignored upstream
      * @throws QueryParsingException
      * @throws IOException
      */

core/src/test/java/org/elasticsearch/index/query/BoostingQueryBuilderTest.java

@@ -0,0 +1,128 @@
+/*
+ * 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.index.query;
+
+import org.apache.lucene.queries.BoostingQuery;
+import org.apache.lucene.search.Query;
+import org.elasticsearch.common.xcontent.ToXContent;
+import org.elasticsearch.common.xcontent.XContentBuilder;
+import org.elasticsearch.common.xcontent.XContentFactory;
+import org.junit.Test;
+
+import java.io.IOException;
+
+import static org.hamcrest.Matchers.is;
+
+public class BoostingQueryBuilderTest extends BaseQueryTestCase<BoostingQueryBuilder> {
+
+    @Override
+    protected BoostingQueryBuilder createTestQueryBuilder() {
+        BoostingQueryBuilder query = new BoostingQueryBuilder();
+        query.positive(RandomQueryBuilder.create(random()));
+        query.negative(RandomQueryBuilder.create(random()));
+        query.negativeBoost(2.0f / randomIntBetween(1, 20));
+        if (randomBoolean()) {
+            query.boost(2.0f / randomIntBetween(1, 20));
+        }
+        return query;
+    }
+
+    @Override
+    protected Query createExpectedQuery(BoostingQueryBuilder queryBuilder, QueryParseContext context) throws IOException {
+        Query positive = queryBuilder.positive().toQuery(context);
+        Query negative = queryBuilder.negative().toQuery(context);
+        BoostingQuery boostingQuery = new BoostingQuery(positive, negative, queryBuilder.negativeBoost());
+        if (queryBuilder.boost() != 1.0f) {
+            boostingQuery.setBoost(queryBuilder.boost());
+        }
+        return boostingQuery;
+    }
+
+    @Test
+    public void testToXContentIllegalArgumentExceptions() throws IOException {
+        BoostingQueryBuilder boostingQueryBuilder = new BoostingQueryBuilder();
+        boostingQueryBuilder.positive(new MatchAllQueryBuilder());
+        try {
+            boostingQueryBuilder.toXContent(XContentFactory.jsonBuilder(), ToXContent.EMPTY_PARAMS);
+            fail("Expected IllegalArgumentException because of missing negative query.");
+        } catch (IllegalArgumentException e) {
+            // expected
+        }
+
+        boostingQueryBuilder = new BoostingQueryBuilder();
+        boostingQueryBuilder.negative(new MatchAllQueryBuilder());
+        try {
+            boostingQueryBuilder.toXContent(XContentFactory.jsonBuilder(), ToXContent.EMPTY_PARAMS);
+            fail("Expected IllegalArgumentException because of missing positive query.");
+        } catch (IllegalArgumentException e) {
+            // expected
+        }
+    }
+
+    /**
+     * tests that we signal upstream queries to ignore this query by returning <tt>null</tt>
+     * if any of the inner query builder is not set
+     */
+    @Test
+    public void testInnerQueryBuilderNull() throws IOException {
+        BoostingQueryBuilder boostingQueryBuilder = new BoostingQueryBuilder();
+        if (randomBoolean()) {
+            boostingQueryBuilder.positive(new MatchAllQueryBuilder()).negative(null);
+        } else {
+            boostingQueryBuilder.positive(null).negative(new MatchAllQueryBuilder());
+        }
+        assertNull(boostingQueryBuilder.toQuery(createContext()));
+    }
+
+    @Test
+    public void testInnerQueryBuilderReturnsNull() throws IOException {
+        QueryBuilder noOpBuilder = new QueryBuilder<QueryBuilder>() {
+
+            @Override
+            public String queryId() {
+                return "dummy";
+            }
+
+            @Override
+            protected void doXContent(XContentBuilder builder, Params params) throws IOException {
+            }
+
+            @Override
+            public Query toQuery(QueryParseContext context) {
+                return null;
+            }
+        };
+        BoostingQueryBuilder boostingQueryBuilder = null;
+        if (randomBoolean()) {
+            boostingQueryBuilder = new BoostingQueryBuilder().positive(new MatchAllQueryBuilder()).negative(noOpBuilder);
+        } else {
+            boostingQueryBuilder = new BoostingQueryBuilder().positive(noOpBuilder).negative(new MatchAllQueryBuilder());
+        }
+        assertNull(boostingQueryBuilder.toQuery(createContext()));
+    }
+
+    @Test
+    public void testValidate() {
+        BoostingQueryBuilder boostingQueryBuilder = new BoostingQueryBuilder();
+        // check for error with negative `negative boost` factor
+        boostingQueryBuilder.negativeBoost(-0.5f);
+        assertThat(boostingQueryBuilder.validate().validationErrors().size(), is(1));
+    }
+}