Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

cbuescher
Copy link
Member

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

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.
Copy link
Contributor

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?)

Copy link
Member Author

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)) {
Copy link
Contributor

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, my mistake.

@jpountz
Copy link
Contributor

jpountz commented Jan 30, 2015

Some minor comments but otherwise looks great!

@cbuescher
Copy link
Member Author

Corrected the two lines from your comments.
Also I was thinking if it would be clearer instead of using PrePostRounding(rounding, -offset, offset) to introduce a new OffsetRounding to hide the whole shifting values around part, but I don't know if this will be used very often. Since PrePostRounding is used in TimeZoneRounding it will stay there anyway.

@jpountz
Copy link
Contributor

jpountz commented Jan 30, 2015

LGTM

Also I was thinking if it would be clearer instead of using PrePostRounding(rounding, -offset, offset) to introduce a new OffsetRounding to hide the whole shifting values around part, but I don't know if this will be used very often. Since PrePostRounding is used in TimeZoneRounding it will stay there anyway.

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?

@cbuescher
Copy link
Member Author

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.

@jpountz
Copy link
Contributor

jpountz commented Jan 30, 2015

@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

@clintongormley
Copy link
Contributor

I'm +1 on remove pre/post_offset on date histograms. Deprecate in 1.5 and remove in master. They're horribly confusing

@cbuescher
Copy link
Member Author

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.
@jpountz So you're okay if I push this to master? Do I have to do anything else in terms of labels (breaking because it removes existing pre/post option?) Also how would I deprecate option in 1.5, any special place for that (docs etc.)

@jpountz
Copy link
Contributor

jpountz commented Feb 2, 2015

@jpountz So you're okay if I push this to master? Do I have to do anything else in terms of labels (breaking because it removes existing pre/post option?) Also how would I deprecate option in 1.5, any special place for that (docs etc.)

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 date_histogram).

@cbuescher cbuescher closed this in 44193e7 Feb 2, 2015
@clintongormley clintongormley changed the title Aggregations: Add 'offset' option to histogram aggregation Add offset option to histogram aggregation Jun 7, 2015
@cbuescher cbuescher deleted the fix/9417 branch March 20, 2024 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Aggs: Add offset support to histograms
3 participants