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 single bucket aggregations #24564

Conversation

Projects
None yet
3 participants
@cbuescher
Copy link
Member

commented May 9, 2017

This is an alternative approach to #24386 that doesn't rely on extending ObjectParsers capabilities and uses manual parsing instead.

core/src/main/java/org/elasticsearch/search/aggregations/bucket/sampler/InternalSampler.java Outdated
@@ -48,11 +48,6 @@ public String getWriteableName() {
}

@Override
protected String getType() {
return "sampler";

This comment has been minimized.

Copy link
@cbuescher

cbuescher May 9, 2017

Author Member

This is weird and prevented the tests from working. We implement this method in InternalAggregation#getType() to return getWriteableName(), and InternalSampler and its subclass UnmappedSampler have different names there. If we used "sampler" for both classes in the XContent output as type prefix we wouldn't be able to distinguish the two when parsing. I wonder if this was added by mistake or for a reason, maybe you can double check this?

This comment has been minimized.

Copy link
@javanna

javanna May 9, 2017

Member

I think the idea was that InternalSampler and UnmappedSampler should have the same type, so that we only parse into InternalSampler. Maybe we meant to call it "sampler" in both cases rather than mapped_sampler but I guess we don't need that.

This comment has been minimized.

Copy link
@javanna

javanna May 9, 2017

Member

Why didn't we have this problem also in the ObjectParser version of this PR?

This comment has been minimized.

Copy link
@cbuescher

cbuescher May 9, 2017

Author Member

I think we did, but I just found it. The fromXContent() tests are curerently skipped if the register the parser under "sampler" but then find "unmapped_sampler" or "mapped_sampler" in the xContent

aggregation.setDocCount(parser.longValue());
}
} else if (token == XContentParser.Token.START_OBJECT) {
if (CommonFields.META.getPreferredName().equals(currentFieldName)) {

This comment has been minimized.

Copy link
@javanna

javanna May 9, 2017

Member

now I see what you meant yesterday saying that we have to parse meta here

@cbuescher cbuescher force-pushed the cbuescher:addParsing-SingleBucket-v2 branch 2 times, most recently May 10, 2017

@cbuescher cbuescher changed the title WIP: Add parsing single bucket using manual parsing Add parsing for single bucket aggregations May 10, 2017

@cbuescher cbuescher removed the WIP label May 10, 2017

@cbuescher

This comment has been minimized.

Copy link
Member Author

commented May 10, 2017

@javanna @tlrx I squashed and tidied this up a bit, theres a small TODO in the tests that I still need to adress but I think the rest is good for a review.

@javanna javanna removed the v6.0.0 label May 10, 2017

@javanna
Copy link
Member

left a comment

LGTM good to go when you resolved the last TODO

@tlrx

tlrx approved these changes May 10, 2017

@cbuescher cbuescher force-pushed the cbuescher:addParsing-SingleBucket-v2 branch 2 times, most recently May 10, 2017

@cbuescher cbuescher force-pushed the cbuescher:addParsing-SingleBucket-v2 branch to 035f238 May 11, 2017

@cbuescher cbuescher merged commit c4fc8ed into elastic:feature/client_aggs_parsing May 11, 2017

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@javanna javanna referenced this pull request May 11, 2017

Closed

Java High Level REST Client plan for first release #23331

58 of 58 tasks complete

@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

Add parsing for single bucket aggregations (elastic#24564)
This adds parsing to all implementations of SingleBucketAggregations. They are mostly similar, so they share the common
base class `ParsedSingleBucketAggregation` and the shared base test `InternalSingleBucketAggregationTestCase`.
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.