Return both concrete fields and aliases in DocumentFieldMappers#getMapper. #31671
Conversation
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
jtibshirani
added
>enhancement
WIP
:Search Foundations/Mapping
Collaborator
|
Pinging @elastic/es-search-aggs |
jtibshirani
force-pushed
the
document-field-mappers
branch
from
5abd62b to
52cfa67
Compare
jtibshirani
force-pushed
the
document-field-mappers
branch
from
52cfa67 to
ccbc3a1
Compare
Contributor
I'd say no. We can look into sharing the load. |
jpountz
reviewed
Jun 29, 2018
Contributor
jpountz
left a comment
jpountz
left a comment
There was a problem hiding this comment.
Change looks good, but we should have a plan to migrate all tests off the deprecated getFieldMapper method.
Contributor
Author
|
Thanks @jpountz for the review. About the deprecated calls in tests: it will still take a bit of time for this feature branch to be ready to merge, and I think that making far-reaching changes right now will increase the chances of merge conflicts. Would you be fine if we updated the tests as a last task before merging? I can add a 'technical debt' item to the meta-issue to make sure we don't drop it. |
Contributor
|
@jtibshirani Yes that would work for me. Thanks for tackling this! |
Contributor
Author
|
Added an item to the meta-issue. Is this okay to merge, or do you have other thoughts? |
Contributor
|
+1 to merge |
9 tasks
jtibshirani
added a commit
that referenced
this pull request
Jul 4, 2018
…pper. (#31671) * Rename DocumentFieldMappers#getMapper to getFieldMapper. * Introduce a new DocumentFieldMappers#getMapper that returns a Mapper. * Fix an issue around field aliases in geo suggestion contexts. * Make sure a field alias can refer to a percolate query. * Remove easy-to-fix uses of DocumentFieldMappers#getFieldMapper. * Include alias mappers in DocumentFieldMappers#getMappers. * Make sure we detect conflicts between dynamic object mappers and field aliases. * Throw an exception if aliases are specified as the target of copy_to.
jtibshirani
added a commit
to jtibshirani/elasticsearch
that referenced
this pull request
Jul 16, 2018
…pper. (elastic#31671) * Rename DocumentFieldMappers#getMapper to getFieldMapper. * Introduce a new DocumentFieldMappers#getMapper that returns a Mapper. * Fix an issue around field aliases in geo suggestion contexts. * Make sure a field alias can refer to a percolate query. * Remove easy-to-fix uses of DocumentFieldMappers#getFieldMapper. * Include alias mappers in DocumentFieldMappers#getMappers. * Make sure we detect conflicts between dynamic object mappers and field aliases. * Throw an exception if aliases are specified as the target of copy_to.
jtibshirani
added a commit
that referenced
this pull request
Jul 17, 2018
…pper. (#31671) * Rename DocumentFieldMappers#getMapper to getFieldMapper. * Introduce a new DocumentFieldMappers#getMapper that returns a Mapper. * Fix an issue around field aliases in geo suggestion contexts. * Make sure a field alias can refer to a percolate query. * Remove easy-to-fix uses of DocumentFieldMappers#getFieldMapper. * Include alias mappers in DocumentFieldMappers#getMappers. * Make sure we detect conflicts between dynamic object mappers and field aliases. * Throw an exception if aliases are specified as the target of copy_to.
jtibshirani
added a commit
that referenced
this pull request
Jul 18, 2018
…pper. (#31671) * Rename DocumentFieldMappers#getMapper to getFieldMapper. * Introduce a new DocumentFieldMappers#getMapper that returns a Mapper. * Fix an issue around field aliases in geo suggestion contexts. * Make sure a field alias can refer to a percolate query. * Remove easy-to-fix uses of DocumentFieldMappers#getFieldMapper. * Include alias mappers in DocumentFieldMappers#getMappers. * Make sure we detect conflicts between dynamic object mappers and field aliases. * Throw an exception if aliases are specified as the target of copy_to.
jtibshirani
added a commit
that referenced
this pull request
Jul 18, 2018
* Add basic support for field aliases in index mappings. (#31287) * Allow for aliases when fetching stored fields. (#31411) * Add tests around accessing field aliases in scripts. (#31417) * Add documentation around field aliases. (#31538) * Add validation for field alias mappings. (#31518) * Return both concrete fields and aliases in DocumentFieldMappers#getMapper. (#31671) * Make sure that field-level security is enforced when using field aliases. (#31807) * Add more comprehensive tests for field aliases in queries + aggregations. (#31565) * Remove the deprecated method DocumentFieldMappers#getFieldMapper. (#32148)
jtibshirani
added a commit
to jtibshirani/elasticsearch
that referenced
this pull request
Jul 24, 2018
jtibshirani
added a commit
that referenced
this pull request
Jul 24, 2018
* Add basic support for field aliases in index mappings. (#31287) * Allow for aliases when fetching stored fields. (#31411) * Add tests around accessing field aliases in scripts. (#31417) * Return both concrete fields and aliases in DocumentFieldMappers#getMapper. (#31671) * Add documentation around field aliases. (#31538) * Add validation for field alias mappings. (#31518) * Make sure that field-level security is enforced when using field aliases. (#31807) * Add more comprehensive tests for field aliases in queries + aggregations. (#31565) * Remove the deprecated method DocumentFieldMappers#getFieldMapper. (#32148) * Ensure that field aliases cannot be used in multi-fields. (#32219) * Make sure that field aliases count towards the total fields limit. (#32222) * Fix a test bug around nested aggregations and field aliases. (#32287) * Make sure the _uid field is correctly loaded in scripts. * Fix the failing test case FieldLevelSecurityTests#testParentChild_parentField. * Enforce that field aliases can only be specified on indexes with a single type.
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.
This PR updates
DocumentFieldMappers#getMapperto return both concrete fields and field aliases. Its method signature now has a top-levelMapperas its return type as opposed to aFieldMapper.To support incremental changes, there is still a
getFieldMappermethod that returns aFieldMapperas before. Almost all callers were migrated to use the method that returns aMapper, except the ~250 instances inserver_testthat require aFieldMapper. ThegetFieldMappermethod has been marked as deprecated.Updating
DocumentFieldMappers#getMappersin this way has a few advantages:MappedFieldTypein the wrong way.Mapper, we force consumers to consider how field aliases should be handled. This also caught a few bugs (see the fixes below around dynamic object mappers and copy_to).Open questions:
FieldMapperandFieldAliasMapper, instead of just using the top-levelMapper. I quickly tried this out and didn't think it made things much cleaner, and also adds complexity to an (already complex) mapper hierarchy. I would be happy to take a more detailed look at it though.