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
Add transform to document before index. #6599
Conversation
Still needs documentation. I'm not super happy with how the transform is done - it turns the XContent into a Map them back into an XContent. Not perfect at all. Also, this has a bunch of TODOs, some of which deserve doing before merging, I imagine. |
=== Immutable Transformation | ||
Once configured the transform script cannot be modified. This is not | ||
because that is technically impossible but instead because madness lies | ||
down that road. |
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.
+1
Thanks @nik9000 this looks very good! API-wise, I think it would be nice to be prepared for other ways to do transformations in the future? Although scripts are powerful, it think we could have dedicated transformers for common transformations like concatenating two string fields or suming up all values of a numeric field (which probably means we also need to support several transformers?). |
-------------------------------------------------- | ||
{ | ||
"example" : { | ||
"transform" : "source['suggest'] = source['content']" |
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.
To be consistent with other scripts, I think we should always require the script
key ie:
"example": {
"transform": {
"script": "......"
}
}
This also leaves the path open for built-in transforms
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.
Will do.
As per @jpountz 's comment, we could keep the path open for built-in transforms later in a format like this perhaps:
|
Would it make sense to support either the "transform": [
{ "add": { "total_price": [ "price", "tax", "markup" ]}},
{ "concat": { "full_name": [ "first", "last" ]}},
{ "script": "......", "lang": "groovy", "params": {....}}
] or the "transform": { "script": "......", "lang": "groovy", "params": {....}} syntax? We do something like this in other places I think. |
Yes, that's what I actually meant. Currently we don't have the short syntax, but as long as the way is open to support both in the future, that's all we need. |
Almost done with @jpountz's code comment on copy-and paste-y-ness. Going to start on api next. It is good we're doing with with Groovy, BTW, because I tried a month ago to execute MVEL on every one of my updates and saw some real instability. Weird error message that I couldn't get in dev when doing the same things. |
We really want to be sure we're testing the real time get sometimes.
Documentation updates and code cleanup
I pushed a fix for the code duplication and rebased because it wasn't going to merge cleanly. Now to fix the syntax. |
Almost done with API - I've noticed the integration tests are starting much much much faster then a month ago. Neat! |
This allows for multiple transforms and should make it easy to implement non-script transforms in the future.
OK! Added @clintongormley's API reformat. I think that made it cleaner actually. I added the infrastructure for multiple transforms of different types but only implemented the script type. |
w00t! I've tagged it for review. thanks @nik9000 |
@@ -276,7 +290,25 @@ private DocumentMapper parse(String type, Map<String, Object> mapping, String de | |||
return documentMapper; | |||
} | |||
|
|||
@SuppressWarnings({"unchecked"}) | |||
private String getRemainingFields(Map<String, ? extends Object> map) { |
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.
This can just be Map<String, ?>
since Object is the base class.
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.
Yeah. Doing it.
I left two minor comments but in general this looks very good to me! This is an exciting feature. |
@jpountz: updated with fixes from comments. |
Thanks Nik, I'll merge it soon! |
Closes #6566