From 770447ce1af04ab7091332d0b0773c68b02bb9b0 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Wed, 16 Jul 2014 15:09:02 -0700 Subject: [PATCH] Scripting: Remove setNextScore in SearchScript. While it would be nice to do this all the way up the chain (into score functions), this at least removes the weird dual setNextScore/setScorer for SearchScripts. closes #6864 --- .../search/function/ScriptScoreFunction.java | 51 +++++++++++++- .../script/AbstractSearchScript.java | 16 +---- .../elasticsearch/script/SearchScript.java | 2 - .../script/expression/ExpressionScript.java | 66 +++---------------- .../groovy/GroovyScriptEngineService.java | 34 ++++------ .../GroovyScriptExecutionException.java | 3 + .../support/FieldDataSourceTests.java | 4 -- .../support/ScriptValuesTests.java | 4 -- 8 files changed, 75 insertions(+), 105 deletions(-) diff --git a/src/main/java/org/elasticsearch/common/lucene/search/function/ScriptScoreFunction.java b/src/main/java/org/elasticsearch/common/lucene/search/function/ScriptScoreFunction.java index bc9fff9483b..ea2e76c3bac 100644 --- a/src/main/java/org/elasticsearch/common/lucene/search/function/ScriptScoreFunction.java +++ b/src/main/java/org/elasticsearch/common/lucene/search/function/ScriptScoreFunction.java @@ -21,17 +21,60 @@ package org.elasticsearch.common.lucene.search.function; import org.apache.lucene.index.AtomicReaderContext; import org.apache.lucene.search.Explanation; +import org.apache.lucene.search.Scorer; import org.elasticsearch.script.ExplainableSearchScript; import org.elasticsearch.script.SearchScript; +import java.io.IOException; import java.util.Map; public class ScriptScoreFunction extends ScoreFunction { + static final class CannedScorer extends Scorer { + protected int docid; + protected float score; + + public CannedScorer() { + super(null); + } + + @Override + public int docID() { + return docid; + } + + @Override + public float score() throws IOException { + return score; + } + + @Override + public int freq() throws IOException { + throw new UnsupportedOperationException(); + } + + @Override + public int nextDoc() throws IOException { + throw new UnsupportedOperationException(); + } + + @Override + public int advance(int target) throws IOException { + throw new UnsupportedOperationException(); + } + + @Override + public long cost() { + return 1; + } + } + private final String sScript; private final Map params; + private final CannedScorer scorer; + private final SearchScript script; @@ -40,6 +83,8 @@ public class ScriptScoreFunction extends ScoreFunction { this.sScript = sScript; this.params = params; this.script = script; + this.scorer = new CannedScorer(); + script.setScorer(scorer); } @Override @@ -50,7 +95,8 @@ public class ScriptScoreFunction extends ScoreFunction { @Override public double score(int docId, float subQueryScore) { script.setNextDocId(docId); - script.setNextScore(subQueryScore); + scorer.docid = docId; + scorer.score = subQueryScore; return script.runAsDouble(); } @@ -59,7 +105,8 @@ public class ScriptScoreFunction extends ScoreFunction { Explanation exp; if (script instanceof ExplainableSearchScript) { script.setNextDocId(docId); - script.setNextScore(subQueryExpl.getValue()); + scorer.docid = docId; + scorer.score = subQueryExpl.getValue(); exp = ((ExplainableSearchScript) script).explain(subQueryExpl); } else { double score = score(docId, subQueryExpl.getValue()); diff --git a/src/main/java/org/elasticsearch/script/AbstractSearchScript.java b/src/main/java/org/elasticsearch/script/AbstractSearchScript.java index 769aac02158..13ac54bd577 100644 --- a/src/main/java/org/elasticsearch/script/AbstractSearchScript.java +++ b/src/main/java/org/elasticsearch/script/AbstractSearchScript.java @@ -29,6 +29,7 @@ import org.elasticsearch.search.lookup.FieldsLookup; import org.elasticsearch.search.lookup.SearchLookup; import org.elasticsearch.search.lookup.SourceLookup; +import java.io.IOException; import java.util.Map; /** @@ -44,16 +45,6 @@ public abstract class AbstractSearchScript extends AbstractExecutableScript impl private SearchLookup lookup; - private float score = Float.NaN; - - /** - * Returns the current score and only applicable when used as a scoring script in a custom score query!. - * For other cases, use {@link #doc()} and get the score from it. - */ - protected final float score() { - return score; - } - /** * Returns the doc lookup allowing to access field data (cached) values as well as the current document score * (where applicable). @@ -128,11 +119,6 @@ public abstract class AbstractSearchScript extends AbstractExecutableScript impl lookup.source().setNextSource(source); } - @Override - public void setNextScore(float score) { - this.score = score; - } - @Override public float runAsFloat() { return ((Number) run()).floatValue(); diff --git a/src/main/java/org/elasticsearch/script/SearchScript.java b/src/main/java/org/elasticsearch/script/SearchScript.java index 33ed6e67fbe..52210590c71 100644 --- a/src/main/java/org/elasticsearch/script/SearchScript.java +++ b/src/main/java/org/elasticsearch/script/SearchScript.java @@ -36,8 +36,6 @@ public interface SearchScript extends ExecutableScript, ReaderContextAware, Scor void setNextSource(Map source); - void setNextScore(float score); - float runAsFloat(); long runAsLong(); diff --git a/src/main/java/org/elasticsearch/script/expression/ExpressionScript.java b/src/main/java/org/elasticsearch/script/expression/ExpressionScript.java index a1e2913ccc2..10b4cc99c4b 100644 --- a/src/main/java/org/elasticsearch/script/expression/ExpressionScript.java +++ b/src/main/java/org/elasticsearch/script/expression/ExpressionScript.java @@ -38,65 +38,25 @@ import java.util.Map; */ class ExpressionScript implements SearchScript { - /** Fake scorer for a single document */ - static class CannedScorer extends Scorer { - protected int docid; - protected float score; - - public CannedScorer() { - super(null); - } - - @Override - public int docID() { - return docid; - } - - @Override - public float score() throws IOException { - return score; - } - - @Override - public int freq() throws IOException { - throw new UnsupportedOperationException(); - } - - @Override - public int nextDoc() throws IOException { - throw new UnsupportedOperationException(); - } - - @Override - public int advance(int target) throws IOException { - throw new UnsupportedOperationException(); - } - - @Override - public long cost() { - return 1; - } - } - final Expression expression; final XSimpleBindings bindings; - final CannedScorer scorer; final ValueSource source; - final Map context; final ReplaceableConstValueSource specialValue; // _value + Map context; + Scorer scorer; FunctionValues values; + int docid; ExpressionScript(Expression e, XSimpleBindings b, ReplaceableConstValueSource v) { expression = e; bindings = b; - scorer = new CannedScorer(); - context = Collections.singletonMap("scorer", scorer); + context = Collections.EMPTY_MAP; source = expression.getValueSource(bindings); specialValue = v; } double evaluate() { - return values.doubleVal(scorer.docid); + return values.doubleVal(docid); } @Override @@ -116,17 +76,7 @@ class ExpressionScript implements SearchScript { @Override public void setNextDocId(int d) { - scorer.docid = d; - } - - @Override - public void setNextScore(float score) { - // TODO: fix this API to remove setNextScore and just use a Scorer - // Expressions know if they use the score or not, and should be able to pull from the scorer only - // if they need it. Right now, score can only be used within a ScriptScoreFunction. But there shouldn't - // be any reason a script values or aggregation can't use the score. It is also possible - // these layers are preventing inlining of scoring into expressions. - scorer.score = score; + docid = d; } @Override @@ -140,8 +90,8 @@ class ExpressionScript implements SearchScript { @Override public void setScorer(Scorer s) { - // noop: The scorer isn't actually ever set. Instead setNextScore is called. - // NOTE: This seems broken. Why can't we just use the scorer and get rid of setNextScore? + scorer = s; + context = Collections.singletonMap("scorer", scorer); } @Override diff --git a/src/main/java/org/elasticsearch/script/groovy/GroovyScriptEngineService.java b/src/main/java/org/elasticsearch/script/groovy/GroovyScriptEngineService.java index 5f12729062f..76bb1f80436 100644 --- a/src/main/java/org/elasticsearch/script/groovy/GroovyScriptEngineService.java +++ b/src/main/java/org/elasticsearch/script/groovy/GroovyScriptEngineService.java @@ -45,6 +45,7 @@ import org.elasticsearch.script.ExecutableScript; import org.elasticsearch.script.ScriptEngineService; import org.elasticsearch.script.ScriptException; import org.elasticsearch.script.SearchScript; +import org.elasticsearch.search.lookup.DocLookup; import org.elasticsearch.search.lookup.SearchLookup; import java.io.IOException; @@ -188,7 +189,6 @@ public class GroovyScriptEngineService extends AbstractComponent implements Scri private final Script script; private final SearchLookup lookup; private final Map variables; - private final UpdateableFloat score; private final ESLogger logger; public GroovyScript(Script script, ESLogger logger) { @@ -200,10 +200,8 @@ public class GroovyScriptEngineService extends AbstractComponent implements Scri this.lookup = lookup; this.logger = logger; this.variables = script.getBinding().getVariables(); - this.score = new UpdateableFloat(0); - // Add the _score variable, which will be updated per-document by - // setting .value on the UpdateableFloat instance - this.variables.put("_score", this.score); + // Add the _score variable, which will access score from lookup.doc() + this.variables.put("_score", new ScoreAccessor()); } @Override @@ -227,12 +225,6 @@ public class GroovyScriptEngineService extends AbstractComponent implements Scri } } - @SuppressWarnings({"unchecked"}) - @Override - public void setNextScore(float score) { - this.score.value = score; - } - @SuppressWarnings({"unchecked"}) @Override public void setNextVar(String name, Object value) { @@ -284,32 +276,34 @@ public class GroovyScriptEngineService extends AbstractComponent implements Scri * so that updating the _score for the next document incurs only the * overhead of setting a member variable */ - private final class UpdateableFloat extends Number { + private final class ScoreAccessor extends Number { - public float value; - - public UpdateableFloat(float value) { - this.value = value; + float score() { + try { + return lookup.doc().score(); + } catch (IOException e) { + throw new GroovyScriptExecutionException("Could not get score", e); + } } @Override public int intValue() { - return (int)value; + return (int)score(); } @Override public long longValue() { - return (long)value; + return (long)score(); } @Override public float floatValue() { - return value; + return score(); } @Override public double doubleValue() { - return value; + return score(); } } } diff --git a/src/main/java/org/elasticsearch/script/groovy/GroovyScriptExecutionException.java b/src/main/java/org/elasticsearch/script/groovy/GroovyScriptExecutionException.java index 5c0a531f0af..cb19b488deb 100644 --- a/src/main/java/org/elasticsearch/script/groovy/GroovyScriptExecutionException.java +++ b/src/main/java/org/elasticsearch/script/groovy/GroovyScriptExecutionException.java @@ -30,6 +30,9 @@ public class GroovyScriptExecutionException extends ElasticsearchException { public GroovyScriptExecutionException(String message) { super(message); } + public GroovyScriptExecutionException(String message, Throwable t) { + super(message, t); + } @Override public RestStatus status() { diff --git a/src/test/java/org/elasticsearch/search/aggregations/support/FieldDataSourceTests.java b/src/test/java/org/elasticsearch/search/aggregations/support/FieldDataSourceTests.java index 4f18bd4ee45..27d0f7f56e5 100644 --- a/src/test/java/org/elasticsearch/search/aggregations/support/FieldDataSourceTests.java +++ b/src/test/java/org/elasticsearch/search/aggregations/support/FieldDataSourceTests.java @@ -90,10 +90,6 @@ public class FieldDataSourceTests extends ElasticsearchTestCase { public void setNextSource(Map source) { } - @Override - public void setNextScore(float score) { - } - @Override public float runAsFloat() { throw new UnsupportedOperationException(); diff --git a/src/test/java/org/elasticsearch/search/aggregations/support/ScriptValuesTests.java b/src/test/java/org/elasticsearch/search/aggregations/support/ScriptValuesTests.java index f1d839d38c2..088ed9fd115 100644 --- a/src/test/java/org/elasticsearch/search/aggregations/support/ScriptValuesTests.java +++ b/src/test/java/org/elasticsearch/search/aggregations/support/ScriptValuesTests.java @@ -81,10 +81,6 @@ public class ScriptValuesTests extends ElasticsearchTestCase { public void setNextSource(Map source) { } - @Override - public void setNextScore(float score) { - } - @Override public float runAsFloat() { throw new UnsupportedOperationException();