Ensure that field collapsing works with field aliases.#50722
Merged
jtibshirani merged 1 commit intoelastic:masterfrom Jan 8, 2020
Merged
Ensure that field collapsing works with field aliases.#50722jtibshirani merged 1 commit intoelastic:masterfrom
jtibshirani merged 1 commit intoelastic:masterfrom
Conversation
Collaborator
|
Pinging @elastic/es-search (:Search/Mapping) |
Previously, the following situation would throw an error: * A search contains a `collapse` on a particular field. * The search spans multiple indices, and in one index the field is mapped as a concrete field, but in another it is a field alias. The error occurs when we attempt to merge `CollapseTopFieldDocs` across shards. When merging, we validate that the name of the collapse field is the same across shards. But the name has already been resolved to the concrete field name, so it will be different on shards where the field was mapped as an alias vs. shards where it was a concrete field. This PR updates the collapse field name in `CollapseTopFieldDocs` to the original requested field, so that it will always be consistent across shards. Note that in elastic#32648, we already made a fix around collapsing on field aliases. However, we didn't test this specific scenario where the field was mapped as an alias in only one of the indices being searched.
1b5c7a2 to
bd49917
Compare
Contributor
Author
|
@jimczi would you be up for reviewing this? I am not completely sure about the approach and would be open to other suggestions. |
jimczi
approved these changes
Jan 8, 2020
Contributor
jimczi
left a comment
There was a problem hiding this comment.
The approach looks good to me, the collapse field used in the top docs is part of the response so +1 to preserve the one provided by the user.
Contributor
Author
|
Thanks for the review. |
This was referenced Jan 8, 2020
jtibshirani
added a commit
that referenced
this pull request
Jan 9, 2020
Follow-up to #50722, which was also backported to 7.5 and 7.6.
SivagurunathanV
pushed a commit
to SivagurunathanV/elasticsearch
that referenced
this pull request
Jan 23, 2020
Previously, the following situation would throw an error: * A search contains a `collapse` on a particular field. * The search spans multiple indices, and in one index the field is mapped as a concrete field, but in another it is a field alias. The error occurs when we attempt to merge `CollapseTopFieldDocs` across shards. When merging, we validate that the name of the collapse field is the same across shards. But the name has already been resolved to the concrete field name, so it will be different on shards where the field was mapped as an alias vs. shards where it was a concrete field. This PR updates the collapse field name in `CollapseTopFieldDocs` to the original requested field, so that it will always be consistent across shards. Note that in elastic#32648, we already made a fix around collapsing on field aliases. However, we didn't test this specific scenario where the field was mapped as an alias in only one of the indices being searched.
SivagurunathanV
pushed a commit
to SivagurunathanV/elasticsearch
that referenced
this pull request
Jan 23, 2020
Follow-up to elastic#50722, which was also backported to 7.5 and 7.6.
This was referenced Feb 3, 2020
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
4 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.
Previously, the following situation would throw an error:
collapseon a particular field.concrete field, but in another it is a field alias.
The error occurs when we attempt to merge
CollapseTopFieldDocsacross shards.When merging, we validate that the name of the collapse field is the same across
shards. But the name has already been resolved to the concrete field name, so it
will be different on shards where the field was mapped as an alias vs. shards
where it was a concrete field.
This PR updates the collapse field name in
CollapseTopFieldDocsto theoriginal requested field, so that it will always be consistent across shards.
Note that in #32648, we already made a fix around collapsing on field aliases.
However, we didn't test this specific scenario (where the field was mapped
differently across indices).
Addresses #50121.