From a4640b4a8a2feefc9d10743ba68fd7a26c81fd2b Mon Sep 17 00:00:00 2001 From: Britta Weber Date: Fri, 19 Sep 2014 18:03:02 +0200 Subject: [PATCH] script with _score: remove dependency of DocLookup and scorer As pointed out in #7487 DocLookup is a variable that is accessible by all scripts for one doc while the query is executed. But the _score and therfore the scorer depends on the current context, that is, which part of query is currently executed. Instead of setting the scorer for DocLookup and have Script access the DocLookup for getting the score, the Scorer should just be explicitely set for each script. DocLookup should not have any reference to a scorer. This was similarly discussed in #7043. This dependency caused a stackoverflow when running script score in combination with an aggregation on _score. Also the wrong scorer was called when nesting several script scores. closes #7487 closes #7819 --- .../script/AbstractSearchScript.java | 2 +- .../elasticsearch/script/ScoreAccessor.java | 9 ++-- .../elasticsearch/script/ScriptService.java | 21 ---------- .../groovy/GroovyScriptEngineService.java | 11 ++--- .../search/lookup/DocLookup.java | 14 ------- .../search/lookup/SearchLookup.java | 4 -- .../aggregations/bucket/DoubleTermsTests.java | 2 +- .../aggregations/bucket/TopHitsTests.java | 2 +- .../search/facet/SimpleFacetsTests.java | 6 +-- .../functionscore/FunctionScoreTests.java | 42 +++++++++++++++++++ 10 files changed, 57 insertions(+), 56 deletions(-) diff --git a/src/main/java/org/elasticsearch/script/AbstractSearchScript.java b/src/main/java/org/elasticsearch/script/AbstractSearchScript.java index 13ac54bd5772c..1b1bb5df9192e 100644 --- a/src/main/java/org/elasticsearch/script/AbstractSearchScript.java +++ b/src/main/java/org/elasticsearch/script/AbstractSearchScript.java @@ -101,7 +101,7 @@ void setLookup(SearchLookup lookup) { @Override public void setScorer(Scorer scorer) { - lookup.setScorer(scorer); + throw new UnsupportedOperationException(); } @Override diff --git a/src/main/java/org/elasticsearch/script/ScoreAccessor.java b/src/main/java/org/elasticsearch/script/ScoreAccessor.java index 38f83f1ac2734..93536e5c29e4d 100644 --- a/src/main/java/org/elasticsearch/script/ScoreAccessor.java +++ b/src/main/java/org/elasticsearch/script/ScoreAccessor.java @@ -19,6 +19,7 @@ package org.elasticsearch.script; +import org.apache.lucene.search.Scorer; import org.elasticsearch.search.lookup.DocLookup; import java.io.IOException; @@ -31,15 +32,15 @@ */ public final class ScoreAccessor extends Number { - final DocLookup doc; + Scorer scorer; - public ScoreAccessor(DocLookup d) { - doc = d; + public ScoreAccessor(Scorer scorer) { + this.scorer = scorer; } float score() { try { - return doc.score(); + return scorer.score(); } catch (IOException e) { throw new RuntimeException("Could not get score", e); } diff --git a/src/main/java/org/elasticsearch/script/ScriptService.java b/src/main/java/org/elasticsearch/script/ScriptService.java index 8d538b38b55c0..e095cc7e5ec45 100644 --- a/src/main/java/org/elasticsearch/script/ScriptService.java +++ b/src/main/java/org/elasticsearch/script/ScriptService.java @@ -236,9 +236,6 @@ public ScriptService(Settings settings, Environment env, Set params) { - return new DocScoreSearchScript(); - } - } - - public static class DocScoreSearchScript extends AbstractFloatSearchScript { - @Override - public float runAsFloat() { - try { - return doc().score(); - } catch (IOException e) { - return 0; - } - } - } } diff --git a/src/main/java/org/elasticsearch/script/groovy/GroovyScriptEngineService.java b/src/main/java/org/elasticsearch/script/groovy/GroovyScriptEngineService.java index f3e4118e6d59f..299d90e71a9c6 100644 --- a/src/main/java/org/elasticsearch/script/groovy/GroovyScriptEngineService.java +++ b/src/main/java/org/elasticsearch/script/groovy/GroovyScriptEngineService.java @@ -43,6 +43,7 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.script.*; import org.elasticsearch.search.lookup.SearchLookup; +import org.elasticsearch.search.suggest.term.TermSuggestion; import java.io.IOException; import java.math.BigDecimal; @@ -186,6 +187,7 @@ public static final class GroovyScript implements ExecutableScript, SearchScript private final SearchLookup lookup; private final Map variables; private final ESLogger logger; + private Scorer scorer; public GroovyScript(Script script, ESLogger logger) { this(script, null, logger); @@ -196,17 +198,12 @@ public GroovyScript(Script script, @Nullable SearchLookup lookup, ESLogger logge this.lookup = lookup; this.logger = logger; this.variables = script.getBinding().getVariables(); - if (lookup != null) { - // Add the _score variable, which will access score from lookup.doc() - this.variables.put("_score", new ScoreAccessor(lookup.doc())); - } } @Override public void setScorer(Scorer scorer) { - if (lookup != null) { - lookup.setScorer(scorer); - } + this.scorer = scorer; + this.variables.put("_score", new ScoreAccessor(scorer)); } @Override diff --git a/src/main/java/org/elasticsearch/search/lookup/DocLookup.java b/src/main/java/org/elasticsearch/search/lookup/DocLookup.java index 9cf56e66da9b8..78d8fb0180354 100644 --- a/src/main/java/org/elasticsearch/search/lookup/DocLookup.java +++ b/src/main/java/org/elasticsearch/search/lookup/DocLookup.java @@ -49,8 +49,6 @@ public class DocLookup implements Map { private AtomicReaderContext reader; - private Scorer scorer; - private int docId = -1; DocLookup(MapperService mapperService, IndexFieldDataService fieldDataService, @Nullable String[] types) { @@ -76,22 +74,10 @@ public void setNextReader(AtomicReaderContext context) { localCacheFieldData.clear(); } - public void setScorer(Scorer scorer) { - this.scorer = scorer; - } - public void setNextDocId(int docId) { this.docId = docId; } - public float score() throws IOException { - return scorer.score(); - } - - public float getScore() throws IOException { - return scorer.score(); - } - @Override public Object get(Object key) { // assume its a string... diff --git a/src/main/java/org/elasticsearch/search/lookup/SearchLookup.java b/src/main/java/org/elasticsearch/search/lookup/SearchLookup.java index 781e24a1c32e6..2dbda01a0e883 100644 --- a/src/main/java/org/elasticsearch/search/lookup/SearchLookup.java +++ b/src/main/java/org/elasticsearch/search/lookup/SearchLookup.java @@ -76,10 +76,6 @@ public DocLookup doc() { return this.docMap; } - public void setScorer(Scorer scorer) { - docMap.setScorer(scorer); - } - public void setNextReader(AtomicReaderContext context) { docMap.setNextReader(context); sourceLookup.setNextReader(context); diff --git a/src/test/java/org/elasticsearch/search/aggregations/bucket/DoubleTermsTests.java b/src/test/java/org/elasticsearch/search/aggregations/bucket/DoubleTermsTests.java index cc4453708840d..886e7abf0ce05 100644 --- a/src/test/java/org/elasticsearch/search/aggregations/bucket/DoubleTermsTests.java +++ b/src/test/java/org/elasticsearch/search/aggregations/bucket/DoubleTermsTests.java @@ -1027,7 +1027,7 @@ public void script_Score() { .setQuery(functionScoreQuery(matchAllQuery()).add(ScoreFunctionBuilders.scriptFunction("doc['" + SINGLE_VALUED_FIELD_NAME + "'].value"))) .addAggregation(terms("terms") .collectMode(randomFrom(SubAggCollectionMode.values())) - .script("ceil(_doc.score()/3)") + .script("ceil(_score.doubleValue()/3)") ).execute().actionGet(); assertSearchResponse(response); diff --git a/src/test/java/org/elasticsearch/search/aggregations/bucket/TopHitsTests.java b/src/test/java/org/elasticsearch/search/aggregations/bucket/TopHitsTests.java index edc438216f94a..0dcec77f83cd3 100644 --- a/src/test/java/org/elasticsearch/search/aggregations/bucket/TopHitsTests.java +++ b/src/test/java/org/elasticsearch/search/aggregations/bucket/TopHitsTests.java @@ -270,7 +270,7 @@ public void testFieldCollapsing() throws Exception { topHits("hits").setSize(1) ) .subAggregation( - max("max_score").script("_doc.score()") + max("max_score").script("_score.doubleValue()") ) ) .get(); diff --git a/src/test/java/org/elasticsearch/search/facet/SimpleFacetsTests.java b/src/test/java/org/elasticsearch/search/facet/SimpleFacetsTests.java index 7375925aeb184..d3a4da1010a53 100644 --- a/src/test/java/org/elasticsearch/search/facet/SimpleFacetsTests.java +++ b/src/test/java/org/elasticsearch/search/facet/SimpleFacetsTests.java @@ -1534,7 +1534,7 @@ public void testHistoFacets() throws Exception { .addFacet(histogramFacet("stats5").field("date").interval(1, TimeUnit.MINUTES)) .addFacet(histogramScriptFacet("stats6").keyField("num").valueScript("doc['num'].value").interval(100)) .addFacet(histogramFacet("stats7").field("num").interval(100)) - .addFacet(histogramScriptFacet("stats8").keyField("num").valueScript("doc.score").interval(100)) + .addFacet(histogramScriptFacet("stats8").keyField("num").valueScript("_score.doubleValue()").interval(100)) .execute().actionGet(); assertSearchResponse(searchResponse); @@ -2266,8 +2266,8 @@ public void testTermsStatsFacets2() throws Exception { for (int i = 0; i < numberOfRuns(); i++) { SearchResponse searchResponse = client().prepareSearch() .setQuery(matchAllQuery()) - .addFacet(termsStatsFacet("stats1").keyField("num").valueScript("doc.score").order(TermsStatsFacet.ComparatorType.COUNT)) - .addFacet(termsStatsFacet("stats2").keyField("num").valueScript("doc.score").order(TermsStatsFacet.ComparatorType.TOTAL)) + .addFacet(termsStatsFacet("stats1").keyField("num").valueScript("_score.doubleValue()").order(TermsStatsFacet.ComparatorType.COUNT)) + .addFacet(termsStatsFacet("stats2").keyField("num").valueScript("_score.doubleValue()").order(TermsStatsFacet.ComparatorType.TOTAL)) .execute().actionGet(); assertSearchResponse(searchResponse); diff --git a/src/test/java/org/elasticsearch/search/functionscore/FunctionScoreTests.java b/src/test/java/org/elasticsearch/search/functionscore/FunctionScoreTests.java index 47c1e06b9eb59..8f8646d5f0a9a 100644 --- a/src/test/java/org/elasticsearch/search/functionscore/FunctionScoreTests.java +++ b/src/test/java/org/elasticsearch/search/functionscore/FunctionScoreTests.java @@ -28,6 +28,7 @@ import org.elasticsearch.index.query.functionscore.FunctionScoreQueryBuilder; import org.elasticsearch.index.query.functionscore.ScoreFunctionBuilder; import org.elasticsearch.index.query.functionscore.weight.WeightBuilder; +import org.elasticsearch.search.aggregations.bucket.terms.Terms; import org.elasticsearch.test.ElasticsearchIntegrationTest; import org.junit.Test; @@ -40,6 +41,7 @@ import static org.elasticsearch.index.query.QueryBuilders.functionScoreQuery; import static org.elasticsearch.index.query.QueryBuilders.termQuery; import static org.elasticsearch.index.query.functionscore.ScoreFunctionBuilders.*; +import static org.elasticsearch.search.aggregations.AggregationBuilders.terms; import static org.elasticsearch.search.builder.SearchSourceBuilder.searchSource; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchResponse; @@ -388,4 +390,44 @@ public void checkWeightOnlyCreatesBoostFunction() throws IOException { assertSearchResponse(response); assertThat(response.getHits().getAt(0).score(), equalTo(2.0f)); } + + @Test + public void testScriptScoresNested() throws IOException { + index(INDEX, TYPE, "1", jsonBuilder().startObject().field("dummy_field", 1).endObject()); + refresh(); + SearchResponse response = client().search( + searchRequest().source( + searchSource().query( + functionScoreQuery( + functionScoreQuery( + functionScoreQuery().add(scriptFunction("1"))) + .add(scriptFunction("_score.doubleValue()"))) + .add(scriptFunction("_score.doubleValue()") + ) + ) + ) + ).actionGet(); + assertSearchResponse(response); + assertThat(response.getHits().getAt(0).score(), equalTo(1.0f)); + } + + @Test + public void testScriptScoresWithAgg() throws IOException { + index(INDEX, TYPE, "1", jsonBuilder().startObject().field("dummy_field", 1).endObject()); + refresh(); + SearchResponse response = client().search( + searchRequest().source( + searchSource().query( + functionScoreQuery() + .add(scriptFunction("_score.doubleValue()") + ) + ).aggregation(terms("score_agg").script("_score.doubleValue()")) + ) + ).actionGet(); + assertSearchResponse(response); + assertThat(response.getHits().getAt(0).score(), equalTo(1.0f)); + assertThat(((Terms) response.getAggregations().asMap().get("score_agg")).getBuckets().get(0).getKeyAsNumber().floatValue(), is(1f)); + assertThat(((Terms) response.getAggregations().asMap().get("score_agg")).getBuckets().get(0).getDocCount(), is(1l)); + } } +