Skip to content

Tests for running composite under nested aggregation #68243

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

Merged
merged 13 commits into from
Apr 12, 2021

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?

@@ -87,6 +87,21 @@
import static org.elasticsearch.search.aggregations.AggregationBuilders.nested;
import static org.hamcrest.Matchers.equalTo;

/**
* 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

@@ -87,6 +87,21 @@
import static org.elasticsearch.search.aggregations.AggregationBuilders.nested;
import static org.hamcrest.Matchers.equalTo;

/**
* 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