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

Moved dynamic field handling in doc parsing to end of parsing #16798

Merged
merged 7 commits into from Mar 10, 2016

Conversation

Projects
None yet
3 participants
@rjernst
Member

rjernst commented Feb 24, 2016

Currently dynamic mappings propgate through call semantics, where deeper
dynamic mappings are merged into higher level mappings through
return values of recursive method calls. This makese it tricky
to handle multiple updates in the same method, for example when
trying to create parent object mappers dynamically for a field name
that contains dots.

This change makes the api for adding mappers a simple list
of new mappers, and moves construction of the root level mapping
update to the end of doc parsing.

Mapping: Moved dynamic field handling in doc parsing to end of parsing
Currently dynamic mappings propgate through call semantics, where deeper
dynamic mappings are merged into higher level mappings through
return values of recursive method calls. This makese it tricky
to handle multiple updates in the same method, for example when
trying to create parent object mappers dynamically for a field name
that contains dots.

This change makes the api for adding mappers a simple list
of new mappers, and moves construction of the root level mapping
update to the end of doc parsing.
public class SimpleObjectMappingTests extends ESSingleNodeTestCase {
public void testDifferentInnerObjectTokenFailure() throws Exception {
String mapping = XContentFactory.jsonBuilder().startObject().startObject("type")
.endObject().endObject().string();
DocumentMapper defaultMapper = createIndex("test").mapperService().documentMapperParser().parse("type", new CompressedXContent(mapping));
try {
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> {

This comment has been minimized.

@jpountz

jpountz Feb 26, 2016

Contributor

+1

@jpountz

jpountz Feb 26, 2016

Contributor

+1

@jpountz

This comment has been minimized.

Show comment
Hide comment
@jpountz

jpountz Feb 26, 2016

Contributor

Looks good in general. My only concern would be about the creation of te mapping update at the root level.

Contributor

jpountz commented Feb 26, 2016

Looks good in general. My only concern would be about the creation of te mapping update at the root level.

@clintongormley clintongormley changed the title from Mapping: Moved dynamic field handling in doc parsing to end of parsing to Moved dynamic field handling in doc parsing to end of parsing Feb 29, 2016

@rjernst

This comment has been minimized.

Show comment
Hide comment
@rjernst

rjernst Mar 9, 2016

Member

@jpountz I pushed an update. While working on simplifying creating the root update, I found the algorithm ended up allowing many fields in the update which were not actually changed from the original mapping. For example, if you have a.b and a.x existing, and you add a.c, a.x would be in the update. In order to handle these cases, I needed to only add objects to the stack once an update has been created. This means we must create a temporary stack if there are intermediate objects needed for a dynamic mapper, and this turns out to be exactly what we need in order to create the root update. I think it looks simpler now, although a little less efficient (but correctness outweighs efficiency!).

Member

rjernst commented Mar 9, 2016

@jpountz I pushed an update. While working on simplifying creating the root update, I found the algorithm ended up allowing many fields in the update which were not actually changed from the original mapping. For example, if you have a.b and a.x existing, and you add a.c, a.x would be in the update. In order to handle these cases, I needed to only add objects to the stack once an update has been created. This means we must create a temporary stack if there are intermediate objects needed for a dynamic mapper, and this turns out to be exactly what we need in order to create the root update. I think it looks simpler now, although a little less efficient (but correctness outweighs efficiency!).

Mapper intermediate = previousIntermediate.getMapper(nameParts[i]);
assert intermediate instanceof ObjectMapper;
parentMappers.add((ObjectMapper)intermediate);
previousIntermediate = (ObjectMapper)intermediate;

This comment has been minimized.

@jpountz

jpountz Mar 10, 2016

Contributor

how is it supposed to work if the intermediate parent does not exist? I tried this PR agains the following document (mappings are initially empty):

PUT test/test/1
{
  "a.b": 3
}

and got the below exception:

[elasticsearch] Caused by: java.lang.NullPointerException
[elasticsearch]     at org.elasticsearch.index.mapper.DocumentParser.addToLastMapper(DocumentParser.java:295)
[elasticsearch]     at org.elasticsearch.index.mapper.DocumentParser.createUpdate(DocumentParser.java:314)
[elasticsearch]     at org.elasticsearch.index.mapper.DocumentParser.createDynamicUpdate(DocumentParser.java:230)
[elasticsearch]     at org.elasticsearch.index.mapper.DocumentParser.parseDocument(DocumentParser.java:105)
[elasticsearch]     at org.elasticsearch.index.mapper.DocumentMapper.parse(DocumentMapper.java:286)
@jpountz

jpountz Mar 10, 2016

Contributor

how is it supposed to work if the intermediate parent does not exist? I tried this PR agains the following document (mappings are initially empty):

PUT test/test/1
{
  "a.b": 3
}

and got the below exception:

[elasticsearch] Caused by: java.lang.NullPointerException
[elasticsearch]     at org.elasticsearch.index.mapper.DocumentParser.addToLastMapper(DocumentParser.java:295)
[elasticsearch]     at org.elasticsearch.index.mapper.DocumentParser.createUpdate(DocumentParser.java:314)
[elasticsearch]     at org.elasticsearch.index.mapper.DocumentParser.createDynamicUpdate(DocumentParser.java:230)
[elasticsearch]     at org.elasticsearch.index.mapper.DocumentParser.parseDocument(DocumentParser.java:105)
[elasticsearch]     at org.elasticsearch.index.mapper.DocumentMapper.parse(DocumentMapper.java:286)

This comment has been minimized.

@rjernst

rjernst Mar 10, 2016

Member

This PR is not meant to handle dots in fields, only to prepare for it. I'm surprised the code got there though...seems like the current check for dots in field names may be happening in the wrong place? I thought it was in mapper creation..

@rjernst

rjernst Mar 10, 2016

Member

This PR is not meant to handle dots in fields, only to prepare for it. I'm surprised the code got there though...seems like the current check for dots in field names may be happening in the wrong place? I thought it was in mapper creation..

This comment has been minimized.

@jpountz

jpountz Mar 10, 2016

Contributor

The check happens in ObjectMapper.parseProperties so that would be triggered later.

@jpountz

jpountz Mar 10, 2016

Contributor

The check happens in ObjectMapper.parseProperties so that would be triggered later.

@jpountz

This comment has been minimized.

Show comment
Hide comment
@jpountz

jpountz Mar 10, 2016

Contributor

LGTM

Contributor

jpountz commented Mar 10, 2016

LGTM

rjernst added a commit that referenced this pull request Mar 10, 2016

Merge pull request #16798 from rjernst/dots2
Moved dynamic field handling in doc parsing to end of parsing

@rjernst rjernst merged commit f3195cb into elastic:master Mar 10, 2016

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@rjernst rjernst deleted the rjernst:dots2 branch Mar 10, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment