Skip to content

Apply boost only once for distance_feature query #63767

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 2 commits into from
Oct 16, 2020
Merged

Apply boost only once for distance_feature query #63767

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6654c68
Apply boost only once for distance_feature query
mayya-sharipova Oct 15, 2020
7cbe160
Remove test
mayya-sharipova Oct 15, 2020
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
28 changes: 28 additions & 0 deletions server/src/internalClusterTest/java/org/elasticsearch/search/query/SearchQueryIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import org.elasticsearch.index.analysis.NormalizingCharFilterFactory;
import org.elasticsearch.index.analysis.TokenizerFactory;
import org.elasticsearch.index.query.BoolQueryBuilder;
import org.elasticsearch.index.query.DistanceFeatureQueryBuilder;
import org.elasticsearch.index.query.MatchQueryBuilder;
import org.elasticsearch.index.query.MultiMatchQueryBuilder;
import org.elasticsearch.index.query.Operator;
Expand Down Expand Up @@ -1626,6 +1627,33 @@ public void testRangeQueryWithLocaleMapping() throws Exception {
assertHitCount(searchResponse, 2L);
}

public void testDistanceFeatureQuery() {
Copy link
Contributor

@jtibshirani jtibshirani Oct 15, 2020

Choose a reason for hiding this comment

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

It would be nice to rely on unit tests like DistanceFeatureQueryBuilderTests or a REST test instead of an integration test. We try to avoid new ESIntegTestCase cases unless a full cluster is really needed.

Copy link
Contributor Author

@mayya-sharipova mayya-sharipova Oct 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@jtibshirani Thanks for the suggestion. I was trying to make this test in REST yml tests, but yml tests don't have nice comparison of float scores (you can't set up delta, and also there are errors with double -> float conversions).

I also considered adding this test in DistanceFeatureQueryBuilderTests, but then I need to work on the Lucene level there. But what I wanted to test is how ES parses this query and what scores are returned. I could not find a smart way to do that in that test. WDYT?

Copy link
Contributor

@jtibshirani jtibshirani Oct 15, 2020

Choose a reason for hiding this comment

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

yml tests don't have nice comparison of float scores

Got it, that's good to know.

But what I wanted to test is how ES parses this query and what scores are returned.

I guess we already test in DistanceFeatureQueryBuilderTests that the query is parsed correctly, since we check LatLonPointDistanceFeatureQuery has a boost of 1.0f. I also don't see a convenient way to test query scoring in a unit test.

Copy link
Contributor Author

@mayya-sharipova mayya-sharipova Oct 15, 2020

Choose a reason for hiding this comment

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

@jtibshirani

I guess we already test in DistanceFeatureQueryBuilderTests that the query is parsed correctly, since we check LatLonPointDistanceFeatureQuery has a boost of 1.0f.

Indeed, the tests in DistanceFeatureQueryBuilderTests should be enough for this PR. I've removed a test from SearchQueryIT.java in 7cbe160. We can introduce this test on scores later, when we need to test the correct scores.

assertAcked(prepareCreate("test").setMapping("date", "type=date"));
client().prepareIndex("test").setId("1").setSource("date", "2020-12-03T00:00:00Z").get();
client().prepareIndex("test").setId("2").setSource("date", "2020-12-02T00:00:00Z").get();
client().prepareIndex("test").setId("3").setSource("date", "2020-12-01T00:00:00Z").get();
refresh();

DistanceFeatureQueryBuilder dqb = QueryBuilders.distanceFeatureQuery(
"date", new DistanceFeatureQueryBuilder.Origin("2020-12-01T00:00:00Z"), "1d");
SearchResponse searchResponse = client().prepareSearch("test").setQuery(dqb).get();
assertHitCount(searchResponse, 3L);
SearchHit[] hits = searchResponse.getHits().getHits();
assertEquals(1.0f, hits[0].getScore(), 0.1f);
assertEquals(0.5f, hits[1].getScore(), 0.1f);
assertEquals(0.33f, hits[2].getScore(), 0.1f);

DistanceFeatureQueryBuilder dqb2 = QueryBuilders.distanceFeatureQuery(
"date", new DistanceFeatureQueryBuilder.Origin("2020-12-01T00:00:00Z"), "1d");
dqb2.boost(2.0f);
SearchResponse searchResponse2 = client().prepareSearch("test").setQuery(dqb2).get();
assertHitCount(searchResponse2, 3L);
SearchHit[] hits2 = searchResponse2.getHits().getHits();
assertEquals(2.0f, hits2[0].getScore(), 0.1f);
assertEquals(1.0f, hits2[1].getScore(), 0.1f);
assertEquals(0.66f, hits2[2].getScore(),0.1f);
}

public void testSearchEmptyDoc() {
assertAcked(prepareCreate("test").setSettings("{\"index.analysis.analyzer.default.type\":\"keyword\"}", XContentType.JSON));
client().prepareIndex("test").setId("1").setSource("{}", XContentType.JSON).get();
Expand Down
3 changes: 2 additions & 1 deletion server/src/main/java/org/elasticsearch/index/query/DistanceFeatureQueryBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,8 @@ protected Query doToQuery(QueryShardContext context) throws IOException {
if (fieldType == null) {
return Queries.newMatchNoDocsQuery("Can't run [" + NAME + "] query on unmapped fields!");
}
return fieldType.distanceFeatureQuery(origin.origin(), pivot, boost, context);
// As we already apply boost in AbstractQueryBuilder::toQuery, we always passing a boost of 1.0 to distanceFeatureQuery
return fieldType.distanceFeatureQuery(origin.origin(), pivot, 1.0f, context);
Copy link
Contributor

@jtibshirani jtibshirani Oct 15, 2020

Choose a reason for hiding this comment

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

When we fix LongScriptFieldDistanceFeatureQuery, I think we could remove the boost parameter altogether from MappedFieldType#distanceFeatureQuery ?

Copy link
Contributor Author

@mayya-sharipova mayya-sharipova Oct 15, 2020

Choose a reason for hiding this comment

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

Good point, I or Nik will do that.

}

String fieldName() {
Expand Down
5 changes: 2 additions & 3 deletions server/src/test/java/org/elasticsearch/index/query/DistanceFeatureQueryBuilderTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,11 @@ protected void doAssertLuceneQuery(DistanceFeatureQueryBuilder queryBuilder,
String fieldName = expectedFieldName(queryBuilder.fieldName());
Object origin = queryBuilder.origin().origin();
String pivot = queryBuilder.pivot();
float boost = queryBuilder.boost;
final Query expectedQuery;
if (fieldName.equals(GEO_POINT_FIELD_NAME)) {
GeoPoint originGeoPoint = (origin instanceof GeoPoint)? (GeoPoint) origin : GeoUtils.parseFromString((String) origin);
double pivotDouble = DistanceUnit.DEFAULT.parse(pivot, DistanceUnit.DEFAULT);
expectedQuery = LatLonPoint.newDistanceFeatureQuery(fieldName, boost, originGeoPoint.lat(), originGeoPoint.lon(), pivotDouble);
expectedQuery = LatLonPoint.newDistanceFeatureQuery(fieldName, 1.0f, originGeoPoint.lat(), originGeoPoint.lon(), pivotDouble);
} else { // if (fieldName.equals(DATE_FIELD_NAME))
DateFieldType fieldType = (DateFieldType) context.getFieldType(fieldName);
long originLong = fieldType.parseToLong(origin, true, null, null, context::nowInMillis);
Expand All @@ -93,7 +92,7 @@ protected void doAssertLuceneQuery(DistanceFeatureQueryBuilder queryBuilder,
} else { // NANOSECONDS
pivotLong = pivotVal.getNanos();
}
expectedQuery = LongPoint.newDistanceFeatureQuery(fieldName, boost, originLong, pivotLong);
expectedQuery = LongPoint.newDistanceFeatureQuery(fieldName, 1.0f, originLong, pivotLong);
}
assertEquals(expectedQuery, query);
}
Expand Down