-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add copy_from option to the Append processor
#132003
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 copy_from option to the Append processor
#132003
Conversation
|
Pinging @elastic/es-data-management (Team:Data Management) |
|
Hi @chrisberkhout, I've created a changelog YAML for you. |
🔍 Preview links for changed docs |
d41a3fe to
89568f5
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.
Happy to have this feature to avoid templating.
This change will also need an accompanying modification to the elasticsearch-specification to add the new parameter at https://github.com/elastic/elasticsearch-specification/blob/e585438d116b00ff34643179e6286e402c0bcaaf/specification/ingest/_types/Processors.ts#L329-L344
89568f5 to
55b8b34
Compare
Thanks. I opened a draft PR for it: elastic/elasticsearch-specification#5056 |
062a5c4 to
726b6b9
Compare
|
Hi @chrisberkhout, I've created a changelog YAML for you. |
ℹ️ Important: Docs version tagging👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version. We use applies_to tags to mark version-specific features and changes. Expand for a quick overviewWhen to use applies_to tags:✅ At the page level to indicate which products/deployments the content applies to (mandatory) What NOT to do:❌ Don't remove or replace information that applies to an older version 🤔 Need help?
|
There's a PR up for that one already (which your comment reminded me of). My intention is to get this one merged and then return to that one so that they're both in for 9.2.0. |
It mentioned 6 types, the full list is 15: Map, List, Set, byte[], double[][], double[], null, String, Integer, Long, Float, Double, Boolean, ZonedDateTime, Date.
To make it more similar to the equivalent docs from the `set` processor.
2e74f27 to
76aae75
Compare
|
Please don't force push on PRs that have already gotten review traction. I'm sure you didn't change anything substantial in those commits when you force pushed them, but you could have, so arguably I have to review it all from scratch now. If I were going to sneak something into a codebase, I'd do it by working with somebody for a while on a review, and then force pushing something interesting back into one of the early commits... |
|
@joegallo Yeah, sorry about that. I used Github's "Update with rebase" button, so no manual changes but I get your point. |
|
No worries! It's a small thing. |
|
@chrisberkhout I added some rest tests (this is a detail you shouldn't have to care about). Is there anything else you meant to do on this PR or are you okay with me adding a ✅ and then merging it (once CI is green)? |
|
@andrewkroh is there anything else you'd like to see here? I'm imagining @chrisberkhout will follow up on elastic/elasticsearch-specification#5056 separately (and that that has been stalled waiting on this to be merged, which has been stalled waiting on me to review). |
| | `media_type` | no | `application/json` | The media type for encoding `value`. Applies only when `value` is a [template snippet](docs-content://manage-data/ingest/transform-enrich/ingest-pipelines.md#template-snippets). Must be one of `application/json`, `text/plain`, or`application/x-www-form-urlencoded`. | | ||
| | `description` | no | - | Description of the processor. Useful for describing the purpose of the processor or its configuration. | | ||
| | `if` | no | - | Conditionally execute the processor. See [Conditionally run a processor](docs-content://manage-data/ingest/transform-enrich/ingest-pipelines.md#conditionally-run-processor). | | ||
| | `ignore_failure` | no | `false` | Ignore failures for the processor. See [Handling pipeline failures](docs-content://manage-data/ingest/transform-enrich/ingest-pipelines.md#handling-pipeline-failures). | |
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.
Consider adding an example below to encourage copy_from usage. Since it doesn't have an ignore_empty_value, then maybe with an if to demonstrate how to accomplish that...
{
"append": {
"if": "ctx.host?.name instanceof String && !ctx.host.name.isEmpty()",
"field": "related.hosts",
"copy_from": "host.name",
"allow_duplicates": false
}
}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'm going to return to an old PR that does add the ability to ignore empty values as my next task, so rather than re-rolling CI on this one I'm going to merge this as is but this comment is a commitment device that I will definitely return to with a link to the newly added docs when I add them.
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.
Summary
A
copy_fromoption for the Append processor, matching the option in the Set processor.This makes it possible to refer to existing non-String values.
Discussion
The alternative is to use a Script processor, which requires Painless code to manually initialize the destination field and to avoid duplicates if necessary.
One variation on this use case is here and in the following 7 script processors.
I'd be happy to add an
ignore_empty_valueoption as well, which would make it easier to collect values that may exist in multiple locations into one destination.gradle check? ✅If you are submitting this code for a class then read our policy for that.