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 filter aggregations #64

Merged
merged 5 commits into from
Oct 27, 2016
Merged

Conversation

johannes-scharlach
Copy link
Collaborator

I also had the need for a filter aggregation, but chose a more generic approach than #42.

I'm not sure if it's valid to choose any filter that bodybuilder (or in particular the new FilterBuilder) generates, but in particular the terms filter works quite well like this.

A signature like function filterAggregation(filterType, filterField, filterValue, aggregationName) seemed to simple, since it would essentially reduce the API to one particular filter.

Let me know what you think about this approach!

@danpaz
Copy link
Owner

danpaz commented Oct 7, 2016

Thanks @johannes-scharlach! At a high level this LGTM and I'll take a closer look in the next week or so with more specific feedback.

Also pinging @arielshad in case if you have any thoughts on this approach.

@johannes-scharlach
Copy link
Collaborator Author

@danpaz I'd love to see this in a near-future version of bodybuilder, so that I don't need to maintain my own fork. Would you like me to rebase on top of the latest changes?

@danpaz
Copy link
Owner

danpaz commented Oct 24, 2016

I don't think a rebase is necessary, thanks. Can you add a section to the readme showing how to make a filter aggregation? Can you also add a test to test/index.js as a sort of integration test showing that the feature works as you expected? Maybe that test and the readme can show how you might create this query from the docs: https://www.elastic.co/guide/en/elasticsearch/reference/2.4/search-aggregations-bucket-filter-aggregation.html#search-aggregations-bucket-filter-aggregation

@johannes-scharlach
Copy link
Collaborator Author

The test is slightly different, but the readme looks like the ES documentation :) Let me know if I should change the test also or you expected something different.

@danpaz
Copy link
Owner

danpaz commented Oct 27, 2016

This is great! I think we can move forward with this while we continue to discuss more structural changes separately in #71 (and chances are a more drastic change would have to be a semver major bump anyway).

@danpaz danpaz merged commit a59a193 into danpaz:master Oct 27, 2016
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