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

keep _parent field while updating child type mapping #24407

Merged
merged 3 commits into from
Jun 7, 2017

Conversation

PnPie
Copy link
Contributor

@PnPie PnPie commented Apr 29, 2017

Be able to update child type mapping without specifying it's _parent field, because it should be considered as unchanged if it is not explicitly specified.
In this case, the merged mapper's parentType field should be null, so we can just merge it to keep the original one instead of throwing an exception of "trying to change it to 'null' "

Close #23381

be able to update child type mapping without specifying _parent
field instead of trying to update it to "null" as previously.
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@clintongormley clintongormley added :Search Foundations/Mapping Index mappings, including merging and defining field types :Parent/Child >bug labels May 26, 2017
@clintongormley
Copy link
Contributor

@colings86 please could somebody review this

@PnPie PnPie changed the title Keep _parent field mapping while updating child mapping (#23381) keep _parent field mapping while updating child mapping May 26, 2017
@PnPie PnPie changed the title keep _parent field mapping while updating child mapping keep _parent field while updating child mapping May 26, 2017
@PnPie PnPie changed the title keep _parent field while updating child mapping keep _parent field while updating child type mapping May 26, 2017
@colings86 colings86 assigned martijnvg and unassigned colings86 May 30, 2017
Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

Thanks @PnPie! This looks good. I left two comments regarding the test.

.startObject("_parent").field("type", "new_parent").endObject()
.endObject().endObject().string();
DocumentMapper modParentMapper = parser.parse("child", new CompressedXContent(modParentMapping));
try {
Copy link
Member

Choose a reason for hiding this comment

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

Replace this try-catch block with an expectThrows(...) call?

Exception e = expectThrows(IllegalArgumentException.class, () ->  initMapper.merge(modParentMapper.mapping(), false))
assertThat(e.getMessage(), containsString("The _parent field's type option can't be changed:"));

.endObject().endObject().endObject().string();
DocumentMapper updatedMapper = parser.parse("child", new CompressedXContent(updatedMapping));
DocumentMapper mergedMapper = initMapper.merge(updatedMapper.mapping(), false);

Copy link
Member

Choose a reason for hiding this comment

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

Maybe also add a merge with current parent field and a new field?

String updatedMapping = XContentFactory.jsonBuilder().startObject().startObject("child")
   .startObject("_parent").field("type", "parent").endObject()
   .startObject("properties")
   .startObject("field2").field("type", "text").endObject()
   .endObject().endObject().endObject().string();
 DocumentMapper updatedMapper = parser.parse("child", new CompressedXContent(updatedMapping));
 DocumentMapper mergedMapper = initMapper.merge(updatedMapper.mapping(), false);

PnPie added 2 commits June 1, 2017 15:42
The parent field should not be updatable, but the it's fieldType is changed and the new one is applied. So this patch make it keep the original one while merging.
@PnPie
Copy link
Contributor Author

PnPie commented Jun 1, 2017

Thanks for your comments @martijnvg, I added a merge case with current parent field while I found a new problem: I cannot find the _parent field with this assertion didn't pass
assertThat(mergedMapper2.mappers().getMapper("_parent"), notNullValue());

And then I found that the parent field name of a document is in format: _parent#[parent type], so in the test it should be _parent#parent, if a new mapper without parent field, it's name is just _parent, and when we do the merge we firstly call the super.doMerge https://github.com/PnPie/elasticsearch/blob/346889a93bb18cda93736a8076aef198b8f86fe1/core/src/main/java/org/elasticsearch/index/mapper/ParentFieldMapper.java#L298 method of ParentFieldMapper and it always replaces the fieldType with the new one's, so in consequence the parent field name is changed to the new one's.

So previously after this we can find the _parent field with
assertThat(mergedMapper.mappers().getMapper("_parent"), notNullValue());
but in fact the parent field shouldn't be changed and it should be _parent#parent, so I modified in ParentFieldMapper's doMerge method and make it keep the original parent field name.

And I also add a test for trying to add parent field to an exiting mapper without parent.

@martijnvg
Copy link
Member

Cool, looks good @PnPie. I'll merge it in soon and backport to 5.x branch.

@martijnvg martijnvg added v5.6.0 and removed v5.5.0 labels Jun 7, 2017
@martijnvg martijnvg merged commit 14913fd into elastic:master Jun 7, 2017
jpountz added a commit to jpountz/elasticsearch that referenced this pull request Jan 15, 2018
A bug introduced in elastic#24407 currently prevents `eager_global_ordinals` from
being updated. This new approach should fix the issue while still allowing
mapping updates to not specify the `_parent` field if it doesn't need
updating, which was the goal of elastic#24407.
jpountz added a commit that referenced this pull request Jan 15, 2018
A bug introduced in #24407 currently prevents `eager_global_ordinals` from
being updated. This new approach should fix the issue while still allowing
mapping updates to not specify the `_parent` field if it doesn't need
updating, which was the goal of #24407.
jpountz added a commit that referenced this pull request Jan 15, 2018
A bug introduced in #24407 currently prevents `eager_global_ordinals` from
being updated. This new approach should fix the issue while still allowing
mapping updates to not specify the `_parent` field if it doesn't need
updating, which was the goal of #24407.
jpountz added a commit that referenced this pull request Jan 16, 2018
A bug introduced in #24407 currently prevents `eager_global_ordinals` from
being updated. This new approach should fix the issue while still allowing
mapping updates to not specify the `_parent` field if it doesn't need
updating, which was the goal of #24407.
jpountz added a commit that referenced this pull request Jan 16, 2018
A bug introduced in #24407 currently prevents `eager_global_ordinals` from
being updated. This new approach should fix the issue while still allowing
mapping updates to not specify the `_parent` field if it doesn't need
updating, which was the goal of #24407.
@clintongormley clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Parent/Child labels Feb 14, 2018
@javanna javanna removed the :Search/Search Search-related issues that do not fall into other categories label Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Search Foundations/Mapping Index mappings, including merging and defining field types v5.6.0 v6.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants