Skip to content

Add not_analyzed to NonStringIndexOption and make it the default option #2026

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 1 commit into from
Apr 12, 2016

Conversation

gmarz
Copy link
Contributor

@gmarz gmarz commented Apr 12, 2016

This PR is questionable because it changes the default behavior of Index(), which previously defaulted to no, but now will default to not_analyzed.

@Mpdreamz @russcam thoughts?

Closes #1979

@gmarz gmarz added the Review label Apr 12, 2016
@russcam
Copy link
Contributor

russcam commented Apr 12, 2016

It is changing behaviour, albeit correcting an original issue 😄

The change is from

  • no -
    Do not add this field value to the index. With this setting, the field will not be queryable.

to

  • not_analyzed -
    Add the field value to the index unchanged, as a single term. This is the default for all fields that support this option except for string fields. not_analyzed fields are usually used with term-level queries for structured search.

So consumers won't be losing functionality by the default change but could potentially be storing more data than they did before. Not a bad thing per se.

I think this is a reasonable behavioural change that implements the right default according to the docs so LGTM. We just need to ensure that we highlight it particularly in the release notes.

It'd also be worth adding the descriptions to the XML comments for the NonStringIndexOption enum

@Mpdreamz
Copy link
Member

LGTM 👍 in 5.0 we need to not have any argument defaults for this one.

@gmarz gmarz merged commit 694b366 into 2.x Apr 12, 2016
@gmarz gmarz deleted the fix/1979 branch April 12, 2016 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants