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.

Sign up for GitHub

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

Jump to bottom

Don't detect source's XContentType in DocumentParser.parseDocument() #26880

Merged
merged 2 commits into from
Oct 10, 2017
Merged

Don't detect source's XContentType in DocumentParser.parseDocument() #26880

merged 2 commits into from
Oct 10, 2017

Conversation

tlrx
Copy link
Member

@tlrx tlrx commented Oct 4, 2017

DocumentParser.parseDocument() auto detects the XContentType of the
document to parse, but this information is already provided by SourceToParse.

This PR also removes an unused parameter and avoids field names to be split twice in the getMapper() method.

@tlrx tlrx added :Search Foundations/Mapping Index mappings, including merging and defining field types >enhancement review v7.0.0 labels Oct 4, 2017
@cbuescher cbuescher self-assigned this Oct 5, 2017
cbuescher
cbuescher reviewed Oct 5, 2017
View reviewed changes
Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

@tlrx thanks, removing unnecessary xContent type detection and the unused atRoot flag looks good to me, I added a querstion about the use of the third change though.

core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java
@@ -481,14 +482,13 @@ private static void parseObjectOrField(ParseContext context, Mapper mapper) thro
private static void parseObject(final ParseContext context, ObjectMapper mapper, String currentFieldName) throws IOException {
assert currentFieldName != null;

Mapper objectMapper = getMapper(mapper, currentFieldName);
final String[] paths = splitAndValidatePath(currentFieldName);
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain what moving out the call to splitAndValidatePath() from getMapper() saves here? As far as I understand it it seems better to centralize this in getMapper, but I might miss something here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I came across this while I was debugging step-by-step this portion of code; I've been surprised that the field name was split and validated twice: one in getMapper() and another right after in the else condition. I can revert this if you prefer.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I understand this now. Makes sense, although it seems nice to hide the call to splitAndValidatePath in the getMapper method.

javanna
javanna approved these changes Oct 5, 2017
View reviewed changes
Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

good catch @tlrx I think that is a leftover of #22691. We already do auto-detection when we don't know the content-type (we don't have it in the translog) before calling this method, so we should not do auto-detection within the method.

cbuescher
cbuescher approved these changes Oct 7, 2017
View reviewed changes
Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

LGTM, I left a tiny suggestion and the current CI failure in PercolatorQuerySearchIT reproduces for me (but not on the previous commit) so checking that would also be good before merging.

core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java Outdated
@@ -929,8 +929,7 @@ private static void parseCopy(String field, ParseContext context) throws IOExcep
}

// looks up a child mapper, but takes into account field names that expand to objects
static Mapper getMapper(ObjectMapper objectMapper, String fieldName) {
String[] subfields = splitAndValidatePath(fieldName);
static Mapper getMapper(ObjectMapper objectMapper, String fieldName, String[] subfields) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: seems this can this be made private.

core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java
@@ -481,14 +482,13 @@ private static void parseObjectOrField(ParseContext context, Mapper mapper) thro
private static void parseObject(final ParseContext context, ObjectMapper mapper, String currentFieldName) throws IOException {
assert currentFieldName != null;

Mapper objectMapper = getMapper(mapper, currentFieldName);
final String[] paths = splitAndValidatePath(currentFieldName);
Copy link
Member

Choose a reason for hiding this comment

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

Okay, I understand this now. Makes sense, although it seems nice to hide the call to splitAndValidatePath in the getMapper method.

@tlrx
Don't detect source's XContentType in DocumentParser.parseDocument()
11d71d1 
DocumentParser.parseDocument() auto detects the XContentType of the
document to parse, but this information is already provided by SourceToParse.
@tlrx
Fix PercolatorQuerySearchIT
af7e2d9
@tlrx tlrx force-pushed the use-contenttype-in-doc-parser branch from 6ee200e to af7e2d9 Compare October 10, 2017 11:46
@tlrx tlrx merged commit 6658ff0 into elastic:master Oct 10, 2017
tlrx added a commit that referenced this pull request Oct 10, 2017
@tlrx
Don't detect source's XContentType in DocumentParser.parseDocument() (#…
5e5670c 
…26880)

DocumentParser.parseDocument() auto detects the XContentType of the
document to parse, but this information is already provided by SourceToParse.
@tlrx tlrx added the v6.1.0 label Oct 10, 2017
tlrx added a commit that referenced this pull request Oct 10, 2017
@tlrx
Don't detect source's XContentType in DocumentParser.parseDocument() (#…
004c409 
…26880)

DocumentParser.parseDocument() auto detects the XContentType of the
document to parse, but this information is already provided by SourceToParse.
@tlrx
Copy link
Member Author

tlrx commented Oct 10, 2017

Thanks @javanna and @cbuescher. This has been backported to 6.0 and 6.x.

@tlrx tlrx added the v6.0.0 label Oct 10, 2017
@tlrx tlrx deleted the use-contenttype-in-doc-parser branch October 10, 2017 13:48
jasontedor added a commit that referenced this pull request Oct 12, 2017
@jasontedor
Merge branch 'master' into ccr
7c1bcfc 
* master: (35 commits)
  Create weights lazily in filter and filters aggregation (#26983)
  Use a dedicated ThreadGroup in rest sniffer (#26897)
  Fire global checkpoint sync under system context
  Update by Query is modified to accept short `script` parameter. (#26841)
  Cat shards bytes (#26952)
  Add support for parsing inline script (#23824) (#26846)
  Change default value to true for transpositions parameter of fuzzy query (#26901)
  Adding unreleased 5.6.4 version number to Version.java
  Rename TCPTransportTests to TcpTransportTests (#26954)
  Fix NPE for /_cat/indices when no primary shard (#26953)
  [DOCS] Fixed indentation of the definition list.
  Fix formatting in channel close test
  Check for closed connection while opening
  Clarify systemd overrides
  [DOCS] Plugin Installation for Windows (#21671)
  Painless: add tests for cached boxing (#24163)
  Don't detect source's XContentType in DocumentParser.parseDocument() (#26880)
  Fix handling of paths containing parentheses
  Allow only a fixed-size receive predictor (#26165)
  Add Homebrew instructions to getting started
  ...
jasontedor added a commit that referenced this pull request Oct 12, 2017
@jasontedor
Merge branch '6.x' into ccr-6.x
b6227db 
* 6.x: (32 commits)
  Use a dedicated ThreadGroup in rest sniffer (#26897)
  Fire global checkpoint sync under system context
  Update by Query is modified to accept short `script` parameter. (#26841)
  Cat shards bytes (#26952)
  Adding unreleased 5.6.4 version number to Version.java
  Rename TCPTransportTests to TcpTransportTests (#26954)
  Fix NPE for /_cat/indices when no primary shard (#26953)
  [DOCS] Fixed indentation of the definition list.
  Check for closed connection while opening
  Clarify systemd overrides
  [DOCS] Plugin Installation for Windows (#21671)
  Painless: add tests for cached boxing (#24163)
  Don't detect source's XContentType in DocumentParser.parseDocument() (#26880)
  Return List instead of an array from settings (#26903)
  Emit deprecation warning for variable size predictor
  Fix handling of paths containing parentheses
  Deprecate variable-size receive predictor
  Add Homebrew instructions to getting started
  ingest: Fix bug that prevent date_index_name processor from accepting timestamps specified as a json number
  Scripting: Fix expressions to temporarily support filter scripts (#26824)
  ...
@lcawl lcawl added v6.0.0-rc2 and removed v6.0.0 labels Oct 30, 2017
@lcawl lcawl removed the v6.1.0 label Dec 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javanna javanna javanna approved these changes

@cbuescher cbuescher cbuescher approved these changes

Assignees

@cbuescher cbuescher

Labels
>enhancement :Search Foundations/Mapping Index mappings, including merging and defining field types v6.0.0-rc2 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tlrx @javanna @cbuescher @colings86 @lcawl