From 620211800e434c097ef04b1b0663c16b6522b69b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Mon, 13 Apr 2015 18:50:03 +0200 Subject: [PATCH] Query refactoring: BoostingQueryBuilder and Parser As part of the refactoring of queries this PR splits the parsers parse() method into a parsing and a query building part, adding serialization, hashCode() and equals() to the builder. Relates to #10217 Closes #11621 --- .../index/query/BoostingQueryBuilder.java | 127 +++++++++++++++-- .../index/query/BoostingQueryParser.java | 35 ++--- .../index/query/QueryBuilder.java | 7 +- .../index/query/BoostingQueryBuilderTest.java | 128 ++++++++++++++++++ 4 files changed, 265 insertions(+), 32 deletions(-) create mode 100644 core/src/test/java/org/elasticsearch/index/query/BoostingQueryBuilderTest.java diff --git a/core/src/main/java/org/elasticsearch/index/query/BoostingQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/BoostingQueryBuilder.java index 7a67ce3a603..901de10330c 100644 --- a/core/src/main/java/org/elasticsearch/index/query/BoostingQueryBuilder.java +++ b/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 { +public class BoostingQueryBuilder extends QueryBuilder implements BoostableQueryBuilder { 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.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); + } } diff --git a/core/src/main/java/org/elasticsearch/index/query/BoostingQueryParser.java b/core/src/main/java/org/elasticsearch/index/query/BoostingQueryParser.java index 897cabaf33c..68cc02a93c5 100644 --- a/core/src/main/java/org/elasticsearch/index/query/BoostingQueryParser.java +++ b/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) { - throw new QueryParsingException(parseContext, "[boosting] query requires 'negative_boost' to be set'"); + if (negativeBoost < 0) { + throw new QueryParsingException(parseContext, "[boosting] query requires 'negative_boost' to be set to be a positive value'"); } - // parsers returned null - if (positiveQuery == null || negativeQuery == null) { - return null; - } - - BoostingQuery boostingQuery = new BoostingQuery(positiveQuery, negativeQuery, negativeBoost); - if (boost != -1) { - boostingQuery.setBoost(boost); - } + BoostingQueryBuilder boostingQuery = new BoostingQueryBuilder(); + boostingQuery.positive(positiveQuery); + boostingQuery.negative(negativeQuery); + boostingQuery.negativeBoost(negativeBoost); + boostingQuery.boost(boost); + boostingQuery.validate(); return boostingQuery; } diff --git a/core/src/main/java/org/elasticsearch/index/query/QueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/QueryBuilder.java index 78038a09f39..787774a4b28 100644 --- a/core/src/main/java/org/elasticsearch/index/query/QueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/QueryBuilder.java @@ -60,9 +60,12 @@ public abstract class QueryBuilder extends ToXContentTo } /** - * Converts this QueryBuilder to a lucene {@link Query} + * Converts this QueryBuilder to a lucene {@link Query}. + * Returns null 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 null if this query should be ignored upstream * @throws QueryParsingException * @throws IOException */ diff --git a/core/src/test/java/org/elasticsearch/index/query/BoostingQueryBuilderTest.java b/core/src/test/java/org/elasticsearch/index/query/BoostingQueryBuilderTest.java new file mode 100644 index 00000000000..5cd1171e2a1 --- /dev/null +++ b/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 { + + @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 null + * 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() { + + @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)); + } +}