Skip to content

Conversation

@jtibshirani
Copy link
Contributor

@jtibshirani jtibshirani commented Feb 9, 2021

This field mapper only lived in its own module so it could be licensed as x-pack
basic. Now it can be moved to core, which matches its status as a core type.

This commit doesn't bring major code simplification yet, but it does improve
how the tests are factored.

This field mapper only lived in its own module so it could be licensed as x-pack
basic. Now it can be moved to core, which matches its status as a core type.
@jtibshirani jtibshirani added :Search Foundations/Mapping Index mappings, including merging and defining field types >refactoring v8.0.0 v7.12.0 labels Feb 9, 2021
@elasticmachine elasticmachine added the Team:Search Meta label for search team label Feb 9, 2021
@elasticmachine
Copy link
Collaborator

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

mappers.put(TextFieldMapper.CONTENT_TYPE, TextFieldMapper.PARSER);

mappers.put(FieldAliasMapper.CONTENT_TYPE, new FieldAliasMapper.TypeParser());
mappers.put(FlattenedFieldMapper.CONTENT_TYPE, FlattenedFieldMapper.PARSER);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I added FlattenedFieldMapper and tried to alphabetize the entries.

This is required because in previous versions, 'flattened' is not in core.
@jtibshirani
Copy link
Contributor Author

A couple notes:

  • I introduced a new package org.elasticsearch.index.mapper.flattened. This is not common practice yet, but felt appropriate for more complex like flattened which have various helper classes.
  • When moving flattened to its own module, we introduced an unnecessary abstraction DynamicKeyFieldMapper. I had planned to remove this while moving flattened back to core, but ended up keeping it. I now find the abstraction pretty clean, and it may be helpful for other field types we're considering like numeric flattened fields.

Copy link
Contributor

@mayya-sharipova mayya-sharipova left a comment

Choose a reason for hiding this comment

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

Thanks @jtibshirani, makes sense. +1 for a new package mapper.flattened.

One extra question I have, do we want to remove x-pack reference in docs/reference/mapping/types/flattened.asciidoc as well?

- skip:
version: " - 7.2.99"
reason: "Flattened fields were implemented in 7.3."
version: " - 7.99.99"
Copy link
Contributor

@mayya-sharipova mayya-sharipova Feb 10, 2021

Choose a reason for hiding this comment

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

Version change because some nodes may not have x-pack installed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These core REST tests generally do not run with x-pack installed. So I bumped the version here, and plan to adjust to cover 7.12 once it's backported.

@jtibshirani
Copy link
Contributor Author

do we want to remove x-pack reference in docs/reference/mapping/types/flattened.asciidoc as well?

I think it makes sense to remove this, I pushed a new commit.

Thanks for the review. Before merging I'd like to get more clarity on our overall design (for example when should a field type live in core vs. mapper-extras?) I'll loop back here with conclusions.

@jtibshirani
Copy link
Contributor Author

I caught up with some others, and the exact design of where different mappers should live is still up for discussion. However I think we can proceed with moving flattened to core -- it has benefits and makes more sense than having it in its own module.

@jtibshirani jtibshirani merged commit 796284a into elastic:master Mar 9, 2021
@jtibshirani jtibshirani deleted the flattened-fields branch March 9, 2021 00:56
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 Team:Search Meta label for search team v7.13.0 v8.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants