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.

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

Fix "key_as_string" for date histogram and epoch_millis/epoch_second format with time zone #19043

Merged
merged 1 commit into from Jun 29, 2016

Conversation

Projects
None yet
4 participants
@cbuescher
Member

cbuescher commented Jun 23, 2016

When doing a date_histogram aggregation with "format":"epoch_millis" or "format" : "epoch_second" and using a time zone other than UTC, the key_as_string ouput in the response does not reflect the UTC timestamp that is used as the key. This happens because when applying the time_zone in DocValueFormat.DateTime to an epoch-based formatter, this adds the time zone offset to the value being formated. Instead we should adjust the added display offset to get back the utc instance in EpochTimePrinter.

Closes #19038

@jpountz

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/search/DocValueFormat.java
@cbuescher

This comment has been minimized.

Show comment
Hide comment
@cbuescher

cbuescher Jun 28, 2016

Member

@jpountz sorry for the confusion about EpochTimePrinter, this is actually part of our codebase and we can correct the time zone adjustment directly there. I'm not completely sure if epoch_millis format is supposed to always output the date instance in UTC, maybe @spinscale can conform this since it look like he was the one that introduced the printer class.

Member

cbuescher commented Jun 28, 2016

@jpountz sorry for the confusion about EpochTimePrinter, this is actually part of our codebase and we can correct the time zone adjustment directly there. I'm not completely sure if epoch_millis format is supposed to always output the date instance in UTC, maybe @spinscale can conform this since it look like he was the one that introduced the printer class.

@cbuescher cbuescher added the :Dates label Jun 28, 2016

@spinscale

This comment has been minimized.

Show comment
Hide comment
@spinscale

spinscale Jun 28, 2016

Member

yes, epoch time is considered the seconds elapsed since 1970 in UTC timezone.

Member

spinscale commented Jun 28, 2016

yes, epoch time is considered the seconds elapsed since 1970 in UTC timezone.

@jpountz

This comment has been minimized.

Show comment
Hide comment
@jpountz

jpountz Jun 28, 2016

Contributor

+1 to always UTC

Contributor

jpountz commented Jun 28, 2016

+1 to always UTC

@@ -378,18 +377,18 @@ public int estimatePrintedLength() {
@Override
public void printTo(StringBuffer buf, long instant, Chronology chrono, int displayOffset, DateTimeZone displayZone, Locale locale) {
if (hasMilliSecondPrecision) {
buf.append(instant);
buf.append(instant - displayOffset);

This comment has been minimized.

@jpountz

jpountz Jun 28, 2016

Contributor

maybe leave a comment where this comes from?

@jpountz

jpountz Jun 28, 2016

Contributor

maybe leave a comment where this comes from?

@jpountz

This comment has been minimized.

Show comment
Hide comment
@jpountz

jpountz Jun 28, 2016

Contributor

LGTM

Contributor

jpountz commented Jun 28, 2016

LGTM

@cbuescher cbuescher changed the title from Fix "key_as_string" for date histogram and epoch_millis/epoch_second forrmat with time zone to Fix "key_as_string" for date histogram and epoch_millis/epoch_second format with time zone Jun 28, 2016

Fix key_as_string for date histogram and epoch_millis/epoch_second fo…
…rmat

When doing a `date_histogram` aggregation with `"format":"epoch_millis"` or
`"format" : "epoch_second"` and using a time zone other than UTC, the
`key_as_string` ouput in the response does not reflect the UTC timestamp that is
used as the key. This happens because when applying the `time_zone` in
DocValueFormat.DateTime to an epoch-based formatter, this adds the time zone
offset to the value being formated. Instead we should adjust the added display
offset to get back the utc instance in EpochTimePrinter.

Closes #19038

@cbuescher cbuescher merged commit 0d81dee into elastic:master Jun 29, 2016

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@cbuescher cbuescher removed the v2.3.4 label Jun 29, 2016

@cbuescher

This comment has been minimized.

Show comment
Hide comment
@cbuescher

cbuescher Jun 29, 2016

Member

Merged to 2.4 with 5301ee3

Member

cbuescher commented Jun 29, 2016

Merged to 2.4 with 5301ee3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment