Optimize parameter parsing in text chunking processor #733

yuye-aws · 2024-05-06T08:10:37Z

Description

There PR optimizes the parameter parsing step in chunking algorithms. For both algorithms, all runtime parameters for text chunking processor is always available. Therefore, default value is not needed.

Issues Resolved

Optimize parameter parsing in text chunking processor.

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed as per the DCO using --signoff

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: yuye-aws <yuyezhu@amazon.com>

yuye-aws · 2024-05-06T08:35:46Z

@zane-neo This PR resolves your comment in #717:

In fact, with field_map and document, we can always calculate a chunk_string_count, we don't need a default value here. Suggesting optimize this code in next PR

Please help review thanks.

yuye-aws · 2024-05-06T08:46:04Z

We need to rerun the CI checks to check if there are any flakey tests.

yuye-aws · 2024-05-06T09:10:06Z

The rolling upgrade BWC test failure is the same in #734

Signed-off-by: yuye-aws <yuyezhu@amazon.com>

yuye-aws · 2024-05-15T08:58:02Z

Hi maintainer, please attach backport-2.x label to this PR.

yuye-aws · 2024-05-15T09:05:55Z

The rolling upgrade BWC test is failing due to cluster health status issue. Can we rerun these tests?

martin-gaievski · 2024-05-15T23:58:01Z

src/main/java/org/opensearch/neuralsearch/processor/chunker/ChunkerParameterParser.java

+     * Throw IllegalArgumentException if parameter is missing or is not an integer.
+     */
+    public static int parseIntegerParameter(final Map<String, Object> parameters, final String fieldName) {
+        if (!parameters.containsKey(fieldName)) {


Suggested change

if (!parameters.containsKey(fieldName)) {

return parseIntegerParameter(parameters, fieldName, null);

I think this design doesn't follow conventions in other place in Java API.
If you calling parse(argument) than it should execute parsing logic
If you calling parse(argument, defaultValue) than it should try to execute parsing logic, and return default value in case that parsing logic return empty value

In your code everywhere where this new method called with null you can call your new method without default value.
And in that new method implementation you can just call this method with defaultValue = null, see my previous comment with suggestion

Oh, I don't think the function is necessary. I have removed it.

If you calling parse(argument, defaultValue) than it should try to execute parsing logic, and return default value in case that parsing logic return empty value

Not very sure what you mean here. We should throw illegal argument exception if necessary.

In your code everywhere where this new method called with null you can call your new method without default value.

Updated by Zan's comment. You can review now.

Signed-off-by: yuye-aws <yuyezhu@amazon.com>

zane-neo · 2024-05-17T02:18:22Z

src/main/java/org/opensearch/neuralsearch/processor/chunker/ChunkerParameterParser.java

-            // all integer parameters are optional
-            return defaultValue;
+            // return the default value when parameter is not existing
+            if (defaultValue == null) {


I suggest splitting this method to three new methods:

parseInteger: Simply parse integer with catching number format exception.

parseIntegerWithDefault: if field not shown, return default value, else return parseInteger.

parseIntegerWithException: if field not shown, throw IllegalStateException, else return parseInteger.

This won't add repeated code also can make sure the method 2 have concise semantic by making the default value type to int, currently default value accepts null which is not in a correct semantic. For runtime parameters, there isn't default value, so throwing exception is the correct semantic for them.

Since all integer parameters are optional. I will not keep the parseIntegerWithException method.

I mean, runtime parameters are always available while non-runtime parameters have default values.

Signed-off-by: yuye-aws <yuyezhu@amazon.com>

yuye-aws · 2024-05-17T03:52:09Z

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

-        this.maxChunkLimit = parseIntegerParameter(chunkerParameters, MAX_CHUNK_LIMIT_FIELD, DEFAULT_MAX_CHUNK_LIMIT);
-        if (maxChunkLimit < 0 && maxChunkLimit != DISABLED_MAX_CHUNK_LIMIT) {
+        this.maxChunkLimit = parseIntegerWithDefault(chunkerParameters, MAX_CHUNK_LIMIT_FIELD, DEFAULT_MAX_CHUNK_LIMIT);
+        if (maxChunkLimit <= 0 && maxChunkLimit != DISABLED_MAX_CHUNK_LIMIT) {


I am making this change to keep it consistent with the error message:
Parameter [%s] must be positive or %s to disable this parameter"

Do you think maxChunkLimit can be 0?

It could be, it's fine to add = 0 here.

martin-gaievski

Overall code looks good to me. One comment - can you add a unit test for ChunkerParameterParser class? As we're adding more methods to it having specialized test class will give more confidence

yuye-aws · 2024-05-20T02:02:05Z

Overall code looks good to me. One comment - can you add a unit test for ChunkerParameterParser class? As we're adding more methods to it having specialized test class will give more confidence

Sure. I will add unit tests today.

Signed-off-by: yuye-aws <yuyezhu@amazon.com>

yuye-aws · 2024-05-20T03:05:56Z

HI @zane-neo and @martin-gaievski ! I have implemented the unit tests for chunker parameter parser. Can you help review again?

yuye-aws · 2024-05-20T03:06:56Z

src/test/java/org/opensearch/neuralsearch/search/query/HybridQueryPhaseSearcherTests.java

@@ -24,7 +24,6 @@
 import java.util.LinkedList;
 import java.util.Map;
 import java.util.Set;
-import java.util.concurrent.ExecutorService;


This change is due to spotless apply

martin-gaievski

looks good to me, thank you

yuye-aws · 2024-05-21T02:12:07Z

@martin-gaievski Can you merge the PR?

yuye-aws · 2024-05-21T02:46:12Z

The last approver is expected to merge the PR.

* Optimize parameter parsing in text chunking processor Signed-off-by: yuye-aws <yuyezhu@amazon.com> * add change log Signed-off-by: yuye-aws <yuyezhu@amazon.com> * fix unit tests in delimiter chunker Signed-off-by: yuye-aws <yuyezhu@amazon.com> * fix unit tests in fixed token length chunker Signed-off-by: yuye-aws <yuyezhu@amazon.com> * remove redundant Signed-off-by: yuye-aws <yuyezhu@amazon.com> * refactor chunker parameter parser Signed-off-by: yuye-aws <yuyezhu@amazon.com> * unit tests for chunker parameter parser Signed-off-by: yuye-aws <yuyezhu@amazon.com> * fix comment Signed-off-by: yuye-aws <yuyezhu@amazon.com> * spotless apply Signed-off-by: yuye-aws <yuyezhu@amazon.com> --------- Signed-off-by: yuye-aws <yuyezhu@amazon.com> (cherry picked from commit 038b1ec)

Signed-off-by: yuye-aws <yuyezhu@amazon.com> (cherry picked from commit 038b1ec) Co-authored-by: yuye-aws <yuyezhu@amazon.com>

* Optimize parameter parsing in text chunking processor Signed-off-by: yuye-aws <yuyezhu@amazon.com> * add change log Signed-off-by: yuye-aws <yuyezhu@amazon.com> * fix unit tests in delimiter chunker Signed-off-by: yuye-aws <yuyezhu@amazon.com> * fix unit tests in fixed token length chunker Signed-off-by: yuye-aws <yuyezhu@amazon.com> * remove redundant Signed-off-by: yuye-aws <yuyezhu@amazon.com> * refactor chunker parameter parser Signed-off-by: yuye-aws <yuyezhu@amazon.com> * unit tests for chunker parameter parser Signed-off-by: yuye-aws <yuyezhu@amazon.com> * fix comment Signed-off-by: yuye-aws <yuyezhu@amazon.com> * spotless apply Signed-off-by: yuye-aws <yuyezhu@amazon.com> --------- Signed-off-by: yuye-aws <yuyezhu@amazon.com>

Optimize parameter parsing in text chunking processor

f0b0ec3

Signed-off-by: yuye-aws <yuyezhu@amazon.com>

yuye-aws 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 May 6, 2024 08:10

yuye-aws added 3 commits May 6, 2024 16:11

add change log

2ca0abf

Signed-off-by: yuye-aws <yuyezhu@amazon.com>

fix unit tests in delimiter chunker

b5ae017

Signed-off-by: yuye-aws <yuyezhu@amazon.com>

fix unit tests in fixed token length chunker

4e60573

Signed-off-by: yuye-aws <yuyezhu@amazon.com>

Merge branch 'main' into Fix/ChunkingParameterParse

7e643ea

Signed-off-by: yuye-aws <yuyezhu@amazon.com>

martin-gaievski reviewed May 15, 2024

View reviewed changes

remove redundant

d009ffa

Signed-off-by: yuye-aws <yuyezhu@amazon.com>

zane-neo reviewed May 17, 2024

View reviewed changes

martin-gaievski added the backport 2.x Label will add auto workflow to backport PR to 2.x branch label May 17, 2024

refactor chunker parameter parser

63e89d1

Signed-off-by: yuye-aws <yuyezhu@amazon.com>

yuye-aws requested review from martin-gaievski and zane-neo May 17, 2024 03:49

yuye-aws commented May 17, 2024

View reviewed changes

zane-neo approved these changes May 17, 2024

View reviewed changes

martin-gaievski reviewed May 17, 2024

View reviewed changes

yuye-aws added 3 commits May 20, 2024 11:04

unit tests for chunker parameter parser

1ebab45

Signed-off-by: yuye-aws <yuyezhu@amazon.com>

fix comment

55cf7f5

Signed-off-by: yuye-aws <yuyezhu@amazon.com>

spotless apply

988152a

Signed-off-by: yuye-aws <yuyezhu@amazon.com>

yuye-aws commented May 20, 2024

View reviewed changes

martin-gaievski approved these changes May 21, 2024

View reviewed changes

yuye-aws requested a review from martin-gaievski May 21, 2024 02:11

zane-neo merged commit 038b1ec into opensearch-project:main May 21, 2024
67 of 73 checks passed

opensearch-trigger-bot bot mentioned this pull request May 21, 2024

[Backport 2.x] Optimize parameter parsing in text chunking processor #754

Merged

martin-gaievski pushed a commit that referenced this pull request May 21, 2024

Optimize parameter parsing in text chunking processor (#733) (#754)

b0753b8

Signed-off-by: yuye-aws <yuyezhu@amazon.com> (cherry picked from commit 038b1ec) Co-authored-by: yuye-aws <yuyezhu@amazon.com>

yuye-aws deleted the Fix/ChunkingParameterParse branch September 10, 2024 09:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize parameter parsing in text chunking processor #733

Optimize parameter parsing in text chunking processor #733

yuye-aws commented May 6, 2024 •

edited

Loading

yuye-aws commented May 6, 2024 •

edited

Loading

yuye-aws commented May 6, 2024

yuye-aws commented May 6, 2024

yuye-aws commented May 15, 2024

yuye-aws commented May 15, 2024

martin-gaievski May 15, 2024

martin-gaievski May 15, 2024

yuye-aws May 17, 2024

yuye-aws May 17, 2024

yuye-aws May 17, 2024

zane-neo May 17, 2024 •

edited

Loading

yuye-aws May 17, 2024

yuye-aws May 17, 2024

yuye-aws May 17, 2024

yuye-aws May 17, 2024

yuye-aws May 17, 2024 •

edited

Loading

zane-neo May 17, 2024

martin-gaievski left a comment

yuye-aws commented May 20, 2024

yuye-aws commented May 20, 2024

yuye-aws May 20, 2024

martin-gaievski left a comment

yuye-aws commented May 21, 2024

yuye-aws commented May 21, 2024

	if (!parameters.containsKey(fieldName)) {
	return parseIntegerParameter(parameters, fieldName, null);

Optimize parameter parsing in text chunking processor #733

Optimize parameter parsing in text chunking processor #733

Conversation

yuye-aws commented May 6, 2024 • edited Loading

Description

Issues Resolved

Check List

yuye-aws commented May 6, 2024 • edited Loading

yuye-aws commented May 6, 2024

yuye-aws commented May 6, 2024

yuye-aws commented May 15, 2024

yuye-aws commented May 15, 2024

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

zane-neo May 17, 2024 • edited Loading

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

yuye-aws May 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-gaievski left a comment

Choose a reason for hiding this comment

yuye-aws commented May 20, 2024

yuye-aws commented May 20, 2024

Choose a reason for hiding this comment

martin-gaievski left a comment

Choose a reason for hiding this comment

yuye-aws commented May 21, 2024

yuye-aws commented May 21, 2024

yuye-aws commented May 6, 2024 •

edited

Loading

yuye-aws commented May 6, 2024 •

edited

Loading

zane-neo May 17, 2024 •

edited

Loading

yuye-aws May 17, 2024 •

edited

Loading