-
Notifications
You must be signed in to change notification settings - Fork 25.3k
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
Tests for running composite under nested aggregation #68243
Conversation
@elasticmachine update branch |
Pinging @elastic/es-analytics-geo (Team:Analytics) |
|
||
@Override | ||
protected IndexReader wrapDirectoryReader(DirectoryReader reader) throws IOException { | ||
if (useNestedDirectoryWrapping) { |
There was a problem hiding this comment.
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. | |||
* |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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. | |||
* |
There was a problem hiding this comment.
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.
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.