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

Use ValueFetcher when loading text snippets to highlight (#63572) #65441

Merged
merged 4 commits into from
Nov 25, 2020

Conversation

romseygeek
Copy link
Contributor

HighlighterUtils.loadFieldValues() loads values directly from the source, and
then callers have to deal with filtering out values that would have been removed
by an ignore_above filter on keyword fields. Instead, we can use the
ValueFetcher for the relevant field, which handles all this logic for us.

Closes #59931.

HighlighterUtils.loadFieldValues() loads values directly from the source, and
then callers have to deal with filtering out values that would have been removed
by an ignore_above filter on keyword fields. Instead, we can use the
ValueFetcher for the relevant field, which handles all this logic for us.

Closes elastic#59931.
@romseygeek romseygeek self-assigned this Nov 24, 2020
@romseygeek
Copy link
Contributor Author

cc @jtibshirani - I've added a new migrate_7_11.asciidoc file with a note about the changed output from normalized keyword fields.

Copy link
Contributor

@jtibshirani jtibshirani left a comment

Choose a reason for hiding this comment

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

I left some suggestions on the migration notes.


See also <<release-highlights>> and <<es-release-notes>>.

// * <<breaking_710_blah_changes>>
Copy link
Contributor

Choose a reason for hiding this comment

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

breaking_710_blah_changes -> breaking_711_blah_changes

//tag::notable-breaking-changes[]

[discrete]
[[breaking_710_search_changes]]
Copy link
Contributor

Choose a reason for hiding this comment

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

breaking_710_search_changes -> breaking_711_search_changes

=== Search changes

[[highlight-normalization]]
.Keyword fields with a custom normalizer will use the normalized form when highlighting
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we mention the change to copy_to as well? This is technically a breaking behavioral change, and it's nice to alert users about it because they could be able to stopping setting store: true on some fields.

@romseygeek
Copy link
Contributor Author

I've fixed the 710->711 substitutions that I'd missed earlier. I'm don't think this is the right place to call out the copy_to changes, as it's not really something you need to be aware of when you upgrade? Maybe I should add a >feature tag to the original PR and put a comment explaining what has been added, so that it gets picked up when we do the release notes?

@jtibshirani
Copy link
Contributor

Maybe I should add a >feature tag to the original PR and put a comment explaining what has been added, so that it gets picked up when we do the release notes?

Sounds good to me!

@romseygeek
Copy link
Contributor Author

@elasticmachine update branch

@romseygeek romseygeek merged commit 92e4b15 into elastic:7.x Nov 25, 2020
@romseygeek romseygeek deleted the highlight/fetcher-7x branch November 25, 2020 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants