Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add interval response parameter to AutoDateInterval histogram #33254

Merged
merged 6 commits into from Sep 5, 2018
Merged

Add interval response parameter to AutoDateInterval histogram #33254

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
21154a2
add interval method to test and class
Aug 29, 2018
afaf927
expose interval to HTTP response
Aug 29, 2018
edd9e6b
update docs to match new behavior
Aug 29, 2018
1fecf53
xcontent parse fixes
Aug 30, 2018
f9904d1
expose interval used instead of re-calculating
Sep 4, 2018
cf918ea
Merge branch 'master' into add-interval-to-autointerval-histogram
Sep 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 6 additions & 3 deletions docs/reference/aggregations/bucket/autodatehistogram-aggregation.asciidoc
View file
Open in desktop
Expand Up @@ -81,7 +81,8 @@ Response:
"key": 1425168000000,
"doc_count": 2
}
]
],
"interval": "1M"
}
}
}
Expand Down Expand Up @@ -174,7 +175,8 @@ starting at midnight UTC on 1 October 2015:
"key": 1443664800000,
"doc_count": 1
}
]
],
"interval": "1h"
}
}
}
Expand Down Expand Up @@ -229,7 +231,8 @@ the specified time zone.
"key": 1443664800000,
"doc_count": 1
}
]
],
"interval": "1h"
}
}
}
Expand Down
18 changes: 11 additions & 7 deletions ...asticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregationBuilder.java
View file
Open in desktop
Expand Up @@ -73,17 +73,17 @@ public class AutoDateHistogramAggregationBuilder
static RoundingInfo[] buildRoundings(DateTimeZone timeZone) {
RoundingInfo[] roundings = new RoundingInfo[6];
roundings[0] = new RoundingInfo(createRounding(DateTimeUnit.SECOND_OF_MINUTE, timeZone),
1000L, 1, 5, 10, 30);
1000L, "s" , 1, 5, 10, 30);
roundings[1] = new RoundingInfo(createRounding(DateTimeUnit.MINUTES_OF_HOUR, timeZone),
60 * 1000L, 1, 5, 10, 30);
60 * 1000L, "m", 1, 5, 10, 30);
roundings[2] = new RoundingInfo(createRounding(DateTimeUnit.HOUR_OF_DAY, timeZone),
60 * 60 * 1000L, 1, 3, 12);
60 * 60 * 1000L, "h", 1, 3, 12);
roundings[3] = new RoundingInfo(createRounding(DateTimeUnit.DAY_OF_MONTH, timeZone),
24 * 60 * 60 * 1000L, 1, 7);
24 * 60 * 60 * 1000L, "d", 1, 7);
roundings[4] = new RoundingInfo(createRounding(DateTimeUnit.MONTH_OF_YEAR, timeZone),
30 * 24 * 60 * 60 * 1000L, 1, 3);
30 * 24 * 60 * 60 * 1000L, "M", 1, 3);
roundings[5] = new RoundingInfo(createRounding(DateTimeUnit.YEAR_OF_CENTURY, timeZone),
365 * 24 * 60 * 60 * 1000L, 1, 5, 10, 20, 50, 100);
365 * 24 * 60 * 60 * 1000L, "y", 1, 5, 10, 20, 50, 100);
return roundings;
}

Expand Down Expand Up @@ -186,24 +186,28 @@ public static class RoundingInfo implements Writeable {
final Rounding rounding;
final int[] innerIntervals;
final long roughEstimateDurationMillis;
final String unitAbbreviation;

public RoundingInfo(Rounding rounding, long roughEstimateDurationMillis, int... innerIntervals) {
public RoundingInfo(Rounding rounding, long roughEstimateDurationMillis, String unitAbbreviation, int... innerIntervals) {
this.rounding = rounding;
this.roughEstimateDurationMillis = roughEstimateDurationMillis;
this.unitAbbreviation = unitAbbreviation;
this.innerIntervals = innerIntervals;
}

public RoundingInfo(StreamInput in) throws IOException {
rounding = Rounding.Streams.read(in);
roughEstimateDurationMillis = in.readVLong();
innerIntervals = in.readIntArray();
unitAbbreviation = in.readString();
}

@Override
public void writeTo(StreamOutput out) throws IOException {
Rounding.Streams.write(rounding, out);
out.writeVLong(roughEstimateDurationMillis);
out.writeIntArray(innerIntervals);
out.writeString(unitAbbreviation);
}

public int getMaximumInnerInterval() {
Expand Down
22 changes: 18 additions & 4 deletions ...ava/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogram.java
View file
Open in desktop
Expand Up @@ -209,9 +209,8 @@ public int hashCode() {
private final BucketInfo bucketInfo;
private final int targetBuckets;


InternalAutoDateHistogram(String name, List<Bucket> buckets, int targetBuckets, BucketInfo emptyBucketInfo, DocValueFormat formatter,
List<PipelineAggregator> pipelineAggregators, Map<String, Object> metaData) {
List<PipelineAggregator> pipelineAggregators, Map<String, Object> metaData) {
super(name, pipelineAggregators, metaData);
this.buckets = buckets;
this.bucketInfo = emptyBucketInfo;
Expand All @@ -238,6 +237,22 @@ protected void doWriteTo(StreamOutput out) throws IOException {
out.writeVInt(targetBuckets);
}

public DateHistogramInterval getInterval() {
RoundingInfo roundingInfo = this.bucketInfo.roundingInfos[this.bucketInfo.roundingIdx];
String unitAbbreviation = roundingInfo.unitAbbreviation;
if (buckets.size() <= 1) {
int innerInterval = roundingInfo.innerIntervals[0];
return new DateHistogramInterval(Integer.toString(innerInterval) + unitAbbreviation);
}
long intervalInMillis = buckets.get(1).key - buckets.get(0).key;
for (int interval : roundingInfo.innerIntervals) {
if (roundingInfo.getRoughEstimateDurationMillis() * interval == intervalInMillis) {
return new DateHistogramInterval(Integer.toString(interval) + unitAbbreviation);
}
}
return new DateHistogramInterval(Integer.toString(roundingInfo.innerIntervals[0]) + unitAbbreviation);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we should calculate the interval here as we could end up with bugs where this is different from the actual interval used. Instead could we have a field for the interval store in this class which is populated int he AutoDateHistogramAggregator.buildAggregation() and also in doReduce() when we build the new instance following a reduce? I think we will have all the information we need in both those places?


@Override
public String getWriteableName() {
return AutoDateHistogramAggregationBuilder.NAME;
Expand Down Expand Up @@ -279,7 +294,6 @@ private static class IteratorAndCurrent {
this.iterator = iterator;
current = iterator.next();
}

}

/**
Expand Down Expand Up @@ -408,7 +422,6 @@ private static class BucketReduceResult {
this.buckets = buckets;
this.roundingInfo = roundingInfo;
this.roundingIdx = roundingIdx;

}
}

Expand Down Expand Up @@ -557,6 +570,7 @@ public XContentBuilder doXContentBody(XContentBuilder builder, Params params) th
bucket.toXContent(builder, params);
}
builder.endArray();
builder.field("interval", getInterval().toString());
return builder;
}

Expand Down
14 changes: 11 additions & 3 deletions ...rg/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogramTests.java
View file
Open in desktop
Expand Up @@ -94,11 +94,11 @@ public void testGetAppropriateRoundingUsesCorrectIntervals() {
// 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);
1000L, "s", 1000);
roundings[1] = new RoundingInfo(createRounding(DateTimeUnit.MINUTES_OF_HOUR, timeZone),
60 * 1000L, 1, 5, 10, 30);
60 * 1000L, "m", 1, 5, 10, 30);
roundings[2] = new RoundingInfo(createRounding(DateTimeUnit.HOUR_OF_DAY, timeZone),
60 * 60 * 1000L, 1, 3, 12);
60 * 60 * 1000L, "h", 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
Expand Down Expand Up @@ -198,6 +198,14 @@ protected void assertReduced(InternalAutoDateHistogram reduced, List<InternalAut
(key, oldValue) -> (oldValue == null ? 0 : oldValue) + bucket.getDocCount());
}
assertEquals(expectedCounts, actualCounts);

DateHistogramInterval expectedInterval;
if (reduced.getBuckets().size() == 1) {
expectedInterval = new DateHistogramInterval(roundingInfo.innerIntervals[0]+roundingInfo.unitAbbreviation);
} else {
expectedInterval = new DateHistogramInterval(innerIntervalToUse+roundingInfo.unitAbbreviation);
}
assertThat(reduced.getInterval(), equalTo(expectedInterval));
}

private int getBucketCount(long lowest, long highest, RoundingInfo roundingInfo, long intervalInMillis) {
Expand Down