From ddf11cc97c03c255818f7a525184e89005edc535 Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Tue, 19 Sep 2023 15:48:58 +0200 Subject: [PATCH 1/3] Adjust ExpressionAggregationScript to support inter-segment concurrency Handling of _value in a script agg does not support search concurrency when using the expression script engine. The reason is that the value gets set globally assuming sequential execution. This commit addresses that by setting the value to the values source associated with the correct leaf reader context, while it was previosly being set on a shared data structure. Closes #99156 --- .../script/expression/MoreExpressionIT.java | 1 - .../ExpressionAggregationScript.java | 2 +- .../ReplaceableConstDoubleValueSource.java | 23 ++++++++++++------- 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/modules/lang-expression/src/internalClusterTest/java/org/elasticsearch/script/expression/MoreExpressionIT.java b/modules/lang-expression/src/internalClusterTest/java/org/elasticsearch/script/expression/MoreExpressionIT.java index 02c17977daf6a..e7d6d127174ec 100644 --- a/modules/lang-expression/src/internalClusterTest/java/org/elasticsearch/script/expression/MoreExpressionIT.java +++ b/modules/lang-expression/src/internalClusterTest/java/org/elasticsearch/script/expression/MoreExpressionIT.java @@ -502,7 +502,6 @@ public void testSpecialValueVariable() throws Exception { assertThat(stats.getAvg(), equalTo(3.0)); } - @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/99156") public void testStringSpecialValueVariable() throws Exception { // i.e. expression script for term aggregations, which is not allowed assertAcked(indicesAdmin().prepareCreate("test").setMapping("text", "type=keyword").get()); diff --git a/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionAggregationScript.java b/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionAggregationScript.java index ec9435d9386b5..df08c0908e182 100644 --- a/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionAggregationScript.java +++ b/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionAggregationScript.java @@ -83,7 +83,7 @@ public void setNextAggregationValue(Object value) { // _value isn't used in script if specialValue == null if (specialValue != null) { if (value instanceof Number) { - specialValue.setValue(((Number) value).doubleValue()); + specialValue.setValue(leaf, ((Number) value).doubleValue()); } else { throw new GeneralScriptException("Cannot use expression with text variable using " + exprScript); } 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 50a70fccdcd44..8aecf2ecdd9d7 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 @@ -15,20 +15,21 @@ import org.apache.lucene.search.IndexSearcher; import java.io.IOException; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; /** * A {@link DoubleValuesSource} which has a stub {@link DoubleValues} that holds a dynamically replaceable constant double. */ final class ReplaceableConstDoubleValueSource extends DoubleValuesSource { - final ReplaceableConstDoubleValues fv; - ReplaceableConstDoubleValueSource() { - fv = new ReplaceableConstDoubleValues(); - } + Map specialValues = new ConcurrentHashMap<>(); @Override public DoubleValues getValues(LeafReaderContext ctx, DoubleValues scores) throws IOException { - return fv; + ReplaceableConstDoubleValues replaceableConstDoubleValues = new ReplaceableConstDoubleValues(); + specialValues.put(ctx, replaceableConstDoubleValues); + return replaceableConstDoubleValues; } @Override @@ -38,8 +39,12 @@ public boolean needsScores() { @Override public Explanation explain(LeafReaderContext ctx, int docId, Explanation scoreExplanation) throws IOException { - if (fv.advanceExact(docId)) return Explanation.match((float) fv.doubleValue(), "ReplaceableConstDoubleValues"); - else return Explanation.noMatch("ReplaceableConstDoubleValues"); + //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"); } @Override @@ -52,7 +57,9 @@ public int hashCode() { return System.identityHashCode(this); } - public void setValue(double v) { + public void setValue(LeafReaderContext ctx, double v) { + ReplaceableConstDoubleValues fv = specialValues.get(ctx); + assert fv != null : "getValues must be called before setValue for any given leaf reader context"; fv.setValue(v); } From a40a36e0129261c0a06decaf3450bcf262900869 Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Tue, 19 Sep 2023 15:52:58 +0200 Subject: [PATCH 2/3] spotless --- .../script/expression/ReplaceableConstDoubleValueSource.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 8aecf2ecdd9d7..08ca39f14f1ab 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 @@ -39,7 +39,7 @@ 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. + // 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"); From d28d8d5655e2f8da6e5fd22e3df6e6f4a55f21c4 Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Tue, 19 Sep 2023 15:54:26 +0200 Subject: [PATCH 3/3] iter --- .../script/expression/ReplaceableConstDoubleValueSource.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 08ca39f14f1ab..903ddaf72340e 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 @@ -23,7 +23,7 @@ */ final class ReplaceableConstDoubleValueSource extends DoubleValuesSource { - Map specialValues = new ConcurrentHashMap<>(); + private final Map specialValues = new ConcurrentHashMap<>(); @Override public DoubleValues getValues(LeafReaderContext ctx, DoubleValues scores) throws IOException {