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

Feat: add support for docvalue_fields and stored_fields in TopHitsAggregation #3176

Merged
merged 3 commits into from
Oct 11, 2024

Conversation

paulMConstant
Copy link
Contributor

Hello,

It is currently impossible to my understanding to add "docvalue_fields" to a top hits aggregation. Here's a proposition using the storedFields field.

Thanks

Copy link
Owner

@Philippus Philippus left a comment

Choose a reason for hiding this comment

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

Stored fields are not the same thing as doc value fields, see https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-metrics-top-hits-aggregation.html. The existing storedFields field is not used in the builder, so that could be added. If you want to add docValueFields a field would have to be added to the TopHitsAggregation case class and an accompanying docvalueFields method similar to what is done in the InnerHit case class.

@paulMConstant
Copy link
Contributor Author

Thank you for the quick review. I fixed the storedFields addition and added docValueFields as done in InnerHit.

@paulMConstant paulMConstant changed the title Feat: add support for docvalue_fields in TopHitsAggregation Feat: add support for docvalue_fields and stored_fields in TopHitsAggregation Oct 11, 2024
@Philippus Philippus merged commit a9e0260 into Philippus:main Oct 11, 2024
3 checks passed
@Philippus
Copy link
Owner

Thank you!

@paulMConstant paulMConstant deleted the paul/main branch October 11, 2024 12:10
@paulMConstant
Copy link
Contributor Author

Thank you for the quick merge !

@paulMConstant
Copy link
Contributor Author

Do you think you could make a release soon ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants