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

Deprecate string in favor of text/keyword. #16877

Merged
merged 1 commit into from
Mar 3, 2016

Conversation

jpountz
Copy link
Contributor

@jpountz jpountz commented Mar 1, 2016

This commit removes the ability to use string fields on indices created on or
after 5.0. Dynamic mappings now generate text fields by default for strings
but there are plans to also add a sub keyword field (in a future PR).

Most of the changes in this commit are just about replacing string with
keyword or text. Some tests have been removed because they existed because of
corner cases of string mappings like setting ignore-above on a text field or
enabling term vectors on a keyword field which are now impossible.

The plan is to remove strings entirely in 6.0.

@clintongormley clintongormley added the :Search Foundations/Mapping Index mappings, including merging and defining field types label Mar 2, 2016
@jpountz
Copy link
Contributor Author

jpountz commented Mar 2, 2016

@rjernst could you have a look?

* @param dynamicType the field type to give the field if the template does not define one
* @param matchType the type of the field in the json document or null if unknown
* @return a mapper builder, or null if there is no template for such a field
*/
public Mapper.Builder findTemplateBuilder(ParseContext context, String name, String dynamicType, String matchType) {
Copy link
Member

Choose a reason for hiding this comment

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

Are there any calls to this version of findTemplateBuilder with matchType string? Or findTemplate below? very confusing how we have so many public variants of this method...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed this should be made simpler.

@rjernst
Copy link
Member

rjernst commented Mar 2, 2016

LGTM, just one comment which could be addressed in a followup.

This commit removes the ability to use string fields on indices created on or
after 5.0. Dynamic mappings now generate text fields by default for strings
but there are plans to also add a sub keyword field (in a future PR).

Most of the changes in this commit are just about replacing string with
keyword or text. Some tests have been removed because they existed because of
corner cases of string mappings like setting ignore-above on a text field or
enabling term vectors on a keyword field which are now impossible.

The plan is to remove strings entirely in 6.0.
@jpountz jpountz force-pushed the deprecate/string branch from 066b56f to eef19be Compare March 3, 2016 09:21
@jpountz jpountz merged commit eef19be into elastic:master Mar 3, 2016
@dakrone
Copy link
Member

dakrone commented Mar 3, 2016

@jpountz I think if this removes string type and throws warnings, it should be marked as breaking and notes added to the migration guide, right?

@jpountz jpountz deleted the deprecate/string branch March 3, 2016 17:43
@jpountz
Copy link
Contributor Author

jpountz commented Mar 3, 2016

I added the breaking label. Regarding documentation, I'm keeping it for later so that the docs can deliver a consistent message about #12394 (I have not documented the text and keyword fields either yet).

@dakrone
Copy link
Member

dakrone commented Mar 3, 2016

Regarding documentation, I'm keeping it for later so that the docs can deliver a consistent message about #12394 (I have not documented the text and keyword fields either yet).

Makes sense, I just don't want to forget it! :)

@epixa
Copy link
Contributor

epixa commented Mar 3, 2016

With regard to documentation/messaging for this, I just want to underscore how impactful this break can be, so we should make sure to highlight it as clearly as possible when we launch 5.0.

This broke the mappings in Kibana, for example, so all CI builds are breaking and Kibana is uninstallable. We can fix master by updating our mappings for the .kibana index, but in order to support users upgrading their installs after the release, we need to either build into Kibana 5.0 an internal re-indexing feature or change Kibana to use versioned indexes behind a .kibana alias (which is our current consensus).

@rjernst
Copy link
Member

rjernst commented Mar 3, 2016

@epixa This change is backwards compatible for existing indexes, and users will already need to reindex in order to take advantage of the numeric changes that will also happen in 5.0 (which come with Lucene 6). This only affected kibana tests since it creates new indexes, so I don't see it as a problem?

@Bargs
Copy link

Bargs commented Mar 3, 2016

I've come across a problem while trying to update the default mappings for the .kibana index.

The current breaking changes guide states:

On all types but string, the index property now only accepts true/false instead of not_analyzed/no. The string field still accepts analyzed/not_analyzed/no

After switching our string field to text, it will now only accept true/false for the index property. How do I get an indexed but not_analyzed text field?

@rjernst
Copy link
Member

rjernst commented Mar 3, 2016

@Bargs That is what the new keyword type is for, see #16589 for the PR which added it, and #12394 for the meta issue describing the split of string to keyword and text.

@Bargs
Copy link

Bargs commented Mar 3, 2016

Thanks @rjernst, just the info I needed!

@rashidkpc
Copy link

Yeah, we're going to need to build out a reindexing-on-upgrade system in Kibana to support this correctly. We create mappings for Kibana objects on demand, and this is going to cause problems.

For example, if a user has saved dashboards and visualizations, but they've never saved a search they'll have the dashboard and visualization title field set to string. If they then try to save a search we'll try to create a mapping for the search type and that will fail due to a conflict across types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking >deprecation :Search Foundations/Mapping Index mappings, including merging and defining field types v5.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants