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
Fix include_in_all for multi field #5522
Conversation
@kzwang Good catch! The reason why I set I think we can make a different fix, which is similar to the approach in copyTo. In ParseContext we can add a |
@martijnvg yes, that's better. I'll modify the PR to do that |
@kzwang Great! |
@martijnvg I've updated the PR |
@@ -348,7 +361,7 @@ public void version(Field version) { | |||
} | |||
|
|||
public boolean includeInAll(Boolean includeInAll, FieldMapper mapper) { | |||
return includeInAll(includeInAll, mapper.fieldType().indexed()); | |||
return !isWithinMultiFields() && includeInAll(includeInAll, mapper.fieldType().indexed()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martijnvg fixed
Looks good! I left one comment. |
this.withinMultiFields = false; | ||
} | ||
|
||
public boolean isWithinMultiFields() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method can be removed since it isn't used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martijnvg removed this method
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. |
Thanks @kzwang your fix has been added to the master and 1.x branches |
Also backported it to 1.1 branch, so the fix will be in version |
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 theincludeInAll
tonull
, then it will be default totrue
ifindex
is not set tono
. I think it should be set tofalse
instead ofnull
.btw, seems
AllFieldMapper.IncludeInAll.unsetIncludeInAll()
will not be used anywhere after this change, should that be removed?closes #5364