Skip to content

Conversation

@not-napoleon
Copy link
Member

@not-napoleon not-napoleon commented Jan 29, 2021

This PR adds REST layer and junit layer tests for running composite aggregation under nested aggregations. It also adds some REST tests for nested, since I was there and we don't have any right now.

This is related to #38863 but I want to merge these tests separately so I can verify the current behavior before that change.

@mark-vieira
Copy link
Contributor

@elasticmachine update branch

@not-napoleon not-napoleon added :Analytics/Aggregations Aggregations >non-issue >test Issues or PRs that are addressing/adding tests Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels Apr 7, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (Team:Analytics)


@Override
protected IndexReader wrapDirectoryReader(DirectoryReader reader) throws IOException {
if (useNestedDirectoryWrapping) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you check if the list is non-empty?


/**
* Tests for the Nested aggregator.
*
Copy link
Member

Choose a reason for hiding this comment

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

<p>.

Silly javadoc, you are too html.

Copy link
Member Author

Choose a reason for hiding this comment

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

Bad HTML. <p> != <br/>. But fine.

Copy link
Member

Choose a reason for hiding this comment

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

? I mean, you could <p> the whole paragraph. I dunno. whatever works.

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

LGTM


/**
* Tests for the Nested aggregator.
*
Copy link
Member

Choose a reason for hiding this comment

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

? I mean, you could <p> the whole paragraph. I dunno. whatever works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/Aggregations Aggregations >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >test Issues or PRs that are addressing/adding tests v7.13.0 v8.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants