-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Allow specify dynamic templates in bulk request #69948
Conversation
07aa9d1
to
6e80821
Compare
Pinging @elastic/es-search (Team:Search) |
e346e5a
to
40e6a57
Compare
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 left some questions. The approach looks good code-wise overall, my comments are mostly about the API.
Another one: does the matched "type" become the content of the dynamic_type variable? Or should it even? I am wondering myself if this should be considered and promoted as a type. To me it is more of an arbitrary name that defined dynamic templates can match against. Should we rather call it match_mapping_hint
?
rest-api-spec/src/main/resources/rest-api-spec/test/bulk/11_type_hints.yml
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/DynamicTemplate.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/DynamicTemplate.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/termvectors/TermVectorsService.java
Outdated
Show resolved
Hide resolved
@javanna Thank you for the review. I tweaked the API using "match_mapping_hint" and I found it's simpler and less confusing. I have updated the PR. Can you take another look? |
@dnhatn apologies if this is obvious: will this be immediately usable through an Ingest script processor (and if so how?), or would that come later? |
That's a good question. Let's discuss it after this iteration. |
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.
The change looks great @dnhatn ! I left some comments but I think the feature is ready.
server/src/main/java/org/elasticsearch/index/mapper/ParseContext.java
Outdated
Show resolved
Hide resolved
rest-api-spec/src/main/resources/rest-api-spec/test/bulk/11_type_hints.yml
Outdated
Show resolved
Hide resolved
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 left some more comments, nothing major though. I like how simple the implementation, thanks for iterating on it. Would it make sense to add docs as part of this PR or do you plan to do it as a followup?
rest-api-spec/src/main/resources/rest-api-spec/test/bulk/11_type_hints.yml
Outdated
Show resolved
Hide resolved
rest-api-spec/src/main/resources/rest-api-spec/test/bulk/11_type_hints.yml
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/index/IndexRequest.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/ParseContext.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/SourceToParse.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/index/mapper/DynamicTemplateTests.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/index/IndexRequest.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/bulk/BulkRequestParser.java
Outdated
Show resolved
Hide resolved
server/src/internalClusterTest/java/org/elasticsearch/index/mapper/DynamicMappingIT.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Adrien Grand <jpountz@gmail.com>
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 left some minor comments, LGTM otherwise. Thanks for all the iterations.
server/src/internalClusterTest/java/org/elasticsearch/index/mapper/DynamicMappingIT.java
Show resolved
Hide resolved
Thanks everyone :). |
Relates elastic#69948
This change exposes the newly introduced parameter `dynamic_templates` in ingest. This parameter can be set by a set processor or a script processor. Relates #69948
This change exposes the newly introduced parameter dynamic_templates in ingest. This parameter can be set by a set processor or a script processor. Relates #69948
This change allows users to specify dynamic templates in a bulk request.
Closes #61939