Make sure that field collapsing supports field aliases. #32648
Merged
Conversation
This file contains 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
jtibshirani
added
>bug
:Search Foundations/Mapping
|
Pinging @elastic/es-search-aggs |
jtibshirani
commented
Aug 6, 2018
| import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; | ||
| import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertNoFailures; | ||
|
|
||
| public class FieldCollapseIT extends ESIntegTestCase { |
There was a problem hiding this comment.
I looked into creating a unit test, but for just adding this single case, it was a lot cleaner/ simpler to write an integration test.
There was a problem hiding this comment.
What about a rest test ? We have rest-api-spec/src/main/resources/rest-api-spec/test/search/110_field_collapsing.yml already so maybe add a case there ?
There was a problem hiding this comment.
I find REST tests more difficult to work with in terms of debugging, so I've been preferring Java integration tests unless I'm actually testing the REST spec. I'm happy to switch this over though, if writing a REST test is more standard.
jimczi
reviewed
Aug 7, 2018
| import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; | ||
| import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertNoFailures; | ||
|
|
||
| public class FieldCollapseIT extends ESIntegTestCase { |
There was a problem hiding this comment.
What about a rest test ? We have rest-api-spec/src/main/resources/rest-api-spec/test/search/110_field_collapsing.yml already so maybe add a case there ?
jtibshirani
force-pushed
the
collapse-on-field-alias
branch
2 times, most recently
from
e128456
to
ffb967d
Compare
|
Thanks @jimczi, I think it's ready for another look. |
jtibshirani
force-pushed
the
collapse-on-field-alias
branch
from
ffb967d
to
3321c7a
Compare
jimczi
approved these changes
Aug 7, 2018
|
@elasticmachine run packaging tests |
jtibshirani
added a commit
that referenced
this pull request
Aug 7, 2018
jtibshirani
added a commit
that referenced
this pull request
Aug 7, 2018
89 tasks
jtibshirani
added a commit
to jtibshirani/elasticsearch
that referenced
this pull request
Jan 8, 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.
jtibshirani
added a commit
that referenced
this pull request
Jan 8, 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 #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.
jtibshirani
added a commit
that referenced
this pull request
Jan 8, 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 #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.
jtibshirani
added a commit
that referenced
this pull request
Jan 8, 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 #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
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.
This was referenced Feb 3, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This component of the search request wasn't covered by the original field alias PR.
Addresses #32623.