Skip to content

Query refactoring: BoostingQueryBuilder and Parser #11621

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 16, 2015
Merged

Query refactoring: BoostingQueryBuilder and Parser #11621

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 117 additions & 10 deletions core/src/main/java/org/elasticsearch/index/query/BoostingQueryBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -35,7 +40,7 @@
* 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";

Expand All @@ -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) {
Expand All @@ -81,25 +126,87 @@ protected void doXContent(XContentBuilder builder, Params params) throws IOExcep
if (negativeQuery == null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we have these same checks in the validate method too? Or only there?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had them in validate() before, but the problem like in #11629 is that we need to support inner clauses that parse to null like "positive": { "constant_score": { "filter": {}}}, so if we have these checks in validate() we will throw errors on queries the DSL currently accepts.
I wonder why this behaviour was introduced in the first place, maybe there is a deeper reason for that, otherwise we should try and get rid of this but I think that's out of scope for this refactoring for now. Do you agree?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, sorry I keep getting confused about this. those queries can be null, we do these checks to prevent NPEs, makes sense.

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);
}
}
35 changes: 15 additions & 20 deletions core/src/main/java/org/elasticsearch/index/query/BoostingQueryParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -29,7 +27,7 @@
/**
*
*/
public class BoostingQueryParser extends BaseQueryParserTemp {
public class BoostingQueryParser extends BaseQueryParser {

@Inject
public BoostingQueryParser() {
Expand All @@ -41,14 +39,14 @@ public String[] names() {
}

@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;
Expand All @@ -58,10 +56,10 @@ public Query parse(QueryParseContext parseContext) throws IOException, QueryPars
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 + "]");
Expand All @@ -83,19 +81,16 @@ public Query parse(QueryParseContext parseContext) throws IOException, QueryPars
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;
}

Expand Down
7 changes: 5 additions & 2 deletions core/src/main/java/org/elasticsearch/index/query/QueryBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,12 @@ public final String getName() {
}

/**
* 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
*/
Expand Down
Loading