From db1081030966812dec534ebc36673551e845fec0 Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Mon, 25 Sep 2023 12:25:10 +0200 Subject: [PATCH] Expression script engine cleanups around _value usage (#99706) 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. --- .../script/expression/ExpressionScriptEngine.java | 4 +--- .../expression/ReplaceableConstDoubleValueSource.java | 9 ++------- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionScriptEngine.java b/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionScriptEngine.java index 7870421817466..7d80a0d401013 100644 --- a/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionScriptEngine.java +++ b/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionScriptEngine.java @@ -371,7 +371,6 @@ 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 { @@ -379,8 +378,7 @@ private static ScoreScript.LeafFactory newScoreScript(Expression expr, SearchLoo 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... diff --git a/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ReplaceableConstDoubleValueSource.java b/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ReplaceableConstDoubleValueSource.java index 903ddaf72340e..697eca6226e3c 100644 --- a/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ReplaceableConstDoubleValueSource.java +++ b/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ReplaceableConstDoubleValueSource.java @@ -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