From d412230cdaa7a463c218af7f2c90041a9bccb416 Mon Sep 17 00:00:00 2001 From: Armin Braun Date: Mon, 13 Aug 2018 20:08:26 +0200 Subject: [PATCH] SCRIPTING: Support BucketAggScript return null (#32811) * As explained in #32790, `BucketAggregationScript` must support `null` as a return value * Closes #32790 --- .../expression/ExpressionScriptEngine.java | 2 +- .../script/BucketAggregationScript.java | 2 +- .../BucketScriptPipelineAggregator.java | 14 +++++---- .../aggregations/pipeline/BucketScriptIT.java | 29 +++++++++++++++++++ .../script/MockScriptEngine.java | 9 ++++-- 5 files changed, 47 insertions(+), 9 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 a3a62225b09fb..23dc0fd276cbe 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 @@ -147,7 +147,7 @@ private static BucketAggregationScript.Factory newBucketAggregationScriptFactory } return new BucketAggregationScript(parameters) { @Override - public double execute() { + public Double execute() { getParams().forEach((name, value) -> { ReplaceableConstDoubleValues placeholder = functionValuesMap.get(name); if (placeholder == null) { diff --git a/server/src/main/java/org/elasticsearch/script/BucketAggregationScript.java b/server/src/main/java/org/elasticsearch/script/BucketAggregationScript.java index 5d4a934969424..5fa8d1fbf94a4 100644 --- a/server/src/main/java/org/elasticsearch/script/BucketAggregationScript.java +++ b/server/src/main/java/org/elasticsearch/script/BucketAggregationScript.java @@ -46,7 +46,7 @@ public Map getParams() { return params; } - public abstract double execute(); + public abstract Double execute(); public interface Factory { BucketAggregationScript newInstance(Map params); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketscript/BucketScriptPipelineAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketscript/BucketScriptPipelineAggregator.java index c8117f9029bb6..042a30695c61d 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketscript/BucketScriptPipelineAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketscript/BucketScriptPipelineAggregator.java @@ -110,13 +110,17 @@ public InternalAggregation reduce(InternalAggregation aggregation, ReduceContext if (skipBucket) { newBuckets.add(bucket); } else { - double returned = factory.newInstance(vars).execute(); - final List aggs = StreamSupport.stream(bucket.getAggregations().spliterator(), false).map( + Double returned = factory.newInstance(vars).execute(); + if (returned == null) { + newBuckets.add(bucket); + } else { + final List aggs = StreamSupport.stream(bucket.getAggregations().spliterator(), false).map( (p) -> (InternalAggregation) p).collect(Collectors.toList()); - aggs.add(new InternalSimpleValue(name(), returned, formatter, new ArrayList<>(), metaData())); - InternalMultiBucketAggregation.InternalBucket newBucket = originalAgg.createBucket(new InternalAggregations(aggs), + aggs.add(new InternalSimpleValue(name(), returned, formatter, new ArrayList<>(), metaData())); + InternalMultiBucketAggregation.InternalBucket newBucket = originalAgg.createBucket(new InternalAggregations(aggs), bucket); - newBuckets.add(newBucket); + newBuckets.add(newBucket); + } } } return originalAgg.create(newBuckets); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/BucketScriptIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/BucketScriptIT.java index 398044edcaf53..9e85455d96de9 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/BucketScriptIT.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/BucketScriptIT.java @@ -117,6 +117,8 @@ protected Map, Object>> pluginScripts() { return value0 + value1 + value2; }); + scripts.put("return null", vars -> null); + return scripts; } } @@ -478,6 +480,33 @@ public void testInlineScriptInsertZeros() { } } + public void testInlineScriptReturnNull() { + SearchResponse response = client() + .prepareSearch("idx") + .addAggregation( + histogram("histo") + .field(FIELD_1_NAME).interval(interval) + .subAggregation( + bucketScript( + "nullField", + new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "return null", Collections.emptyMap()) + ) + ) + ).execute().actionGet(); + + assertSearchResponse(response); + + Histogram histo = response.getAggregations().get("histo"); + assertThat(histo, notNullValue()); + assertThat(histo.getName(), equalTo("histo")); + List buckets = histo.getBuckets(); + + for (int i = 0; i < buckets.size(); ++i) { + Histogram.Bucket bucket = buckets.get(i); + assertNull(bucket.getAggregations().get("nullField")); + } + } + public void testStoredScript() { assertAcked(client().admin().cluster().preparePutStoredScript() .setId("my_script") diff --git a/test/framework/src/main/java/org/elasticsearch/script/MockScriptEngine.java b/test/framework/src/main/java/org/elasticsearch/script/MockScriptEngine.java index 8083931e73d03..0d340a91d4cea 100644 --- a/test/framework/src/main/java/org/elasticsearch/script/MockScriptEngine.java +++ b/test/framework/src/main/java/org/elasticsearch/script/MockScriptEngine.java @@ -111,8 +111,13 @@ public void execute(Map ctx) { } else if (context.instanceClazz.equals(BucketAggregationScript.class)) { BucketAggregationScript.Factory factory = parameters -> new BucketAggregationScript(parameters) { @Override - public double execute() { - return ((Number) script.apply(getParams())).doubleValue(); + public Double execute() { + Object ret = script.apply(getParams()); + if (ret == null) { + return null; + } else { + return ((Number) ret).doubleValue(); + } } }; return context.factoryClazz.cast(factory);