Skip to content

Commit

Permalink
Forbid negative field boosts in analyzed queries
Browse files Browse the repository at this point in the history
This change forbids negative field boost in the `query_string`, `simple_query_string`
and `multi_match` queries.
Negative boosts are not allowed in Lucene 8 (scores must be positive).
The backport of this change to 6x will turn the error into a deprecation warning
in order to raise the awareness of this breaking change in 7.0.

Closes elastic#33309
  • Loading branch information
jimczi committed Jan 28, 2019
1 parent e401ab1 commit 89e57f5
Show file tree
Hide file tree
Showing 9 changed files with 62 additions and 10 deletions.
4 changes: 2 additions & 2 deletions docs/reference/migration/migrate_7_0/search.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,8 @@ instead which is a more appropriate value for a scenario where scores are not av
[float]
==== Negative boosts are not allowed

Setting a negative `boost` in a query, deprecated in 6x, are not allowed in this version.
To deboost a specific query you can use a `boost` comprise between 0 and 1.
Setting a negative `boost` for a query or a field, deprecated in 6x, is not allowed in this version.
To deboost a specific query or field you can use a `boost` comprise between 0 and 1.

[float]
==== Negative scores are not allowed in Function Score Query
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ protected AbstractQueryBuilder() {

protected AbstractQueryBuilder(StreamInput in) throws IOException {
boost = in.readFloat();
checkNegativeBoost(boost);
queryName = in.readOptionalString();
}

Expand Down Expand Up @@ -139,17 +140,21 @@ public final float boost() {
return this.boost;
}

protected final void checkNegativeBoost(float boost) {
if (Float.compare(boost, 0f) < 0) {
throw new IllegalArgumentException("negative [boost] are not allowed in [" + toString() + "], " +
"use a value between 0 and 1 to deboost");
}
}

/**
* Sets the boost for this query. Documents matching this query will (in addition to the normal
* weightings) have their score multiplied by the boost provided.
*/
@SuppressWarnings("unchecked")
@Override
public final QB boost(float boost) {
if (Float.compare(boost, 0f) < 0) {
throw new IllegalArgumentException("negative [boost] are not allowed in [" + toString() + "], " +
"use a value between 0 and 1 to deboost");
}
checkNegativeBoost(boost);
this.boost = boost;
return (QB) this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,10 @@ public MultiMatchQueryBuilder(StreamInput in) throws IOException {
int size = in.readVInt();
fieldsBoosts = new TreeMap<>();
for (int i = 0; i < size; i++) {
fieldsBoosts.put(in.readString(), in.readFloat());
String field = in.readString();
float boost = in.readFloat();
checkNegativeBoost(boost);
fieldsBoosts.put(field, boost);
}
type = Type.readFromStream(in);
operator = Operator.readFromStream(in);
Expand Down Expand Up @@ -295,6 +298,7 @@ public MultiMatchQueryBuilder field(String field, float boost) {
if (Strings.isEmpty(field)) {
throw new IllegalArgumentException("supplied field is null or empty.");
}
checkNegativeBoost(boost);
this.fieldsBoosts.put(field, boost);
return this;
}
Expand All @@ -303,6 +307,9 @@ public MultiMatchQueryBuilder field(String field, float boost) {
* Add several fields to run the query against with a specific boost.
*/
public MultiMatchQueryBuilder fields(Map<String, Float> fields) {
for (float fieldBoost : fields.values()) {
checkNegativeBoost(fieldBoost);
}
this.fieldsBoosts.putAll(fields);
return this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,10 @@ public QueryStringQueryBuilder(StreamInput in) throws IOException {
defaultField = in.readOptionalString();
int size = in.readVInt();
for (int i = 0; i < size; i++) {
fieldsAndWeights.put(in.readString(), in.readFloat());
String field = in.readString();
Float weight = in.readFloat();
checkNegativeBoost(weight);
fieldsAndWeights.put(field, weight);
}
defaultOperator = Operator.readFromStream(in);
analyzer = in.readOptionalString();
Expand Down Expand Up @@ -264,6 +267,7 @@ public QueryStringQueryBuilder field(String field) {
* Adds a field to run the query string against with a specific boost.
*/
public QueryStringQueryBuilder field(String field, float boost) {
checkNegativeBoost(boost);
this.fieldsAndWeights.put(field, boost);
return this;
}
Expand All @@ -272,6 +276,9 @@ public QueryStringQueryBuilder field(String field, float boost) {
* Add several fields to run the query against with a specific boost.
*/
public QueryStringQueryBuilder fields(Map<String, Float> fields) {
for (float fieldBoost : fields.values()) {
checkNegativeBoost(fieldBoost);
}
this.fieldsAndWeights.putAll(fields);
return this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ public SimpleQueryStringBuilder(StreamInput in) throws IOException {
for (int i = 0; i < size; i++) {
String field = in.readString();
Float weight = in.readFloat();
checkNegativeBoost(weight);
fields.put(field, weight);
}
fieldsAndWeights.putAll(fields);
Expand Down Expand Up @@ -223,13 +224,17 @@ public SimpleQueryStringBuilder field(String field, float boost) {
if (Strings.isEmpty(field)) {
throw new IllegalArgumentException("supplied field is null or empty");
}
checkNegativeBoost(boost);
this.fieldsAndWeights.put(field, boost);
return this;
}

/** Add several fields to run the query against with a specific boost. */
public SimpleQueryStringBuilder fields(Map<String, Float> fields) {
Objects.requireNonNull(fields, "fields cannot be null");
for (float fieldBoost : fields.values()) {
checkNegativeBoost(fieldBoost);
}
this.fieldsAndWeights.putAll(fields);
return this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,11 +97,10 @@ private List<Query> buildFieldQueries(MultiMatchQueryBuilder.Type type, Map<Stri
// ignore unmapped fields
continue;
}
Float boostValue = fieldNames.get(fieldName);
float boostValue = fieldNames.getOrDefault(fieldName, 1.0f);
Query query = parse(type.matchQueryType(), fieldName, value);
query = Queries.maybeApplyMinimumShouldMatch(query, minimumShouldMatch);
if (query != null
&& boostValue != null
&& boostValue != AbstractQueryBuilder.DEFAULT_BOOST
&& query instanceof MatchNoDocsQuery == false) {
query = new BoostQuery(query, boostValue);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,15 @@ public void testWithStopWords() throws Exception {
assertEquals(expected, query);
}

public void testNegativeFieldBoost() {
IllegalArgumentException exc = expectThrows(IllegalArgumentException.class,
() -> new MultiMatchQueryBuilder("the quick fox")
.field(STRING_FIELD_NAME, -1.0f)
.field(STRING_FIELD_NAME_2)
.toQuery(createShardContext()));
assertThat(exc.getMessage(), containsString("negative [boost]"));
}

private static IndexMetaData newIndexMeta(String name, Settings oldIndexSettings, Settings indexSettings) {
Settings build = Settings.builder().put(oldIndexSettings)
.put(indexSettings)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
import org.elasticsearch.index.search.QueryStringQueryParser;
import org.elasticsearch.search.internal.SearchContext;
import org.elasticsearch.test.AbstractQueryTestCase;
import org.hamcrest.CoreMatchers;
import org.hamcrest.Matchers;

import java.io.IOException;
Expand Down Expand Up @@ -1471,6 +1472,15 @@ public void testAnalyzedPrefix() throws Exception {
assertEquals(expected, query);
}

public void testNegativeFieldBoost() {
IllegalArgumentException exc = expectThrows(IllegalArgumentException.class,
() -> new QueryStringQueryBuilder("the quick fox")
.field(STRING_FIELD_NAME, -1.0f)
.field(STRING_FIELD_NAME_2)
.toQuery(createShardContext()));
assertThat(exc.getMessage(), CoreMatchers.containsString("negative [boost]"));
}

private static IndexMetaData newIndexMeta(String name, Settings oldIndexSettings, Settings indexSettings) {
Settings build = Settings.builder().put(oldIndexSettings)
.put(indexSettings)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
import java.util.Map;
import java.util.Set;

import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.Matchers.anyOf;
import static org.hamcrest.Matchers.either;
import static org.hamcrest.Matchers.equalTo;
Expand Down Expand Up @@ -718,6 +719,15 @@ public void testUnmappedFieldNoTokenWithAndOperator() throws IOException {
assertEquals(expected, query);
}

public void testNegativeFieldBoost() {
IllegalArgumentException exc = expectThrows(IllegalArgumentException.class,
() -> new SimpleQueryStringBuilder("the quick fox")
.field(STRING_FIELD_NAME, -1.0f)
.field(STRING_FIELD_NAME_2)
.toQuery(createShardContext()));
assertThat(exc.getMessage(), containsString("negative [boost]"));
}

private static IndexMetaData newIndexMeta(String name, Settings oldIndexSettings, Settings indexSettings) {
Settings build = Settings.builder().put(oldIndexSettings)
.put(indexSettings)
Expand Down

0 comments on commit 89e57f5

Please sign in to comment.