-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
option to histogram aggregation
#9505
Conversation
Histogram aggragation now supports an 'offset' option to move bucket boundaries for an interval X from 0, X, 2X, 3X,... to by an offset Y to Y, X+Y, 2X+Y, 3X+Y... While this was possible before using the 'pre_offset' and 'post_offset' options, those were not documented and are removed here in favour of the new 'offset' option. Closes elastic#9417
==== Offset | ||
|
||
By default the bucket keys start with 0 and then continue in even spaced steps of `interval`, e.g. if the interval is 10 the first buckets | ||
(assuming there is data inside them) will be [0 - 9), [10-19), [20-29). The bucket boundaries can be shifted by using the `offset` option. |
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.
Do you mean [0 - 9]
or [0 - 10)
instead of [0 - 9)
(I'm assuming '[' means including while '(' means excluding?)
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.
Sure, will correct that.
preOffset = parser.longValue(); | ||
} else if ("post_offset".equals(currentFieldName) || "postOffset".equals(currentFieldName)) { | ||
postOffset = parser.longValue(); | ||
} else if ("offset".equals(currentFieldName) || "offset".equals(currentFieldName)) { |
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.
Since it is the same in camel and underscore case, you can only check it once?
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.
Sure, my mistake.
Some minor comments but otherwise looks great! |
Corrected the two lines from your comments. |
LGTM
Actually I think we should only have an offset parameter in the date histogram too. So if we do the same change in date_histogram then we'll be able to simplify this offset rounding like you suggest? |
I will look into that, but in date histogram the pre/post options are documented, so don't know if anybody uses them. Also they seem to behave in a slightly different way (using date time format). @jpountz you think this should also be part of this PR? I can try to make the changes and you can look over them, if not a good idea I can revert. |
@cbuescher I don't mind having two PRs or doing everything in a single one. FYI there is this other issue open about cleaning up date_histogram which mentions offsets: #9062 |
I'm +1 on remove pre/post_offset on date histograms. Deprecate in 1.5 and remove in master. They're horribly confusing |
I looked at #9062 and I think I would like to do that in a different PR, just because I have to get a bit more familiar with the time zones. Then probably can rename the PrePostRounding so something more suitable if they are not used anywhere else. |
Yes I'm good with pushing to master. In my opinion, there is nothing to worry about in terms of deprecation since this option was never documented (which will be different on the |
offset
option to histogram aggregation
Histogram aggragation now supports an 'offset' option to move bucket boundaries
for an interval X from 0, X, 2X, 3X,... to by an offset Y to Y, X+Y, 2X+Y, 3X+Y...
While this was possible before using the 'pre_offset' and 'post_offset' options, those
were not documented and are removed here in favour of the new 'offset' option.
Closes #9417