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 back support for ip range aggregations. #17859

Merged
merged 1 commit into from
May 13, 2016

Conversation

jpountz
Copy link
Contributor

@jpountz jpountz commented Apr 19, 2016

This commit adds support for range aggregations on ip fields. However it will
only work on 5.x indices.

Closes #17700

@jpountz
Copy link
Contributor Author

jpountz commented Apr 20, 2016

it will only work on 5.x indices.

In a follow-up PR, we could make it work on legacy indices too if we think it's worth doing. But I think this PR is a good start.

@clintongormley
Copy link

In a follow-up PR, we could make it work on legacy indices too if we think it's worth doing. But I think this PR is a good start.

I think it would be worth doing.

Range(String key, String mask) {
String[] splits = mask.split("/");
if (splits.length != 2) {
throw new IllegalArgumentException("Expected [ip/prefix_length] but got [" + mask + "], which contains more than one [/]");
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this message could be improved as it looks this exception would be thrown if the mask contains no / characters as well?

@colings86
Copy link
Contributor

I left a few minor comments but otherwise LGTM

This commit adds support for range aggregations on `ip` fields. However it will
only work on 5.x indices.

Closes elastic#17700
@jpountz jpountz force-pushed the enhancement/binary_range_agg branch from 5055d09 to 638da06 Compare May 13, 2016 15:22
@jpountz jpountz merged commit 638da06 into elastic:master May 13, 2016
@jpountz jpountz deleted the enhancement/binary_range_agg branch May 13, 2016 15:23
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.

3 participants