-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Allow composite aggregation under filter or reverse_nested parent #11499
Allow composite aggregation under filter or reverse_nested parent #11499
Conversation
Compatibility status:Checks if related components are compatible with change 6274d2e Incompatible componentsIncompatible components: [https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/performance-analyzer-rca.git] Skipped componentsCompatible componentsCompatible components: [https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/k-nn.git] |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #11499 +/- ##
============================================
- Coverage 71.33% 71.31% -0.02%
- Complexity 59300 59304 +4
============================================
Files 4921 4921
Lines 278989 278989
Branches 40543 40543
============================================
- Hits 199014 198973 -41
- Misses 63444 63483 +39
- Partials 16531 16533 +2 ☔ View full report in Codecov by Sentry. |
❕ Gradle check result for a491340: UNSTABLE
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
65bbddb
to
f17a66a
Compare
❌ Gradle check result for f17a66a: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
f17a66a
to
c2d4f83
Compare
❕ Gradle check result for c2d4f83: UNSTABLE
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
❌ Gradle check result for 99f6ee9: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Michael Froh <[email protected]>
❌ Gradle check result for 6274d2e: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
❌ Gradle check result for 6274d2e: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
This change modifies 3 lines of code:
While I jumped through hoops to add unit test coverage for the 2 relevant lines, codecov/patch is failing because I did not add unit test coverage for the typo fix. I respectfully reject codecov/patch's opinion on this PR. |
Do we need any documentation updates to say those composite aggregation are now allowed? |
Right now we have no documentation on composite aggregation, as far as I can tell. I think writing the docs can happen independent of this fix. |
Sure, having an issue here https://github.com/opensearch-project/documentation-website/ for 2.12.0 would be enough |
…1499) * Allow composite aggregation under filter parent Composite aggregations are able to run under a filter aggregation with no change required (other than not throwing an exception). Also cleaned up FilterAggregatorFactory a little. Signed-off-by: Michael Froh <[email protected]> * Add changelog entry Signed-off-by: Michael Froh <[email protected]> * Add support for reverse nested agg too Signed-off-by: Michael Froh <[email protected]> * Add unit test coverage Signed-off-by: Michael Froh <[email protected]> * Skip new tests in pre-3.0 mixed cluster Signed-off-by: Michael Froh <[email protected]> --------- Signed-off-by: Michael Froh <[email protected]> (cherry picked from commit 24b0cc9) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Thanks for getting this across the finish line @msfroh! |
…1499) * Allow composite aggregation under filter parent Composite aggregations are able to run under a filter aggregation with no change required (other than not throwing an exception). Also cleaned up FilterAggregatorFactory a little. Signed-off-by: Michael Froh <[email protected]> * Add changelog entry Signed-off-by: Michael Froh <[email protected]> * Add support for reverse nested agg too Signed-off-by: Michael Froh <[email protected]> * Add unit test coverage Signed-off-by: Michael Froh <[email protected]> * Skip new tests in pre-3.0 mixed cluster Signed-off-by: Michael Froh <[email protected]> --------- Signed-off-by: Michael Froh <[email protected]> (cherry picked from commit 24b0cc9) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
This change was not backported to 2.x in time to make the 2.12 release and therefore will be pending until the 2.13 release. Signed-off-by: Andrew Ross <[email protected]>
This change was not backported to 2.x in time to make the 2.12 release and therefore will be pending until the 2.13 release. Signed-off-by: Andrew Ross <[email protected]>
…1499) (#12151) * Allow composite aggregation under filter parent Composite aggregations are able to run under a filter aggregation or a reverse_nested aggreation, with no change required (other than not throwing an exception). Also cleaned up FilterAggregatorFactory a little. --------- (cherry picked from commit 24b0cc9) Signed-off-by: Michael Froh <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…ensearch-project#11499) * Allow composite aggregation under filter parent Composite aggregations are able to run under a filter aggregation with no change required (other than not throwing an exception). Also cleaned up FilterAggregatorFactory a little. Signed-off-by: Michael Froh <[email protected]> * Add changelog entry Signed-off-by: Michael Froh <[email protected]> * Add support for reverse nested agg too Signed-off-by: Michael Froh <[email protected]> * Add unit test coverage Signed-off-by: Michael Froh <[email protected]> * Skip new tests in pre-3.0 mixed cluster Signed-off-by: Michael Froh <[email protected]> --------- Signed-off-by: Michael Froh <[email protected]>
…h-project#12311) This change was not backported to 2.x in time to make the 2.12 release and therefore will be pending until the 2.13 release. Signed-off-by: Andrew Ross <[email protected]>
…ensearch-project#11499) * Allow composite aggregation under filter parent Composite aggregations are able to run under a filter aggregation with no change required (other than not throwing an exception). Also cleaned up FilterAggregatorFactory a little. Signed-off-by: Michael Froh <[email protected]> * Add changelog entry Signed-off-by: Michael Froh <[email protected]> * Add support for reverse nested agg too Signed-off-by: Michael Froh <[email protected]> * Add unit test coverage Signed-off-by: Michael Froh <[email protected]> * Skip new tests in pre-3.0 mixed cluster Signed-off-by: Michael Froh <[email protected]> --------- Signed-off-by: Michael Froh <[email protected]>
…h-project#12311) This change was not backported to 2.x in time to make the 2.12 release and therefore will be pending until the 2.13 release. Signed-off-by: Andrew Ross <[email protected]>
Hi all, thanks for the great work! Do you know when this feature will be available in AWS OpenSearch service? Currently the latest supported version is 2.11. Feel free to ignore if it's unrelated, thanks! |
…ensearch-project#11499) * Allow composite aggregation under filter parent Composite aggregations are able to run under a filter aggregation with no change required (other than not throwing an exception). Also cleaned up FilterAggregatorFactory a little. Signed-off-by: Michael Froh <[email protected]> * Add changelog entry Signed-off-by: Michael Froh <[email protected]> * Add support for reverse nested agg too Signed-off-by: Michael Froh <[email protected]> * Add unit test coverage Signed-off-by: Michael Froh <[email protected]> * Skip new tests in pre-3.0 mixed cluster Signed-off-by: Michael Froh <[email protected]> --------- Signed-off-by: Michael Froh <[email protected]> Signed-off-by: Shivansh Arora <[email protected]>
…h-project#12311) This change was not backported to 2.x in time to make the 2.12 release and therefore will be pending until the 2.13 release. Signed-off-by: Andrew Ross <[email protected]> Signed-off-by: Shivansh Arora <[email protected]>
Description
Composite aggregations are able to run under a filter aggregation with no change required (other than not throwing an exception). Similarly, they can run under a reverse nested aggregation.
Also cleaned up FilterAggregatorFactory a little.
Related Issues
Resolves #11131
Check List
New functionality has been documented.New functionality has javadoc addedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.