Fix match_only_text bugs if defined as multi-field#130188
Merged
martijnvg merged 8 commits intoelastic:mainfrom Jun 30, 2025
Merged
Fix match_only_text bugs if defined as multi-field#130188martijnvg merged 8 commits intoelastic:mainfrom
martijnvg merged 8 commits intoelastic:mainfrom
Conversation
Bugs starting to occur when elastic#129126 was merged. Closes elastic#129737
Collaborator
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
kkrik-es
reviewed
Jun 28, 2025
| if (searchExecutionContext.isSourceSynthetic()) { | ||
| if (searchExecutionContext.isSourceSynthetic() && withinMultiField == false && hasCompatibleMultiFields == false) { | ||
| String name = storedFieldNameForSyntheticSource(); | ||
| // TODO: go the parent field and load either via stored fields or doc values the values instead synthesizing complete source |
Member
There was a problem hiding this comment.
Suggested change
| // TODO: go the parent field and load either via stored fields or doc values the values instead synthesizing complete source | |
| // TODO: go the parent field and load either via stored fields or doc values, instead of synthesizing the complete source. |
kkrik-es
reviewed
Jun 28, 2025
| if (searchExecutionContext.isSourceSynthetic() && withinMultiField == false && hasCompatibleMultiFields == false) { | ||
| String name = storedFieldNameForSyntheticSource(); | ||
| // TODO: go the parent field and load either via stored fields or doc values the values instead synthesizing complete source | ||
| // (in case of synthetic source and if this field is a multi field, then it will not have a stored field.) |
Member
There was a problem hiding this comment.
Suggested change
| // (in case of synthetic source and if this field is a multi field, then it will not have a stored field.) | |
| // In case of synthetic source and if this field is a multi field, then it will not have a stored field. |
kkrik-es
reviewed
Jun 28, 2025
| ); | ||
| } | ||
| if (searchExecutionContext.isSourceSynthetic()) { | ||
| if (searchExecutionContext.isSourceSynthetic() && withinMultiField == false && hasCompatibleMultiFields == false) { |
Member
There was a problem hiding this comment.
Nit: these two variables are only used here? If so, replace them with one?
Member
Author
There was a problem hiding this comment.
This is actually for the follow up. When there is a keyword multi field then doc values or stored field of the multi field is used, otherwise if stored field / doc values of parent keyword field is used.
kkrik-es
reviewed
Jun 28, 2025
| if (searchExecutionContext.isSourceSynthetic()) { | ||
| if (searchExecutionContext.isSourceSynthetic() && withinMultiField == false && hasCompatibleMultiFields == false) { | ||
| String name = storedFieldNameForSyntheticSource(); | ||
| // TODO: go the parent field and load either via stored fields or doc values the values instead synthesizing complete source |
Member
There was a problem hiding this comment.
Does this comment belong to the next block that invokes SourceProvider?
Member
Author
There was a problem hiding this comment.
Yes, the values would be loaded via source provider, which is slow. I will fix this.
kkrik-es
reviewed
Jun 28, 2025
| } | ||
| ); | ||
| } else { | ||
| var kwd = TextFieldMapper.SyntheticSourceHelper.getKeywordFieldMapperForSyntheticSource(this); |
Member
There was a problem hiding this comment.
Is keyword the only multifield combination with match_only_text?
Member
Author
There was a problem hiding this comment.
Yes, in other cases the match_only_text field will be stored.
kkrik-es
approved these changes
Jun 28, 2025
Member
kkrik-es
left a comment
There was a problem hiding this comment.
A few comments, but approving to get you going.
…tored field or doc values of parent keyword field or stored field or doc values of multi field.
martijnvg
added a commit
to martijnvg/elasticsearch
that referenced
this pull request
Jun 30, 2025
* Fix match_only_text bugs if defined as multi-field Bugs starting to occur when elastic#129126 was merged. Closes elastic#129737
Collaborator
💔 Backport failed
You can use sqren/backport to manually backport by running |
elasticsearchmachine
pushed a commit
that referenced
this pull request
Jun 30, 2025
martijnvg
added a commit
to martijnvg/elasticsearch
that referenced
this pull request
Jun 30, 2025
Backporting elastic#130188 to 8.19 branch. Bugs starting to occur when elastic#129126 was merged. Closes elastic#129737
elasticsearchmachine
pushed a commit
that referenced
this pull request
Jun 30, 2025
mridula-s109
pushed a commit
to mridula-s109/elasticsearch
that referenced
this pull request
Jul 3, 2025
* Fix match_only_text bugs if defined as multi-field Bugs starting to occur when elastic#129126 was merged. Closes elastic#129737
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bugs starting to occur when #129126 was merged.
Closes #129737
This is a bug, but marking as non-issue. Given that it has not been released in a stateful release.