From 02e20e41d88164ce1c29bc15a12032672bd3025e Mon Sep 17 00:00:00 2001 From: Paul Sanwald Date: Wed, 8 Aug 2018 17:06:51 -0400 Subject: [PATCH 1/7] count down from innerIntervals instead of up. Also add test case for incorrect reference to innerintervals --- .../AutoDateHistogramAggregationBuilder.java | 2 +- .../histogram/InternalAutoDateHistogram.java | 8 ++-- .../InternalAutoDateHistogramTests.java | 40 +++++++++++++------ 3 files changed, 32 insertions(+), 18 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 50a0c85c041c8..b97670ddb5730 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 @@ -156,7 +156,7 @@ public int getNumBuckets() { return new AutoDateHistogramAggregatorFactory(name, config, numBuckets, roundings, context, parent, subFactoriesBuilder, metaData); } - private static Rounding createRounding(DateTimeUnit interval, DateTimeZone timeZone) { + static Rounding createRounding(DateTimeUnit interval, DateTimeZone timeZone) { Rounding.Builder tzRoundingBuilder = Rounding.builder(interval); if (timeZone != null) { tzRoundingBuilder.timeZone(timeZone); 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 6a78ca6724988..61b34f96a8bf7 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,8 +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) { + static int getAppropriateRounding(long minKey, long maxKey, int roundingIdx, + RoundingInfo[] roundings, int targetBuckets) { if (roundingIdx == roundings.length - 1) { return roundingIdx; } @@ -480,7 +480,7 @@ private int getAppropriateRounding(long minKey, long maxKey, int roundingIdx, currentKey = currentRounding.nextRoundingValue(currentKey); } currentRoundingIdx++; - } while (requiredBuckets > (targetBuckets * roundings[roundingIdx].getMaximumInnerInterval()) + } while (requiredBuckets > (targetBuckets * roundings[currentRoundingIdx-1].getMaximumInnerInterval()) && currentRoundingIdx < roundings.length); // The loop will increase past the correct rounding index here so we // need to subtract one to get the rounding index we need 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 981d263d7d633..92676b9395521 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,6 +20,7 @@ package org.elasticsearch.search.aggregations.bucket.histogram; import org.elasticsearch.common.io.stream.Writeable; +import org.elasticsearch.common.rounding.DateTimeUnit; import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.aggregations.InternalAggregations; import org.elasticsearch.search.aggregations.ParsedMultiBucketAggregation; @@ -28,17 +29,18 @@ import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; import org.elasticsearch.test.InternalMultiBucketAggregationTestCase; import org.joda.time.DateTime; +import org.joda.time.DateTimeZone; -import java.util.ArrayList; -import java.util.Collections; -import java.util.HashMap; -import java.util.List; -import java.util.Map; -import java.util.TreeMap; +import java.time.Instant; +import java.time.OffsetDateTime; +import java.time.ZoneOffset; +import java.util.*; import static org.elasticsearch.common.unit.TimeValue.timeValueHours; import static org.elasticsearch.common.unit.TimeValue.timeValueMinutes; import static org.elasticsearch.common.unit.TimeValue.timeValueSeconds; +import static org.elasticsearch.search.aggregations.bucket.histogram.AutoDateHistogramAggregationBuilder.createRounding; +import static org.hamcrest.Matchers.equalTo; public class InternalAutoDateHistogramTests extends InternalMultiBucketAggregationTestCase { @@ -77,6 +79,22 @@ protected InternalAutoDateHistogram createTestInstance(String name, return new InternalAutoDateHistogram(name, buckets, targetBuckets, bucketInfo, format, pipelineAggregators, metaData); } + public void testGetAppropriateRounding() { + RoundingInfo[] roundings = new RoundingInfo[6]; + DateTimeZone timeZone = DateTimeZone.UTC; + roundings[0] = new RoundingInfo(createRounding(DateTimeUnit.SECOND_OF_MINUTE, timeZone), + 1000L, 1000); + 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); + + OffsetDateTime timestamp = Instant.parse("2018-01-01T00:00:01.000Z").atOffset(ZoneOffset.UTC); + int result = InternalAutoDateHistogram.getAppropriateRounding(timestamp.toEpochSecond()*1000, + timestamp.plusDays(1).toEpochSecond()*1000, 0, roundings, 25); + assertThat(result, equalTo(2)); + } + @Override protected void assertReduced(InternalAutoDateHistogram reduced, List inputs) { int roundingIdx = 0; @@ -100,9 +118,11 @@ protected void assertReduced(InternalAutoDateHistogram reduced, List= 0; j--) { + int interval = roundingInfo.innerIntervals[j]; if (normalizedDuration / interval < maxNumberOfBuckets()) { innerIntervalToUse = interval; } @@ -137,12 +157,6 @@ protected void assertReduced(InternalAutoDateHistogram reduced, List instanceReader() { return InternalAutoDateHistogram::new; From 87ce3baac787fdb3e25fa44bcaf2c76765bb5822 Mon Sep 17 00:00:00 2001 From: Paul Sanwald Date: Wed, 8 Aug 2018 20:49:24 -0400 Subject: [PATCH 2/7] fix wildcard imports --- .../bucket/histogram/InternalAutoDateHistogramTests.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) 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 92676b9395521..8cb1689b517ea 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 @@ -34,7 +34,12 @@ import java.time.Instant; import java.time.OffsetDateTime; import java.time.ZoneOffset; -import java.util.*; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.TreeMap; import static org.elasticsearch.common.unit.TimeValue.timeValueHours; import static org.elasticsearch.common.unit.TimeValue.timeValueMinutes; From 27f7422426a1f94d190a309e140113602956a9af Mon Sep 17 00:00:00 2001 From: Paul Sanwald Date: Thu, 9 Aug 2018 13:26:08 -0400 Subject: [PATCH 3/7] more bug fixes and testing --- .../InternalAutoDateHistogramTests.java | 24 ++++++++++++------- .../test/InternalAggregationTestCase.java | 2 ++ 2 files changed, 17 insertions(+), 9 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 8cb1689b517ea..274e9ff4ff4cc 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 @@ -84,9 +84,16 @@ protected InternalAutoDateHistogram createTestInstance(String name, return new InternalAutoDateHistogram(name, buckets, targetBuckets, bucketInfo, format, pipelineAggregators, metaData); } - public void testGetAppropriateRounding() { + /* + This test was added to reproduce a bug where getAppropriateRounding was only ever using the first innerIntervals + passed in, instead of using the interval associated with the loop. + */ + public void testGetAppropriateRoundingUsesCorrectIntervals() { RoundingInfo[] roundings = new RoundingInfo[6]; DateTimeZone timeZone = DateTimeZone.UTC; + // Since we pass 0 as the starting index to getAppropriateRounding, we'll also use + // an innerInterval that is quite large, such that targetBuckets * roundings[i].getMaximumInnerInterval() + // will be larger than the estimate. roundings[0] = new RoundingInfo(createRounding(DateTimeUnit.SECOND_OF_MINUTE, timeZone), 1000L, 1000); roundings[1] = new RoundingInfo(createRounding(DateTimeUnit.MINUTES_OF_HOUR, timeZone), @@ -95,6 +102,9 @@ public void testGetAppropriateRounding() { 60 * 60 * 1000L, 1, 3, 12); OffsetDateTime timestamp = Instant.parse("2018-01-01T00:00:01.000Z").atOffset(ZoneOffset.UTC); + // We want to pass a roundingIdx of zero, because in order to reproduce this bug, we need the function + // to increment the rounding (because the bug was that the function would not use the innerIntervals + // from the new rounding. int result = InternalAutoDateHistogram.getAppropriateRounding(timestamp.toEpochSecond()*1000, timestamp.plusDays(1).toEpochSecond()*1000, 0, roundings, 25); assertThat(result, equalTo(2)); @@ -102,13 +112,6 @@ public void testGetAppropriateRounding() { @Override protected void assertReduced(InternalAutoDateHistogram reduced, List inputs) { - int roundingIdx = 0; - for (InternalAutoDateHistogram histogram : inputs) { - if (histogram.getBucketInfo().roundingIdx > roundingIdx) { - roundingIdx = histogram.getBucketInfo().roundingIdx; - } - } - RoundingInfo roundingInfo = roundingInfos[roundingIdx]; long lowest = Long.MAX_VALUE; long highest = 0; @@ -124,11 +127,14 @@ protected void assertReduced(InternalAutoDateHistogram reduced, List= 0; j--) { int interval = roundingInfo.innerIntervals[j]; - if (normalizedDuration / interval < maxNumberOfBuckets()) { + if (normalizedDuration / interval < reduced.getTargetBuckets()) { innerIntervalToUse = interval; } } diff --git a/test/framework/src/main/java/org/elasticsearch/test/InternalAggregationTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/InternalAggregationTestCase.java index 15e44853a97ba..aadba316b3cb0 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/InternalAggregationTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/InternalAggregationTestCase.java @@ -19,6 +19,7 @@ package org.elasticsearch.test; +import com.carrotsearch.randomizedtesting.annotations.Seed; import org.apache.lucene.util.SetOnce; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.bytes.BytesReference; @@ -229,6 +230,7 @@ protected T createUnmappedInstance(String name, return createTestInstance(name, pipelineAggregators, metaData); } + @Seed("ED2D551807CFA25B") public void testReduceRandom() { String name = randomAlphaOfLength(5); List inputs = new ArrayList<>(); From 91b0f8e016c0b8313a0e6def0e8ddee7a217e8c5 Mon Sep 17 00:00:00 2001 From: Paul Sanwald Date: Fri, 10 Aug 2018 07:09:15 -0400 Subject: [PATCH 4/7] remove seed used for testing --- .../org/elasticsearch/test/InternalAggregationTestCase.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/framework/src/main/java/org/elasticsearch/test/InternalAggregationTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/InternalAggregationTestCase.java index aadba316b3cb0..15e44853a97ba 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/InternalAggregationTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/InternalAggregationTestCase.java @@ -19,7 +19,6 @@ package org.elasticsearch.test; -import com.carrotsearch.randomizedtesting.annotations.Seed; import org.apache.lucene.util.SetOnce; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.bytes.BytesReference; @@ -230,7 +229,6 @@ protected T createUnmappedInstance(String name, return createTestInstance(name, pipelineAggregators, metaData); } - @Seed("ED2D551807CFA25B") public void testReduceRandom() { String name = randomAlphaOfLength(5); List inputs = new ArrayList<>(); From 43cfe4a9c1a98f104ca1dd3e1afb15ddcbef3e2d Mon Sep 17 00:00:00 2001 From: Paul Sanwald Date: Tue, 14 Aug 2018 13:42:20 -0400 Subject: [PATCH 5/7] WIP commit that correctly calculates buckets --- .../InternalAutoDateHistogramTests.java | 98 +++++++++++++++---- 1 file changed, 79 insertions(+), 19 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 274e9ff4ff4cc..7194414d8f739 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 @@ -68,7 +68,8 @@ protected InternalAutoDateHistogram createTestInstance(String name, int nbBuckets = randomNumberOfBuckets(); int targetBuckets = randomIntBetween(1, nbBuckets * 2 + 1); List buckets = new ArrayList<>(nbBuckets); - long startingDate = System.currentTimeMillis(); + + long startingDate = /*1534192789667L;*/ System.currentTimeMillis(); long interval = randomIntBetween(1, 3); long intervalMillis = randomFrom(timeValueSeconds(interval), timeValueMinutes(interval), timeValueHours(interval)).getMillis(); @@ -131,43 +132,102 @@ protected void assertReduced(InternalAutoDateHistogram reduced, List= 0; j--) { - int interval = roundingInfo.innerIntervals[j]; - if (normalizedDuration / interval < reduced.getTargetBuckets()) { - innerIntervalToUse = interval; + long innerIntervalToUse = roundingInfo.innerIntervals[0]; + + int innerIntervalIndex = 0; + if (normalizedDuration != 0) { + for (int j = roundingInfo.innerIntervals.length-1; j >= 0; j--) { + int interval = roundingInfo.innerIntervals[j]; + if (normalizedDuration / interval < reduced.getBuckets().size()) { + innerIntervalToUse = interval; + innerIntervalIndex = j; + } } } + + long intervalInMillis = innerIntervalToUse * roundingInfo.getRoughEstimateDurationMillis(); + int bucketCount = getBucketCount(lowest, highest, roundingInfo, intervalInMillis); + + if (bucketCount > reduced.getBuckets().size()) { + for (int i = innerIntervalIndex; i < roundingInfo.innerIntervals.length; i++) { + if (getBucketCount(lowest, highest, roundingInfo, roundingInfo.innerIntervals[i]*roundingInfo.getRoughEstimateDurationMillis()) <= reduced.getBuckets().size()) { + innerIntervalToUse = roundingInfo.innerIntervals[i]; + intervalInMillis = innerIntervalToUse * roundingInfo.getRoughEstimateDurationMillis(); + } + } + } + + /* + + if (bucketCount != reduced.getBuckets().size()) { + + for (long keyForBucket = roundingInfo.rounding.round(lowest); + keyForBucket <= highest; + keyForBucket = keyForBucket + intervalInMillis) { + logger.info("Expected key: "+keyForBucket); + } + for (InternalAutoDateHistogram.Bucket bucket : reduced.getBuckets()) { + logger.info("Actual Key: "+bucket.key); + } + logger.info("innerIntervalToUse "+innerIntervalToUse); + logger.info("roundingInfo.getRoughEstimateDurationMillis() "+roundingInfo.getRoughEstimateDurationMillis()); + logger.info("intervalInMillis "+intervalInMillis); + logger.info("bucketCount "+bucketCount); + logger.info("highest "+highest); + logger.info("lowest "+lowest); + long diff = highest - lowest; + logger.info("highest - lowest "+diff); + logger.info("normalizedDuration "+normalizedDuration); + } + */ + Map expectedCounts = new TreeMap<>(); - long intervalInMillis = innerIntervalToUse*roundingInfo.getRoughEstimateDurationMillis(); + for (long keyForBucket = roundingInfo.rounding.round(lowest); - keyForBucket <= highest; + keyForBucket <= roundingInfo.rounding.round(highest); keyForBucket = keyForBucket + intervalInMillis) { expectedCounts.put(keyForBucket, 0L); + } + if (roundingInfo.rounding.round(lowest) == roundingInfo.rounding.round(highest) && expectedCounts.isEmpty()) { + expectedCounts.put(roundingInfo.rounding.round(lowest), 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); - } + /* + for (InternalAutoDateHistogram histogram : inputs) { + for (Histogram.Bucket bucket : histogram.getBuckets()) { + long roundedBucketKey = roundingInfo.rounding.round(((DateTime) bucket.getKey()).getMillis()); + long docCount = bucket.getDocCount(); + if (roundedBucketKey >= keyForBucket + && roundedBucketKey < keyForBucket + intervalInMillis) { + expectedCounts.compute(keyForBucket, + (key, oldValue) -> (oldValue == null ? 0 : oldValue) + docCount); + } else if (roundedBucketKey == (keyForBucket + intervalInMillis) && roundedBucketKey == roundingInfo.rounding.round(highest)) { + expectedCounts.compute(keyForBucket, + (key, oldValue) -> (oldValue == null ? 0 : oldValue) + docCount); } } } + */ Map actualCounts = new TreeMap<>(); for (Histogram.Bucket bucket : reduced.getBuckets()) { actualCounts.compute(((DateTime) bucket.getKey()).getMillis(), - (key, oldValue) -> (oldValue == null ? 0 : oldValue) + bucket.getDocCount()); + (key, oldValue) -> (oldValue == null ? 0 : oldValue) + /*bucket.getDocCount()*/0); } assertEquals(expectedCounts, actualCounts); } + private int getBucketCount(long lowest, long highest, RoundingInfo roundingInfo, long intervalInMillis) { + int bucketCount = 0; + for (long keyForBucket = roundingInfo.rounding.round(lowest); + keyForBucket <= roundingInfo.rounding.round(highest); + keyForBucket = keyForBucket + intervalInMillis) { + bucketCount++; + } + return bucketCount; + } + @Override protected Writeable.Reader instanceReader() { return InternalAutoDateHistogram::new; From ae2957c00a5d4303192b41ba310825272c49a5b7 Mon Sep 17 00:00:00 2001 From: Paul Sanwald Date: Tue, 14 Aug 2018 17:45:41 -0400 Subject: [PATCH 6/7] update logic in assertReduced to correctly implement logic for bucket slicing --- .../InternalAutoDateHistogramTests.java | 76 ++++++++----------- 1 file changed, 30 insertions(+), 46 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 7194414d8f739..77eb367ecc987 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 @@ -69,7 +69,7 @@ protected InternalAutoDateHistogram createTestInstance(String name, int targetBuckets = randomIntBetween(1, nbBuckets * 2 + 1); List buckets = new ArrayList<>(nbBuckets); - long startingDate = /*1534192789667L;*/ System.currentTimeMillis(); + long startingDate = System.currentTimeMillis(); long interval = randomIntBetween(1, 3); long intervalMillis = randomFrom(timeValueSeconds(interval), timeValueMinutes(interval), timeValueHours(interval)).getMillis(); @@ -116,6 +116,7 @@ protected void assertReduced(InternalAutoDateHistogram reduced, List= 0; j--) { int interval = roundingInfo.innerIntervals[j]; @@ -148,72 +151,53 @@ protected void assertReduced(InternalAutoDateHistogram reduced, List reduced.getBuckets().size()) { for (int i = innerIntervalIndex; i < roundingInfo.innerIntervals.length; i++) { - if (getBucketCount(lowest, highest, roundingInfo, roundingInfo.innerIntervals[i]*roundingInfo.getRoughEstimateDurationMillis()) <= reduced.getBuckets().size()) { + long newIntervalMillis = roundingInfo.innerIntervals[i] * roundingInfo.getRoughEstimateDurationMillis(); + if (getBucketCount(lowest, highest, roundingInfo, newIntervalMillis) <= reduced.getBuckets().size()) { innerIntervalToUse = roundingInfo.innerIntervals[i]; intervalInMillis = innerIntervalToUse * roundingInfo.getRoughEstimateDurationMillis(); } } } - /* - - if (bucketCount != reduced.getBuckets().size()) { - - for (long keyForBucket = roundingInfo.rounding.round(lowest); - keyForBucket <= highest; - keyForBucket = keyForBucket + intervalInMillis) { - logger.info("Expected key: "+keyForBucket); - } - for (InternalAutoDateHistogram.Bucket bucket : reduced.getBuckets()) { - logger.info("Actual Key: "+bucket.key); - } - logger.info("innerIntervalToUse "+innerIntervalToUse); - logger.info("roundingInfo.getRoughEstimateDurationMillis() "+roundingInfo.getRoughEstimateDurationMillis()); - logger.info("intervalInMillis "+intervalInMillis); - logger.info("bucketCount "+bucketCount); - logger.info("highest "+highest); - logger.info("lowest "+lowest); - long diff = highest - lowest; - logger.info("highest - lowest "+diff); - logger.info("normalizedDuration "+normalizedDuration); - } - */ - Map expectedCounts = new TreeMap<>(); - for (long keyForBucket = roundingInfo.rounding.round(lowest); keyForBucket <= roundingInfo.rounding.round(highest); keyForBucket = keyForBucket + intervalInMillis) { expectedCounts.put(keyForBucket, 0L); - } - if (roundingInfo.rounding.round(lowest) == roundingInfo.rounding.round(highest) && expectedCounts.isEmpty()) { - expectedCounts.put(roundingInfo.rounding.round(lowest), 0L); - } - /* - for (InternalAutoDateHistogram histogram : inputs) { - for (Histogram.Bucket bucket : histogram.getBuckets()) { - long roundedBucketKey = roundingInfo.rounding.round(((DateTime) bucket.getKey()).getMillis()); - long docCount = bucket.getDocCount(); - if (roundedBucketKey >= keyForBucket - && roundedBucketKey < keyForBucket + intervalInMillis) { - expectedCounts.compute(keyForBucket, - (key, oldValue) -> (oldValue == null ? 0 : oldValue) + docCount); - } else if (roundedBucketKey == (keyForBucket + intervalInMillis) && roundedBucketKey == roundingInfo.rounding.round(highest)) { - expectedCounts.compute(keyForBucket, - (key, oldValue) -> (oldValue == null ? 0 : oldValue) + docCount); + // Iterate through the input buckets, and for each bucket, determine if it's inside + // the range of the bucket in the outer loop. if it is, add the doc count to the total + // for that bucket. + + for (InternalAutoDateHistogram histogram : inputs) { + for (Histogram.Bucket bucket : histogram.getBuckets()) { + long roundedBucketKey = roundingInfo.rounding.round(((DateTime) bucket.getKey()).getMillis()); + long docCount = bucket.getDocCount(); + if (roundedBucketKey >= keyForBucket + && roundedBucketKey < keyForBucket + intervalInMillis) { + expectedCounts.compute(keyForBucket, + (key, oldValue) -> (oldValue == null ? 0 : oldValue) + docCount); + } } } } - */ + + // If there is only a single bucket, and we haven't added it above, add a bucket with no documents. + // this step is necessary because of the roundedBucketKey < keyForBucket + intervalInMillis above. + if (roundingInfo.rounding.round(lowest) == roundingInfo.rounding.round(highest) && expectedCounts.isEmpty()) { + expectedCounts.put(roundingInfo.rounding.round(lowest), 0L); + } + // pick out the actual reduced values to the make the assertion more readable Map actualCounts = new TreeMap<>(); for (Histogram.Bucket bucket : reduced.getBuckets()) { actualCounts.compute(((DateTime) bucket.getKey()).getMillis(), - (key, oldValue) -> (oldValue == null ? 0 : oldValue) + /*bucket.getDocCount()*/0); + (key, oldValue) -> (oldValue == null ? 0 : oldValue) + bucket.getDocCount()); } assertEquals(expectedCounts, actualCounts); } From 9dd64368ca243ef675df0a60c6ea587b5fc1918d Mon Sep 17 00:00:00 2001 From: Paul Sanwald Date: Thu, 16 Aug 2018 12:15:48 -0400 Subject: [PATCH 7/7] fix indentation and whitespace --- .../bucket/histogram/InternalAutoDateHistogram.java | 2 +- .../bucket/histogram/InternalAutoDateHistogramTests.java | 2 -- 2 files changed, 1 insertion(+), 3 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 61b34f96a8bf7..7bc2b9a317838 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 @@ -480,7 +480,7 @@ static int getAppropriateRounding(long minKey, long maxKey, int roundingIdx, currentKey = currentRounding.nextRoundingValue(currentKey); } currentRoundingIdx++; - } while (requiredBuckets > (targetBuckets * roundings[currentRoundingIdx-1].getMaximumInnerInterval()) + } while (requiredBuckets > (targetBuckets * roundings[currentRoundingIdx - 1].getMaximumInnerInterval()) && currentRoundingIdx < roundings.length); // The loop will increase past the correct rounding index here so we // need to subtract one to get the rounding index we need 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 77eb367ecc987..b7c5bf03ac552 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 @@ -80,8 +80,6 @@ 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); }