From f8b7b38745838d4140daac71f9b03260eddc2cd1 Mon Sep 17 00:00:00 2001 From: Igor Motov Date: Tue, 24 Nov 2020 09:40:06 -0500 Subject: [PATCH 1/2] Return an error when a rate aggregation cannot calculate bucket sizes In some cases when the rate aggregation is not a child of a date histogram aggregation, it is not possible to determine the actual size of the date histogram bucket. In this case the rate aggregation now throws an exception. Closes #63703 --- .../metrics/rate-aggregation.asciidoc | 5 ++ .../org/elasticsearch/common/Rounding.java | 24 ++++--- .../histogram/DateHistogramAggregator.java | 18 +++++ .../histogram/SizedBucketAggregator.java | 11 ++- .../elasticsearch/common/RoundingTests.java | 33 ++++++--- .../rate/AbstractRateAggregator.java | 12 +++- .../analytics/rate/RateAggregatorTests.java | 71 +++++++++++++++++++ 7 files changed, 151 insertions(+), 23 deletions(-) diff --git a/docs/reference/aggregations/metrics/rate-aggregation.asciidoc b/docs/reference/aggregations/metrics/rate-aggregation.asciidoc index 6aa010400584b..8e0d79119e217 100644 --- a/docs/reference/aggregations/metrics/rate-aggregation.asciidoc +++ b/docs/reference/aggregations/metrics/rate-aggregation.asciidoc @@ -261,6 +261,11 @@ convert the bucket size into the rate. By default the interval of the `date_hist `"rate": "quarter"`:: compatible with only with `month`, `quarter` and `year` calendar intervals `"rate": "year"`:: compatible with only with `month`, `quarter` and `year` calendar intervals +There is also an additional limitations if the date histogram is not a direct parent of the rate histogram. In this case both rate interval +and histogram interval have to be in the same group: [`second`, ` minute`, `hour`, `day`, `week`] or [`month`, `quarter`, `year`]. For +example, if the date histogram is `month` based, only rate intervals of `month`, `quarter` or `year` are supported. If the date histogram +is `day` based, only `second`, ` minute`, `hour`, `day`, and `week` rate intervals are supported. + ==== Script The `rate` aggregation also supports scripting. For example, if we need to adjust out prices before calculating rates, we could use diff --git a/server/src/main/java/org/elasticsearch/common/Rounding.java b/server/src/main/java/org/elasticsearch/common/Rounding.java index 081a08a2f4a44..9d94c8fc0c657 100644 --- a/server/src/main/java/org/elasticsearch/common/Rounding.java +++ b/server/src/main/java/org/elasticsearch/common/Rounding.java @@ -290,7 +290,7 @@ public interface Prepared { * Given the rounded value, returns the size between this value and the * next rounded value in specified units if possible. */ - double roundingSize(long utcMillis, DateTimeUnit timeUnit); + double roundingSize(Long utcMillis, DateTimeUnit timeUnit); /** * If this rounding mechanism precalculates rounding points then * this array stores dates such that each date between each entry. @@ -609,16 +609,22 @@ public String toString() { private abstract class TimeUnitPreparedRounding extends PreparedRounding { @Override - public double roundingSize(long utcMillis, DateTimeUnit timeUnit) { + public double roundingSize(Long utcMillis, DateTimeUnit timeUnit) { if (timeUnit.isMillisBased == unit.isMillisBased) { return (double) unit.ratio / timeUnit.ratio; } else { - if (unit.isMillisBased == false) { + if (unit.isMillisBased == false && utcMillis != null) { return (double) (nextRoundingValue(utcMillis) - utcMillis) / timeUnit.ratio; } else { - throw new IllegalArgumentException("Cannot use month-based rate unit [" + timeUnit.shortName + - "] with non-month based calendar interval histogram [" + unit.shortName + - "] only week, day, hour, minute and second are supported for this histogram"); + if (timeUnit.isMillisBased == false) { + throw new IllegalArgumentException("Cannot use month-based rate unit [" + timeUnit.shortName + + "] with non-month based calendar interval histogram [" + unit.shortName + + "] only week, day, hour, minute and second are supported for this histogram"); + } else { + throw new IllegalArgumentException("Cannot use non month-based rate unit [" + timeUnit.shortName + + "] with month-based calendar interval histogram [" + unit.shortName + + "] only month, quarter and year are supported for this histogram"); + } } } } @@ -995,7 +1001,7 @@ private long roundKey(long value, long interval) { private abstract class TimeIntervalPreparedRounding extends PreparedRounding { @Override - public double roundingSize(long utcMillis, DateTimeUnit timeUnit) { + public double roundingSize(Long utcMillis, DateTimeUnit timeUnit) { if (timeUnit.isMillisBased) { return (double) interval / timeUnit.ratio; } else { @@ -1262,7 +1268,7 @@ public long nextRoundingValue(long utcMillis) { } @Override - public double roundingSize(long utcMillis, DateTimeUnit timeUnit) { + public double roundingSize(Long utcMillis, DateTimeUnit timeUnit) { return delegatePrepared.roundingSize(utcMillis, timeUnit); } @@ -1350,7 +1356,7 @@ public long nextRoundingValue(long utcMillis) { } @Override - public double roundingSize(long utcMillis, DateTimeUnit timeUnit) { + public double roundingSize(Long utcMillis, DateTimeUnit timeUnit) { return delegate.roundingSize(utcMillis, timeUnit); } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramAggregator.java index 125ca3d2c50c6..8132d195623c1 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramAggregator.java @@ -344,6 +344,15 @@ public double bucketSize(long bucket, Rounding.DateTimeUnit unitSize) { } } + @Override + public double bucketSize(Rounding.DateTimeUnit unitSize) { + if (unitSize != null) { + return preparedRounding.roundingSize(null, unitSize); + } else { + return 1.0; + } + } + static class FromDateRange extends AdaptingAggregator implements SizedBucketAggregator { private final DocValueFormat format; private final Rounding rounding; @@ -431,5 +440,14 @@ public double bucketSize(long bucket, DateTimeUnit unitSize) { return 1.0; } } + + @Override + public double bucketSize(DateTimeUnit unitSize) { + if (unitSize != null) { + return preparedRounding.roundingSize(null, unitSize); + } else { + return 1.0; + } + } } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/SizedBucketAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/SizedBucketAggregator.java index 3a4b611a30c07..c49a9d49565df 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/SizedBucketAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/SizedBucketAggregator.java @@ -22,8 +22,17 @@ import org.elasticsearch.common.Rounding; /** - * An aggregator capable of reporting bucket sizes in milliseconds. Used by RateAggregator for calendar-based buckets. + * An aggregator capable of reporting bucket sizes in requested units. Used by RateAggregator for calendar-based buckets. */ public interface SizedBucketAggregator { + /** + * Reports size of the particular bucket in requested units. + */ double bucketSize(long bucket, Rounding.DateTimeUnit unit); + + /** + * Reports size of all buckets in requested units. Throws an exception if it is not possible to calculate without knowing + * the concrete bucket. + */ + double bucketSize(Rounding.DateTimeUnit unit); } diff --git a/server/src/test/java/org/elasticsearch/common/RoundingTests.java b/server/src/test/java/org/elasticsearch/common/RoundingTests.java index 5e7392caf9fdc..ebda1dc17f7a4 100644 --- a/server/src/test/java/org/elasticsearch/common/RoundingTests.java +++ b/server/src/test/java/org/elasticsearch/common/RoundingTests.java @@ -979,24 +979,26 @@ public void testFixedIntervalRoundingSize() { public void testMillisecondsBasedUnitCalendarRoundingSize() { Rounding unitRounding = Rounding.builder(Rounding.DateTimeUnit.HOUR_OF_DAY).build(); Rounding.Prepared prepared = unitRounding.prepare(time("2010-01-01T00:00:00.000Z"), time("2020-01-01T00:00:00.000Z")); - assertThat(prepared.roundingSize(time("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.SECOND_OF_MINUTE), + assertThat(prepared.roundingSize(timeOrNull("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.SECOND_OF_MINUTE), closeTo(3600.0, 0.000001)); - assertThat(prepared.roundingSize(time("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.MINUTES_OF_HOUR), closeTo(60.0, 0.000001)); - assertThat(prepared.roundingSize(time("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.HOUR_OF_DAY), closeTo(1.0, 0.000001)); - assertThat(prepared.roundingSize(time("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.DAY_OF_MONTH), + assertThat(prepared.roundingSize(timeOrNull("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.MINUTES_OF_HOUR), + closeTo(60.0, 0.000001)); + assertThat(prepared.roundingSize(timeOrNull("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.HOUR_OF_DAY), + closeTo(1.0, 0.000001)); + assertThat(prepared.roundingSize(timeOrNull("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.DAY_OF_MONTH), closeTo(1 / 24.0, 0.000001)); - assertThat(prepared.roundingSize(time("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.WEEK_OF_WEEKYEAR), + assertThat(prepared.roundingSize(timeOrNull("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.WEEK_OF_WEEKYEAR), closeTo(1 / 168.0, 0.000001)); IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, - () -> prepared.roundingSize(time("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.MONTH_OF_YEAR)); + () -> prepared.roundingSize(timeOrNull("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.MONTH_OF_YEAR)); assertThat(ex.getMessage(), equalTo("Cannot use month-based rate unit [month] with non-month based calendar interval " + "histogram [hour] only week, day, hour, minute and second are supported for this histogram")); ex = expectThrows(IllegalArgumentException.class, - () -> prepared.roundingSize(time("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.QUARTER_OF_YEAR)); + () -> prepared.roundingSize(timeOrNull("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.QUARTER_OF_YEAR)); assertThat(ex.getMessage(), equalTo("Cannot use month-based rate unit [quarter] with non-month based calendar interval " + "histogram [hour] only week, day, hour, minute and second are supported for this histogram")); ex = expectThrows(IllegalArgumentException.class, - () -> prepared.roundingSize(time("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.YEAR_OF_CENTURY)); + () -> prepared.roundingSize(timeOrNull("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.YEAR_OF_CENTURY)); assertThat(ex.getMessage(), equalTo("Cannot use month-based rate unit [year] with non-month based calendar interval " + "histogram [hour] only week, day, hour, minute and second are supported for this histogram")); } @@ -1005,10 +1007,11 @@ public void testNonMillisecondsBasedUnitCalendarRoundingSize() { Rounding unitRounding = Rounding.builder(Rounding.DateTimeUnit.QUARTER_OF_YEAR).build(); Rounding.Prepared prepared = unitRounding.prepare(time("2010-01-01T00:00:00.000Z"), time("2020-01-01T00:00:00.000Z")); long firstQuarter = prepared.round(time("2015-01-01T00:00:00.000Z")); + Long firstQuarterOrNull = randomBoolean() ? firstQuarter : null; // Ratio based - assertThat(prepared.roundingSize(firstQuarter, Rounding.DateTimeUnit.MONTH_OF_YEAR), closeTo(3.0, 0.000001)); - assertThat(prepared.roundingSize(firstQuarter, Rounding.DateTimeUnit.QUARTER_OF_YEAR), closeTo(1.0, 0.000001)); - assertThat(prepared.roundingSize(firstQuarter, Rounding.DateTimeUnit.YEAR_OF_CENTURY), closeTo(0.25, 0.000001)); + assertThat(prepared.roundingSize(firstQuarterOrNull, Rounding.DateTimeUnit.MONTH_OF_YEAR), closeTo(3.0, 0.000001)); + assertThat(prepared.roundingSize(firstQuarterOrNull, Rounding.DateTimeUnit.QUARTER_OF_YEAR), closeTo(1.0, 0.000001)); + assertThat(prepared.roundingSize(firstQuarterOrNull, Rounding.DateTimeUnit.YEAR_OF_CENTURY), closeTo(0.25, 0.000001)); // Real interval based assertThat(prepared.roundingSize(firstQuarter, Rounding.DateTimeUnit.SECOND_OF_MINUTE), closeTo(7776000.0, 0.000001)); assertThat(prepared.roundingSize(firstQuarter, Rounding.DateTimeUnit.MINUTES_OF_HOUR), closeTo(129600.0, 0.000001)); @@ -1017,6 +1020,10 @@ public void testNonMillisecondsBasedUnitCalendarRoundingSize() { long thirdQuarter = prepared.round(time("2015-07-01T00:00:00.000Z")); assertThat(prepared.roundingSize(thirdQuarter, Rounding.DateTimeUnit.DAY_OF_MONTH), closeTo(92.0, 0.000001)); assertThat(prepared.roundingSize(thirdQuarter, Rounding.DateTimeUnit.HOUR_OF_DAY), closeTo(2208.0, 0.000001)); + IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, + () -> prepared.roundingSize(null, Rounding.DateTimeUnit.SECOND_OF_MINUTE)); + assertThat(ex.getMessage(), equalTo("Cannot use non month-based rate unit [second] with month-based calendar interval histogram " + + "[quarter] only month, quarter and year are supported for this histogram")); } public void testFixedRoundingPoints() { @@ -1139,6 +1146,10 @@ private static long time(String time) { return time(time, ZoneOffset.UTC); } + private static Long timeOrNull(String time) { + return randomBoolean() ? time(time, ZoneOffset.UTC) : null; + } + private static long time(String time, ZoneId zone) { TemporalAccessor accessor = DateFormatter.forPattern("date_optional_time").withZone(zone).parse(time); return DateFormatters.from(accessor).toInstant().toEpochMilli(); diff --git a/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/rate/AbstractRateAggregator.java b/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/rate/AbstractRateAggregator.java index 1f4e673872e54..9f63e20d00be6 100644 --- a/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/rate/AbstractRateAggregator.java +++ b/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/rate/AbstractRateAggregator.java @@ -80,7 +80,7 @@ public double metric(long owningBucketOrd) { if (sizedBucketAggregator == null || valuesSource == null || owningBucketOrd >= sums.size()) { return 0.0; } - return sums.get(owningBucketOrd) / sizedBucketAggregator.bucketSize(owningBucketOrd, rateUnit); + return sums.get(owningBucketOrd) / divisor(owningBucketOrd); } @Override @@ -88,7 +88,15 @@ public InternalAggregation buildAggregation(long bucket) { if (valuesSource == null || bucket >= sums.size()) { return buildEmptyAggregation(); } - return new InternalRate(name, sums.get(bucket), sizedBucketAggregator.bucketSize(bucket, rateUnit), format, metadata()); + return new InternalRate(name, sums.get(bucket), divisor(bucket), format, metadata()); + } + + private double divisor(long owningBucketOrd) { + if (sizedBucketAggregator == parent) { + return sizedBucketAggregator.bucketSize(owningBucketOrd, rateUnit); + } else { + return sizedBucketAggregator.bucketSize(rateUnit); + } } @Override diff --git a/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/rate/RateAggregatorTests.java b/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/rate/RateAggregatorTests.java index fd3ff11b5bef0..8682724db0854 100644 --- a/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/rate/RateAggregatorTests.java +++ b/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/rate/RateAggregatorTests.java @@ -377,6 +377,77 @@ public void testKeywordSandwich() throws IOException { }, dateType, numType, keywordType); } + public void testUnsupportedKeywordSandwich() throws IOException { + String rate; + String histogram; + boolean millisecondBasedRate = randomBoolean(); + if (millisecondBasedRate) { + rate = randomFrom("second", "minute", "day", "week"); + histogram = randomFrom("month", "quarter", "year"); + } else { + rate = randomFrom("month", "quarter", "year"); + histogram = randomFrom("second", "minute", "day", "week"); + } + + MappedFieldType numType = new NumberFieldMapper.NumberFieldType("val", NumberFieldMapper.NumberType.INTEGER); + MappedFieldType dateType = dateFieldType(DATE_FIELD); + MappedFieldType keywordType = new KeywordFieldMapper.KeywordFieldType("term"); + RateAggregationBuilder rateAggregationBuilder = new RateAggregationBuilder("my_rate").rateUnit(rate).field("val"); + if (randomBoolean()) { + rateAggregationBuilder.rateMode("sum"); + } + TermsAggregationBuilder termsAggregationBuilder = new TermsAggregationBuilder("my_term").field("term") + .subAggregation(rateAggregationBuilder); + DateHistogramAggregationBuilder dateHistogramAggregationBuilder = new DateHistogramAggregationBuilder("my_date").field(DATE_FIELD) + .calendarInterval(new DateHistogramInterval(histogram)) + .subAggregation(termsAggregationBuilder); + + IllegalArgumentException ex = expectThrows( + IllegalArgumentException.class, () -> testCase(dateHistogramAggregationBuilder, new MatchAllDocsQuery(), iw -> { + iw.addDocument( + doc( + "2010-03-11T01:07:45", + new NumericDocValuesField("val", 1), + new IntPoint("val", 1), + new SortedSetDocValuesField("term", new BytesRef("a")) + ) + ); + iw.addDocument( + doc( + "2010-03-12T01:07:45", + new NumericDocValuesField("val", 2), + new IntPoint("val", 2), + new SortedSetDocValuesField("term", new BytesRef("a")) + ) + ); + iw.addDocument( + doc( + "2010-04-01T03:43:34", + new NumericDocValuesField("val", 3), + new IntPoint("val", 3), + new SortedSetDocValuesField("term", new BytesRef("a")) + ) + ); + iw.addDocument( + doc( + "2010-04-27T03:43:34", + new NumericDocValuesField("val", 4), + new IntPoint("val", 4), + new SortedSetDocValuesField("term", new BytesRef("b")) + ) + ); + }, (Consumer) dh -> { + fail("Shouldn't be here"); + }, dateType, numType, keywordType)); + if (millisecondBasedRate) { + assertEquals("Cannot use non month-based rate unit [" + rate + "] with month-based calendar interval histogram [" + + histogram + "] only month, quarter and year are supported for this histogram", ex.getMessage()); + } else { + assertEquals("Cannot use month-based rate unit [" + rate + "] with non-month based calendar interval histogram [" + + histogram + "] only week, day, hour, minute and second are supported for this histogram", ex.getMessage()); + } + } + public void testKeywordSandwichWithSorting() throws IOException { MappedFieldType numType = new NumberFieldMapper.NumberFieldType("val", NumberFieldMapper.NumberType.INTEGER); MappedFieldType dateType = dateFieldType(DATE_FIELD); From fcd0356fbfbfd3cda8c44ffbe64ace843132d3f3 Mon Sep 17 00:00:00 2001 From: Igor Motov Date: Tue, 24 Nov 2020 15:55:09 -0500 Subject: [PATCH 2/2] Address review comments --- .../org/elasticsearch/common/Rounding.java | 72 ++++++++++++++----- .../histogram/DateHistogramAggregator.java | 4 +- .../elasticsearch/common/RoundingTests.java | 56 ++++++++++----- .../analytics/rate/RateAggregatorTests.java | 2 +- 4 files changed, 96 insertions(+), 38 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/Rounding.java b/server/src/main/java/org/elasticsearch/common/Rounding.java index 9d94c8fc0c657..83aa349732e55 100644 --- a/server/src/main/java/org/elasticsearch/common/Rounding.java +++ b/server/src/main/java/org/elasticsearch/common/Rounding.java @@ -290,7 +290,11 @@ public interface Prepared { * Given the rounded value, returns the size between this value and the * next rounded value in specified units if possible. */ - double roundingSize(Long utcMillis, DateTimeUnit timeUnit); + double roundingSize(long utcMillis, DateTimeUnit timeUnit); + /** + * Returns the size of each rounding bucket in timeUnits. + */ + double roundingSize(DateTimeUnit timeUnit); /** * If this rounding mechanism precalculates rounding points then * this array stores dates such that each date between each entry. @@ -609,22 +613,41 @@ public String toString() { private abstract class TimeUnitPreparedRounding extends PreparedRounding { @Override - public double roundingSize(Long utcMillis, DateTimeUnit timeUnit) { - if (timeUnit.isMillisBased == unit.isMillisBased) { - return (double) unit.ratio / timeUnit.ratio; + public double roundingSize(long utcMillis, DateTimeUnit timeUnit) { + if (unit.isMillisBased) { + if (timeUnit.isMillisBased) { + return (double) unit.ratio / timeUnit.ratio; + } else { + throw new IllegalArgumentException("Cannot use month-based rate unit [" + timeUnit.shortName + + "] with non-month based calendar interval histogram [" + unit.shortName + + "] only week, day, hour, minute and second are supported for this histogram"); + } } else { - if (unit.isMillisBased == false && utcMillis != null) { + if (timeUnit.isMillisBased) { return (double) (nextRoundingValue(utcMillis) - utcMillis) / timeUnit.ratio; } else { - if (timeUnit.isMillisBased == false) { - throw new IllegalArgumentException("Cannot use month-based rate unit [" + timeUnit.shortName + - "] with non-month based calendar interval histogram [" + unit.shortName + - "] only week, day, hour, minute and second are supported for this histogram"); - } else { - throw new IllegalArgumentException("Cannot use non month-based rate unit [" + timeUnit.shortName + - "] with month-based calendar interval histogram [" + unit.shortName + - "] only month, quarter and year are supported for this histogram"); - } + return (double) unit.ratio / timeUnit.ratio; + } + } + } + + @Override + public double roundingSize(DateTimeUnit timeUnit) { + if (unit.isMillisBased) { + if (timeUnit.isMillisBased) { + return (double) unit.ratio / timeUnit.ratio; + } else { + throw new IllegalArgumentException("Cannot use month-based rate unit [" + timeUnit.shortName + + "] with non-month based calendar interval histogram [" + unit.shortName + + "] only week, day, hour, minute and second are supported for this histogram"); + } + } else { + if (timeUnit.isMillisBased) { + throw new IllegalArgumentException("Cannot use non month-based rate unit [" + timeUnit.shortName + + "] with calendar interval histogram [" + unit.shortName + + "] only month, quarter and year are supported for this histogram"); + } else { + return (double) unit.ratio / timeUnit.ratio; } } } @@ -1001,7 +1024,12 @@ private long roundKey(long value, long interval) { private abstract class TimeIntervalPreparedRounding extends PreparedRounding { @Override - public double roundingSize(Long utcMillis, DateTimeUnit timeUnit) { + public double roundingSize(long utcMillis, DateTimeUnit timeUnit) { + return roundingSize(timeUnit); + } + + @Override + public double roundingSize(DateTimeUnit timeUnit) { if (timeUnit.isMillisBased) { return (double) interval / timeUnit.ratio; } else { @@ -1268,10 +1296,15 @@ public long nextRoundingValue(long utcMillis) { } @Override - public double roundingSize(Long utcMillis, DateTimeUnit timeUnit) { + public double roundingSize(long utcMillis, DateTimeUnit timeUnit) { return delegatePrepared.roundingSize(utcMillis, timeUnit); } + @Override + public double roundingSize(DateTimeUnit timeUnit) { + return delegatePrepared.roundingSize(timeUnit); + } + @Override public long[] fixedRoundingPoints() { // TODO we can likely translate here @@ -1356,10 +1389,15 @@ public long nextRoundingValue(long utcMillis) { } @Override - public double roundingSize(Long utcMillis, DateTimeUnit timeUnit) { + public double roundingSize(long utcMillis, DateTimeUnit timeUnit) { return delegate.roundingSize(utcMillis, timeUnit); } + @Override + public double roundingSize(DateTimeUnit timeUnit) { + return delegate.roundingSize(timeUnit); + } + @Override public long[] fixedRoundingPoints() { return Arrays.copyOf(values, max); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramAggregator.java index 8132d195623c1..dec0d13c21573 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramAggregator.java @@ -347,7 +347,7 @@ public double bucketSize(long bucket, Rounding.DateTimeUnit unitSize) { @Override public double bucketSize(Rounding.DateTimeUnit unitSize) { if (unitSize != null) { - return preparedRounding.roundingSize(null, unitSize); + return preparedRounding.roundingSize(unitSize); } else { return 1.0; } @@ -444,7 +444,7 @@ public double bucketSize(long bucket, DateTimeUnit unitSize) { @Override public double bucketSize(DateTimeUnit unitSize) { if (unitSize != null) { - return preparedRounding.roundingSize(null, unitSize); + return preparedRounding.roundingSize(unitSize); } else { return 1.0; } diff --git a/server/src/test/java/org/elasticsearch/common/RoundingTests.java b/server/src/test/java/org/elasticsearch/common/RoundingTests.java index ebda1dc17f7a4..c43ebfb8128cb 100644 --- a/server/src/test/java/org/elasticsearch/common/RoundingTests.java +++ b/server/src/test/java/org/elasticsearch/common/RoundingTests.java @@ -979,26 +979,48 @@ public void testFixedIntervalRoundingSize() { public void testMillisecondsBasedUnitCalendarRoundingSize() { Rounding unitRounding = Rounding.builder(Rounding.DateTimeUnit.HOUR_OF_DAY).build(); Rounding.Prepared prepared = unitRounding.prepare(time("2010-01-01T00:00:00.000Z"), time("2020-01-01T00:00:00.000Z")); - assertThat(prepared.roundingSize(timeOrNull("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.SECOND_OF_MINUTE), + assertThat(prepared.roundingSize(time("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.SECOND_OF_MINUTE), + closeTo(3600.0, 0.000001)); + assertThat(prepared.roundingSize(Rounding.DateTimeUnit.SECOND_OF_MINUTE), closeTo(3600.0, 0.000001)); - assertThat(prepared.roundingSize(timeOrNull("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.MINUTES_OF_HOUR), + assertThat(prepared.roundingSize(time("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.MINUTES_OF_HOUR), closeTo(60.0, 0.000001)); - assertThat(prepared.roundingSize(timeOrNull("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.HOUR_OF_DAY), + assertThat(prepared.roundingSize(Rounding.DateTimeUnit.MINUTES_OF_HOUR), + closeTo(60.0, 0.000001)); + assertThat(prepared.roundingSize(time("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.HOUR_OF_DAY), + closeTo(1.0, 0.000001)); + assertThat(prepared.roundingSize(Rounding.DateTimeUnit.HOUR_OF_DAY), closeTo(1.0, 0.000001)); - assertThat(prepared.roundingSize(timeOrNull("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.DAY_OF_MONTH), + assertThat(prepared.roundingSize(time("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.DAY_OF_MONTH), + closeTo(1 / 24.0, 0.000001)); + assertThat(prepared.roundingSize(Rounding.DateTimeUnit.DAY_OF_MONTH), closeTo(1 / 24.0, 0.000001)); - assertThat(prepared.roundingSize(timeOrNull("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.WEEK_OF_WEEKYEAR), + assertThat(prepared.roundingSize(time("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.WEEK_OF_WEEKYEAR), + closeTo(1 / 168.0, 0.000001)); + assertThat(prepared.roundingSize(Rounding.DateTimeUnit.WEEK_OF_WEEKYEAR), closeTo(1 / 168.0, 0.000001)); IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, - () -> prepared.roundingSize(timeOrNull("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.MONTH_OF_YEAR)); + () -> prepared.roundingSize(time("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.MONTH_OF_YEAR)); + assertThat(ex.getMessage(), equalTo("Cannot use month-based rate unit [month] with non-month based calendar interval " + + "histogram [hour] only week, day, hour, minute and second are supported for this histogram")); + ex = expectThrows(IllegalArgumentException.class, + () -> prepared.roundingSize(Rounding.DateTimeUnit.MONTH_OF_YEAR)); assertThat(ex.getMessage(), equalTo("Cannot use month-based rate unit [month] with non-month based calendar interval " + "histogram [hour] only week, day, hour, minute and second are supported for this histogram")); ex = expectThrows(IllegalArgumentException.class, - () -> prepared.roundingSize(timeOrNull("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.QUARTER_OF_YEAR)); + () -> prepared.roundingSize(time("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.QUARTER_OF_YEAR)); + assertThat(ex.getMessage(), equalTo("Cannot use month-based rate unit [quarter] with non-month based calendar interval " + + "histogram [hour] only week, day, hour, minute and second are supported for this histogram")); + ex = expectThrows(IllegalArgumentException.class, + () -> prepared.roundingSize(Rounding.DateTimeUnit.QUARTER_OF_YEAR)); assertThat(ex.getMessage(), equalTo("Cannot use month-based rate unit [quarter] with non-month based calendar interval " + "histogram [hour] only week, day, hour, minute and second are supported for this histogram")); ex = expectThrows(IllegalArgumentException.class, - () -> prepared.roundingSize(timeOrNull("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.YEAR_OF_CENTURY)); + () -> prepared.roundingSize(time("2015-01-01T00:00:00.000Z"), Rounding.DateTimeUnit.YEAR_OF_CENTURY)); + assertThat(ex.getMessage(), equalTo("Cannot use month-based rate unit [year] with non-month based calendar interval " + + "histogram [hour] only week, day, hour, minute and second are supported for this histogram")); + ex = expectThrows(IllegalArgumentException.class, + () -> prepared.roundingSize(Rounding.DateTimeUnit.YEAR_OF_CENTURY)); assertThat(ex.getMessage(), equalTo("Cannot use month-based rate unit [year] with non-month based calendar interval " + "histogram [hour] only week, day, hour, minute and second are supported for this histogram")); } @@ -1007,11 +1029,13 @@ public void testNonMillisecondsBasedUnitCalendarRoundingSize() { Rounding unitRounding = Rounding.builder(Rounding.DateTimeUnit.QUARTER_OF_YEAR).build(); Rounding.Prepared prepared = unitRounding.prepare(time("2010-01-01T00:00:00.000Z"), time("2020-01-01T00:00:00.000Z")); long firstQuarter = prepared.round(time("2015-01-01T00:00:00.000Z")); - Long firstQuarterOrNull = randomBoolean() ? firstQuarter : null; // Ratio based - assertThat(prepared.roundingSize(firstQuarterOrNull, Rounding.DateTimeUnit.MONTH_OF_YEAR), closeTo(3.0, 0.000001)); - assertThat(prepared.roundingSize(firstQuarterOrNull, Rounding.DateTimeUnit.QUARTER_OF_YEAR), closeTo(1.0, 0.000001)); - assertThat(prepared.roundingSize(firstQuarterOrNull, Rounding.DateTimeUnit.YEAR_OF_CENTURY), closeTo(0.25, 0.000001)); + assertThat(prepared.roundingSize(firstQuarter, Rounding.DateTimeUnit.MONTH_OF_YEAR), closeTo(3.0, 0.000001)); + assertThat(prepared.roundingSize(firstQuarter, Rounding.DateTimeUnit.QUARTER_OF_YEAR), closeTo(1.0, 0.000001)); + assertThat(prepared.roundingSize(firstQuarter, Rounding.DateTimeUnit.YEAR_OF_CENTURY), closeTo(0.25, 0.000001)); + assertThat(prepared.roundingSize(Rounding.DateTimeUnit.MONTH_OF_YEAR), closeTo(3.0, 0.000001)); + assertThat(prepared.roundingSize(Rounding.DateTimeUnit.QUARTER_OF_YEAR), closeTo(1.0, 0.000001)); + assertThat(prepared.roundingSize(Rounding.DateTimeUnit.YEAR_OF_CENTURY), closeTo(0.25, 0.000001)); // Real interval based assertThat(prepared.roundingSize(firstQuarter, Rounding.DateTimeUnit.SECOND_OF_MINUTE), closeTo(7776000.0, 0.000001)); assertThat(prepared.roundingSize(firstQuarter, Rounding.DateTimeUnit.MINUTES_OF_HOUR), closeTo(129600.0, 0.000001)); @@ -1021,8 +1045,8 @@ public void testNonMillisecondsBasedUnitCalendarRoundingSize() { assertThat(prepared.roundingSize(thirdQuarter, Rounding.DateTimeUnit.DAY_OF_MONTH), closeTo(92.0, 0.000001)); assertThat(prepared.roundingSize(thirdQuarter, Rounding.DateTimeUnit.HOUR_OF_DAY), closeTo(2208.0, 0.000001)); IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, - () -> prepared.roundingSize(null, Rounding.DateTimeUnit.SECOND_OF_MINUTE)); - assertThat(ex.getMessage(), equalTo("Cannot use non month-based rate unit [second] with month-based calendar interval histogram " + + () -> prepared.roundingSize(Rounding.DateTimeUnit.SECOND_OF_MINUTE)); + assertThat(ex.getMessage(), equalTo("Cannot use non month-based rate unit [second] with calendar interval histogram " + "[quarter] only month, quarter and year are supported for this histogram")); } @@ -1146,10 +1170,6 @@ private static long time(String time) { return time(time, ZoneOffset.UTC); } - private static Long timeOrNull(String time) { - return randomBoolean() ? time(time, ZoneOffset.UTC) : null; - } - private static long time(String time, ZoneId zone) { TemporalAccessor accessor = DateFormatter.forPattern("date_optional_time").withZone(zone).parse(time); return DateFormatters.from(accessor).toInstant().toEpochMilli(); diff --git a/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/rate/RateAggregatorTests.java b/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/rate/RateAggregatorTests.java index 8682724db0854..42676ddad870c 100644 --- a/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/rate/RateAggregatorTests.java +++ b/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/rate/RateAggregatorTests.java @@ -440,7 +440,7 @@ public void testUnsupportedKeywordSandwich() throws IOException { fail("Shouldn't be here"); }, dateType, numType, keywordType)); if (millisecondBasedRate) { - assertEquals("Cannot use non month-based rate unit [" + rate + "] with month-based calendar interval histogram [" + + assertEquals("Cannot use non month-based rate unit [" + rate + "] with calendar interval histogram [" + histogram + "] only month, quarter and year are supported for this histogram", ex.getMessage()); } else { assertEquals("Cannot use month-based rate unit [" + rate + "] with non-month based calendar interval histogram [" +