Skip to content

Commit

Permalink
Expression script engine cleanups around _value usage (elastic#99706)
Browse files Browse the repository at this point in the history
We have found some inconsistencies as part of elastic#99667, around the current usages of ReplaceableConstDoubleValueSource in ExpressionsScriptEngine. It looks like _value is exposed to the bindings of score scripts, but setValue is never called hence it will always be 0. That can be replaced with a constant double values source, but the next question is whether it even needs to be added to the bindings then.

Another cleanup discussed in elastic#99667 is throwing UnsupportedOperationException from ReplaceableConstDoubleValueSource#explain as it should never be called. Implementing the method means we need to test it which makes little sense if the method is never called in production code.
  • Loading branch information
javanna authored Sep 25, 2023
1 parent 923a944 commit db10810
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -371,16 +371,14 @@ private static ScoreScript.LeafFactory newScoreScript(Expression expr, SearchLoo
// NOTE: if we need to do anything complicated with bindings in the future, we can just extend Bindings,
// instead of complicating SimpleBindings (which should stay simple)
SimpleBindings bindings = new SimpleBindings();
ReplaceableConstDoubleValueSource specialValue = null;
boolean needsScores = false;
for (String variable : expr.variables) {
try {
if (variable.equals("_score")) {
bindings.add("_score", DoubleValuesSource.SCORES);
needsScores = true;
} else if (variable.equals("_value")) {
specialValue = new ReplaceableConstDoubleValueSource();
bindings.add("_value", specialValue);
bindings.add("_value", DoubleValuesSource.constant(0));
// noop: _value is special for aggregations, and is handled in ExpressionScriptBindings
// TODO: if some uses it in a scoring expression, they will get a nasty failure when evaluating...need a
// way to know this is for aggregations and so _value is ok to have...
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,8 @@ public boolean needsScores() {
}

@Override
public Explanation explain(LeafReaderContext ctx, int docId, Explanation scoreExplanation) throws IOException {
// TODO where is this explain called? I bet it's never tested, and probably never called.
ReplaceableConstDoubleValues fv = specialValues.get(ctx);
if (fv.advanceExact(docId)) {
return Explanation.match((float) fv.doubleValue(), "ReplaceableConstDoubleValues");
}
return Explanation.noMatch("ReplaceableConstDoubleValues");
public Explanation explain(LeafReaderContext ctx, int docId, Explanation scoreExplanation) {
throw new UnsupportedOperationException("explain is not supported for _value and should never be called");
}

@Override
Expand Down

0 comments on commit db10810

Please sign in to comment.