Skip to content

Rework FieldMapper and MappedFieldType #56814

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.

Sign up for GitHub

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

Jump to bottom
Closed
5 of 10 tasks
romseygeek opened this issue May 15, 2020 · 2 comments
Closed
5 of 10 tasks

Rework FieldMapper and MappedFieldType #56814

romseygeek opened this issue May 15, 2020 · 2 comments
Assignees
@romseygeek
Labels
>enhancement Meta :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch

Comments

@romseygeek
Copy link
Contributor

romseygeek commented May 15, 2020 •
edited
Loading

We currently have two abstractions that encapsulate different mapping types within elasticsearch. FieldMapper defines how a particular field from a json document should be indexed into lucene, while MappedFieldType defines factory methods for building queries against those fields. However, MappedFieldType also extends lucene's FieldType and so holds some index structure configuration, which really belongs on FieldMapper. As a result, it also handles some merging and consistency checking logic dealing with mapping updates, which again only really effect index-time structures.

We should rework the relationship here, decoupling MappedFieldType from FieldType and keeping it as a simple source of query factory methods. This would also make it easier to generate field types that point to other fields, adding functionality on the way. For example, if we reworked field aliases to be a simple pointer field type, then we could remove a lot of special-casing logic in MapperService that has to handle normal fields and aliases separately.

Because of the fairly complex coupling already involved here, this will need to be done in several steps:

  • Move mapping update consistency checks from MappedFieldType to FieldMapper (Move merge compatibility logic from MappedFieldType to FieldMapper #56915)
  • Stop MappedFieldType extending FieldType, and move index configuration to FieldMapper (MappedFieldType should not extend FieldType #57666)
  • Add a new abstraction on MappedFieldType encapsulating 'text search' config - analyzers, FieldType, etc (Exclude WindowsFS from SharedClusterSnapshotRestoreIT (#58020) #58023)
  • Remove base FieldMapper class references to lucene's FieldType
  • Remove 'global' FieldMapper fields that refer to specific data types
    • analyzers and similarity should move to TextSearchInfo
    • eager_global_ordinals only applies to keyword and join
    • index, doc_values and store won't apply to search-time only mappers, so maybe move them as well?
  • Make FieldMapper.fieldType() abstract, and don't require a MappedFieldType to be passed to the base class constructor. This will allow for late-binding of search fields, so that they can reference other field mappers.
  • Rename MappedFieldType to SearchField
@romseygeek romseygeek added >enhancement :Search Foundations/Mapping Index mappings, including merging and defining field types labels May 15, 2020
@romseygeek romseygeek self-assigned this May 15, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/Mapping)

@elasticmachine elasticmachine added the Team:Search Meta label for search team label May 15, 2020
This was referenced May 15, 2020
Closed
Merged
romseygeek added a commit that referenced this issue May 20, 2020
@romseygeek
Move merge compatibility logic from MappedFieldType to FieldMapper (#…
f82d74b 
…56915)

Merging logic is currently split between FieldMapper, with its merge() method, and
MappedFieldType, which checks for merging compatibility. The compatibility checks
are called from a third class, MappingMergeValidator. This makes it difficult to reason
about what is or is not compatible in updates, and even what is in fact updateable - we
have a number of tests that check compatibility on changes in mapping configuration
that are not in fact possible.

This commit refactors the compatibility logic so that it all sits on FieldMapper, and
makes it called at merge time. It adds a new FieldMapperTestCase base class that
FieldMapper tests can extend, and moves the compatibility testing machinery from
FieldTypeTestCase to here.

Relates to #56814
romseygeek added a commit that referenced this issue May 20, 2020
@romseygeek
Move merge compatibility logic from MappedFieldType to FieldMapper (#…
18bfbed 
…56915)

Merging logic is currently split between FieldMapper, with its merge() method, and
MappedFieldType, which checks for merging compatibility. The compatibility checks
are called from a third class, MappingMergeValidator. This makes it difficult to reason
about what is or is not compatible in updates, and even what is in fact updateable - we
have a number of tests that check compatibility on changes in mapping configuration
that are not in fact possible.

This commit refactors the compatibility logic so that it all sits on FieldMapper, and
makes it called at merge time. It adds a new FieldMapperTestCase base class that
FieldMapper tests can extend, and moves the compatibility testing machinery from
FieldTypeTestCase to here.

Relates to #56814
@romseygeek romseygeek mentioned this issue Jun 4, 2020
Merged
romseygeek added a commit that referenced this issue Jun 15, 2020
@romseygeek
MappedFieldType should not extend FieldType (#57666)
3b69682 
MappedFieldType is a combination of two concerns:

* an extension of lucene's FieldType, defining how a field should be indexed
* a set of query factory methods, defining how a field should be searched

We want to break these two concerns apart. This commit is a first step to doing this, breaking
the inheritance relationship between MappedFieldType and FieldType. MappedFieldType 
instead has a series of boolean flags defining whether or not the field is searchable or 
aggregatable, and FieldMapper has a separate FieldType passed to its constructor defining 
how indexing should be done.

Relates to #56814
romseygeek added a commit to romseygeek/elasticsearch that referenced this issue Jun 16, 2020
@romseygeek
MappedFieldType should not extend FieldType (elastic#57666)
3d1f71e 
MappedFieldType is a combination of two concerns:

* an extension of lucene's FieldType, defining how a field should be indexed
* a set of query factory methods, defining how a field should be searched

We want to break these two concerns apart. This commit is a first step to doing this, breaking
the inheritance relationship between MappedFieldType and FieldType. MappedFieldType
instead has a series of boolean flags defining whether or not the field is searchable or
aggregatable, and FieldMapper has a separate FieldType passed to its constructor defining
how indexing should be done.

Relates to elastic#56814
@romseygeek romseygeek mentioned this issue Jun 16, 2020
Merged
romseygeek added a commit that referenced this issue Jun 16, 2020
@romseygeek
MappedFieldType should not extend FieldType (#58160)
12a3f6d 
MappedFieldType is a combination of two concerns:

* an extension of lucene's FieldType, defining how a field should be indexed
* a set of query factory methods, defining how a field should be searched

We want to break these two concerns apart. This commit is a first step to doing this, breaking
the inheritance relationship between MappedFieldType and FieldType. MappedFieldType
instead has a series of boolean flags defining whether or not the field is searchable or
aggregatable, and FieldMapper has a separate FieldType passed to its constructor defining
how indexing should be done.

Relates to #56814
@mfussenegger mfussenegger mentioned this issue Jun 22, 2020
Open
7 tasks
@javanna javanna added the Meta label Jun 25, 2020
@romseygeek
Copy link
Contributor Author

  • Make FieldMapper.fieldType() abstract, and don't require a MappedFieldType to be passed to the base class constructor. This will allow for late-binding of search fields, so that they can reference other field mappers.
  • Rename MappedFieldType to SearchField

I don't think these two are necessary any more; the latter renaming is just noise now, and we already have MappedFieldTypes that are separate from FieldMappers in runtime fields. So I think this can be closed as complete.

@javanna javanna added Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch and removed Team:Search Meta label for search team labels Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@romseygeek romseygeek

Labels
>enhancement Meta :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@javanna @romseygeek @elasticmachine