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

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<String, Object> 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());

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();

src/main/java/org/elasticsearch/script/SearchScript.java

@@ -36,8 +36,6 @@ public interface SearchScript extends ExecutableScript, ReaderContextAware, Scor
 
     void setNextSource(Map<String, Object> source);
 
-    void setNextScore(float score);
-
     float runAsFloat();
 
     long runAsLong();

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<String, CannedScorer> context;
     final ReplaceableConstValueSource specialValue; // _value
+    Map<String, Scorer> context;
+    Scorer scorer;
     FunctionValues values;
+    int docid;
 
     ExpressionScript(Expression e, XSimpleBindings b, ReplaceableConstValueSource v) {
         expression = e;
         bindings = b;
-        scorer = new CannedScorer();
+        context = Collections.EMPTY_MAP;
-        context = Collections.singletonMap("scorer", scorer);
         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;
+        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;
     }
 
     @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.
+        scorer = s;
-         // NOTE: This seems broken.  Why can't we just use the scorer and get rid of setNextScore?
+        context = Collections.singletonMap("scorer", scorer);
     }
 
     @Override

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<String, Object> 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 access score from lookup.doc()
-            // Add the _score variable, which will be updated per-document by
+            this.variables.put("_score", new ScoreAccessor());
-            // setting .value on the UpdateableFloat instance
-            this.variables.put("_score", this.score);
         }
 
         @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;
+            float score() {
-
+                try {
-            public UpdateableFloat(float value) {
+                    return lookup.doc().score();
-                this.value = value;
+                } 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();
             }
         }
     }

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() {

src/test/java/org/elasticsearch/search/aggregations/support/FieldDataSourceTests.java

@@ -90,10 +90,6 @@ public class FieldDataSourceTests extends ElasticsearchTestCase {
             public void setNextSource(Map<String, Object> source) {
             }
 
-            @Override
-            public void setNextScore(float score) {
-            }
-
             @Override
             public float runAsFloat() {
                 throw new UnsupportedOperationException();

src/test/java/org/elasticsearch/search/aggregations/support/ScriptValuesTests.java

@@ -81,10 +81,6 @@ public class ScriptValuesTests extends ElasticsearchTestCase {
         public void setNextSource(Map<String, Object> source) {
         }
 
-        @Override
-        public void setNextScore(float score) {
-        }
-
         @Override
         public float runAsFloat() {
             throw new UnsupportedOperationException();