Skip to content

Conversation

@tlrx
Copy link
Member

@tlrx tlrx commented May 16, 2017

This pull request adds the parsing logic for the InternalTopHits aggregation.

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.

LGTM thanks @tlrx

Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

@tlrx thanks, looks great, left two nits and a question for clarification.

Copy link
Member

Choose a reason for hiding this comment

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

nit: could be one line

Copy link
Member

Choose a reason for hiding this comment

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

Luca mentioned in another PR that we might not need the need @bfore here since the parent method already has it.

Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why we need to sort here? I thought hits is an array, so we should parse back the hits in the same order as in the original object, or do I miss something?

Copy link
Member

Choose a reason for hiding this comment

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

good point I am curious too now :) I hadn't noticed this at first

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a leftover, this is indeed not necessary. Thanks for catching it Christoph!

@tlrx tlrx force-pushed the add-parsing-top-hits branch from 469552f to 23cc872 Compare May 18, 2017 07:37
@tlrx tlrx merged commit 0558753 into elastic:feature/client_aggs_parsing May 18, 2017
@tlrx
Copy link
Member Author

tlrx commented May 18, 2017

Thanks @javanna @cbuescher

@tlrx tlrx deleted the add-parsing-top-hits branch May 18, 2017 07:38
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