-
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
[Transform] avoid mapping problems with index templates #51368
[Transform] avoid mapping problems with index templates #51368
Conversation
… with index templates fixes elastic#51321
Pinging @elastic/ml-core (:ml/Transform) |
* | ||
* @param fieldMappings field mappings to inject to | ||
*/ | ||
static void insertNestedObjectMappings(Map<String, String> fieldMappings) { |
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.
essential change in this file, called on line 176
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.
👍 thanks for differentiating the code changes from the formatting changes
run elasticsearch-ci/packaging-sample-matrix-unix |
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.
Left a comment about a comment
fieldMappings.keySet().stream().filter(key -> key.contains(".")).forEach(key -> { | ||
int pos; | ||
String objectKey = key; | ||
// lastIndexOf returns -1 on mismatch, but to disallow empty strings check for > 0 |
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.
An empty string would also return -1. Do you mean disallow strings containing a single '.' that is at the beginning? Wouldn't that be an error case anyway?
I ran a quick check in jshell:
jshell> "".lastIndexOf('.')
$3 ==> -1
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.
yes, when I worked on this PR, I discovered that you could create fields with '.' at start or end, which did not work properly (e.g. bulk indexing errors). I fixed it in #51369.
So, you are right: this code is "defense in depth".
* | ||
* @param fieldMappings field mappings to inject to | ||
*/ | ||
static void insertNestedObjectMappings(Map<String, String> fieldMappings) { |
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.
👍 thanks for differentiating the code changes from the formatting changes
insert explict mappings for objects in nested output to avoid clashes with index templates fixes elastic#51321
insert explict mappings for objects in nested output to avoid clashes with index templates
fixes #51321