From 31f26ec1152ce652151c70801ac7af298cd91b30 Mon Sep 17 00:00:00 2001 From: Colin Goodheart-Smithe Date: Mon, 27 Apr 2015 17:10:03 +0100 Subject: [PATCH] review comment fixes --- .../search/aggregations/AggregatorFactories.java | 12 ------------ .../aggregations/InternalMultiBucketAggregation.java | 9 --------- .../aggregations/bucket/BucketsAggregator.java | 9 ++++----- .../bucket/histogram/InternalHistogram.java | 4 +--- .../aggregations/bucket/range/InternalRange.java | 10 ++++------ .../search/aggregations/reducers/BucketHelpers.java | 4 ++-- .../reducers/bucketmetrics/MaxBucketParser.java | 2 +- .../reducers/derivative/DerivativeParser.java | 2 +- .../aggregations/reducers/movavg/MovAvgParser.java | 2 +- .../aggregations/reducers/DateDerivativeTests.java | 1 - .../reducers/moving/avg/MovAvgTests.java | 8 ++++---- 11 files changed, 18 insertions(+), 45 deletions(-) diff --git a/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java b/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java index 84318096080..4bbc8ba662c 100644 --- a/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java +++ b/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java @@ -160,14 +160,6 @@ public class AggregatorFactories { return new AggregatorFactories(factories.toArray(new AggregatorFactory[factories.size()]), orderedReducers); } - /* - * L ← Empty list that will contain the sorted nodes while there are - * unmarked nodes do select an unmarked node n visit(n) function - * visit(node n) if n has a temporary mark then stop (not a DAG) if n is - * not marked (i.e. has not been visited yet) then mark n temporarily - * for each node m with an edge from n to m do visit(m) mark n - * permanently unmark n temporarily add n to head of L - */ private List resolveReducerOrder(List reducerFactories, List aggFactories) { Map reducerFactoriesMap = new HashMap<>(); for (ReducerFactory factory : reducerFactories) { @@ -184,10 +176,6 @@ public class AggregatorFactories { ReducerFactory factory = unmarkedFactories.get(0); resolveReducerOrder(aggFactoryNames, reducerFactoriesMap, orderedReducers, unmarkedFactories, temporarilyMarked, factory); } - List orderedReducerNames = new ArrayList<>(); - for (ReducerFactory reducerFactory : orderedReducers) { - orderedReducerNames.add(reducerFactory.getName()); - } return orderedReducers; } diff --git a/src/main/java/org/elasticsearch/search/aggregations/InternalMultiBucketAggregation.java b/src/main/java/org/elasticsearch/search/aggregations/InternalMultiBucketAggregation.java index 856b96979f2..db2ac49bf38 100644 --- a/src/main/java/org/elasticsearch/search/aggregations/InternalMultiBucketAggregation.java +++ b/src/main/java/org/elasticsearch/search/aggregations/InternalMultiBucketAggregation.java @@ -98,13 +98,4 @@ public abstract class InternalMultiBucketAggregation { - - public abstract String type(); - - public abstract A create(List buckets, A prototype); - - public abstract B createBucket(InternalAggregations aggregations, B prototype); - } } diff --git a/src/main/java/org/elasticsearch/search/aggregations/bucket/BucketsAggregator.java b/src/main/java/org/elasticsearch/search/aggregations/bucket/BucketsAggregator.java index 93fa360b113..041c15a5dc1 100644 --- a/src/main/java/org/elasticsearch/search/aggregations/bucket/BucketsAggregator.java +++ b/src/main/java/org/elasticsearch/search/aggregations/bucket/BucketsAggregator.java @@ -43,8 +43,7 @@ public abstract class BucketsAggregator extends AggregatorBase { private final BigArrays bigArrays; private IntArray docCounts; - public BucketsAggregator(String name, AggregatorFactories factories, - AggregationContext context, Aggregator parent, + public BucketsAggregator(String name, AggregatorFactories factories, AggregationContext context, Aggregator parent, List reducers, Map metaData) throws IOException { super(name, factories, context, parent, reducers, metaData); bigArrays = context.bigArrays(); @@ -113,11 +112,11 @@ public abstract class BucketsAggregator extends AggregatorBase { */ protected final InternalAggregations bucketAggregations(long bucket) throws IOException { final InternalAggregation[] aggregations = new InternalAggregation[subAggregators.length]; - for (int i = 0; i < subAggregators.length; i++) { + for (int i = 0; i < subAggregators.length; i++) { aggregations[i] = subAggregators[i].buildAggregation(bucket); - } + } return new InternalAggregations(Arrays.asList(aggregations)); - } + } /** * Utility method to build empty aggregations of the sub aggregators. diff --git a/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalHistogram.java b/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalHistogram.java index 1fb919558d5..5c10e0d3ad4 100644 --- a/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalHistogram.java +++ b/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalHistogram.java @@ -234,7 +234,7 @@ public class InternalHistogram extends Inter } - public static class Factory extends InternalMultiBucketAggregation.Factory, B> { + public static class Factory { protected Factory() { } @@ -249,13 +249,11 @@ public class InternalHistogram extends Inter return new InternalHistogram<>(name, buckets, order, minDocCount, emptyBucketInfo, formatter, keyed, this, reducers, metaData); } - @Override public InternalHistogram create(List buckets, InternalHistogram prototype) { return new InternalHistogram<>(prototype.name, buckets, prototype.order, prototype.minDocCount, prototype.emptyBucketInfo, prototype.formatter, prototype.keyed, this, prototype.reducers(), prototype.metaData); } - @Override public B createBucket(InternalAggregations aggregations, B prototype) { return (B) new Bucket(prototype.key, prototype.docCount, prototype.getKeyed(), prototype.formatter, this, aggregations); } diff --git a/src/main/java/org/elasticsearch/search/aggregations/bucket/range/InternalRange.java b/src/main/java/org/elasticsearch/search/aggregations/bucket/range/InternalRange.java index 1bf62b9abb6..db0ccee33e5 100644 --- a/src/main/java/org/elasticsearch/search/aggregations/bucket/range/InternalRange.java +++ b/src/main/java/org/elasticsearch/search/aggregations/bucket/range/InternalRange.java @@ -225,7 +225,7 @@ public class InternalRange> extends InternalMultiBucketAggregation.Factory { + public static class Factory> { public String type() { return TYPE.name(); @@ -236,18 +236,16 @@ public class InternalRange(name, ranges, formatter, keyed, reducers, metaData); } - - public B createBucket(String key, double from, double to, long docCount, InternalAggregations aggregations, boolean keyed, @Nullable ValueFormatter formatter) { + public B createBucket(String key, double from, double to, long docCount, InternalAggregations aggregations, boolean keyed, + @Nullable ValueFormatter formatter) { return (B) new Bucket(key, from, to, docCount, aggregations, keyed, formatter); } - @Override public R create(List ranges, R prototype) { return (R) new InternalRange<>(prototype.name, ranges, prototype.formatter, prototype.keyed, prototype.reducers(), prototype.metaData); - } + } - @Override public B createBucket(InternalAggregations aggregations, B prototype) { return (B) new Bucket(prototype.getKey(), prototype.from, prototype.to, prototype.getDocCount(), aggregations, prototype.keyed, prototype.formatter); diff --git a/src/main/java/org/elasticsearch/search/aggregations/reducers/BucketHelpers.java b/src/main/java/org/elasticsearch/search/aggregations/reducers/BucketHelpers.java index f6cdd8ca1f9..4ac1bff7cfb 100644 --- a/src/main/java/org/elasticsearch/search/aggregations/reducers/BucketHelpers.java +++ b/src/main/java/org/elasticsearch/search/aggregations/reducers/BucketHelpers.java @@ -53,7 +53,7 @@ public class BucketHelpers { * "ignore": empty buckets will simply be ignored */ public static enum GapPolicy { - INSERT_ZEROS((byte) 0, "insert_zeros"), IGNORE((byte) 1, "ignore"); + INSERT_ZEROS((byte) 0, "insert_zeros"), SKIP((byte) 1, "skip"); /** * Parse a string GapPolicy into the byte enum @@ -172,7 +172,7 @@ public class BucketHelpers { switch (gapPolicy) { case INSERT_ZEROS: return 0.0; - case IGNORE: + case SKIP: default: return Double.NaN; } diff --git a/src/main/java/org/elasticsearch/search/aggregations/reducers/bucketmetrics/MaxBucketParser.java b/src/main/java/org/elasticsearch/search/aggregations/reducers/bucketmetrics/MaxBucketParser.java index 7d773747a8d..87afd890e34 100644 --- a/src/main/java/org/elasticsearch/search/aggregations/reducers/bucketmetrics/MaxBucketParser.java +++ b/src/main/java/org/elasticsearch/search/aggregations/reducers/bucketmetrics/MaxBucketParser.java @@ -47,7 +47,7 @@ public class MaxBucketParser implements Reducer.Parser { String currentFieldName = null; String[] bucketsPaths = null; String format = null; - GapPolicy gapPolicy = GapPolicy.IGNORE; + GapPolicy gapPolicy = GapPolicy.SKIP; while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { if (token == XContentParser.Token.FIELD_NAME) { diff --git a/src/main/java/org/elasticsearch/search/aggregations/reducers/derivative/DerivativeParser.java b/src/main/java/org/elasticsearch/search/aggregations/reducers/derivative/DerivativeParser.java index cfca5c60978..3536377644b 100644 --- a/src/main/java/org/elasticsearch/search/aggregations/reducers/derivative/DerivativeParser.java +++ b/src/main/java/org/elasticsearch/search/aggregations/reducers/derivative/DerivativeParser.java @@ -45,7 +45,7 @@ public class DerivativeParser implements Reducer.Parser { String currentFieldName = null; String[] bucketsPaths = null; String format = null; - GapPolicy gapPolicy = GapPolicy.IGNORE; + GapPolicy gapPolicy = GapPolicy.SKIP; while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { if (token == XContentParser.Token.FIELD_NAME) { diff --git a/src/main/java/org/elasticsearch/search/aggregations/reducers/movavg/MovAvgParser.java b/src/main/java/org/elasticsearch/search/aggregations/reducers/movavg/MovAvgParser.java index 5d79b1d1e7a..0850587de35 100644 --- a/src/main/java/org/elasticsearch/search/aggregations/reducers/movavg/MovAvgParser.java +++ b/src/main/java/org/elasticsearch/search/aggregations/reducers/movavg/MovAvgParser.java @@ -64,7 +64,7 @@ public class MovAvgParser implements Reducer.Parser { String[] bucketsPaths = null; String format = null; - GapPolicy gapPolicy = GapPolicy.IGNORE; + GapPolicy gapPolicy = GapPolicy.SKIP; int window = 5; Map settings = null; String model = "simple"; diff --git a/src/test/java/org/elasticsearch/search/aggregations/reducers/DateDerivativeTests.java b/src/test/java/org/elasticsearch/search/aggregations/reducers/DateDerivativeTests.java index ede94abd973..b1ac6756f1e 100644 --- a/src/test/java/org/elasticsearch/search/aggregations/reducers/DateDerivativeTests.java +++ b/src/test/java/org/elasticsearch/search/aggregations/reducers/DateDerivativeTests.java @@ -51,7 +51,6 @@ import static org.hamcrest.core.IsNull.notNullValue; import static org.hamcrest.core.IsNull.nullValue; @ElasticsearchIntegrationTest.SuiteScopeTest -//@AwaitsFix(bugUrl = "Fix factory selection for serialisation of Internal derivative") public class DateDerivativeTests extends ElasticsearchIntegrationTest { private DateTime date(int month, int day) { diff --git a/src/test/java/org/elasticsearch/search/aggregations/reducers/moving/avg/MovAvgTests.java b/src/test/java/org/elasticsearch/search/aggregations/reducers/moving/avg/MovAvgTests.java index cd6ac6cf490..ae0f89ae868 100644 --- a/src/test/java/org/elasticsearch/search/aggregations/reducers/moving/avg/MovAvgTests.java +++ b/src/test/java/org/elasticsearch/search/aggregations/reducers/moving/avg/MovAvgTests.java @@ -121,7 +121,7 @@ public class MovAvgTests extends ElasticsearchIntegrationTest { alpha = randomDouble(); beta = randomDouble(); - gapPolicy = randomBoolean() ? BucketHelpers.GapPolicy.IGNORE : BucketHelpers.GapPolicy.INSERT_ZEROS; + gapPolicy = randomBoolean() ? BucketHelpers.GapPolicy.SKIP : BucketHelpers.GapPolicy.INSERT_ZEROS; metric = randomMetric("the_metric", VALUE_FIELD); mockHisto = ReducerHelperTests.generateHistogram(interval, numBuckets, randomDouble(), randomDouble()); @@ -172,7 +172,7 @@ public class MovAvgTests extends ElasticsearchIntegrationTest { // Gaps only apply to metric values, not doc _counts if (mockBucket.count == 0 && target.equals(MetricTarget.VALUE)) { // If there was a gap in doc counts and we are ignoring, just skip this bucket - if (gapPolicy.equals(BucketHelpers.GapPolicy.IGNORE)) { + if (gapPolicy.equals(BucketHelpers.GapPolicy.SKIP)) { values.add(null); continue; } else if (gapPolicy.equals(BucketHelpers.GapPolicy.INSERT_ZEROS)) { @@ -726,7 +726,7 @@ public class MovAvgTests extends ElasticsearchIntegrationTest { assertThat(current, notNullValue()); currentValue = current.value(); - if (gapPolicy.equals(BucketHelpers.GapPolicy.IGNORE)) { + if (gapPolicy.equals(BucketHelpers.GapPolicy.SKIP)) { // if we are ignoring, movavg could go up (double_exp) or stay the same (simple, linear, single_exp) assertThat(Double.compare(lastValue, currentValue), lessThanOrEqualTo(0)); } else if (gapPolicy.equals(BucketHelpers.GapPolicy.INSERT_ZEROS)) { @@ -785,7 +785,7 @@ public class MovAvgTests extends ElasticsearchIntegrationTest { assertThat(current, notNullValue()); currentValue = current.value(); - if (gapPolicy.equals(BucketHelpers.GapPolicy.IGNORE)) { + if (gapPolicy.equals(BucketHelpers.GapPolicy.SKIP)) { // if we are ignoring, movavg could go up (double_exp) or stay the same (simple, linear, single_exp) assertThat(Double.compare(lastValue, currentValue), lessThanOrEqualTo(0)); } else if (gapPolicy.equals(BucketHelpers.GapPolicy.INSERT_ZEROS)) {