Skip to content

Commit

Permalink
Forbid negative field boosts in analyzed queries (#37930)
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 #33309
  • Loading branch information
jimczi authored Feb 1, 2019
1 parent 57b1d24 commit 6fa93ca
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 @@ -159,8 +159,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 6fa93ca

Please sign in to comment.