Apply boost only once for distance_feature query #63767
Conversation
Currently if distance_feature query contains boost, it incorrectly gets applied twice: in AbstractQueryBuilder::toQuery and we also pass this boost to Lucene's LongPoint.newDistanceFeatureQuery. As a result we get incorrect scores. This fixes this error to ensure that boost is applied only once. Closes elastic#63691
mayya-sharipova
added
the
:Search/Search
|
Pinging @elastic/es-search (:Search/Search) |
mayya-sharipova
added
>bug
v7.1.1
v8.0.0
and removed
Team:Search
|
I will update release notes for 7.11 with this change. |
|
@nik9000 I was wondering if we need to adjust also the corresponding |
Probably! |
Do you want to do it as part of this PR or should I take it as a follow up? |
Let's do it in a follow up. If you have time, please do it. If you are busy, I don't mind doing this. |
My stack is non-trivial at the moment but I've added it. It'll be next week probably. |
| @@ -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); | |||
There was a problem hiding this comment.
When we fix LongScriptFieldDistanceFeatureQuery, I think we could remove the boost parameter altogether from MappedFieldType#distanceFeatureQuery ?
There was a problem hiding this comment.
Good point, I or Nik will do that.
| @@ -1626,6 +1627,33 @@ public void testRangeQueryWithLocaleMapping() throws Exception { | |||
| assertHitCount(searchResponse, 2L); | |||
| } | |||
|
|
|||
| public void testDistanceFeatureQuery() { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Currently if distance_feature query contains boost, it incorrectly gets applied twice: in AbstractQueryBuilder::toQuery and we also pass this boost to Lucene's LongPoint.newDistanceFeatureQuery. As a result we get incorrect scores. This fixes this error to ensure that boost is applied only once. Closes #63691
Currently if distance_feature query contains boost, it incorrectly gets applied twice: in AbstractQueryBuilder::toQuery and we also pass this boost to Lucene's LongPoint.newDistanceFeatureQuery. As a result we get incorrect scores. This fixes this error to ensure that boost is applied only once. Closes #63691
This drops the `boost` parameter of the `distance_feature` query builder internally, relying on our query building infrastructure to wrap the query in a `boosting` query. Relates to elastic#63767
This drops the `boost` parameter of the `distance_feature` query builder internally, relying on our query building infrastructure to wrap the query in a `boosting` query. Relates to #63767
This drops the `boost` parameter of the `distance_feature` query builder internally, relying on our query building infrastructure to wrap the query in a `boosting` query. Relates to elastic#63767
Currently if distance_feature query contains boost,
it incorrectly gets applied twice: in AbstractQueryBuilder::toQuery and
we also pass this boost to Lucene's LongPoint.newDistanceFeatureQuery.
As a result we get incorrect scores.
This fixes this error to ensure that boost is applied only once.
Closes #63691