Skip to content

Convert KeywordFieldMapper to parametrized form #60645

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
merged 7 commits into from
Aug 12, 2020

Conversation

romseygeek
Copy link
Contributor

This makes KeywordFieldMapper extend ParametrizedFieldMapper, with explicitly
defined parameters.

In addition, we add a new option to Parameter, restrictedStringParam, which
accepts a restricted set of string options.

@romseygeek romseygeek added :Search Foundations/Mapping Index mappings, including merging and defining field types >refactoring v8.0.0 v7.10.0 labels Aug 4, 2020
@romseygeek romseygeek self-assigned this Aug 4, 2020
@elasticmachine
Copy link
Collaborator

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

@elasticmachine elasticmachine added the Team:Search Meta label for search team label Aug 4, 2020
Map<String, Object> fieldTypeConfig = dynamicTemplate.mappingForName("__dummy__", defaultDynamicType);
fieldTypeConfig.remove("type");
String templateName = "__dynamic__" + dynamicTemplate.name();
Map<String, Object> fieldTypeConfig = dynamicTemplate.mappingForName(templateName, defaultDynamicType);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This yields better error messages for dynamic mappings, including the name of the dynamic mapping itself and the type of the dynamically created field.

MapperParsingException e = expectThrows(MapperParsingException.class,
() -> TypeParsers.parseField(builder, builder.name, mapping, parserContext));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

KeywordFieldMapper.Builder doesn't match the signature of parseField anymore (because we don't actually use it in parsing), so I've adjusted these to directly test the parseMeta method.

@romseygeek
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/docs

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.

@romseygeek Thanks, the PR overall LGTM, I just left a couple of comments.

@@ -92,149 +83,134 @@ public KeywordField(String field, BytesRef term) {

}

public static class Builder extends FieldMapper.Builder<Builder> {
private static KeywordFieldMapper toType(FieldMapper in) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lines 69-71 and 80-82 seem to be unused

Copy link
Contributor Author

Choose a reason for hiding this comment

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

++ they are, will remove

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 @romseygeek, the PR LGTM

@romseygeek
Copy link
Contributor Author

@elasticmachine update branch

@romseygeek romseygeek merged commit 2ef5902 into elastic:master Aug 12, 2020
romseygeek added a commit that referenced this pull request Aug 12, 2020
This makes KeywordFieldMapper extend ParametrizedFieldMapper, with explicitly
defined parameters.

In addition, we add a new option to Parameter, restrictedStringParam, which
accepts a restricted set of string options.
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.10.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants