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 parsing for InternalGeoCentroid #24371

Conversation

Projects
None yet
2 participants
@cbuescher
Copy link
Member

commented Apr 27, 2017

This adds parsing to the InternalGeoCentroid aggregation. One problem I encountered here is that we don't render the count parameter that is available through the count() method in the GeoCentroid interface to REST (see #24366). After discussing this it looks like we should add this parameter to the REST output of the aggregation. I did this in this PR already but I think I will also open a separate PR to do this in master already.
I'm not sure we can do anything in 5.x if we backport, we will need to work with a dummy constant for the count value there since it is not available via REST.

@javanna
Copy link
Member

left a comment

LGTM

...main/java/org/elasticsearch/search/aggregations/metrics/geocentroid/InternalGeoCentroid.java Outdated
return renderXContent(builder, params, centroid, count);
}

static XContentBuilder renderXContent(XContentBuilder builder, Params params, GeoPoint centroid, long count) throws IOException {

This comment has been minimized.

Copy link
@javanna

javanna Apr 28, 2017

Member

out of curiosity: do we have similar shared methods for toXContent in some other place? not a huge deal but I am a bit on the fence on adding these. Ideally the two impls wouldn't depend on each other and one day will be split apart. On the other hand we can ignore this and fix it later when needed.

This comment has been minimized.

Copy link
@cbuescher

cbuescher Apr 28, 2017

Author Member

No, we don't. I tried this in one PR by sharing something similar in the interface but that isn't such a good solution either. In former cases the two toXContent were slightliy different, in this case they are really similar, thats why I didn't want to copy them. But I get the point about dependencies and can copy the toXContent over if you prefer.
One other note on this: we often share ParseFields or at least the field keys between InternalX and ParsedX, should we also already start pulling them out, e.g. into the interfaces? Maybe better later when we eventually do the splitting at some point in the future?

This comment has been minimized.

Copy link
@javanna

javanna Apr 28, 2017

Member

Good point about the parse fields. Let's not worry too much for now. We can't be consistent if we don't enforce these constraints. Let's share these simple things for now and one day we will change that when we need to.

@cbuescher cbuescher force-pushed the cbuescher:addParsing-InternalGeoCentroid branch Apr 28, 2017

@cbuescher
Copy link
Member Author

left a comment

@javanna thanks, I reverted the shared xContent rendering and will merge this to the feature branch with the added count field in InternalGeoCentroid. I will also open an issue that we merge this part to master individually.

@cbuescher

This comment has been minimized.

Copy link
Member Author

commented Apr 28, 2017

@javanna I opened #24387 to add the 'count' parameter to 'geo_centroid' on master already. It should be similar to 49c36da. I can either merge this with our feature branch and we get #24387 in later via merge with master or we can leave this open until the PR against master is merged.

@javanna

This comment has been minimized.

Copy link
Member

commented Apr 28, 2017

@cbuescher your call I am fine either way.

@cbuescher cbuescher force-pushed the cbuescher:addParsing-InternalGeoCentroid branch Apr 28, 2017

@cbuescher cbuescher force-pushed the cbuescher:addParsing-InternalGeoCentroid branch to d50961e Apr 28, 2017

@cbuescher cbuescher merged commit 4d14143 into elastic:feature/client_aggs_parsing Apr 28, 2017

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@cbuescher cbuescher added v6.0.0 and removed v6.0.0 labels May 15, 2017

javanna added a commit to javanna/elasticsearch that referenced this pull request May 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.