Skip to content

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.

4 participants