[Feature]: add ignore missing field to text chunking processors #907

IanMenendez · 2024-09-13T22:45:46Z

Description

Adds ignore missing field to text chunking processors

Related Issues

Check List

New functionality includes testing.
New functionality has been documented.
API changes companion pull request created.
Commits are signed per the DCO using --signoff.
Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Ian Menendez <ianfmenendezd@gmail.com>

yuye-aws

The code looks good to me

DEVELOPER_GUIDE.md

yuye-aws

Just need a few changes to the integration test.

src/test/java/org/opensearch/neuralsearch/processor/TextChunkingProcessorIT.java

yuye-aws · 2024-09-14T01:08:58Z

Thanks for creating this PR @IanMenendez ! Please also update CHANGELOG.md. We are after the 2.17 code freeze date in https://opensearch.org/releases.html, 2.18 will be the first release for you change to take effect.

yuye-aws · 2024-09-14T01:09:56Z

@zhichao-aws Can you help attach the backport 2.x label to this PR. Also, after @IanMenendez deal with the review comments, please approve the CI workflow.

Signed-off-by: Ian Menendez <ianfmenendezd@gmail.com>

src/test/java/org/opensearch/neuralsearch/processor/TextChunkingProcessorIT.java

Signed-off-by: Ian Menendez <ianfmenendezd@gmail.com>

yuye-aws

Code looks good to me. The integration tests also passed. Thanks for your contribution.

yuye-aws · 2024-09-14T03:23:56Z

Pinging @zane-neo @zhichao-aws @martin-gaievski for review.

vibrantvarun · 2024-09-18T02:54:56Z

@IanMenendez Please add BWC tests
cc: @yuye-aws

src/main/java/org/opensearch/neuralsearch/processor/TextChunkingProcessor.java

src/test/java/org/opensearch/neuralsearch/processor/TextChunkingProcessorIT.java

src/test/resources/processor/chunker/TextChunkingTestMissingFieldDocument.json

martin-gaievski

Apart from minor comments my main concerns are:

extra integ test, please move test cases to uni tests
field name, I suggested one in the issue

src/main/java/org/opensearch/neuralsearch/processor/TextChunkingProcessor.java

src/test/java/org/opensearch/neuralsearch/processor/TextChunkingProcessorIT.java

src/main/java/org/opensearch/neuralsearch/processor/TextChunkingProcessor.java

yuye-aws · 2024-09-18T04:20:34Z

@IanMenendez Please add BWC tests cc: @yuye-aws

@IanMenendez The BWC test file for text_chunking processor are located in

qa/restart-upgrade/src/test/java/org/opensearch/neuralsearch/bwc/TextChunkingProcessorIT.java
qa/rolling-upgrade/src/test/java/org/opensearch/neuralsearch/bwc/TextChunkingProcessorIT.java
You can take a look whether the BWC tests are needed.

Signed-off-by: Ian Menendez <ianfmenendezd@gmail.com>

IanMenendez · 2024-09-21T20:58:40Z

@IanMenendez Please add BWC tests cc: @yuye-aws

Why would BWC tests be needed for this feature?

yuye-aws · 2024-09-23T02:44:12Z

+1 I don't think BWC tests are needed

yuye-aws · 2024-09-23T02:45:08Z

@martin-gaievski Can you help review again?

martin-gaievski · 2024-09-28T01:24:13Z

@IanMenendez thanks for addressing most comments. Please check one of previous comments, I've unresolved conversation.

In future, can you please reply to each comment:

whether you're making changes or not, just ask will be good
if you're making the change please state whether you agree with suggestion/request, and if not please put your reasoning

Signed-off-by: Ian Menendez <ianfmenendezd@gmail.com>

martin-gaievski · 2024-09-30T17:40:42Z

To avoid failures in BWC please update you main so it got code from #916, then rebase on main

Signed-off-by: Ian Menendez <ianfmenendezd@gmail.com>

vibrantvarun · 2024-10-01T16:29:34Z

src/main/java/org/opensearch/neuralsearch/processor/factory/TextChunkingProcessorFactory.java

@@ -45,6 +48,16 @@ public TextChunkingProcessor create(
    ) throws Exception {
        Map<String, Object> fieldMap = readMap(TYPE, processorTag, config, FIELD_MAP_FIELD);
        Map<String, Object> algorithmMap = readMap(TYPE, processorTag, config, ALGORITHM_FIELD);
-        return new TextChunkingProcessor(processorTag, description, fieldMap, algorithmMap, environment, clusterService, analysisRegistry);
+        boolean ignoreMissing = readBooleanProperty(TYPE, processorTag, config, IGNORE_MISSING, DEFAULT_IGNORE_MISSING);
+        return new TextChunkingProcessor(


How about applying builder pattern and create a DTO object to form TextChunkingProcessor Object? Currently, argument size in the TextChunkingProcessor looks too long.

Thoughts @martin-gaievski ?

We can do this in a separate PR aiming refactoring of plugin classes, for scope of this PR I think we're good with adding just a field.

martin-gaievski · 2024-10-01T18:05:53Z

@IanMenendez can you please create a documentation PR, we need to add the description for new field. You can kick off the process from this page

IanMenendez · 2024-10-01T18:10:24Z

@IanMenendez can you please create a documentation PR, we need to add the description for new field. You can kick off the process from this page

There is one already approved here opensearch-project/documentation-website#8266

noticed that changelog entry is missing

martin-gaievski · 2024-10-01T18:16:36Z

@IanMenendez could you please move your entry in CHANGELOG file under 2.x/Enhancements category. Every Feature change should go through additional security review, but for Enhancement there is no such requirement

Signed-off-by: Ian Menendez <ianfmenendezd@gmail.com>

IanMenendez · 2024-10-02T01:51:23Z

@martin-gaievski changed changelog

yuye-aws · 2024-10-02T02:31:55Z

Running the CI. Will approve it once all the checks have been passed

feat: add ignore missing field to text chunking processor

ffcd3a9

Signed-off-by: Ian Menendez <ianfmenendezd@gmail.com>

IanMenendez requested review from heemin32, navneet1v, VijayanB, vamshin, jmazanec15, naveentatikonda, junqiu-lei, martin-gaievski, sean-zheng-amazon, model-collapse, zane-neo, ylwu-amzn, jngz-es, vibrantvarun and zhichao-aws as code owners September 13, 2024 22:45

IanMenendez mentioned this pull request Sep 13, 2024

[Feature]: add ignore missing to text chunking processor opensearch-project/documentation-website#8266

Open

1 task

yuye-aws reviewed Sep 14, 2024

View reviewed changes

DEVELOPER_GUIDE.md Outdated Show resolved Hide resolved

yuye-aws reviewed Sep 14, 2024

View reviewed changes

yuye-aws mentioned this pull request Sep 14, 2024

[FEATURE] Add ignore missing field to text chunking processor #906

Open

fix: fix tests naming

d41d3ec

Signed-off-by: Ian Menendez <ianfmenendezd@gmail.com>

IanMenendez requested a review from yuye-aws September 14, 2024 01:33

yuye-aws reviewed Sep 14, 2024

View reviewed changes

src/test/java/org/opensearch/neuralsearch/processor/TextChunkingProcessorIT.java Outdated Show resolved Hide resolved

fix: test

790b6a2

Signed-off-by: Ian Menendez <ianfmenendezd@gmail.com>

yuye-aws approved these changes Sep 14, 2024

View reviewed changes

vibrantvarun reviewed Sep 18, 2024

View reviewed changes

martin-gaievski requested changes Sep 18, 2024

View reviewed changes

Ian Menendez and others added 3 commits September 21, 2024 17:22

Merge branch 'main' into feature/text-chunking-processor-ignore-missing

06fe680

fix: test instead of it test

d905d70

Signed-off-by: Ian Menendez <ianfmenendezd@gmail.com>

fix: fix test

ee66547

Signed-off-by: Ian Menendez <ianfmenendezd@gmail.com>

IanMenendez requested review from martin-gaievski and vibrantvarun September 23, 2024 21:09

Merge branch 'main' into feature/text-chunking-processor-ignore-missing

e61baa5

Signed-off-by: Ian Menendez <ianfmenendezd@gmail.com>

IanMenendez added 2 commits October 1, 2024 09:01

Merge branch 'main' into feature/text-chunking-processor-ignore-missing

579918c

Signed-off-by: Ian Menendez <ianfmenendezd@gmail.com>

feat: additional method to when processing a chunk

dbae4a3

Signed-off-by: Ian Menendez <ianfmenendezd@gmail.com>

vibrantvarun reviewed Oct 1, 2024

View reviewed changes

martin-gaievski previously approved these changes Oct 1, 2024

View reviewed changes

martin-gaievski added backport 2.x Label will add auto workflow to backport PR to 2.x branch v2.18.0 labels Oct 1, 2024

chore: changelog

7ae3889

Signed-off-by: Ian Menendez <ianfmenendezd@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature]: add ignore missing field to text chunking processors #907

[Feature]: add ignore missing field to text chunking processors #907

IanMenendez commented Sep 13, 2024

yuye-aws left a comment

yuye-aws left a comment

yuye-aws commented Sep 14, 2024

yuye-aws commented Sep 14, 2024

yuye-aws left a comment

yuye-aws commented Sep 14, 2024

vibrantvarun commented Sep 18, 2024 •

edited

Loading

martin-gaievski left a comment

yuye-aws commented Sep 18, 2024 •

edited

Loading

IanMenendez commented Sep 21, 2024 •

edited

Loading

yuye-aws commented Sep 23, 2024

yuye-aws commented Sep 23, 2024

martin-gaievski commented Sep 28, 2024

martin-gaievski commented Sep 30, 2024

vibrantvarun Oct 1, 2024

martin-gaievski Oct 1, 2024

martin-gaievski commented Oct 1, 2024

IanMenendez commented Oct 1, 2024

martin-gaievski commented Oct 1, 2024 •

edited

Loading

IanMenendez commented Oct 2, 2024

yuye-aws commented Oct 2, 2024

[Feature]: add ignore missing field to text chunking processors #907

Are you sure you want to change the base?

[Feature]: add ignore missing field to text chunking processors #907

Conversation

IanMenendez commented Sep 13, 2024

Description

Related Issues

Check List

yuye-aws left a comment

Choose a reason for hiding this comment

yuye-aws left a comment

Choose a reason for hiding this comment

yuye-aws commented Sep 14, 2024

yuye-aws commented Sep 14, 2024

yuye-aws left a comment

Choose a reason for hiding this comment

yuye-aws commented Sep 14, 2024

vibrantvarun commented Sep 18, 2024 • edited Loading

martin-gaievski left a comment

Choose a reason for hiding this comment

yuye-aws commented Sep 18, 2024 • edited Loading

IanMenendez commented Sep 21, 2024 • edited Loading

yuye-aws commented Sep 23, 2024

yuye-aws commented Sep 23, 2024

martin-gaievski commented Sep 28, 2024

martin-gaievski commented Sep 30, 2024

vibrantvarun Oct 1, 2024

Choose a reason for hiding this comment

martin-gaievski Oct 1, 2024

Choose a reason for hiding this comment

martin-gaievski commented Oct 1, 2024

IanMenendez commented Oct 1, 2024

martin-gaievski commented Oct 1, 2024 • edited Loading

IanMenendez commented Oct 2, 2024

yuye-aws commented Oct 2, 2024

vibrantvarun commented Sep 18, 2024 •

edited

Loading

yuye-aws commented Sep 18, 2024 •

edited

Loading

IanMenendez commented Sep 21, 2024 •

edited

Loading

martin-gaievski commented Oct 1, 2024 •

edited

Loading