Skip to content

Pull the flattened field type into a mapper plugin. #43250

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

Merged

Conversation

jtibshirani
Copy link
Contributor

This PR shows a prototype approach to pulling FlatObjectFieldMapper into its
own MapperPlugin. It introduces two new methods on FieldMapper:

  • supportsKeyedFieldLookup(), which specifies whether the field type supports
    looking for subfields
  • keyedFieldType(String key), which gives the opportunity to return a special
    field type for a subfield

FieldTypeLookup has been updated to refer to 'keyed lookup' instead of
'flattened objects'. I pulled most tests into a new flattened module
(although some still need to be migrated).

@jtibshirani jtibshirani added :Search Foundations/Mapping Index mappings, including merging and defining field types >refactoring labels Jun 14, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@jtibshirani jtibshirani force-pushed the flattened-mapper-plugin branch 3 times, most recently from 553b042 to 64755d2 Compare June 14, 2019 22:29
@jtibshirani jtibshirani force-pushed the flattened-mapper-plugin branch from 64755d2 to b8f9ac6 Compare June 14, 2019 22:51
Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

The approach works for me. I'm not seeing this functionality as something that will be used by any other field, so I'd have a slight preference for replacing MappedFieldType#supportsKeyedLookups() with a marker interface, and then moving keyedFieldType to that interface, in order to not add more API surface area to MappedFieldType.

@jtibshirani
Copy link
Contributor Author

Thanks @jpountz for taking a look. I understand the concern around not adding too many methods to FieldMapper -- using a marker interface works for me too. It doesn't sound like there are big objections from you or @jimczi in terms of the approach, so I'll plan to clean up this prototype.

One note for context: I thought there may be a use case for this lookup functionality outside of flattened fields, but after digging into the details with the other developers I was consulting with, we agreed it is not a good fit. So for the near future flattened fields will be the only consumer of the new marker interface.

* Migrate the remaining tests around aggregations.
* Improve the naming and documentation around 'dynamic key lookup'.
@jtibshirani jtibshirani marked this pull request as ready for review June 17, 2019 23:30
@jtibshirani
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/1
@elasticmachine run elasticsearch-ci/oss-distro-docs

@jtibshirani jtibshirani force-pushed the flattened-mapper-plugin branch from 3bbc4d6 to 4955916 Compare June 18, 2019 22:09
@jtibshirani jtibshirani merged commit 232f494 into elastic:object-fields Jun 27, 2019
@jtibshirani jtibshirani deleted the flattened-mapper-plugin branch June 27, 2019 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>refactoring :Search Foundations/Mapping Index mappings, including merging and defining field types
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants