Fix include_in_all for multi field

[kzwang]

As in document http://www.elasticsearch.org/guide/en/elasticsearch/reference/current/mapping-core-types.html#_include_in_all

The include_in_all should be ignored for multi-field. Currently it will set the includeInAll to null, then it will be default to true if index is not set to no. I think it should be set to false instead of null.

btw, seems AllFieldMapper.IncludeInAll.unsetIncludeInAll() will not be used anywhere after this change, should that be removed?

closes #5364

[martijnvg]

@kzwang Good catch! The reason why I set includeInAll to null, is to not serialise the include_in_all at all if it isn't used. This makes the mapping / cluster state smaller.

I think we can make a different fix, which is similar to the approach in copyTo. In ParseContext we can add a withinMultiFields boolean field and if that is set then the ParseContext#includeInAll(Boolean,boolean) return false. The withinMultiFields boolean field should be set to true in the AbstractFieldMapper.MultiFields#parse(ABF, ParseContext) and be set back to false once it exits that method.

[kzwang]

@martijnvg yes, that's better. I'll modify the PR to do that

[martijnvg]

@kzwang Great!

[kzwang]

@martijnvg I've updated the PR

[martijnvg]

Instead of this line can add the following to the method being (includeInAll with the two booleans) invoked here?

if (withinMultiFields) {
   return false;
}

Then it will be consistent with withinCopyTo.

[kzwang]

@martijnvg fixed

[martijnvg]

Looks good! I left one comment.

[martijnvg]

This method can be removed since it isn't used.

[kzwang]

@martijnvg removed this method

[martijnvg]

I left more one comment, you wan to change that? Other then that it looks good to me and I'll merge it in then.

[martijnvg]

Thanks @kzwang your fix has been added to the master and 1.x branches

[martijnvg]

Also backported it to 1.1 branch, so the fix will be in version 1.1.1 as well.