Skip to content
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

Add basic support for field aliases in index mappings. #31287

Merged
merged 7 commits into from
Jun 18, 2018

Conversation

jtibshirani
Copy link
Contributor

@jtibshirani jtibshirani commented Jun 13, 2018

Functionality covered in this PR:

  • Almost all parts of the search request support field aliases. In particular, aliases can be used in queries, aggregations, and sort fields, as well as when requesting docvalue_fields, suggestions, and highlights.
  • Field aliases can be used in the capabilities API.
  • If field names are provided as wildcards, field aliases will be matched in addition to concrete fields.
  • A concrete field can be added in one mappings update, and an alias added in a separate update.
  • If a concrete field type is updated, any aliases it has will always refer to the latest version.
  • Attempts to write to a field alias will cause document parsing to fail.

This PR has become quite large -- I tried to keep each commit in the PR well-scoped, but let me know if it would help to split this into smaller PRs.

@colings86 colings86 added the :Search Foundations/Mapping Index mappings, including merging and defining field types label Jun 13, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@jtibshirani jtibshirani force-pushed the feature/field-aliases branch 3 times, most recently from 5279839 to b08f5a4 Compare June 13, 2018 20:34
@jtibshirani jtibshirani force-pushed the feature/field-aliases branch 2 times, most recently from 4ca2f04 to c3cb62b Compare June 15, 2018 18:01
@jtibshirani jtibshirani changed the base branch from master to field-aliases June 15, 2018 20:12
@jtibshirani jtibshirani changed the title Add support for field aliases in index mappings. Add basic support for field aliases in index mappings. Jun 15, 2018
@jtibshirani jtibshirani force-pushed the feature/field-aliases branch 2 times, most recently from 9aed225 to 115f1c5 Compare June 15, 2018 22:25
@jtibshirani jtibshirani removed the WIP label Jun 16, 2018
@jtibshirani jtibshirani requested a review from jpountz June 16, 2018 01:11
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.

👍 I like the way it is done.

} else if (mapper instanceof FieldAliasMapper) {
throw new IllegalArgumentException("Cannot write to a field alias [" + mapper.name() + "].");
} else {
throw new IllegalStateException("The provided mapper [" + mapper.name() + "] has an unrecognized type.");
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add the class of the field to make debugging easier?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

import java.util.Iterator;
import java.util.Map;

public class FieldAliasMapper extends Mapper {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's make final and add some javadocs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Object pathField = node.remove(Names.PATH);
String path = XContentMapValues.nodeStringValue(pathField, null);
if (path == null) {
throw new MapperParsingException("The [path] property must be specified.");
Copy link
Contributor

Choose a reason for hiding this comment

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

let's add the field name in the error message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

String resolvedField = aliasToConcreteName.get(field);
return resolvedField == null
? fullNameToFieldType.get(field)
: fullNameToFieldType.get(resolvedField);
Copy link
Contributor

Choose a reason for hiding this comment

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

things would be a bit easier to read for me this way:

String resolvedField = aliasToConcreteName.getOrDefault(field, field);
return fullNameToFieldType.get(resolvedField);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, will change.

if (mapper instanceof RootObjectMapper) {
// root mapper isn't really an object mapper
} else if (mapper instanceof ObjectMapper) {
objectMappers.add((ObjectMapper)mapper);
} else if (mapper instanceof FieldMapper) {
fieldMappers.add((FieldMapper)mapper);
} else if (mapper instanceof FieldAliasMapper) {
fieldAliasMappers.add((FieldAliasMapper) mapper);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should add an else block that throws an exception like you added in other places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@jtibshirani jtibshirani force-pushed the feature/field-aliases branch 2 times, most recently from 67c17ad to 71a7820 Compare June 18, 2018 18:37
@jtibshirani
Copy link
Contributor Author

@elasticmachine run sample packaging tests

@jtibshirani jtibshirani merged commit 12c45fa into elastic:field-aliases Jun 18, 2018
@jtibshirani jtibshirani deleted the feature/field-aliases branch June 18, 2018 21:15
jtibshirani added a commit that referenced this pull request Jun 21, 2018
* Add basic support for field aliases through a new top-level mapper type.
* Add tests for queries, aggregations, sorting, and fetching doc values.
* Make sure we properly handle wildcard fields in query string queries.
* Allow for aliases when requesting suggestions.
* Allow for aliases when requesting highlights.
* Add a test for field capabilities.
jtibshirani added a commit that referenced this pull request Jun 21, 2018
* Add basic support for field aliases through a new top-level mapper type.
* Add tests for queries, aggregations, sorting, and fetching doc values.
* Make sure we properly handle wildcard fields in query string queries.
* Allow for aliases when requesting suggestions.
* Allow for aliases when requesting highlights.
* Add a test for field capabilities.
jtibshirani added a commit that referenced this pull request Jun 26, 2018
* Add basic support for field aliases through a new top-level mapper type.
* Add tests for queries, aggregations, sorting, and fetching doc values.
* Make sure we properly handle wildcard fields in query string queries.
* Allow for aliases when requesting suggestions.
* Allow for aliases when requesting highlights.
* Add a test for field capabilities.
jtibshirani added a commit that referenced this pull request Jul 4, 2018
* Add basic support for field aliases through a new top-level mapper type.
* Add tests for queries, aggregations, sorting, and fetching doc values.
* Make sure we properly handle wildcard fields in query string queries.
* Allow for aliases when requesting suggestions.
* Allow for aliases when requesting highlights.
* Add a test for field capabilities.
jtibshirani added a commit to jtibshirani/elasticsearch that referenced this pull request Jul 16, 2018
* Add basic support for field aliases through a new top-level mapper type.
* Add tests for queries, aggregations, sorting, and fetching doc values.
* Make sure we properly handle wildcard fields in query string queries.
* Allow for aliases when requesting suggestions.
* Allow for aliases when requesting highlights.
* Add a test for field capabilities.
jtibshirani added a commit that referenced this pull request Jul 17, 2018
* Add basic support for field aliases through a new top-level mapper type.
* Add tests for queries, aggregations, sorting, and fetching doc values.
* Make sure we properly handle wildcard fields in query string queries.
* Allow for aliases when requesting suggestions.
* Allow for aliases when requesting highlights.
* Add a test for field capabilities.
jtibshirani added a commit that referenced this pull request Jul 18, 2018
* Add basic support for field aliases through a new top-level mapper type.
* Add tests for queries, aggregations, sorting, and fetching doc values.
* Make sure we properly handle wildcard fields in query string queries.
* Allow for aliases when requesting suggestions.
* Allow for aliases when requesting highlights.
* Add a test for field capabilities.
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
Labels
>feature :Search Foundations/Mapping Index mappings, including merging and defining field types
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants