Skip to content

Commit

Permalink
Aggs: Fix rounding issues when using date_histogram and time zones
Browse files Browse the repository at this point in the history 
This fix enhances the internal time zone conversion in the
TimeZoneRounding classes that were the cause of issues with
strange date bucket keys in elastic#9491 and elastic#7673.

Closes elastic#9491
Closes elastic#7673
  • Loading branch information
Christoph Büscher committed Feb 23, 2015
1 parent 2ef8399 commit 59bae3e
Show file tree
Hide file tree
Showing 2 changed files with 60 additions and 19 deletions.
37 changes: 19 additions & 18 deletions src/main/java/org/elasticsearch/common/rounding/TimeZoneRounding.java
View file
Expand Up @@ -156,21 +156,21 @@ public byte id() {

@Override
public long roundKey(long utcMillis) {
long offset = preTz.getOffset(utcMillis);
long time = utcMillis + offset;
return field.roundFloor(time) - offset;
long local = preTz.convertUTCToLocal(utcMillis);
return preTz.convertLocalToUTC(field.roundFloor(local), true, utcMillis);
}

@Override
public long valueForKey(long time) {
// now apply post Tz
time = time + postTz.getOffset(time);
return time;
return postTz.convertUTCToLocal(time);
}

@Override
public long nextRoundingValue(long value) {
return durationField.add(value, 1);
public long nextRoundingValue(long time) {
long currentWithoutPostZone = postTz.convertLocalToUTC(time, true);
long nextWithoutPostZone = durationField.add(currentWithoutPostZone, 1);
return postTz.convertUTCToLocal(nextWithoutPostZone);
}

@Override
Expand Down Expand Up @@ -268,21 +268,22 @@ public byte id() {

@Override
public long roundKey(long utcMillis) {
long time = utcMillis + preTz.getOffset(utcMillis);
return field.roundFloor(time);
long local = preTz.convertUTCToLocal(utcMillis);
return field.roundFloor(local);
}

@Override
public long valueForKey(long time) {
// after rounding, since its day level (and above), its actually UTC!
// now apply post Tz
time = time + postTz.getOffset(time);
return time;
return postTz.convertUTCToLocal(time);
}

@Override
public long nextRoundingValue(long value) {
return durationField.add(value, 1);
public long nextRoundingValue(long currentWithPostZone) {
long currentWithoutPostZone = postTz.convertLocalToUTC(currentWithPostZone, true);
long nextWithoutPostZone = durationField.add(currentWithoutPostZone, 1);
return postTz.convertUTCToLocal(nextWithoutPostZone);
}

@Override
Expand Down Expand Up @@ -375,17 +376,17 @@ public byte id() {

@Override
public long roundKey(long utcMillis) {
long time = utcMillis + preTz.getOffset(utcMillis);
long time = preTz.convertUTCToLocal(utcMillis);
return Rounding.Interval.roundKey(time, interval);
}

@Override
public long valueForKey(long key) {
long time = Rounding.Interval.roundValue(key, interval);
// now, time is still in local, move it to UTC
time = time - preTz.getOffset(time);
time = preTz.convertLocalToUTC(time, true);
// now apply post Tz
time = time + postTz.getOffset(time);
time = postTz.convertUTCToLocal(time);
return time;
}

Expand Down Expand Up @@ -435,7 +436,7 @@ public byte id() {

@Override
public long roundKey(long utcMillis) {
long time = utcMillis + preTz.getOffset(utcMillis);
long time = preTz.convertUTCToLocal(utcMillis);
return Rounding.Interval.roundKey(time, interval);
}

Expand All @@ -444,7 +445,7 @@ public long valueForKey(long key) {
long time = Rounding.Interval.roundValue(key, interval);
// after rounding, since its day level (and above), its actually UTC!
// now apply post Tz
time = time + postTz.getOffset(time);
time = postTz.convertUTCToLocal(time);
return time;
}

Expand Down
42 changes: 41 additions & 1 deletion src/test/java/org/elasticsearch/search/aggregations/bucket/DateHistogramTests.java
View file
Expand Up @@ -44,11 +44,13 @@
import java.util.Arrays;
import java.util.Collection;
import java.util.List;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;

import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery;
import static org.elasticsearch.search.aggregations.AggregationBuilders.*;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchResponse;
import static org.hamcrest.Matchers.*;
import static org.hamcrest.core.IsNull.notNullValue;
Expand Down Expand Up @@ -1204,7 +1206,7 @@ public void singleValueField_WithExtendedBounds() throws Exception {

@Test
public void singleValue_WithMultipleDateFormatsFromMapping() throws Exception {

String mappingJson = jsonBuilder().startObject().startObject("type").startObject("properties").startObject("date").field("type", "date").field("format", "dateOptionalTime||dd-MM-yyyy").endObject().endObject().endObject().endObject().string();
prepareCreate("idx2").addMapping("type", mappingJson).execute().actionGet();
IndexRequestBuilder[] reqs = new IndexRequestBuilder[5];
Expand Down Expand Up @@ -1263,6 +1265,44 @@ public void testIssue6965() {
assertThat(bucket.getDocCount(), equalTo(3l));
}

public void testDSTBoundaryIssue9491() throws InterruptedException, ExecutionException {
assertAcked(client().admin().indices().prepareCreate("test9491").addMapping("type", "d", "type=date").get());
indexRandom(true,
client().prepareIndex("test9491", "type").setSource("d", "2014-10-08T13:00:00Z"),
client().prepareIndex("test9491", "type").setSource("d", "2014-11-08T13:00:00Z"));
ensureSearchable("test9491");
SearchResponse response = client().prepareSearch("test9491")
.addAggregation(dateHistogram("histo").field("d").interval(DateHistogram.Interval.YEAR).preZone("Asia/Jerusalem")
.preZoneAdjustLargeInterval(true))
.execute().actionGet();
assertSearchResponse(response);
Histogram histo = response.getAggregations().get("histo");
assertThat(histo.getBuckets().size(), equalTo(1));
assertThat(histo.getBuckets().get(0).getKey(), equalTo("2013-12-31T22:00:00.000Z"));
assertThat(histo.getBuckets().get(0).getDocCount(), equalTo(2L));
}

public void testIssue7673() throws InterruptedException, ExecutionException {
assertAcked(client().admin().indices().prepareCreate("test7673").addMapping("type", "d", "type=date").get());
indexRandom(true,
client().prepareIndex("test7673", "type").setSource("d", "2013-07-01T00:00:00Z"),
client().prepareIndex("test7673", "type").setSource("d", "2013-09-01T00:00:00Z"));
ensureSearchable("test7673");
SearchResponse response = client().prepareSearch("test7673")
.addAggregation(dateHistogram("histo").field("d").interval(DateHistogram.Interval.MONTH).postZone("-02:00")
.minDocCount(0))
.execute().actionGet();
assertSearchResponse(response);
Histogram histo = response.getAggregations().get("histo");
assertThat(histo.getBuckets().size(), equalTo(3));
assertThat(histo.getBuckets().get(0).getKey(), equalTo("2013-06-30T22:00:00.000Z"));
assertThat(histo.getBuckets().get(0).getDocCount(), equalTo(1L));
assertThat(histo.getBuckets().get(1).getKey(), equalTo("2013-07-31T22:00:00.000Z"));
assertThat(histo.getBuckets().get(1).getDocCount(), equalTo(0L));
assertThat(histo.getBuckets().get(2).getKey(), equalTo("2013-08-31T22:00:00.000Z"));
assertThat(histo.getBuckets().get(2).getDocCount(), equalTo(1L));
}

/**
* see issue #9634, negative interval in date_histogram should raise exception
*/
Expand Down

0 comments on commit 59bae3e

Please sign in to comment.