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
jimczi committed Feb 1, 2019
1 parent e0b32cf commit bf9bd0d
Show file tree
Hide file tree
Showing 9 changed files with 65 additions and 12 deletions.
8 changes: 4 additions & 4 deletions docs/reference/migration/migrate_6_0/search.asciidoc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,10 +153,10 @@ information.
[float]
===== Negative scores are deprecated in Function Score Query

Negative scores in the Function Score Query are deprecated. If a negative
Negative scores in the Function Score Query are deprecated. If a negative
score is produced as a result of computation (e.g. in `script_score` or
`field_value_factor` functions), a deprecation warning will be issued in
this major version, and an error will be thrown in the next major version.
this major version, and an error will be thrown in the next major version.

[float]
==== Fielddata on `_uid`
Expand Down Expand Up @@ -266,8 +266,8 @@ rewrite any prefix query on the field to a a single term query that matches the
[float]
==== Negative boosts are deprecated

Setting a negative `boost` in a query is deprecated and will throw an error in the next 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 is deprecated and will throw an error in the next
major version. To deboost a specific query or field you can use a `boost` comprise between 0 and 1.

[float]
==== Limit the number of open scroll contexts
Expand Down
13 changes: 9 additions & 4 deletions server/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ protected AbstractQueryBuilder() {

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

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

protected final void checkNegativeBoost(float boost) {
if (Float.compare(boost, 0f) < 0) {
deprecationLogger.deprecatedAndMaybeLog("negative boost", "setting a negative [boost] on a query " +
"is deprecated and will throw an error in the next version. You can 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) {
deprecationLogger.deprecatedAndMaybeLog("negative boost", "setting a negative [boost] on a query " +
"is deprecated and will throw an error in the next version. You can use a value between 0 and 1 to deboost.");
}
checkNegativeBoost(boost);
this.boost = boost;
return (QB) this;
}
Expand Down
9 changes: 8 additions & 1 deletion server/src/main/java/org/elasticsearch/index/query/MultiMatchQueryBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,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 @@ -293,6 +296,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 @@ -301,6 +305,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
9 changes: 8 additions & 1 deletion server/src/main/java/org/elasticsearch/index/query/QueryStringQueryBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,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 @@ -339,6 +342,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 @@ -347,6 +351,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
5 changes: 5 additions & 0 deletions server/src/main/java/org/elasticsearch/index/query/SimpleQueryStringBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,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 @@ -258,13 +259,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
3 changes: 1 addition & 2 deletions server/src/main/java/org/elasticsearch/index/search/MultiMatchQuery.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,11 +96,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
10 changes: 10 additions & 0 deletions server/src/test/java/org/elasticsearch/index/query/MultiMatchQueryBuilderTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,7 @@ public void testWithStopWords() throws Exception {
assertEquals(expected, query);
}

<<<<<<< HEAD
public void testDisMaxDeprecation() throws Exception {
String json =
"{\n" +
Expand All @@ -494,6 +495,15 @@ public void testDisMaxDeprecation() throws Exception {

parseQuery(json);
assertWarnings("Deprecated field [use_dis_max] used, replaced by [use tie_breaker instead]");
=======
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]"));
>>>>>>> 89e57f53acc... Forbid negative field boosts in analyzed queries
}

private static IndexMetaData newIndexMeta(String name, Settings oldIndexSettings, Settings indexSettings) {
Expand Down
10 changes: 10 additions & 0 deletions server/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuilderTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,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 org.joda.time.DateTimeZone;

Expand Down Expand Up @@ -1488,6 +1489,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
10 changes: 10 additions & 0 deletions server/src/test/java/org/elasticsearch/index/query/SimpleQueryStringBuilderTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,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 @@ -739,6 +740,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 bf9bd0d

Please sign in to comment.