Skip to content

Add option for Append Processor to skip/allow empty values #105718

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Add option for Append Processor to skip/allow empty values #105718

wants to merge 13 commits into from

Conversation

limotova
Copy link
Contributor

If one is using templates to generate values, this option can be helpful to avoid adding values that resolve into empty strings

Fixes #104813

@limotova
Add option for Append Processor to skip/allow empty values
5cbbd5e 
If one is using templates to generate values, this option can be helpful
to avoid adding values that resolve into empty strings

Fixes elastic#104813
@github-actions GitHub Actions
Copy link
Contributor

Documentation preview:

@elasticsearchmachine elasticsearchmachine added v8.14.0 needs:triage Requires assignment of a team area label external-contributor Pull request authored by a developer outside the Elasticsearch team labels Feb 22, 2024
@limotova limotova mentioned this pull request Feb 22, 2024
Open
@tlrx tlrx added the :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP label Feb 22, 2024
@elasticsearchmachine elasticsearchmachine added the Team:Data Management Meta label for data/management team label Feb 22, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

@elasticsearchmachine elasticsearchmachine removed the needs:triage Requires assignment of a team area label label Feb 22, 2024
@joegallo joegallo self-requested a review March 8, 2024 18:18
joegallo
joegallo requested changes Mar 8, 2024
View reviewed changes
Copy link
Contributor

@joegallo joegallo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to have this mirror the set processor's ignore_empty_value option, rather than creating a new option name with no precedent -- it's functionally the same as that option, right (albeit with the opposite sense)?

@limotova limotova requested a review from joegallo March 9, 2024 04:40
@joegallo joegallo self-assigned this Mar 29, 2024
@romulets
Copy link
Member

This feature is actually pretty useful. I'm my use-case, I'm appending a lot of fields. To keep checking if they are not null on context is pretty repetitive. Right now we have a painless script that removes all empty values. But would be awesome to have this out of the box.

@elasticsearchmachine
Copy link
Collaborator

Hi @limotova, I've created a changelog YAML for you.

@github-actions GitHub Actions
Copy link
Contributor

It looks like this PR modifies one or more .asciidoc files. These files are being migrated to Markdown, and any changes merged now will be lost. See the migration guide for details.

@joegallo
Copy link
Contributor

joegallo commented Sep 5, 2025

I'm resurrecting this PR and finally getting around to merging it (it is a terrible shame that I've let it sit here for so long, sorry for that!). I merged main in, which was a bit of a task because of the great amount of time that has passed. In the course of that I dropped the docs changes, so I'll need to resurrect those in a separate commit.

@joegallo
Handle nulls as well
27c4e9a 
The original version of this PR predated `copy_from`, so the value
wouldn't have been able to be null here. But now that `copy_from`
exists we should handle null as well as empty strings.
@joegallo
Rewrite setFieldValue in terms of valueNotEmpty
661766e 
Since it handles nulls now, this logic is identical.
@joegallo
Rewrite another setFieldValue arity, too
bea6446 
This one is a bit trickier, so I've lampshaded the surprising part
with a comment, and also there's a newly added test on main via
@joegallo
Verify an invariant
1675b1f 
This arity of setFieldValue doesn't handle the single-value
ignore_empty_value case (like in a set processor), it only handles the
multi-value ignore_empty_values case (like in an append
processor). The same thing applies to allow_duplicates, that is also
an append-only concern.
@joegallo
Copy link
Contributor

joegallo commented Sep 5, 2025

Oops, my one commit comment got truncated because I wasn't careful about syntax, that was intended to read:

Rewrite another setFieldValue arity, too

This one is a bit trickier, so I've lampshaded the surprising part
with a comment, and also there's a newly added test on main via
#134217

@nik9000

This comment was marked as resolved.

Sign in to view
@nik9000

This comment was marked as resolved.

Sign in to view
@joegallo joegallo mentioned this pull request Sep 8, 2025
Merged
@joegallo
Copy link
Contributor

joegallo commented Sep 9, 2025

I merged main into this PR again, so that brings #134217 and #134319 in. They all play nicely together on my machine but we'll see what CI has to say about it.

@joegallo
Copy link
Contributor

joegallo commented Sep 9, 2025

A todo list for myself of what I'd still like to see here:

  • I need to resurrect the docs changes in markdown rather than asciidoc, including the example(s) I promised earlier in Add copy_from option to the Append processor #132003 (comment)
  • I need to add a test only feature, and also some rest tests
  • I think it makes sense to add more test coverage of ignore_empty_values with copy_from (that could probably be done with the previously mentioned rest tests)

limotova
limotova commented Sep 11, 2025
View reviewed changes
Copy link
Contributor Author

@limotova limotova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't approve my own PR haha but the changes look reasonable to me, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joegallo joegallo Awaiting requested review from joegallo

Requested changes must be addressed to merge this pull request.

Assignees

@joegallo joegallo

Labels
:Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >enhancement external-contributor Pull request authored by a developer outside the Elasticsearch team Team:Data Management Meta label for data/management team v9.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

FR: Allow Append processor to ignore empty values
7 participants
@limotova @elasticsearchmachine @romulets @joegallo @nik9000 @tlrx @mark-vieira