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 min and max aggregators for long and double #84

Merged
merged 2 commits into from
Apr 27, 2017

Conversation

azymnis
Copy link
Contributor

@azymnis azymnis commented Apr 19, 2017

This fixes the existing min and max aggregators. Based on the druid docs these should be doubleMin, doubleMax, longMin, and longMax.

@azymnis
Copy link
Contributor Author

azymnis commented Apr 25, 2017

Anyone to look at this?

Copy link
Member

@gianm gianm left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

The patch generally looks good. I left one comment about potentially making things easier for users upgrading. 👍 from me after that.

Could you please also fill out the CLA at http://druid.io/community/cla.html?

def doublesum(raw_metric):
return {"type": "doubleSum", "fieldName": raw_metric}


def min(raw_metric):
Copy link
Member

Choose a reason for hiding this comment

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

Could you leave the old ones in for compatibility with older python clients? They should turn into doubleMin and doubleMax since the old min/max aggregators acted like the double versions.

Maybe also add a docstring to the old min/max versions saying they're deprecated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I'll do that

@azymnis
Copy link
Contributor Author

azymnis commented Apr 25, 2017

@gianm thanks for the review. I added back the deprecated methods and also signed the CLA

Copy link
Member

@gianm gianm left a comment

Choose a reason for hiding this comment

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

Thanks @azymnis, lgtm!

@gianm gianm merged commit 78bc5c2 into druid-io:master Apr 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants