RemoveProcessor updated to support fieldsToKeep #83665

zembrzuski · 2022-02-08T16:28:23Z

Relates to #83010

- Enhancement related to issue 83010 [elastic#83010]

elasticmachine · 2022-02-08T16:34:41Z

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

danhermann · 2022-02-08T16:34:50Z

danhermann · 2022-02-17T18:05:33Z

@zembrzuski, thank you for working on this. In thinking a bit about it, I think the fields to keep should always include the metadata fields on the document. These are necessary for ingestion to work properly and their removal causes a variety of problems. I'll probably have a few more suggestions after a more thorough review, but I do know this change will be necessary.

…o issue_83010

zembrzuski · 2022-02-21T18:25:26Z

@zembrzuski, thank you for working on this. In thinking a bit about it, I think the fields to keep should always include the metadata fields on the document. These are necessary for ingestion to work properly and their removal causes a variety of problems. I'll probably have a few more suggestions after a more thorough review, but I do know this change will be necessary.

Hi @danhermann
I have just pushed another patch. Now, only fields that are not metadata are eligible for being removed from the document. I also added some unit tests to make sure that every metadata is present in the copy of the document.

Tks so much for reviewing my PR. This is one of my first PRs, and I am very excited about contributing to ElasticSearch.

danhermann · 2022-02-28T17:07:03Z

@elasticmachine update branch

danhermann

Hi @zembrzuski, thank you for working on this. It looks pretty good. There are just a few changes to be made below and then I think we'll be able to get it merged.

danhermann · 2022-02-28T22:38:33Z

modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/RemoveProcessor.java

+    RemoveProcessor(String tag, String description, List<TemplateScript.Factory> fieldsToRemove, boolean ignoreMissing) {
+        super(tag, description);
+        this.fieldsToRemove = new ArrayList<>(fieldsToRemove);
+        this.fieldsToKeep = new ArrayList<>();
+        this.ignoreMissing = ignoreMissing;


As far as I can see, this constructor has only one use in a test class. I'd suggest removing it and updating that one reference.

danhermann · 2022-02-28T22:41:24Z

modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/RemoveProcessor.java

+                .filter(documentField -> IngestDocument.Metadata.isMetadata(documentField) == false)
+                .filter(documentField -> shouldKeep(documentField, fieldsToKeep, document) == false)
+                .forEach(documentField -> removeWhenPresent(document, documentField));
+        }


Let's add an else block here. I know that one of either fieldsToKeep or fieldsToRemove must be empty, but I prefer not to skip the entire block rather than iterate over an empty list in the latter case.

danhermann · 2022-02-28T22:49:15Z

server/src/main/java/org/elasticsearch/ingest/IngestDocument.java

+        public static boolean isMetadata(String field) {
+            return Arrays.stream(Metadata.values()).anyMatch(e -> e.fieldName.equals(field));
+        }


Let's statically initialize a list or set of metadata field names so we're not creating a new stream every time the processor is executed.

danhermann · 2022-02-28T22:54:43Z

server/src/main/java/org/elasticsearch/ingest/IngestDocument.java

+            if (v instanceof Map) {
+                allFields.addAll(getAllFields((Map) v, prefix + k + "."));
+            }


Minor: let's use instanceof pattern matching here

Suggested change

if (v instanceof Map) {

allFields.addAll(getAllFields((Map) v, prefix + k + "."));

}

if (v instanceof Map mapValue) {

allFields.addAll(getAllFields(mapValue, prefix + k + "."));

}

danhermann · 2022-02-28T23:04:15Z

modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/RemoveProcessor.java

+            if (compiledTemplatesToRemove.isEmpty() && compiledTemplatesToKeep.isEmpty()) {
+                throw new IllegalArgumentException(
+                    "missing field [processors.remove.keep] or [processors.remove.field]. Please specify one of them."
+                );
+            }
+
+            if (compiledTemplatesToRemove.isEmpty() == false && compiledTemplatesToKeep.isEmpty() == false) {
+                throw new IllegalArgumentException(
+                    "Too many fields specified. Please specify either [processors.remove.keep] or [processors.remove.field]."
+                );
+            }


For the sake of consistency, let's use the same exception and message style as other processors with mutually exclusive configuration options. See the network direction processor for an example.

danhermann · 2022-02-28T23:09:33Z

docs/changelog/83665.yaml

@@ -0,0 +1,5 @@
+pr: 83665
+summary: "RemoveProcessor updated to support fieldsToKeep"
+area: Infra/Scripting


Technically, this falls into the "ingest" category:

Suggested change

area: Infra/Scripting

area: Ingest

zembrzuski · 2022-03-02T18:28:26Z

Hi @danhermann
Thanks for reviewing this PR! I've just pushed the code changes asked in the code review.

danhermann · 2022-03-03T11:57:25Z

@zembrzuski, I think you just need to update the error message in the RemoveProcessorFactory tests so they pass.

zembrzuski · 2022-03-03T12:24:14Z

@zembrzuski, I think you just need to update the error message in the RemoveProcessorFactory tests so they pass.

Ohh, my bad. I've just fixed these tests.

danhermann · 2022-03-03T12:41:55Z

@elasticmachine update branch

danhermann · 2022-03-03T12:59:14Z

@elasticmachine run elasticsearch-ci/rest-compatibility

danhermann · 2022-03-04T17:03:55Z

@zembrzuski, everything here looks good. One minor change needed and then we can get this merged:

On the following two pairs of lines, processors.remove.keep should just be keep and processors.remove.field should just be field:
https://github.com/elastic/elasticsearch/pull/83665/files#diff-e44059a95b4557dd2e3c310c64cca96ce1c0dc1f479cfa40c2c32634a91c8512R116-R117

https://github.com/elastic/elasticsearch/pull/83665/files#diff-e44059a95b4557dd2e3c310c64cca96ce1c0dc1f479cfa40c2c32634a91c8512R125-R126

That will change the error messages a bit, so a couple of the unit tests will likely need to be updated accordingly.

…o issue_83010

zembrzuski · 2022-03-07T12:30:43Z

Hi @danhermann
I've just updated the messages and the unit tests! Tks!

danhermann · 2022-03-07T12:35:36Z

Hi @danhermann I've just updated the messages and the unit tests! Tks!

Thanks, @zembrzuski. It looks like the formatting check is complaining about a couple lines now. You should be able to fix that by running ./gradlew :modules:ingest-common:spotlessApply.

zembrzuski · 2022-03-07T12:49:42Z

Hi @danhermann I've just updated the messages and the unit tests! Tks!

Thanks, @zembrzuski. It looks like the formatting check is complaining about a couple lines now. You should be able to fix that by running ./gradlew :modules:ingest-common:spotlessApply.

Ohh, I'm sorry. I've read about checkstyle in the contribution guideline but I forgot to check it this time.

danhermann · 2022-03-07T15:34:44Z

Looks great, @zembrzuski! I've merged this in and it will be available in the next release. Thanks again for your contribution!

ddolcimascolo · 2022-11-28T17:29:35Z

Hey guys, so this landed in 8.2 right? Is it possible that the documentation was forgotten? I can't find anything related to the new keep configuration option in https://www.elastic.co/guide/en/elasticsearch/reference/current/remove-processor.html

I'll give it a try anyway :)

RemoveProcessor updated to support fieldsToKeep

14feb11

- Enhancement related to issue 83010 [elastic#83010]

elasticsearchmachine added v8.2.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Feb 8, 2022

zembrzuski mentioned this pull request Feb 8, 2022

Remove all fields other than a defined list #83010

Closed

danhermann self-assigned this Feb 8, 2022

danhermann added the :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP label Feb 8, 2022

elasticmachine added the Team:Data Management Meta label for data/management team label Feb 8, 2022

danhermann added the >enhancement label Feb 8, 2022

danhermann self-requested a review February 8, 2022 16:36

zembrzuski added 2 commits February 21, 2022 14:31

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

33d8ce8

…o issue_83010

Filtering Metadata in RemoveProcessor

9c49d4d

Merge branch 'master' into issue_83010

da696f3

danhermann suggested changes Feb 28, 2022

View reviewed changes

zembrzuski force-pushed the issue_83010 branch 2 times, most recently from 23e71f2 to 42f610a Compare March 2, 2022 18:25

addressing code review issues

4c0fe94

zembrzuski force-pushed the issue_83010 branch from 42f610a to 4c0fe94 Compare March 2, 2022 18:34

fixed unit tests of RemoveProcessorFactoryTests

8588abe

Merge branch 'master' into issue_83010

124c774

zembrzuski added 2 commits March 7, 2022 09:27

udpated validation messages in RemoveProcessor.java

410faa8

Merge branch 'issue_83010' of github.com:zembrzuski/elasticsearch int…

eb725a6

…o issue_83010

fixing checkstyle errors

89f3860

danhermann merged commit eadb566 into elastic:master Mar 7, 2022

ddolcimascolo mentioned this pull request Jan 11, 2023

Documentation missing for the "keep" option of the Remove ingest processor #92826

Closed

kilfoyle mentioned this pull request Jan 11, 2023

[Docs] Update remove processor with 'keep' option #92836

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RemoveProcessor updated to support fieldsToKeep #83665

RemoveProcessor updated to support fieldsToKeep #83665

zembrzuski commented Feb 8, 2022 •

edited

Loading

elasticmachine commented Feb 8, 2022

danhermann commented Feb 8, 2022

danhermann commented Feb 17, 2022

zembrzuski commented Feb 21, 2022

danhermann commented Feb 28, 2022

danhermann left a comment

danhermann Feb 28, 2022

zembrzuski Mar 2, 2022

danhermann Feb 28, 2022

zembrzuski Mar 2, 2022

danhermann Feb 28, 2022

zembrzuski Mar 2, 2022

danhermann Feb 28, 2022

zembrzuski Mar 2, 2022

danhermann Feb 28, 2022

zembrzuski Mar 2, 2022

danhermann Feb 28, 2022

zembrzuski Mar 2, 2022

zembrzuski commented Mar 2, 2022

danhermann commented Mar 3, 2022

zembrzuski commented Mar 3, 2022

danhermann commented Mar 3, 2022

danhermann commented Mar 3, 2022

danhermann commented Mar 4, 2022

zembrzuski commented Mar 7, 2022

danhermann commented Mar 7, 2022

zembrzuski commented Mar 7, 2022

danhermann commented Mar 7, 2022

ddolcimascolo commented Nov 28, 2022

RemoveProcessor updated to support fieldsToKeep #83665

RemoveProcessor updated to support fieldsToKeep #83665

Conversation

zembrzuski commented Feb 8, 2022 • edited Loading

elasticmachine commented Feb 8, 2022

danhermann commented Feb 8, 2022

danhermann commented Feb 17, 2022

zembrzuski commented Feb 21, 2022

danhermann commented Feb 28, 2022

danhermann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zembrzuski commented Mar 2, 2022

danhermann commented Mar 3, 2022

zembrzuski commented Mar 3, 2022

danhermann commented Mar 3, 2022

danhermann commented Mar 3, 2022

danhermann commented Mar 4, 2022

zembrzuski commented Mar 7, 2022

danhermann commented Mar 7, 2022

zembrzuski commented Mar 7, 2022

danhermann commented Mar 7, 2022

ddolcimascolo commented Nov 28, 2022

zembrzuski commented Feb 8, 2022 •

edited

Loading