From ab80130c108878883f65f89f015cb5cdaf738d02 Mon Sep 17 00:00:00 2001 From: Colin Goodheart-Smithe Date: Wed, 1 Jul 2015 09:59:27 +0100 Subject: [PATCH] Aggregations: Makes SKIP Gap Policy work correctly for Bucket Script aggregation This change means that when the skip gap policy is used, the bucket script aggregation will skip executing the script on a bucket if any of the required bucket_paths are missing for the bucket. No aggregation will be added to the bucket, and the aggregation will move to the next bucket. --- .../BucketScriptPipelineAggregator.java | 32 ++++++++++++------- .../pipeline/BucketScriptTests.java | 31 ++++++------------ 2 files changed, 30 insertions(+), 33 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketscript/BucketScriptPipelineAggregator.java b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketscript/BucketScriptPipelineAggregator.java index 11086c70040..3b2a4a5a09b 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketscript/BucketScriptPipelineAggregator.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketscript/BucketScriptPipelineAggregator.java @@ -112,26 +112,36 @@ public class BucketScriptPipelineAggregator extends PipelineAggregator { if (script.getParams() != null) { vars.putAll(script.getParams()); } + boolean skipBucket = false; for (Map.Entry entry : bucketsPathsMap.entrySet()) { String varName = entry.getKey(); String bucketsPath = entry.getValue(); Double value = resolveBucketValue(originalAgg, bucket, bucketsPath, gapPolicy); + if (GapPolicy.SKIP == gapPolicy && (value == null || Double.isNaN(value))) { + skipBucket = true; + break; + } vars.put(varName, value); } - ExecutableScript executableScript = reduceContext.scriptService().executable(compiledScript, vars); - Object returned = executableScript.run(); - if (returned == null) { + if (skipBucket) { newBuckets.add(bucket); } else { - if (!(returned instanceof Number)) { - throw new AggregationExecutionException("series_arithmetic script for reducer [" + name() + "] must return a Number"); + ExecutableScript executableScript = reduceContext.scriptService().executable(compiledScript, vars); + Object returned = executableScript.run(); + if (returned == null) { + newBuckets.add(bucket); + } else { + if (!(returned instanceof Number)) { + throw new AggregationExecutionException("series_arithmetic script for reducer [" + name() + + "] must return a Number"); + } + List aggs = new ArrayList<>(Lists.transform(bucket.getAggregations().asList(), FUNCTION)); + aggs.add(new InternalSimpleValue(name(), ((Number) returned).doubleValue(), formatter, + new ArrayList(), metaData())); + InternalMultiBucketAggregation.InternalBucket newBucket = originalAgg.createBucket(new InternalAggregations(aggs), + (InternalMultiBucketAggregation.InternalBucket) bucket); + newBuckets.add(newBucket); } - List aggs = new ArrayList<>(Lists.transform(bucket.getAggregations().asList(), FUNCTION)); - aggs.add(new InternalSimpleValue(name(), ((Number) returned).doubleValue(), formatter, new ArrayList(), - metaData())); - InternalMultiBucketAggregation.InternalBucket newBucket = originalAgg.createBucket(new InternalAggregations(aggs), - (InternalMultiBucketAggregation.InternalBucket) bucket); - newBuckets.add(newBucket); } } return originalAgg.create(newBuckets); diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/pipeline/BucketScriptTests.java b/core/src/test/java/org/elasticsearch/search/aggregations/pipeline/BucketScriptTests.java index 93d14dc0011..8667e9c28f2 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/pipeline/BucketScriptTests.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/pipeline/BucketScriptTests.java @@ -46,6 +46,7 @@ import static org.elasticsearch.search.aggregations.pipeline.PipelineAggregatorB import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchResponse; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.notNullValue; +import static org.hamcrest.Matchers.nullValue; @ElasticsearchIntegrationTest.SuiteScopeTest public class BucketScriptTests extends ElasticsearchIntegrationTest { @@ -119,9 +120,7 @@ public class BucketScriptTests extends ElasticsearchIntegrationTest { Histogram.Bucket bucket = buckets.get(i); if (bucket.getDocCount() == 0) { SimpleValue seriesArithmetic = bucket.getAggregations().get("seriesArithmetic"); - assertThat(seriesArithmetic, notNullValue()); - double seriesArithmeticValue = seriesArithmetic.value(); - assertTrue(Double.isNaN(seriesArithmeticValue)); + assertThat(seriesArithmetic, nullValue()); } else { Sum field2Sum = bucket.getAggregations().get("field2Sum"); assertThat(field2Sum, notNullValue()); @@ -167,9 +166,7 @@ public class BucketScriptTests extends ElasticsearchIntegrationTest { Histogram.Bucket bucket = buckets.get(i); if (bucket.getDocCount() == 0) { SimpleValue seriesArithmetic = bucket.getAggregations().get("seriesArithmetic"); - assertThat(seriesArithmetic, notNullValue()); - double seriesArithmeticValue = seriesArithmetic.value(); - assertTrue(Double.isNaN(seriesArithmeticValue)); + assertThat(seriesArithmetic, nullValue()); } else { Sum field2Sum = bucket.getAggregations().get("field2Sum"); assertThat(field2Sum, notNullValue()); @@ -213,9 +210,7 @@ public class BucketScriptTests extends ElasticsearchIntegrationTest { Histogram.Bucket bucket = buckets.get(i); if (bucket.getDocCount() == 0) { SimpleValue seriesArithmetic = bucket.getAggregations().get("seriesArithmetic"); - assertThat(seriesArithmetic, notNullValue()); - double seriesArithmeticValue = seriesArithmetic.value(); - assertTrue(Double.isNaN(seriesArithmeticValue)); + assertThat(seriesArithmetic, nullValue()); } else { Sum field2Sum = bucket.getAggregations().get("field2Sum"); assertThat(field2Sum, notNullValue()); @@ -259,9 +254,7 @@ public class BucketScriptTests extends ElasticsearchIntegrationTest { Histogram.Bucket bucket = buckets.get(i); if (bucket.getDocCount() == 0) { SimpleValue seriesArithmetic = bucket.getAggregations().get("seriesArithmetic"); - assertThat(seriesArithmetic, notNullValue()); - double seriesArithmeticValue = seriesArithmetic.value(); - assertTrue(Double.isNaN(seriesArithmeticValue)); + assertThat(seriesArithmetic, nullValue()); } else { Sum field2Sum = bucket.getAggregations().get("field2Sum"); assertThat(field2Sum, notNullValue()); @@ -309,9 +302,7 @@ public class BucketScriptTests extends ElasticsearchIntegrationTest { Histogram.Bucket bucket = buckets.get(i); if (bucket.getDocCount() == 0) { SimpleValue seriesArithmetic = bucket.getAggregations().get("seriesArithmetic"); - assertThat(seriesArithmetic, notNullValue()); - double seriesArithmeticValue = seriesArithmetic.value(); - assertTrue(Double.isNaN(seriesArithmeticValue)); + assertThat(seriesArithmetic, nullValue()); } else { Sum field2Sum = bucket.getAggregations().get("field2Sum"); assertThat(field2Sum, notNullValue()); @@ -392,7 +383,7 @@ public class BucketScriptTests extends ElasticsearchIntegrationTest { .subAggregation(sum("field4Sum").field(FIELD_4_NAME)) .subAggregation( bucketScript("seriesArithmetic").setBucketsPaths("field2Sum", "field3Sum", "field4Sum").script( - new Script("my_script", ScriptType.INDEXED, null, null)).gapPolicy(GapPolicy.INSERT_ZEROS))).execute().actionGet(); + new Script("my_script", ScriptType.INDEXED, null, null)))).execute().actionGet(); assertSearchResponse(response); @@ -405,9 +396,7 @@ public class BucketScriptTests extends ElasticsearchIntegrationTest { Histogram.Bucket bucket = buckets.get(i); if (bucket.getDocCount() == 0) { SimpleValue seriesArithmetic = bucket.getAggregations().get("seriesArithmetic"); - assertThat(seriesArithmetic, notNullValue()); - double seriesArithmeticValue = seriesArithmetic.value(); - assertThat(seriesArithmeticValue, equalTo(0.0)); + assertThat(seriesArithmetic, nullValue()); } else { Sum field2Sum = bucket.getAggregations().get("field2Sum"); assertThat(field2Sum, notNullValue()); @@ -476,9 +465,7 @@ public class BucketScriptTests extends ElasticsearchIntegrationTest { Histogram.Bucket bucket = buckets.get(i); if (bucket.getDocCount() == 0) { SimpleValue seriesArithmetic = bucket.getAggregations().get("seriesArithmetic"); - assertThat(seriesArithmetic, notNullValue()); - double seriesArithmeticValue = seriesArithmetic.value(); - assertTrue(Double.isNaN(seriesArithmeticValue)); + assertThat(seriesArithmetic, nullValue()); } else { Sum field2Sum = bucket.getAggregations().get("field2Sum"); assertThat(field2Sum, notNullValue());