Expression script engine cleanups around _value usage #99706
Conversation
|
run elasticsearch-ci/part-1 |
|
Heya @original-brownbear you may have more history on this, although it has been a long while. We had some discussion in #99667 around the need for explain in
I am not sure exactly what that means, but I am not sure whether we need to keep _value as a constant (0) or we can even stop adding it to the bindings. |
javanna
changed the title
|
@javanna I'm really sorry but this PR and all the context around it is 100% gone from my memory :/ |
| 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... |
There was a problem hiding this comment.
@rjernst could you eyeball this? do we still need to add it to the bindings even if constant? Shall we try and clean up the TODO? Does it still make sense to you? There is another one, exactly the same text in newAggregationScript above.
There was a problem hiding this comment.
Here is my recollection:
When we added contexts to scripting, we had to match what was possible in the existing scripts. There were two types of scripts, one was a general purpose script that all of the variables were passed, and another which was used by both aggregations and scoring that had some special variables available. Since there were only these two script types, much of the functionality simply didn't work when used in certain places.
_value is the best example of this special case. It is filled in when an general aggregation script operates on multiple values for the same document. This doesn't make sense for scoring scripts, since they are only executed once for a document, rather than per value. I think that we added _value into score scripts in order to avoid breaking users who might have _value in their scripts, even if it was always 0.
IMO we should remove _value altogether, it just doesn't make sense, since it is always 0. Remember though that we lack a clean way to do deprecations in scripting. We have done it before, and it is easier here in expressions since we do not operate in the reduced permission environment of painless scripts. I think starting to clean this up by making it a constant is fine for now, but I suggest following up with deprecating and removing _value from score scripts.
There was a problem hiding this comment.
Also note that _value is not available in painless score scripts, it is only in expression scripts.
There was a problem hiding this comment.
Thanks for all the context Ryan!
Bummer! Totally understandable, thanks for checking. |
javanna
added
:Core/Infra/Scripting
javanna
changed the title
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
We have found some inconsistencies as part of #99667, around the current usages of
ReplaceableConstDoubleValueSourceinExpressionsScriptEngine. It looks like_valueis exposed to the bindings of score scripts, butsetValueis never called hence it will always be0. 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.