Skip to content
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

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 6 commits into from
+53 −3

Conversation

IanMenendez
Copy link
Contributor

Description

Adds ignore missing field to text chunking processors

Related Issues

#906

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.

@IanMenendez IanMenendez mentioned this pull request Sep 13, 2024
Open
1 task
yuye-aws
yuye-aws reviewed Sep 14, 2024
View reviewed changes
Copy link
Member

@yuye-aws yuye-aws left a comment

Choose a reason for hiding this comment

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

The code looks good to me

DEVELOPER_GUIDE.md Outdated Show resolved Hide resolved
yuye-aws
yuye-aws reviewed Sep 14, 2024
View reviewed changes
Copy link
Member

@yuye-aws yuye-aws left a comment

Choose a reason for hiding this comment

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

Just need a few changes to the integration test.

src/test/java/org/opensearch/neuralsearch/processor/TextChunkingProcessorIT.java Outdated Show resolved Hide resolved
src/test/java/org/opensearch/neuralsearch/processor/TextChunkingProcessorIT.java Outdated Show resolved Hide resolved
src/test/java/org/opensearch/neuralsearch/processor/TextChunkingProcessorIT.java Outdated Show resolved Hide resolved
src/test/java/org/opensearch/neuralsearch/processor/TextChunkingProcessorIT.java Outdated Show resolved Hide resolved
src/test/java/org/opensearch/neuralsearch/processor/TextChunkingProcessorIT.java Outdated Show resolved Hide resolved
src/test/java/org/opensearch/neuralsearch/processor/TextChunkingProcessorIT.java Outdated Show resolved Hide resolved
@yuye-aws
Copy link
Member

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
Copy link
Member

@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.

@yuye-aws yuye-aws mentioned this pull request Sep 14, 2024
Open
@IanMenendez
fix: fix tests naming
d41d3ec 
Signed-off-by: Ian Menendez <[email protected]>
@IanMenendez
fix: test
790b6a2 
Signed-off-by: Ian Menendez <[email protected]>
yuye-aws
yuye-aws approved these changes Sep 14, 2024
View reviewed changes
Copy link
Member

@yuye-aws yuye-aws left a comment

Choose a reason for hiding this comment

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

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

@yuye-aws
Copy link
Member

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

@vibrantvarun
Copy link
Member

vibrantvarun commented Sep 18, 2024 •
edited
Loading

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

martin-gaievski
martin-gaievski requested changes Sep 18, 2024
View reviewed changes
Copy link
Member

@martin-gaievski martin-gaievski left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
src/main/java/org/opensearch/neuralsearch/processor/TextChunkingProcessor.java Show resolved Hide resolved
src/main/java/org/opensearch/neuralsearch/processor/TextChunkingProcessor.java Outdated
List<String> chunkedResult = chunkLeafType(chunkObject, runtimeParameters);
sourceAndMetadataMap.put(String.valueOf(targetKey), chunkedResult);

if (!(ignoreMissing && chunkObject == null)) {
Copy link
Member

Choose a reason for hiding this comment

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

please refactor condition to a small method with a meaningful name, like shouldProcessChunk. Also please use Objects.isNull for checking if object is null

Copy link
Member

@martin-gaievski martin-gaievski Sep 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think this one got addressed completely, I don't see condition being moved to a method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@martin-gaievski

In my opinion there is no point in having a method for a 2 variables boolean expression that is used just one time.

It will convolute the code more than help

src/main/java/org/opensearch/neuralsearch/processor/TextChunkingProcessor.java Outdated Show resolved Hide resolved
src/test/java/org/opensearch/neuralsearch/processor/TextChunkingProcessorIT.java Outdated Show resolved Hide resolved
src/test/java/org/opensearch/neuralsearch/processor/TextChunkingProcessorIT.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/neuralsearch/processor/TextChunkingProcessor.java Show resolved Hide resolved
@yuye-aws
Copy link
Member

yuye-aws commented Sep 18, 2024 •
edited
Loading

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

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

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

@IanMenendez
Copy link
Contributor Author

IanMenendez commented Sep 21, 2024 •
edited
Loading

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

Why would BWC tests be needed for this feature?

@yuye-aws
Copy link
Member

+1 I don't think BWC tests are needed

@yuye-aws
Copy link
Member

@martin-gaievski Can you help review again?

@martin-gaievski
Copy link
Member

@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

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

@yuye-aws yuye-aws yuye-aws approved these changes

@heemin32 heemin32 Awaiting requested review from heemin32 heemin32 is a code owner

@navneet1v navneet1v Awaiting requested review from navneet1v navneet1v is a code owner

@VijayanB VijayanB Awaiting requested review from VijayanB VijayanB is a code owner

@vamshin vamshin Awaiting requested review from vamshin vamshin is a code owner

@jmazanec15 jmazanec15 Awaiting requested review from jmazanec15 jmazanec15 is a code owner

@naveentatikonda naveentatikonda Awaiting requested review from naveentatikonda naveentatikonda is a code owner

@junqiu-lei junqiu-lei Awaiting requested review from junqiu-lei junqiu-lei is a code owner

@sean-zheng-amazon sean-zheng-amazon Awaiting requested review from sean-zheng-amazon sean-zheng-amazon is a code owner

@model-collapse model-collapse Awaiting requested review from model-collapse model-collapse is a code owner

@zane-neo zane-neo Awaiting requested review from zane-neo zane-neo is a code owner

@ylwu-amzn ylwu-amzn Awaiting requested review from ylwu-amzn ylwu-amzn is a code owner

@jngz-es jngz-es Awaiting requested review from jngz-es jngz-es is a code owner

@zhichao-aws zhichao-aws Awaiting requested review from zhichao-aws zhichao-aws is a code owner

@martin-gaievski martin-gaievski Awaiting requested review from martin-gaievski martin-gaievski is a code owner

@vibrantvarun vibrantvarun Awaiting requested review from vibrantvarun vibrantvarun is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@IanMenendez @yuye-aws @vibrantvarun @martin-gaievski