From 2c0145a6c0b7ea1e0a23d67c10b839cb5260187d Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Wed, 21 Oct 2020 12:01:18 -0400 Subject: [PATCH] Drop boost from runtime distance feature query (#63949) (#64005) 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 --- .../index/mapper/DateFieldMapper.java | 5 +-- .../index/mapper/GeoPointFieldMapper.java | 5 +-- .../index/mapper/MappedFieldType.java | 2 +- .../query/DistanceFeatureQueryBuilder.java | 3 +- .../mapper/DateScriptFieldType.java | 5 ++- .../LongScriptFieldDistanceFeatureQuery.java | 23 ++++--------- .../mapper/DateScriptFieldTypeTests.java | 4 +-- ...gScriptFieldDistanceFeatureQueryTests.java | 34 ++++++------------- 8 files changed, 30 insertions(+), 51 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java index 635ba77eeabc4..c3137e549937b 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java @@ -441,10 +441,11 @@ public static long parseToLong( } @Override - public Query distanceFeatureQuery(Object origin, String pivot, float boost, QueryShardContext context) { + public Query distanceFeatureQuery(Object origin, String pivot, QueryShardContext context) { long originLong = parseToLong(origin, true, null, null, context::nowInMillis); TimeValue pivotTime = TimeValue.parseTimeValue(pivot, "distance_feature.pivot"); - return resolution.distanceFeatureQuery(name(), boost, originLong, pivotTime); + // As we already apply boost in AbstractQueryBuilder::toQuery, we always passing a boost of 1.0 to distanceFeatureQuery + return resolution.distanceFeatureQuery(name(), 1.0f, originLong, pivotTime); } @Override diff --git a/server/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java index 3c69c3f1a9e9b..d84f578df38bb 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java @@ -194,7 +194,7 @@ public IndexFieldData.Builder fielddataBuilder(String fullyQualifiedIndexName, S } @Override - public Query distanceFeatureQuery(Object origin, String pivot, float boost, QueryShardContext context) { + public Query distanceFeatureQuery(Object origin, String pivot, QueryShardContext context) { GeoPoint originGeoPoint; if (origin instanceof GeoPoint) { originGeoPoint = (GeoPoint) origin; @@ -205,7 +205,8 @@ public Query distanceFeatureQuery(Object origin, String pivot, float boost, Quer "Must be of type [geo_point] or [string] for geo_point fields!"); } double pivotDouble = DistanceUnit.DEFAULT.parse(pivot, DistanceUnit.DEFAULT); - return LatLonPoint.newDistanceFeatureQuery(name(), boost, originGeoPoint.lat(), originGeoPoint.lon(), pivotDouble); + // As we already apply boost in AbstractQueryBuilder::toQuery, we always passing a boost of 1.0 to distanceFeatureQuery + return LatLonPoint.newDistanceFeatureQuery(name(), 1.0f, originGeoPoint.lat(), originGeoPoint.lon(), pivotDouble); } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java b/server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java index 720c9a44d9ae4..23481f3e1b93b 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java @@ -305,7 +305,7 @@ public SpanQuery spanPrefixQuery(String value, SpanMultiTermQueryWrapper.SpanRew + "] which is of type [" + typeName() + "]"); } - public Query distanceFeatureQuery(Object origin, String pivot, float boost, QueryShardContext context) { + public Query distanceFeatureQuery(Object origin, String pivot, QueryShardContext context) { throw new IllegalArgumentException("Illegal data type of [" + typeName() + "]!"+ "[" + DistanceFeatureQueryBuilder.NAME + "] query can only be run on a date, date_nanos or geo_point field type!"); } diff --git a/server/src/main/java/org/elasticsearch/index/query/DistanceFeatureQueryBuilder.java b/server/src/main/java/org/elasticsearch/index/query/DistanceFeatureQueryBuilder.java index 9c6a97bf1c300..8517088ce673a 100644 --- a/server/src/main/java/org/elasticsearch/index/query/DistanceFeatureQueryBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/DistanceFeatureQueryBuilder.java @@ -112,8 +112,7 @@ protected Query doToQuery(QueryShardContext context) throws IOException { if (fieldType == null) { return Queries.newMatchNoDocsQuery("Can't run [" + NAME + "] query on unmapped fields!"); } - // 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); + return fieldType.distanceFeatureQuery(origin.origin(), pivot, context); } String fieldName() { diff --git a/x-pack/plugin/runtime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/mapper/DateScriptFieldType.java b/x-pack/plugin/runtime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/mapper/DateScriptFieldType.java index c69148fe43751..c90675ea31b91 100644 --- a/x-pack/plugin/runtime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/mapper/DateScriptFieldType.java +++ b/x-pack/plugin/runtime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/mapper/DateScriptFieldType.java @@ -81,7 +81,7 @@ public DateScriptFieldData.Builder fielddataBuilder(String fullyQualifiedIndexNa } @Override - public Query distanceFeatureQuery(Object origin, String pivot, float boost, QueryShardContext context) { + public Query distanceFeatureQuery(Object origin, String pivot, QueryShardContext context) { checkAllowExpensiveQueries(context); return DateFieldType.handleNow(context, now -> { long originLong = DateFieldType.parseToLong( @@ -98,8 +98,7 @@ public Query distanceFeatureQuery(Object origin, String pivot, float boost, Quer leafFactory(context)::newInstance, name(), originLong, - pivotTime.getMillis(), - boost + pivotTime.getMillis() ); }); } diff --git a/x-pack/plugin/runtime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/query/LongScriptFieldDistanceFeatureQuery.java b/x-pack/plugin/runtime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/query/LongScriptFieldDistanceFeatureQuery.java index b8e56ad141d12..ce3166adf71df 100644 --- a/x-pack/plugin/runtime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/query/LongScriptFieldDistanceFeatureQuery.java +++ b/x-pack/plugin/runtime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/query/LongScriptFieldDistanceFeatureQuery.java @@ -27,20 +27,17 @@ public final class LongScriptFieldDistanceFeatureQuery extends AbstractScriptFieldQuery { private final long origin; private final long pivot; - private final float boost; public LongScriptFieldDistanceFeatureQuery( Script script, Function leafFactory, String fieldName, long origin, - long pivot, - float boost + long pivot ) { super(script, fieldName, leafFactory); this.origin = origin; this.pivot = pivot; - this.boost = boost; } @Override @@ -70,12 +67,11 @@ public Explanation explain(LeafReaderContext context, int doc) { AbstractLongFieldScript script = scriptContextFunction().apply(context); script.runForDoc(doc); long value = valueWithMinAbsoluteDistance(script); - float weight = LongScriptFieldDistanceFeatureQuery.this.boost * boost; - float score = score(weight, distanceFor(value)); + float score = score(boost, distanceFor(value)); return Explanation.match( score, "Distance score, computed as weight * pivot / (pivot + abs(value - origin)) from:", - Explanation.match(weight, "weight"), + Explanation.match(boost, "weight"), Explanation.match(pivot, "pivot"), Explanation.match(origin, "origin"), Explanation.match(value, "current value") @@ -105,7 +101,7 @@ public float matchCost() { } }; disi = TwoPhaseIterator.asDocIdSetIterator(twoPhase); - this.weight = LongScriptFieldDistanceFeatureQuery.this.boost * boost; + this.weight = boost; } @Override @@ -179,15 +175,14 @@ public String toString(String field) { } b.append(getClass().getSimpleName()); b.append("(origin=").append(origin); - b.append(",pivot=").append(pivot); - b.append(",boost=").append(boost).append(")"); + b.append(",pivot=").append(pivot).append(")"); return b.toString(); } @Override public int hashCode() { - return Objects.hash(super.hashCode(), origin, pivot, boost); + return Objects.hash(super.hashCode(), origin, pivot); } @Override @@ -196,7 +191,7 @@ public boolean equals(Object obj) { return false; } LongScriptFieldDistanceFeatureQuery other = (LongScriptFieldDistanceFeatureQuery) obj; - return origin == other.origin && pivot == other.pivot && boost == other.boost; + return origin == other.origin && pivot == other.pivot; } @Override @@ -214,8 +209,4 @@ long origin() { long pivot() { return pivot; } - - float boost() { - return boost; - } } diff --git a/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/DateScriptFieldTypeTests.java b/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/DateScriptFieldTypeTests.java index 9e6e015d520f9..c10b470a06464 100644 --- a/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/DateScriptFieldTypeTests.java +++ b/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/DateScriptFieldTypeTests.java @@ -222,7 +222,7 @@ public void testDistanceFeatureQuery() throws IOException { ); try (DirectoryReader reader = iw.getReader()) { IndexSearcher searcher = newSearcher(reader); - Query query = simpleMappedFieldType().distanceFeatureQuery(1595432181354L, "1ms", 1, mockContext()); + Query query = simpleMappedFieldType().distanceFeatureQuery(1595432181354L, "1ms", mockContext()); TopDocs docs = searcher.search(query, 4); assertThat(docs.scoreDocs, arrayWithSize(3)); assertThat(readSource(reader, docs.scoreDocs[0].doc), equalTo("{\"timestamp\": [1595432181354]}")); @@ -251,7 +251,7 @@ public void testDistanceFeatureQueryInLoop() throws IOException { } private Query randomDistanceFeatureQuery(MappedFieldType ft, QueryShardContext ctx) { - return ft.distanceFeatureQuery(randomDate(), randomTimeValue(), randomFloat(), ctx); + return ft.distanceFeatureQuery(randomDate(), randomTimeValue(), ctx); } @Override diff --git a/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/query/LongScriptFieldDistanceFeatureQueryTests.java b/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/query/LongScriptFieldDistanceFeatureQueryTests.java index db14fe468389b..236a7f8d4e0f0 100644 --- a/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/query/LongScriptFieldDistanceFeatureQueryTests.java +++ b/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/query/LongScriptFieldDistanceFeatureQueryTests.java @@ -35,19 +35,12 @@ public class LongScriptFieldDistanceFeatureQueryTests extends AbstractScriptFiel protected LongScriptFieldDistanceFeatureQuery createTestInstance() { long origin = randomLong(); long pivot = randomValueOtherThan(origin, ESTestCase::randomLong); - return new LongScriptFieldDistanceFeatureQuery(randomScript(), leafFactory, randomAlphaOfLength(5), origin, pivot, randomFloat()); + return new LongScriptFieldDistanceFeatureQuery(randomScript(), leafFactory, randomAlphaOfLength(5), origin, pivot); } @Override protected LongScriptFieldDistanceFeatureQuery copy(LongScriptFieldDistanceFeatureQuery orig) { - return new LongScriptFieldDistanceFeatureQuery( - orig.script(), - leafFactory, - orig.fieldName(), - orig.origin(), - orig.pivot(), - orig.boost() - ); + return new LongScriptFieldDistanceFeatureQuery(orig.script(), leafFactory, orig.fieldName(), orig.origin(), orig.pivot()); } @Override @@ -56,8 +49,7 @@ protected LongScriptFieldDistanceFeatureQuery mutate(LongScriptFieldDistanceFeat String fieldName = orig.fieldName(); long origin = orig.origin(); long pivot = orig.pivot(); - float boost = orig.boost(); - switch (randomInt(4)) { + switch (randomInt(3)) { case 0: script = randomValueOtherThan(script, this::randomScript); break; @@ -70,13 +62,10 @@ protected LongScriptFieldDistanceFeatureQuery mutate(LongScriptFieldDistanceFeat case 3: pivot = randomValueOtherThan(origin, () -> randomValueOtherThan(orig.pivot(), ESTestCase::randomLong)); break; - case 4: - boost = randomValueOtherThan(boost, ESTestCase::randomFloat); - break; default: fail(); } - return new LongScriptFieldDistanceFeatureQuery(script, leafFactory, fieldName, origin, pivot, boost); + return new LongScriptFieldDistanceFeatureQuery(script, leafFactory, fieldName, origin, pivot); } @Override @@ -109,12 +98,13 @@ public void execute() { leafFactory, "test", 1595432181351L, - 6L, - between(1, 100) + 3L ); - TopDocs td = searcher.search(query, 1); - assertThat(td.scoreDocs[0].score, equalTo(query.boost())); + TopDocs td = searcher.search(query, 2); + assertThat(td.scoreDocs[0].score, equalTo(1.0f)); assertThat(td.scoreDocs[0].doc, equalTo(1)); + assertThat(td.scoreDocs[1].score, equalTo(.5f)); + assertThat(td.scoreDocs[1].doc, equalTo(0)); } } } @@ -128,7 +118,7 @@ public void testMaxScore() throws IOException { float boost = randomFloat(); assertThat( query.createWeight(searcher, ScoreMode.COMPLETE, boost).scorer(reader.leaves().get(0)).getMaxScore(randomInt()), - equalTo(query.boost() * boost) + equalTo(boost) ); } } @@ -138,9 +128,7 @@ public void testMaxScore() throws IOException { protected void assertToString(LongScriptFieldDistanceFeatureQuery query) { assertThat( query.toString(query.fieldName()), - equalTo( - "LongScriptFieldDistanceFeatureQuery(origin=" + query.origin() + ",pivot=" + query.pivot() + ",boost=" + query.boost() + ")" - ) + equalTo("LongScriptFieldDistanceFeatureQuery(origin=" + query.origin() + ",pivot=" + query.pivot() + ")") ); }