-
Notifications
You must be signed in to change notification settings - Fork 25.2k
[Aggregations] Added _meta construct to inject metadata #6465
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
Conversation
This PR adds the abillity to associate a bit of state with each individual aggregation. Usecases: - Typed clients can inject some state so that the deserialization process has enough information to deserialize the aggregations into the correct type. Facets return `_type` to signal what kind of facet the response corresponds with. - UI state, since aggregations can be nested at arbitraty depths injecting pieces of UI state can greatly simplify programmming with aggregations since there are no two tree datastructures to walk anymore only the aggregation data.
…earch#6465 is good enough to be merged starting from Elasticsearch 1.3 we can use _meta to greatly simplify parsing
"terms": { | ||
"field": "title" | ||
}, | ||
"_meta": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's get rid of the underscore... we moved away from that (no real need for it as all the keys under the agg are controlled by us)
Thanks for review @uboness ! I wrapped the I opted for Will update both property names to |
why would it break? it's under the agg name, isn't it? in any case... still... not good enough reason to introduce inconsistency in the api. That is, if we feel that something will break apis, better break them and target for a later release (one that enables breaking changes) rather than introduce inconsistency in the api (something we'll need to cary later for a long time) |
…structor of Aggregations no more buildInternals*
@uboness I've updated the PR with all your feedback I only have one question regarding the AggregationBuilder.java changes. |
builder.field(CommonFields.META); | ||
builder.copyCurrentStructure(parser); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the way that we usually do this is by making toXContent final, write the name and metadata, and delegate to a doXContentBody
method. This ensures that no aggregation can by-pass the serialization of the metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @colings86
@Mpdreamz where are you on this one? |
closing in favour of #8279 |
This PR adds the ability to associate a bit of state with each individual aggregation.
Use cases:
process has enough information to deserialize the aggregations into
the correct type. Facets return
_type
to signal what kind of facet theresponse corresponds with.
injecting pieces of UI state can greatly simplify programming with
aggregations since there are no two tree data-structures to walk anymore only the aggregation data.
thank you @martijnvg for the help reading the
byte[]
representation of _meta properlygeo_bounds
aggregation to wrap the result in a"aggregationname" : {}
Here's a gist of a sense session you can use to review this branch
Example request:
Example response: