github-mirrors/elasticsearch
1
0
Fork 0
mirror of https://github.com/elastic/elasticsearch.git synced 2025-04-24 15:17:30 -04:00
Code Issues Projects Releases Packages Wiki Activity

Expression script engine cleanups around _value usage (#99706)

Browse source 
We have found some inconsistencies as part of #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 #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.
This commit is contained in:
Luca Cavanna 2023-09-25 12:25:10 +02:00 • committed by GitHub
parent 923a944214
commit db10810309
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 3 additions and 10 deletions
Download patch file Download diff file Expand all files Collapse all files

4
modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionScriptEngine.java
View file

@ -371,7 +371,6 @@ public class ExpressionScriptEngine implements ScriptEngine {
// 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 {
@ -379,8 +378,7 @@ public class ExpressionScriptEngine implements ScriptEngine {
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...

9
modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ReplaceableConstDoubleValueSource.java
View file

@ -38,13 +38,8 @@ final class ReplaceableConstDoubleValueSource extends DoubleValuesSource {
}
@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