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

Added pre and post offset to histogram aggregation #6980

Closed
wants to merge 0 commits into from
Closed

Added pre and post offset to histogram aggregation #6980

wants to merge 0 commits into from

Conversation

colings86
Copy link
Contributor

Added preOffset and postOffset parameters to the API for the histogram aggregation which work in the same way as in the date histogram

Closes #6605

@@ -127,6 +127,59 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeVLong(interval);
}
}

public static class PrePostIntervalRounding extends TimeZoneRounding {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks very similar to PrePostTimeZoneRounding. Do you think we can reuse it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I tried doing that and creating a PrePostRounding which extended Rounding, but TimeZoneRounding.Builder expects everything thats returned to be an instance of TimeZoneRounding so would mean changing FactorTimeZoneRounding to accept a Rounding delegate and change the build() method to return Rounding instead of TimeZoneRounding. Not sure if that makes things too messy?

Copy link
Contributor

Choose a reason for hiding this comment

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

TimeZoneRounding.Builder expects everything thats returned to be an instance of TimeZoneRounding

TimeZoneRounding doesn't add anything on top of Rounding so I think it's ok to return/expect Rounding instances here.

@jpountz jpountz removed the review label Jul 24, 2014
@jpountz
Copy link
Contributor

jpountz commented Jul 24, 2014

LGTM

@jpountz jpountz removed the review label Jul 24, 2014
@colings86
Copy link
Contributor Author

pushed to master and 1.x

@colings86 colings86 closed this Jul 24, 2014
@colings86 colings86 self-assigned this Aug 21, 2014
@colings86 colings86 deleted the feature/histogramOffsets branch August 21, 2014 15:11
@clintongormley clintongormley changed the title Aggregations: Added pre and post offset to histogram aggregation Added pre and post offset to histogram aggregation Jun 6, 2015
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.

Aggregations: Histogram Aggregation bounds offsets
3 participants