SCRIPTING: Support BucketAggScript return null (elastic#32811)

* As explained in elastic#32790, `BucketAggregationScript` must support `null` as a return value * Closes elastic#32790
original-brownbear · Aug 14, 2018 · 125ad89 · 125ad89
1 parent 5f656d7
commit 125ad89
Show file tree

Hide file tree

Showing 5 changed files with 47 additions and 9 deletions.
diff --git a/...-expression/src/main/java/org/elasticsearch/script/expression/ExpressionScriptEngine.java b/...-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
@@ -46,7 +46,7 @@ public Map<String, Object> getParams() {
         return params;
     }
 
-    public abstract double execute();
+    public abstract Double execute();
 
     public interface Factory {
         BucketAggregationScript newInstance(Map<String, Object> params);

diff --git a/...asticsearch/search/aggregations/pipeline/bucketscript/BucketScriptPipelineAggregator.java b/...asticsearch/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<InternalAggregation> aggs = StreamSupport.stream(bucket.getAggregations().spliterator(), false).map(
+                Double returned = factory.newInstance(vars).execute();
+                if (returned == null) {
+                    newBuckets.add(bucket);
+                } else {
+                    final List<InternalAggregation> 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
@@ -117,6 +117,8 @@ protected Map<String, Function<Map<String, Object>, 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<? extends Histogram.Bucket> 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
@@ -111,8 +111,13 @@ public void execute(Map<String, Object> 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);