Skip to content

Conversation

@cbuescher
Copy link
Member

This adds parsing to the InternalFilters aggregation.

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

this keyed thing is a bit of a nightmare, I wonder how users deal with that. Anyways that's beyond the scope of our current effort. LGTM!

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure, but shall we do this eagerly rather than on demand?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't mind, this is the way it is done in InternalFilters currently, thats why I kept it.

Copy link
Member

Choose a reason for hiding this comment

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

mention here too that this is what we do also in the corresponding builder

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 do

Copy link
Member

@tlrx tlrx left a comment

Choose a reason for hiding this comment

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

LGTM

@cbuescher cbuescher removed the v6.0.0 label May 15, 2017
This adds parsing to the InternalFilters aggregation.
@cbuescher cbuescher force-pushed the addParsing-InternalFilters branch from 4cd2190 to 86ed37a Compare May 15, 2017 13:13
@cbuescher cbuescher merged commit 60505c9 into elastic:feature/client_aggs_parsing May 15, 2017
javanna pushed a commit to javanna/elasticsearch that referenced this pull request May 23, 2017
This adds parsing to the InternalFilters aggregation.
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.

3 participants