Skip to content

Commit

Permalink
Scripting: Remove setNextScore in SearchScript.
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@rjernst
rjernst committed Jul 16, 2014
1 parent 67300ba commit 88e4deb
Show file tree
Hide file tree
Showing 9 changed files with 114 additions and 110 deletions.
51 changes: 49 additions & 2 deletions src/main/java/org/elasticsearch/common/lucene/search/function/ScriptScoreFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,60 @@

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;


Expand All @@ -40,6 +83,8 @@ public ScriptScoreFunction(String sScript, Map<String, Object> params, SearchScr
this.sScript = sScript;
this.params = params;
this.script = script;
this.scorer = new CannedScorer();
script.setScorer(scorer);
}

@Override
Expand All @@ -50,7 +95,8 @@ public void setNextReader(AtomicReaderContext ctx) {
@Override
public double score(int docId, float subQueryScore) {
script.setNextDocId(docId);
script.setNextScore(subQueryScore);
scorer.docid = docId;
scorer.score = subQueryScore;
return script.runAsDouble();
}

Expand All @@ -59,7 +105,8 @@ public Explanation explainScore(int docId, Explanation subQueryExpl) {
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());
Expand Down
16 changes: 1 addition & 15 deletions src/main/java/org/elasticsearch/script/AbstractSearchScript.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.elasticsearch.search.lookup.SearchLookup;
import org.elasticsearch.search.lookup.SourceLookup;

import java.io.IOException;
import java.util.Map;

/**
Expand All @@ -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).
Expand Down Expand Up @@ -128,11 +119,6 @@ public void setNextSource(Map<String, Object> source) {
lookup.source().setNextSource(source);
}

@Override
public void setNextScore(float score) {
this.score = score;
}

@Override
public float runAsFloat() {
return ((Number) run()).floatValue();
Expand Down
2 changes: 0 additions & 2 deletions src/main/java/org/elasticsearch/script/SearchScript.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
66 changes: 8 additions & 58 deletions src/main/java/org/elasticsearch/script/expression/ExpressionScript.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,65 +38,25 @@
*/
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.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
Expand All @@ -116,17 +76,7 @@ public long cost() {

@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
Expand All @@ -140,8 +90,8 @@ public void setNextReader(AtomicReaderContext leaf) {

@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
Expand Down
34 changes: 14 additions & 20 deletions src/main/java/org/elasticsearch/script/groovy/GroovyScriptEngineService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
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;
Expand Down Expand Up @@ -188,7 +189,6 @@ public static final class GroovyScript implements ExecutableScript, SearchScript
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) {
Expand All @@ -200,10 +200,8 @@ public GroovyScript(Script script, SearchLookup lookup, ESLogger logger) {
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
Expand All @@ -227,12 +225,6 @@ public void setNextDocId(int doc) {
}
}

@SuppressWarnings({"unchecked"})
@Override
public void setNextScore(float score) {
this.score.value = score;
}

@SuppressWarnings({"unchecked"})
@Override
public void setNextVar(String name, Object value) {
Expand Down Expand Up @@ -284,32 +276,34 @@ public Object unwrap(Object value) {
* so that updating the _score for the next document incurs only the
* overhead of setting a member variable
*/
private final class UpdateableFloat extends Number {

public float value;
private final class ScoreAccessor extends Number {

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();
}
}
}
Expand Down
3 changes: 3 additions & 0 deletions src/main/java/org/elasticsearch/script/groovy/GroovyScriptExecutionException.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
Loading

0 comments on commit 88e4deb

Please sign in to comment.