Skip to content

Fix _terms_enum display values #94080

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 2 commits into from
Feb 23, 2023
Merged

Fix _terms_enum display values #94080

merged 2 commits into from
Feb 23, 2023

Conversation

cbuescher
Copy link
Member

A recent change (#93839) introduced _terms_enum support for fields that need to convert their internal bytes representation to thr proper string representation for display purposes. The constant_keyword and flattened field types didn't implement a 'valueForDisplay' method yet, so the underlying BytesRef was printed directly in the response.

Closes #94041

A recent change (elastic#93839) introduced _terms_enum support for fields that need to
convert their internal bytes representation to thr proper string representation
for display purposes. The constant_keyword and flattened field types didn't
implement a 'valueForDisplay' method yet, so the underlying BytesRef was printed
directly in the response.

Closes elastic#94041
@cbuescher cbuescher added >bug :Search/Search Search-related issues that do not fall into other categories v8.8.0 labels Feb 23, 2023
@cbuescher cbuescher requested a review from romseygeek February 23, 2023 13:53
@elasticsearchmachine elasticsearchmachine added the Team:Search Meta label for search team label Feb 23, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search (Team:Search)

@elasticsearchmachine
Copy link
Collaborator

Hi @cbuescher, I've created a changelog YAML for you.

Copy link
Contributor

@romseygeek romseygeek left a comment

Choose a reason for hiding this comment

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

LGTM. It might be worth, in a follow-up, creating a test in MapperTestCase that everything that supports terms can enable, so that we ensure we keep coverage on this?

@cbuescher
Copy link
Member Author

It might be worth, in a follow-up, creating a test in MapperTestCase that everything that supports terms can enable, so that we ensure we keep coverage on this?

The problem here is that the conversion to a human-readable representation of the BytesRef underlying the terms_enum functionality happens in the transport action, not the mappers, so any testing about that would have to happen higher up in the call chain. Thats why I added some simple rest tests here, I was surprised about this gap in testing the return values for different field types as well and found that the existing "10_basic" yaml test in for terms_enum is a bit to complex to really add simple, easy to understand tests, so I added the one in this PR.

@cbuescher cbuescher merged commit 7e937ac into elastic:main Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

_terms_enum returns invalid response
3 participants