Skip to content

Conversation

@cbuescher
Copy link
Member

This adds parsing and related tests to the InternalAdjacencyMatrix aggregation.
Based on #24698 which should go into master first.

@cbuescher cbuescher requested review from javanna and tlrx May 16, 2017 08:48
@javanna javanna changed the title Add parsing for InternalAdjecencyMatrix aggregation Add parsing for InternalAdjacencyMatrix aggregation May 16, 2017
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.

left a small comment, LGTM otherwise

Copy link
Member

Choose a reason for hiding this comment

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

there's a typo in the name: rename to ParsedAdjacencyMatrix ?

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, I left some comments

Copy link
Member

Choose a reason for hiding this comment

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

I think you can use the ParsedMultiBucketAggregation.ParsedBucket#toXContent() method and not declare it here again?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, you are right, although this has the potential to render a keyed response or the key_as_string field if this class is not used correctly, but I guess its worth it.

Copy link
Member

Choose a reason for hiding this comment

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

I agree

Copy link
Member

Choose a reason for hiding this comment

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

extra lines

Copy link
Member

Choose a reason for hiding this comment

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

I think you could use
return parseXContent(parser, false, ParsedBucket::new, (p, bucket) -> bucket.key = p.text());

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, again I used a modified method here to not parse a.g. KEY_AS_STRING but I think its worth the saving to just reuse the more general super class method since we know the response doesn't contain certain fields.

Copy link
Member

Choose a reason for hiding this comment

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

I agree too

@cbuescher cbuescher force-pushed the addParsing-InternalAdjecencyMatrix branch from 871f722 to 5a4d757 Compare May 16, 2017 10:09
@cbuescher cbuescher merged commit ef7c2e6 into elastic:feature/client_aggs_parsing May 16, 2017
javanna pushed 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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants