-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Don't detect source's XContentType in DocumentParser.parseDocument() #26880
Conversation
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.
@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.
@@ -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); |
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.
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.
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.
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.
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.
Okay, I understand this now. Makes sense, although it seems nice to hide the call to splitAndValidatePath in the getMapper method.
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.
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.
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.
@@ -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) { |
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.
nit: seems this can this be made private.
@@ -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); |
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.
Okay, I understand this now. Makes sense, although it seems nice to hide the call to splitAndValidatePath in the getMapper method.
DocumentParser.parseDocument() auto detects the XContentType of the document to parse, but this information is already provided by SourceToParse.
6ee200e
to
af7e2d9
Compare
…26880) DocumentParser.parseDocument() auto detects the XContentType of the document to parse, but this information is already provided by SourceToParse.
…26880) DocumentParser.parseDocument() auto detects the XContentType of the document to parse, but this information is already provided by SourceToParse.
Thanks @javanna and @cbuescher. This has been backported to 6.0 and 6.x. |
* 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 ...
* 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) ...
DocumentParser.parseDocument()
auto detects theXContentType
of thedocument 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.