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
cbuescher
added
review
>enhancement
v2.0.0-beta1
:Analytics/Aggregations
| ==== 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.
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.
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.
Since it is the same in camel and underscore case, you can only check it once?
|
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 |
clintongormley
changed the title
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