-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Scripting: JSON parsing and writing in watcher #63278
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
Conversation
…earch into honza-painless-json
|
Pinging @elastic/es-core-features (:Core/Features/Watcher) |
|
Pinging @elastic/es-core-infra (:Core/Infra/Scripting) |
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. Would prefer @rjernst look too since I wrote a few of the tests.
|
@stu-elastic Thanks for separating the docs out into a different PR. Still LGTM. |
|
@elasticmachine run elasticsearch-ci/2 |
rjernst
left a comment
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 looks good, but we also need to consider how this is documented for users. There should be something in the painless reference or watcher docs?
modules/lang-painless/src/main/java/org/elasticsearch/painless/api/Json.java
Show resolved
Hide resolved
rjernst
left a comment
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
|
@elasticmachine run elasticsearch-ci/2 |
Co-authored-by: Honza Král Co-authored-by: Jack Conradson
|
Hello @stu-elastic ! Is there any chance that same functionality would be added to other painless context beside watcher? |
|
Seems weird just to have it in Watcher. Specifically I was hoping to use it in painless execute API to allow unit testing of the result. Current behaviour prints out a Java .toString which is pretty impossible to parse correctly in clients. |
|
Hi @agone1 and @djmcgreal-cc. We're being cautious about rolling out this feature in other contexts because parsing JSON can create a lot of objects leading to GC pressure if it's run in, say, an aggregation. @agone1 What contexts are you interested in? Can you cut a new issue for discussion? @djmcgreal-cc Good point, I've added it (and some other per-context whitelists) to the execute api in #67035. |
Json.load(String)Json.dump(Object)Json.dump(Object, boolean).