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

Don't expose TextFieldMapper subfields #64597

Merged
merged 37 commits into from
Nov 5, 2020

Conversation

romseygeek
Copy link
Contributor

@romseygeek romseygeek commented Nov 4, 2020

TextFieldMapper can optionally index data into subfields for accelerated
prefix and phrase queries. Currently, these subfields are implemented
as FieldMappers in their own right, made available via TextFieldMapper's
iterator() method and with their own standalone MappedFieldType objects.

This has the disadvantage that these subfields are directly available for
searching, and appear in APIs such as field caps. In addition, because
exists queries are not implemented on them, an exists query against an
object which contains a text field with one of the subfields enabled can
throw an error (see #63585).

This commit reworks the subfields so that they are no longer implemented
as FieldMappers, and are no longer exposed to classes outside
TextFieldMapper either as MappedFieldTypes or as FieldMappers. The
parent TextFieldMapper handles indexing and analyzer registration,
PhraseFieldType is removed entirely, and PrefixFieldType is retained as
a private implementation for fast prefix queries but is unavailable for
querying directly.

Fixes #63585
Closes #63446
Closes #64585

romseygeek and others added 30 commits October 20, 2020 14:00
@romseygeek romseygeek added >bug >breaking :Search Foundations/Mapping Index mappings, including merging and defining field types >refactoring v8.0.0 v7.11.0 labels Nov 4, 2020
@romseygeek romseygeek self-assigned this Nov 4, 2020
@elasticmachine
Copy link
Collaborator

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

@elasticmachine elasticmachine added the Team:Search Meta label for search team label Nov 4, 2020
@romseygeek
Copy link
Contributor Author

I've marked this as >breaking as it means that you'll no longer be able to search directly on the indexed subfields, but I think that's a good thing so I don't think we need to worry about backwards compatibility here.

Copy link
Contributor

@jtibshirani jtibshirani left a comment

Choose a reason for hiding this comment

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

This makes sense to me!

@@ -330,7 +329,7 @@ private TextFieldType buildFieldType(FieldType fieldType, BuilderContext context
return ft;
}

private PrefixFieldMapper buildPrefixMapper(BuilderContext context, FieldType fieldType, TextFieldType tft) {
private SubFieldInfo buildPrefixMapper(BuilderContext context, FieldType fieldType, TextFieldType tft) {
Copy link
Contributor

Choose a reason for hiding this comment

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

buildPrefixMapper -> buildPrefixInfo

@@ -407,12 +405,18 @@ private PhraseFieldMapper buildPhraseMapper(FieldType fieldType, TextFieldType p
public TextFieldMapper build(BuilderContext context) {
FieldType fieldType = TextParams.buildFieldType(index, store, indexOptions, norms, termVectors);
TextFieldType tft = buildFieldType(fieldType, context);
PhraseFieldMapper phraseFieldMapper = buildPhraseMapper(fieldType, tft);
PrefixFieldMapper prefixFieldMapper = buildPrefixMapper(context, fieldType, tft);
SubFieldInfo phraseFieldMapper = buildPhraseInfo(fieldType, tft);
Copy link
Contributor

Choose a reason for hiding this comment

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

We could switch to the *Info naming here too,

@romseygeek romseygeek merged commit cea93d1 into elastic:master Nov 5, 2020
@romseygeek romseygeek deleted the mapper/text-subfields branch November 5, 2020 13:16
romseygeek added a commit that referenced this pull request Nov 5, 2020
TextFieldMapper can optionally index data into subfields for accelerated
prefix and phrase queries. Currently, these subfields are implemented
as FieldMappers in their own right, made available via TextFieldMapper's
iterator() method and with their own standalone MappedFieldType objects.

This has the disadvantage that these subfields are directly available for
searching, and appear in APIs such as field caps. In addition, because
exists queries are not implemented on them, an exists query against an
object which contains a text field with one of the subfields enabled can
throw an error (see #63585).

This commit reworks the subfields so that they are no longer implemented
as FieldMappers, and are no longer exposed to classes outside
TextFieldMapper either as MappedFieldTypes or as FieldMappers. The
parent TextFieldMapper handles indexing and analyzer registration,
PhraseFieldType is removed entirely, and PrefixFieldType is retained as
a private implementation for fast prefix queries but is unavailable for
querying directly.

Fixes #63585
Closes #63446
romseygeek added a commit to romseygeek/elasticsearch that referenced this pull request Jan 13, 2021
romseygeek added a commit that referenced this pull request Jan 13, 2021
romseygeek added a commit that referenced this pull request Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking-java >bug >refactoring :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Meta label for search team v7.11.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exists query fail on text field with sub-fields option Text fields should not expose internal field types
5 participants