-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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
Add offset
to date_histogram, replacing pre_offsest
and post_offset
#9597
Conversation
…ffset' and 'post_offset' Add offset option to 'date_histogram' replacing and simplifying the previous 'pre_offset' and 'post_offset' options. This change is part of a larger clean up task for `date_histogram` from issue elastic#9062.
@@ -203,41 +201,29 @@ public byte id() { | |||
|
|||
@Override | |||
public long roundKey(long value) { | |||
return rounding.roundKey(value + preOffset); | |||
return rounding.roundKey(value - offset); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change alone makes offsets much easier to understand!
@jpountz added some minor changes. Should this be marked as 'breaking'? I will put the offset option alongside the old options on the 1.x branch now, will issue separate PR for that. |
} | ||
|
||
@Override | ||
public void readFrom(StreamInput in) throws IOException { | ||
rounding = Rounding.Streams.read(in); | ||
if (in.getVersion().before(Version.V_1_4_0_Beta1)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool you are removing these BWC conditionals!! thanks!! I missed them
LGTM
Yes. I see you already did. :)
Sounds good, thanks! |
Merged with master with d2f852a. |
offset
to date_histogram, replacing pre_offsest
and post_offset
Add offset option to 'date_histogram' replacing and simplifying the previous 'pre_offset' and 'post_offset' options.
This change is part of a larger clean up task for
date_histogram
from issue #9062.