Skip to content
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

Added size attribute to MultiTermsAggregation #627

Merged
merged 2 commits into from
Sep 25, 2023
Merged

Added size attribute to MultiTermsAggregation #627

merged 2 commits into from
Sep 25, 2023

Conversation

fabiankopatschek
Copy link
Contributor

Description

Added size attribute to MultiTermsAggregation to retreive other hit sizes than the default value of the attribute.
Code for the added attribute was taken from TermsAggregation class.
This pull request replaces #614, as dblock suggested that the changes should go to the main branch.
Please backfort this code to 2.x.

Issues Resolved

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

dblock
dblock previously approved these changes Sep 18, 2023
@dblock dblock added the backport 2.x Backport to 2.x branch label Sep 18, 2023
Copy link
Collaborator

@VachaShah VachaShah left a comment

Choose a reason for hiding this comment

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

@fabiankopatschek Thank you for adding this! Some integ tests related to aggregations are failing. Can you take a look?

@fabiankopatschek
Copy link
Contributor Author

@fabiankopatschek Thank you for adding this! Some integ tests related to aggregations are failing. Can you take a look?

@VachaShah I did some manual testing and some research. The multi_terms feature was implemented with version 2.1.0 of opensearch.
Is there a mechanism to run specific integration tests only against a specified minimum version of opensearch?

@VachaShah
Copy link
Collaborator

@fabiankopatschek Thank you for adding this! Some integ tests related to aggregations are failing. Can you take a look?

@VachaShah I did some manual testing and some research. The multi_terms feature was implemented with version 2.1.0 of opensearch. Is there a mechanism to run specific integration tests only against a specified minimum version of opensearch?

Yes, you can do something similar to https://github.com/opensearch-project/opensearch-java/blob/main/java-client/src/test/java/org/opensearch/client/opensearch/integTest/AbstractRequestIT.java#L633.

@fabiankopatschek
Copy link
Contributor Author

@fabiankopatschek Thank you for adding this! Some integ tests related to aggregations are failing. Can you take a look?

@VachaShah I did some manual testing and some research. The multi_terms feature was implemented with version 2.1.0 of opensearch. Is there a mechanism to run specific integration tests only against a specified minimum version of opensearch?

Yes, you can do something similar to https://github.com/opensearch-project/opensearch-java/blob/main/java-client/src/test/java/org/opensearch/client/opensearch/integTest/AbstractRequestIT.java#L633.

@VachaShah Thank you for the link.
I added this part to the failing integrationTests and did some local testing with 1.3.12 and 2.5.0.

@VachaShah
Copy link
Collaborator

@fabiankopatschek Thank you for adding this! Some integ tests related to aggregations are failing. Can you take a look?

@VachaShah I did some manual testing and some research. The multi_terms feature was implemented with version 2.1.0 of opensearch. Is there a mechanism to run specific integration tests only against a specified minimum version of opensearch?

Yes, you can do something similar to https://github.com/opensearch-project/opensearch-java/blob/main/java-client/src/test/java/org/opensearch/client/opensearch/integTest/AbstractRequestIT.java#L633.

@VachaShah Thank you for the link. I added this part to the failing integrationTests and did some local testing with 1.3.12 and 2.5.0.

Looks great! Can you rebase and fix the merge conflicts in Changelog and we are good to go.

Signed-off-by: Fabian Kopatschek <fabian.kopatschek@yahoo.de>
Signed-off-by: Fabian Kopatschek <fabian.kopatschek@yahoo.de>
@fabiankopatschek
Copy link
Contributor Author

@fabiankopatschek Thank you for adding this! Some integ tests related to aggregations are failing. Can you take a look?

@VachaShah I did some manual testing and some research. The multi_terms feature was implemented with version 2.1.0 of opensearch. Is there a mechanism to run specific integration tests only against a specified minimum version of opensearch?

Yes, you can do something similar to https://github.com/opensearch-project/opensearch-java/blob/main/java-client/src/test/java/org/opensearch/client/opensearch/integTest/AbstractRequestIT.java#L633.

@VachaShah Thank you for the link. I added this part to the failing integrationTests and did some local testing with 1.3.12 and 2.5.0.

Looks great! Can you rebase and fix the merge conflicts in Changelog and we are good to go.

Done :)

@dblock dblock merged commit 1583f72 into opensearch-project:main Sep 25, 2023
30 checks passed
@VachaShah VachaShah added backport 2.x Backport to 2.x branch and removed backport 2.x Backport to 2.x branch labels Oct 4, 2023
VachaShah pushed a commit to VachaShah/opensearch-java that referenced this pull request Oct 4, 2023
* Added size attribute to MultiTermsAggregation

Signed-off-by: Fabian Kopatschek <fabian.kopatschek@yahoo.de>

* Added integrationTest check if MultiTermsAggregation is supported

Signed-off-by: Fabian Kopatschek <fabian.kopatschek@yahoo.de>

---------

Signed-off-by: Fabian Kopatschek <fabian.kopatschek@yahoo.de>
Signed-off-by: Vacha Shah <vachshah@amazon.com>
reta pushed a commit that referenced this pull request Oct 4, 2023
* Added size attribute to MultiTermsAggregation



* Added integrationTest check if MultiTermsAggregation is supported



---------

Signed-off-by: Fabian Kopatschek <fabian.kopatschek@yahoo.de>
Signed-off-by: Vacha Shah <vachshah@amazon.com>
Co-authored-by: fabiankopatschek <34305396+fabiankopatschek@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants