Skip to content

Conversation

@cbuescher
Copy link
Member

@cbuescher cbuescher commented Apr 27, 2017

This change removes the count() method from the GeoCentroid interface used for the geo_centroid aggregation. It seems to be unused at least in our own codebase and we also don't write the count value to the aggregation Rest response but only use it in the reduce method.
This is the main reason for removing the method from the interface. When adding parsing for the aggregation responses for the
Highl level Rest Client we strive to provide all information available via the REST API to the user through the aggregation interface and it only makes sense to provide values through the interface that are also provided via REST.
The method could be deprecated in 5.x if necessary.

@cbuescher
Copy link
Member Author

cbuescher commented Apr 27, 2017

@colings86 assigning to you first again because you might have an opinion whether count() is still needed in the interface or not. Otherwise please reassign.

@cbuescher
Copy link
Member Author

Also it looks like it's not possible to get the count value using the InternalGeoCentroid#getProperty() method.

@colings86
Copy link
Contributor

I think we should keep count here since its useful to know how many values contributed to the centroid. We could add it to the getProperty() method but I don't see that as being an urgent thing

@cbuescher
Copy link
Member Author

Then we should also add it to the REST response. Do you think we can do that in v6.0? Otherwise we have no way of parsing that value back on the client side.

@colings86
Copy link
Contributor

Yes, I think it would be useful to add it to the REST response in v6.0. /cc @jpountz @polyfractal do you agree?

@polyfractal
Copy link
Contributor

++ adding to response. Could definitely see this being used, e.g. on a chart, size the centroid marker based on number of values in the centroid.

@javanna
Copy link
Member

javanna commented Apr 28, 2017

If we add this field to the REST response, what's keeping us from adding it in 5.x as well? Adding a field should not be considered a breaking change should it?

@cbuescher
Copy link
Member Author

I'll close this in favour of #24387

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.

5 participants