From 2fcf1ee94f3845df4fbfba68f22a698ef48d32d3 Mon Sep 17 00:00:00 2001 From: Paul Sanwald Date: Sun, 22 Jul 2018 16:08:34 -0400 Subject: [PATCH 1/6] Refactor test roundings to be configured in one place --- .../AutoDateHistogramAggregationBuilder.java | 44 ++++++++++++++----- .../histogram/InternalAutoDateHistogram.java | 2 +- .../InternalAutoDateHistogramTests.java | 29 +++++------- 3 files changed, 43 insertions(+), 32 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregationBuilder.java index 366060835d891..6a9216ef92b81 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregationBuilder.java @@ -42,6 +42,7 @@ import org.elasticsearch.search.aggregations.support.ValuesSourceParserHelper; import org.elasticsearch.search.aggregations.support.ValuesSourceType; import org.elasticsearch.search.internal.SearchContext; +import org.joda.time.DateTimeZone; import java.io.IOException; import java.util.Arrays; @@ -53,7 +54,7 @@ public class AutoDateHistogramAggregationBuilder public static final String NAME = "auto_date_histogram"; - public static final ParseField NUM_BUCKETS_FIELD = new ParseField("buckets"); + private static final ParseField NUM_BUCKETS_FIELD = new ParseField("buckets"); private static final ObjectParser PARSER; static { @@ -63,6 +64,12 @@ public class AutoDateHistogramAggregationBuilder PARSER.declareInt(AutoDateHistogramAggregationBuilder::setNumBuckets, NUM_BUCKETS_FIELD); } + private final RoundingInfo[] roundings; + + public RoundingInfo[] getRoundings() { + return roundings; + } + public static AutoDateHistogramAggregationBuilder parse(String aggregationName, XContentParser parser) throws IOException { return PARSER.parse(parser, new AutoDateHistogramAggregationBuilder(aggregationName), null); } @@ -72,18 +79,39 @@ public static AutoDateHistogramAggregationBuilder parse(String aggregationName, /** Create a new builder with the given name. */ public AutoDateHistogramAggregationBuilder(String name) { super(name, ValuesSourceType.NUMERIC, ValueType.DATE); + roundings = new RoundingInfo[6]; + initRoundings(); } /** Read from a stream, for internal use only. */ public AutoDateHistogramAggregationBuilder(StreamInput in) throws IOException { super(in, ValuesSourceType.NUMERIC, ValueType.DATE); numBuckets = in.readVInt(); + roundings = new RoundingInfo[6]; + initRoundings(); } protected AutoDateHistogramAggregationBuilder(AutoDateHistogramAggregationBuilder clone, Builder factoriesBuilder, Map metaData) { super(clone, factoriesBuilder, metaData); this.numBuckets = clone.numBuckets; + roundings = new RoundingInfo[6]; + initRoundings(); + } + + private void initRoundings() { + roundings[0] = new RoundingInfo(createRounding(DateTimeUnit.SECOND_OF_MINUTE, timeZone()), + 1000L, 1, 5, 10, 30); + roundings[1] = new RoundingInfo(createRounding(DateTimeUnit.MINUTES_OF_HOUR, timeZone()), + 60 * 1000L, 1, 5, 10, 30); + roundings[2] = new RoundingInfo(createRounding(DateTimeUnit.HOUR_OF_DAY, timeZone()), + 60 * 60 * 1000L, 1, 3, 12); + roundings[3] = new RoundingInfo(createRounding(DateTimeUnit.DAY_OF_MONTH, timeZone()), + 24 * 60 * 60 * 1000L, 1, 7); + roundings[4] = new RoundingInfo(createRounding(DateTimeUnit.MONTH_OF_YEAR, timeZone()), + 30 * 24 * 60 * 60 * 1000L, 1, 3); + roundings[5] = new RoundingInfo(createRounding(DateTimeUnit.YEAR_OF_CENTURY, timeZone()), + 365 * 24 * 60 * 60 * 1000L, 1, 5, 10, 20, 50, 100); } @Override @@ -116,14 +144,6 @@ public int getNumBuckets() { @Override protected ValuesSourceAggregatorFactory innerBuild(SearchContext context, ValuesSourceConfig config, AggregatorFactory parent, Builder subFactoriesBuilder) throws IOException { - RoundingInfo[] roundings = new RoundingInfo[6]; - roundings[0] = new RoundingInfo(createRounding(DateTimeUnit.SECOND_OF_MINUTE), 1000L, 1, 5, 10, 30); - roundings[1] = new RoundingInfo(createRounding(DateTimeUnit.MINUTES_OF_HOUR), 60 * 1000L, 1, 5, 10, 30); - roundings[2] = new RoundingInfo(createRounding(DateTimeUnit.HOUR_OF_DAY), 60 * 60 * 1000L, 1, 3, 12); - roundings[3] = new RoundingInfo(createRounding(DateTimeUnit.DAY_OF_MONTH), 24 * 60 * 60 * 1000L, 1, 7); - roundings[4] = new RoundingInfo(createRounding(DateTimeUnit.MONTH_OF_YEAR), 30 * 24 * 60 * 60 * 1000L, 1, 3); - roundings[5] = new RoundingInfo(createRounding(DateTimeUnit.YEAR_OF_CENTURY), 365 * 24 * 60 * 60 * 1000L, 1, 5, 10, 20, 50, 100); - int maxRoundingInterval = Arrays.stream(roundings,0, roundings.length-1) .map(rounding -> rounding.innerIntervals) .flatMapToInt(Arrays::stream) @@ -139,10 +159,10 @@ public int getNumBuckets() { return new AutoDateHistogramAggregatorFactory(name, config, numBuckets, roundings, context, parent, subFactoriesBuilder, metaData); } - private Rounding createRounding(DateTimeUnit interval) { + private Rounding createRounding(DateTimeUnit interval, DateTimeZone timeZone) { Rounding.Builder tzRoundingBuilder = Rounding.builder(interval); - if (timeZone() != null) { - tzRoundingBuilder.timeZone(timeZone()); + if (timeZone != null) { + tzRoundingBuilder.timeZone(timeZone); } Rounding rounding = tzRoundingBuilder.build(); return rounding; diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogram.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogram.java index 27c195cbdae75..586368d64e5a8 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogram.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogram.java @@ -289,7 +289,7 @@ private static class IteratorAndCurrent { * rounding returned across all the shards so the resolution of the buckets * is the same and they can be reduced together. */ - private BucketReduceResult reduceBuckets(List aggregations, ReduceContext reduceContext) { + public BucketReduceResult reduceBuckets(List aggregations, ReduceContext reduceContext) { // First we need to find the highest level rounding used across all the // shards diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogramTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogramTests.java index 96811ce424c91..aa2b12f3226db 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogramTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogramTests.java @@ -20,8 +20,6 @@ package org.elasticsearch.search.aggregations.bucket.histogram; import org.elasticsearch.common.io.stream.Writeable; -import org.elasticsearch.common.rounding.DateTimeUnit; -import org.elasticsearch.common.rounding.Rounding; import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.aggregations.InternalAggregations; import org.elasticsearch.search.aggregations.ParsedMultiBucketAggregation; @@ -51,14 +49,9 @@ public class InternalAutoDateHistogramTests extends InternalMultiBucketAggregati public void setUp() throws Exception { super.setUp(); format = randomNumericDocValueFormat(); - - roundingInfos = new RoundingInfo[6]; - roundingInfos[0] = new RoundingInfo(Rounding.builder(DateTimeUnit.SECOND_OF_MINUTE).build(), 1, 5, 10, 30); - roundingInfos[1] = new RoundingInfo(Rounding.builder(DateTimeUnit.MINUTES_OF_HOUR).build(), 1, 5, 10, 30); - roundingInfos[2] = new RoundingInfo(Rounding.builder(DateTimeUnit.HOUR_OF_DAY).build(), 1, 3, 12); - roundingInfos[3] = new RoundingInfo(Rounding.builder(DateTimeUnit.DAY_OF_MONTH).build(), 1, 7); - roundingInfos[4] = new RoundingInfo(Rounding.builder(DateTimeUnit.MONTH_OF_YEAR).build(), 1, 3); - roundingInfos[5] = new RoundingInfo(Rounding.builder(DateTimeUnit.YEAR_OF_CENTURY).build(), 1, 10, 20, 50, 100); + AutoDateHistogramAggregationBuilder aggregationBuilder = new AutoDateHistogramAggregationBuilder("_name"); + // TODO[PCS]: timezone set automagically here? + roundingInfos = aggregationBuilder.getRoundings(); } @Override @@ -92,11 +85,15 @@ protected void assertReduced(InternalAutoDateHistogram reduced, List expectedCounts = new TreeMap<>(); - for (Histogram histogram : inputs) { + for (InternalAutoDateHistogram histogram : inputs) { for (Histogram.Bucket bucket : histogram.getBuckets()) { - expectedCounts.compute(roundingInfos[roundingIdx].rounding.round(((DateTime) bucket.getKey()).getMillis()), - (key, oldValue) -> (oldValue == null ? 0 : oldValue) + bucket.getDocCount()); + long bucketKey = ((DateTime) bucket.getKey()).getMillis(); + long count = bucket.getDocCount(); + expectedCounts.compute(roundingInfo.rounding.round(bucketKey), + (key, oldValue) -> (oldValue == null ? 0 : oldValue) + count); } } Map actualCounts = new TreeMap<>(); @@ -117,12 +114,6 @@ protected Class implementationClass() { return ParsedAutoDateHistogram.class; } - @Override - @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/32215") - public void testReduceRandom() { - super.testReduceRandom(); - } - @Override protected InternalAutoDateHistogram mutateInstance(InternalAutoDateHistogram instance) { String name = instance.getName(); From 4cccc877f38498b1a54d286a5c2c7e2035392638 Mon Sep 17 00:00:00 2001 From: Paul Sanwald Date: Mon, 23 Jul 2018 09:16:18 -0400 Subject: [PATCH 2/6] refactor roundings to be computed dynamically --- .../AutoDateHistogramAggregationBuilder.java | 45 +++++++++---------- .../histogram/InternalAutoDateHistogram.java | 15 ++++--- .../InternalAutoDateHistogramTests.java | 2 +- 3 files changed, 31 insertions(+), 31 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregationBuilder.java index 6a9216ef92b81..1a9f0e1ccc0f6 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregationBuilder.java @@ -64,9 +64,26 @@ public class AutoDateHistogramAggregationBuilder PARSER.declareInt(AutoDateHistogramAggregationBuilder::setNumBuckets, NUM_BUCKETS_FIELD); } - private final RoundingInfo[] roundings; - - public RoundingInfo[] getRoundings() { + /** + * + * Build roundings, computed dynamically as roundings are time zone dependent. + * The current implementation probably should not be invoked in a tight loop. + * @return Array of RoundingInfo + */ + RoundingInfo[] buildRoundings() { + RoundingInfo[] roundings = new RoundingInfo[6]; + roundings[0] = new RoundingInfo(createRounding(DateTimeUnit.SECOND_OF_MINUTE, timeZone()), + 1000L, 1, 5, 10, 30); + roundings[1] = new RoundingInfo(createRounding(DateTimeUnit.MINUTES_OF_HOUR, timeZone()), + 60 * 1000L, 1, 5, 10, 30); + roundings[2] = new RoundingInfo(createRounding(DateTimeUnit.HOUR_OF_DAY, timeZone()), + 60 * 60 * 1000L, 1, 3, 12); + roundings[3] = new RoundingInfo(createRounding(DateTimeUnit.DAY_OF_MONTH, timeZone()), + 24 * 60 * 60 * 1000L, 1, 7); + roundings[4] = new RoundingInfo(createRounding(DateTimeUnit.MONTH_OF_YEAR, timeZone()), + 30 * 24 * 60 * 60 * 1000L, 1, 3); + roundings[5] = new RoundingInfo(createRounding(DateTimeUnit.YEAR_OF_CENTURY, timeZone()), + 365 * 24 * 60 * 60 * 1000L, 1, 5, 10, 20, 50, 100); return roundings; } @@ -79,39 +96,18 @@ public static AutoDateHistogramAggregationBuilder parse(String aggregationName, /** Create a new builder with the given name. */ public AutoDateHistogramAggregationBuilder(String name) { super(name, ValuesSourceType.NUMERIC, ValueType.DATE); - roundings = new RoundingInfo[6]; - initRoundings(); } /** Read from a stream, for internal use only. */ public AutoDateHistogramAggregationBuilder(StreamInput in) throws IOException { super(in, ValuesSourceType.NUMERIC, ValueType.DATE); numBuckets = in.readVInt(); - roundings = new RoundingInfo[6]; - initRoundings(); } protected AutoDateHistogramAggregationBuilder(AutoDateHistogramAggregationBuilder clone, Builder factoriesBuilder, Map metaData) { super(clone, factoriesBuilder, metaData); this.numBuckets = clone.numBuckets; - roundings = new RoundingInfo[6]; - initRoundings(); - } - - private void initRoundings() { - roundings[0] = new RoundingInfo(createRounding(DateTimeUnit.SECOND_OF_MINUTE, timeZone()), - 1000L, 1, 5, 10, 30); - roundings[1] = new RoundingInfo(createRounding(DateTimeUnit.MINUTES_OF_HOUR, timeZone()), - 60 * 1000L, 1, 5, 10, 30); - roundings[2] = new RoundingInfo(createRounding(DateTimeUnit.HOUR_OF_DAY, timeZone()), - 60 * 60 * 1000L, 1, 3, 12); - roundings[3] = new RoundingInfo(createRounding(DateTimeUnit.DAY_OF_MONTH, timeZone()), - 24 * 60 * 60 * 1000L, 1, 7); - roundings[4] = new RoundingInfo(createRounding(DateTimeUnit.MONTH_OF_YEAR, timeZone()), - 30 * 24 * 60 * 60 * 1000L, 1, 3); - roundings[5] = new RoundingInfo(createRounding(DateTimeUnit.YEAR_OF_CENTURY, timeZone()), - 365 * 24 * 60 * 60 * 1000L, 1, 5, 10, 20, 50, 100); } @Override @@ -144,6 +140,7 @@ public int getNumBuckets() { @Override protected ValuesSourceAggregatorFactory innerBuild(SearchContext context, ValuesSourceConfig config, AggregatorFactory parent, Builder subFactoriesBuilder) throws IOException { + RoundingInfo[] roundings = buildRoundings(); int maxRoundingInterval = Arrays.stream(roundings,0, roundings.length-1) .map(rounding -> rounding.innerIntervals) .flatMapToInt(Arrays::stream) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogram.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogram.java index 586368d64e5a8..2bcfc29ca229d 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogram.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogram.java @@ -499,7 +499,7 @@ public InternalAggregation doReduce(List aggregations, Redu reducedBucketsResult.roundingInfo, reduceContext); // Now finally see if we need to merge consecutive buckets together to make a coarser interval at the same rounding - reducedBucketsResult = maybeMergeConsecutiveBuckets(reducedBucketsResult, reduceContext); + reducedBucketsResult = maybeMergeConsecutiveBuckets(reducedBucketsResult, reduceContext, targetBuckets, format); } BucketInfo bucketInfo = new BucketInfo(this.bucketInfo.roundingInfos, reducedBucketsResult.roundingIdx, @@ -509,7 +509,10 @@ public InternalAggregation doReduce(List aggregations, Redu pipelineAggregators(), getMetaData()); } - private BucketReduceResult maybeMergeConsecutiveBuckets(BucketReduceResult reducedBucketsResult, ReduceContext reduceContext) { + private static BucketReduceResult maybeMergeConsecutiveBuckets(BucketReduceResult reducedBucketsResult, + ReduceContext reduceContext, + int targetBuckets, + DocValueFormat format) { List buckets = reducedBucketsResult.buckets; RoundingInfo roundingInfo = reducedBucketsResult.roundingInfo; int roundingIdx = reducedBucketsResult.roundingIdx; @@ -517,15 +520,15 @@ private BucketReduceResult maybeMergeConsecutiveBuckets(BucketReduceResult reduc for (int interval : roundingInfo.innerIntervals) { int resultingBuckets = buckets.size() / interval; if (resultingBuckets <= targetBuckets) { - return mergeConsecutiveBuckets(buckets, interval, roundingIdx, roundingInfo, reduceContext); + return mergeConsecutiveBuckets(buckets, interval, roundingIdx, roundingInfo, reduceContext, format); } } } return reducedBucketsResult; } - private BucketReduceResult mergeConsecutiveBuckets(List reducedBuckets, int mergeInterval, int roundingIdx, - RoundingInfo roundingInfo, ReduceContext reduceContext) { + private static BucketReduceResult mergeConsecutiveBuckets(List reducedBuckets, int mergeInterval, int roundingIdx, + RoundingInfo roundingInfo, ReduceContext reduceContext, DocValueFormat format) { List mergedBuckets = new ArrayList<>(); List sameKeyedBuckets = new ArrayList<>(); @@ -539,7 +542,7 @@ private BucketReduceResult mergeConsecutiveBuckets(List reducedBuckets, key = roundingInfo.rounding.round(bucket.key); } reduceContext.consumeBucketsAndMaybeBreak(-countInnerBucket(bucket) - 1); - sameKeyedBuckets.add(createBucket(key, bucket.docCount, bucket.aggregations)); + sameKeyedBuckets.add(new Bucket(Math.round(key), bucket.docCount, format, bucket.aggregations)); } if (sameKeyedBuckets.isEmpty() == false) { reduceContext.consumeBucketsAndMaybeBreak(1); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogramTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogramTests.java index aa2b12f3226db..c31cec65570ba 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogramTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogramTests.java @@ -51,7 +51,7 @@ public void setUp() throws Exception { format = randomNumericDocValueFormat(); AutoDateHistogramAggregationBuilder aggregationBuilder = new AutoDateHistogramAggregationBuilder("_name"); // TODO[PCS]: timezone set automagically here? - roundingInfos = aggregationBuilder.getRoundings(); + roundingInfos = aggregationBuilder.buildRoundings(); } @Override From 3bddd141c5f712c4e7a0d5aabd7ca9468aea29e1 Mon Sep 17 00:00:00 2001 From: Paul Sanwald Date: Mon, 23 Jul 2018 17:57:43 -0400 Subject: [PATCH 3/6] Refactor test to respect inner intervals --- .../histogram/InternalAutoDateHistogram.java | 5 ++- .../InternalAutoDateHistogramTests.java | 41 +++++++++++++++++-- 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogram.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogram.java index 2bcfc29ca229d..6a8dfa3f3eac1 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogram.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogram.java @@ -418,7 +418,7 @@ private BucketReduceResult addEmptyBuckets(BucketReduceResult currentResult, Red return currentResult; } int roundingIdx = getAppropriateRounding(list.get(0).key, list.get(list.size() - 1).key, currentResult.roundingIdx, - bucketInfo.roundingInfos); + bucketInfo.roundingInfos, targetBuckets); RoundingInfo roundingInfo = bucketInfo.roundingInfos[roundingIdx]; Rounding rounding = roundingInfo.rounding; // merge buckets using the new rounding @@ -447,7 +447,8 @@ private BucketReduceResult addEmptyBuckets(BucketReduceResult currentResult, Red return new BucketReduceResult(list, roundingInfo, roundingIdx); } - private int getAppropriateRounding(long minKey, long maxKey, int roundingIdx, RoundingInfo[] roundings) { + private static int getAppropriateRounding(long minKey, long maxKey, int roundingIdx, + RoundingInfo[] roundings, int targetBuckets) { if (roundingIdx == roundings.length - 1) { return roundingIdx; } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogramTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogramTests.java index c31cec65570ba..01b9d01ba8328 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogramTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogramTests.java @@ -87,15 +87,48 @@ protected void assertReduced(InternalAutoDateHistogram reduced, List expectedCounts = new TreeMap<>(); + long lowest = Long.MAX_VALUE; + long highest = 0; for (InternalAutoDateHistogram histogram : inputs) { for (Histogram.Bucket bucket : histogram.getBuckets()) { long bucketKey = ((DateTime) bucket.getKey()).getMillis(); - long count = bucket.getDocCount(); - expectedCounts.compute(roundingInfo.rounding.round(bucketKey), - (key, oldValue) -> (oldValue == null ? 0 : oldValue) + count); + if (bucketKey < lowest) { + lowest = bucketKey; + } + if (bucketKey > highest) { + highest = bucketKey; + } + } + } + long normalizedDuration = (highest - lowest) / roundingInfo.getRoughEstimateDurationMillis(); + long innerIntervalToUse = 0; + for (int interval : roundingInfo.innerIntervals) { + if (normalizedDuration / interval < maxNumberOfBuckets()) { + innerIntervalToUse = interval; + } + } + Map expectedCounts = new TreeMap<>(); + long intervalInMillis = innerIntervalToUse*roundingInfo.getRoughEstimateDurationMillis(); + for (long keyForBucket = roundingInfo.rounding.round(lowest); + keyForBucket <= highest; + keyForBucket = keyForBucket + intervalInMillis) { + expectedCounts.put(keyForBucket, 0L); + + for (InternalAutoDateHistogram histogram : inputs) { + for (Histogram.Bucket bucket : histogram.getBuckets()) { + long bucketKey = ((DateTime) bucket.getKey()).getMillis(); + long roundedBucketKey = roundingInfo.rounding.round(bucketKey); + if (roundedBucketKey >= keyForBucket + && roundedBucketKey < keyForBucket + intervalInMillis) { + long count = bucket.getDocCount(); + expectedCounts.compute(keyForBucket, + (key, oldValue) -> (oldValue == null ? 0 : oldValue) + count); + } + } } } + + Map actualCounts = new TreeMap<>(); for (Histogram.Bucket bucket : reduced.getBuckets()) { actualCounts.compute(((DateTime) bucket.getKey()).getMillis(), From f9fa709952368a2b59d0c867644157607b08af27 Mon Sep 17 00:00:00 2001 From: Paul Sanwald Date: Mon, 23 Jul 2018 20:41:45 -0400 Subject: [PATCH 4/6] revert a few changes that were not necessary to this PR --- .../histogram/InternalAutoDateHistogram.java | 22 +++++++++---------- 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogram.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogram.java index 6a8dfa3f3eac1..6a78ca6724988 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogram.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogram.java @@ -289,7 +289,7 @@ private static class IteratorAndCurrent { * rounding returned across all the shards so the resolution of the buckets * is the same and they can be reduced together. */ - public BucketReduceResult reduceBuckets(List aggregations, ReduceContext reduceContext) { + private BucketReduceResult reduceBuckets(List aggregations, ReduceContext reduceContext) { // First we need to find the highest level rounding used across all the // shards @@ -418,7 +418,7 @@ private BucketReduceResult addEmptyBuckets(BucketReduceResult currentResult, Red return currentResult; } int roundingIdx = getAppropriateRounding(list.get(0).key, list.get(list.size() - 1).key, currentResult.roundingIdx, - bucketInfo.roundingInfos, targetBuckets); + bucketInfo.roundingInfos); RoundingInfo roundingInfo = bucketInfo.roundingInfos[roundingIdx]; Rounding rounding = roundingInfo.rounding; // merge buckets using the new rounding @@ -447,8 +447,8 @@ private BucketReduceResult addEmptyBuckets(BucketReduceResult currentResult, Red return new BucketReduceResult(list, roundingInfo, roundingIdx); } - private static int getAppropriateRounding(long minKey, long maxKey, int roundingIdx, - RoundingInfo[] roundings, int targetBuckets) { + private int getAppropriateRounding(long minKey, long maxKey, int roundingIdx, + RoundingInfo[] roundings) { if (roundingIdx == roundings.length - 1) { return roundingIdx; } @@ -500,7 +500,7 @@ public InternalAggregation doReduce(List aggregations, Redu reducedBucketsResult.roundingInfo, reduceContext); // Now finally see if we need to merge consecutive buckets together to make a coarser interval at the same rounding - reducedBucketsResult = maybeMergeConsecutiveBuckets(reducedBucketsResult, reduceContext, targetBuckets, format); + reducedBucketsResult = maybeMergeConsecutiveBuckets(reducedBucketsResult, reduceContext); } BucketInfo bucketInfo = new BucketInfo(this.bucketInfo.roundingInfos, reducedBucketsResult.roundingIdx, @@ -510,10 +510,8 @@ public InternalAggregation doReduce(List aggregations, Redu pipelineAggregators(), getMetaData()); } - private static BucketReduceResult maybeMergeConsecutiveBuckets(BucketReduceResult reducedBucketsResult, - ReduceContext reduceContext, - int targetBuckets, - DocValueFormat format) { + private BucketReduceResult maybeMergeConsecutiveBuckets(BucketReduceResult reducedBucketsResult, + ReduceContext reduceContext) { List buckets = reducedBucketsResult.buckets; RoundingInfo roundingInfo = reducedBucketsResult.roundingInfo; int roundingIdx = reducedBucketsResult.roundingIdx; @@ -521,15 +519,15 @@ private static BucketReduceResult maybeMergeConsecutiveBuckets(BucketReduceResul for (int interval : roundingInfo.innerIntervals) { int resultingBuckets = buckets.size() / interval; if (resultingBuckets <= targetBuckets) { - return mergeConsecutiveBuckets(buckets, interval, roundingIdx, roundingInfo, reduceContext, format); + return mergeConsecutiveBuckets(buckets, interval, roundingIdx, roundingInfo, reduceContext); } } } return reducedBucketsResult; } - private static BucketReduceResult mergeConsecutiveBuckets(List reducedBuckets, int mergeInterval, int roundingIdx, - RoundingInfo roundingInfo, ReduceContext reduceContext, DocValueFormat format) { + private BucketReduceResult mergeConsecutiveBuckets(List reducedBuckets, int mergeInterval, int roundingIdx, + RoundingInfo roundingInfo, ReduceContext reduceContext) { List mergedBuckets = new ArrayList<>(); List sameKeyedBuckets = new ArrayList<>(); From 1d2f50b926d9f28d56c749f093213f6c20aeddd4 Mon Sep 17 00:00:00 2001 From: Paul Sanwald Date: Tue, 24 Jul 2018 14:23:49 -0400 Subject: [PATCH 5/6] address review comments: make buildRoundings() static and pass timeZone in --- .../AutoDateHistogramAggregationBuilder.java | 18 +++++++++--------- .../InternalAutoDateHistogramTests.java | 7 ++++--- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregationBuilder.java index 1a9f0e1ccc0f6..50a0c85c041c8 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregationBuilder.java @@ -70,19 +70,19 @@ public class AutoDateHistogramAggregationBuilder * The current implementation probably should not be invoked in a tight loop. * @return Array of RoundingInfo */ - RoundingInfo[] buildRoundings() { + static RoundingInfo[] buildRoundings(DateTimeZone timeZone) { RoundingInfo[] roundings = new RoundingInfo[6]; - roundings[0] = new RoundingInfo(createRounding(DateTimeUnit.SECOND_OF_MINUTE, timeZone()), + roundings[0] = new RoundingInfo(createRounding(DateTimeUnit.SECOND_OF_MINUTE, timeZone), 1000L, 1, 5, 10, 30); - roundings[1] = new RoundingInfo(createRounding(DateTimeUnit.MINUTES_OF_HOUR, timeZone()), + roundings[1] = new RoundingInfo(createRounding(DateTimeUnit.MINUTES_OF_HOUR, timeZone), 60 * 1000L, 1, 5, 10, 30); - roundings[2] = new RoundingInfo(createRounding(DateTimeUnit.HOUR_OF_DAY, timeZone()), + roundings[2] = new RoundingInfo(createRounding(DateTimeUnit.HOUR_OF_DAY, timeZone), 60 * 60 * 1000L, 1, 3, 12); - roundings[3] = new RoundingInfo(createRounding(DateTimeUnit.DAY_OF_MONTH, timeZone()), + roundings[3] = new RoundingInfo(createRounding(DateTimeUnit.DAY_OF_MONTH, timeZone), 24 * 60 * 60 * 1000L, 1, 7); - roundings[4] = new RoundingInfo(createRounding(DateTimeUnit.MONTH_OF_YEAR, timeZone()), + roundings[4] = new RoundingInfo(createRounding(DateTimeUnit.MONTH_OF_YEAR, timeZone), 30 * 24 * 60 * 60 * 1000L, 1, 3); - roundings[5] = new RoundingInfo(createRounding(DateTimeUnit.YEAR_OF_CENTURY, timeZone()), + roundings[5] = new RoundingInfo(createRounding(DateTimeUnit.YEAR_OF_CENTURY, timeZone), 365 * 24 * 60 * 60 * 1000L, 1, 5, 10, 20, 50, 100); return roundings; } @@ -140,7 +140,7 @@ public int getNumBuckets() { @Override protected ValuesSourceAggregatorFactory innerBuild(SearchContext context, ValuesSourceConfig config, AggregatorFactory parent, Builder subFactoriesBuilder) throws IOException { - RoundingInfo[] roundings = buildRoundings(); + RoundingInfo[] roundings = buildRoundings(timeZone()); int maxRoundingInterval = Arrays.stream(roundings,0, roundings.length-1) .map(rounding -> rounding.innerIntervals) .flatMapToInt(Arrays::stream) @@ -156,7 +156,7 @@ public int getNumBuckets() { return new AutoDateHistogramAggregatorFactory(name, config, numBuckets, roundings, context, parent, subFactoriesBuilder, metaData); } - private Rounding createRounding(DateTimeUnit interval, DateTimeZone timeZone) { + private static Rounding createRounding(DateTimeUnit interval, DateTimeZone timeZone) { Rounding.Builder tzRoundingBuilder = Rounding.builder(interval); if (timeZone != null) { tzRoundingBuilder.timeZone(timeZone); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogramTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogramTests.java index 01b9d01ba8328..43335c74698c6 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogramTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogramTests.java @@ -49,9 +49,6 @@ public class InternalAutoDateHistogramTests extends InternalMultiBucketAggregati public void setUp() throws Exception { super.setUp(); format = randomNumericDocValueFormat(); - AutoDateHistogramAggregationBuilder aggregationBuilder = new AutoDateHistogramAggregationBuilder("_name"); - // TODO[PCS]: timezone set automagically here? - roundingInfos = aggregationBuilder.buildRoundings(); } @Override @@ -59,6 +56,9 @@ protected InternalAutoDateHistogram createTestInstance(String name, List pipelineAggregators, Map metaData, InternalAggregations aggregations) { + AutoDateHistogramAggregationBuilder aggregationBuilder = new AutoDateHistogramAggregationBuilder("_name"); + roundingInfos = AutoDateHistogramAggregationBuilder.buildRoundings(aggregationBuilder.timeZone()); + int nbBuckets = randomNumberOfBuckets(); int targetBuckets = randomIntBetween(1, nbBuckets * 2 + 1); List buckets = new ArrayList<>(nbBuckets); @@ -74,6 +74,7 @@ protected InternalAutoDateHistogram createTestInstance(String name, InternalAggregations subAggregations = new InternalAggregations(Collections.emptyList()); BucketInfo bucketInfo = new BucketInfo(roundingInfos, randomIntBetween(0, roundingInfos.length - 1), subAggregations); + return new InternalAutoDateHistogram(name, buckets, targetBuckets, bucketInfo, format, pipelineAggregators, metaData); } From 55a06aac292203e83c1e93c9bcd0a10d93bf980d Mon Sep 17 00:00:00 2001 From: Paul Sanwald Date: Wed, 25 Jul 2018 16:51:37 -0400 Subject: [PATCH 6/6] Remove unnecessary builder --- .../bucket/histogram/InternalAutoDateHistogramTests.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogramTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogramTests.java index 43335c74698c6..a14fca63154d7 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogramTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogramTests.java @@ -56,9 +56,8 @@ protected InternalAutoDateHistogram createTestInstance(String name, List pipelineAggregators, Map metaData, InternalAggregations aggregations) { - AutoDateHistogramAggregationBuilder aggregationBuilder = new AutoDateHistogramAggregationBuilder("_name"); - roundingInfos = AutoDateHistogramAggregationBuilder.buildRoundings(aggregationBuilder.timeZone()); + roundingInfos = AutoDateHistogramAggregationBuilder.buildRoundings(null); int nbBuckets = randomNumberOfBuckets(); int targetBuckets = randomIntBetween(1, nbBuckets * 2 + 1); List buckets = new ArrayList<>(nbBuckets);