add support for builder constructor in neural query builder #1047

will-hwang · 2024-12-30T23:35:22Z

Description

Adds support for builder constructor for Neural Query Builder

This PR adds support for builder constructor with @Builder lombok annotation in NeuralQueryBuilder

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

#1025

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.

bzhangam · 2024-12-30T23:58:19Z

src/main/java/org/opensearch/neuralsearch/query/NeuralQueryBuilder.java

@@ -23,6 +23,7 @@
 import java.util.Objects;
 import java.util.function.Supplier;

+import lombok.Builder;


I think we want to validate the NeuralQueryBuilder before we create it like what we are doing in the KNNQueryBuilder. And to enforce this we will also need to either remove @NoArgsConstructor
@AllArgsConstructor or make them private.

Currently I think the builder is mainly used for our internal testing but I think it's good to enforce that only valid NeuralQueryBuilder can be created through our builder in case in future people accidentally use it create some invalid NeuralQueryBuilder in our business logic.

Besides I think currently we are using AllArgsConstructor to create NeuralQueryBuilder for some tests can we replace them with the builder you add? In this way if we need to add a new field we don't need to modify the tests that don't care about the new field.

+1 to @bzhangam's comment. You can have reference from kNN's query builder: https://github.com/opensearch-project/k-NN/blob/main/src/main/java/org/opensearch/knn/index/query/KNNQueryBuilder.java#L142

addressed in the newest commit

junqiu-lei · 2024-12-31T19:13:55Z

Please sign off your commits and add this PR to CHANGELOG.

src/main/java/org/opensearch/neuralsearch/query/NeuralQueryBuilder.java

Signed-off-by: will-hwang <[email protected]>

…tantiation Signed-off-by: will-hwang <[email protected]>

Signed-off-by: will-hwang <[email protected]>

martin-gaievski

Why there are no changes in any of the callers? If we made both existing constructors private then I assume we should replace calls to that constructor(s) with the new builder class

src/main/java/org/opensearch/neuralsearch/query/NeuralQueryBuilder.java

martin-gaievski · 2025-01-03T02:52:08Z

src/main/java/org/opensearch/neuralsearch/query/NeuralQueryBuilder.java

+        public NeuralQueryBuilder build() {
+            validateQueryParameters(fieldName, queryText, queryImage);
+            int k = this.k == null ? DEFAULT_K : this.k;
+            boolean queryTypeIsProvided = validateKNNQueryType(k, maxDistance, minScore);


why we're adding this logic here, it wasn't part of the original constructor?

As part of this change, we wanted to add validation before instantiation so ensure that only valid NeuralSearchBuilder can be instantiated. See previous comment here: #1047 (comment)

Let me know your thoughts!

I'm rely on @junqiu-lei expertise here, I'm good if he's good

I think we don't need int k = this.k == null ? DEFAULT_K : this.k; at line 204.

If the default k is provided before validateKNNQueryType, when user use radial search parameter(only provide minScore or maxDistance), then validateKNNQueryType will return false and set to use default k at line 207.

The CI is failed in radial integ test due to this change:

NeuralQueryIT > testQueryWithBoostAndImageQueryAndRadialQuery FAILED java.lang.IllegalArgumentException: Only one of k, max_distance, or min_score can be provided at __randomizedtesting.SeedInfo.seed([D0122DFF0BE48EA8:885F5A920D6E8D5B]:0) at org.opensearch.neuralsearch.query.NeuralQueryBuilder.validateKNNQueryType(NeuralQueryBuilder.java:540) at org.opensearch.neuralsearch.query.NeuralQueryBuilder$Builder.build(NeuralQueryBuilder.java:205) at org.opensearch.neuralsearch.query.NeuralQueryIT.testQueryWithBoostAndImageQueryAndRadialQuery(NeuralQueryIT.java:151)

will-hwang · 2025-01-03T04:15:09Z

Why there are no changes in any of the callers? If we made both existing constructors private then I assume we should replace calls to that constructor(s) with the new builder class

@martin-gaievski All the callers have been changed to the new builder class. See the changes in all 14 files

martin-gaievski · 2025-01-03T16:32:40Z

Why there are no changes in any of the callers? If we made both existing constructors private then I assume we should replace calls to that constructor(s) with the new builder class

@martin-gaievski All the callers have been changed to the new builder class. See the changes in all 14 files

Those are all test classes; I mean classes related to application logic. I do see one reference that hasn't been changed (https://github.com/opensearch-project/neural-search/blob/main/src/main/java/org/opensearch/neuralsearch/plugin/NeuralSearch.java#L112). Do you have an idea why that hasn't been changed after you made the constructor private?

Signed-off-by: will-hwang <[email protected]>

will-hwang · 2025-01-03T19:45:32Z

Why there are no changes in any of the callers? If we made both existing constructors private then I assume we should replace calls to that constructor(s) with the new builder class

@martin-gaievski All the callers have been changed to the new builder class. See the changes in all 14 files

Those are all test classes; I mean classes related to application logic. I do see one reference that hasn't been changed (https://github.com/opensearch-project/neural-search/blob/main/src/main/java/org/opensearch/neuralsearch/plugin/NeuralSearch.java#L112). Do you have an idea why that hasn't been changed after you made the constructor private?

As per QuerySpec specification:

public QuerySpec(     
    String name,
    org. opensearch. core. common. io. stream. Writeable. Reader<T> reader,
    org. opensearch. index. query. QueryParser<T> parser )
}

the parameter expects a Reader type, not a NeuralSearchQuery instance. I believe this is why compiler allows NeuralQueryBuilder::new. I've changed the callers to the new builder class in other classes, including in flaky test suite.

heemin32 · 2025-01-03T20:13:11Z

Why there are no changes in any of the callers? If we made both existing constructors private then I assume we should replace calls to that constructor(s) with the new builder class

@martin-gaievski All the callers have been changed to the new builder class. See the changes in all 14 files

Those are all test classes; I mean classes related to application logic. I do see one reference that hasn't been changed (https://github.com/opensearch-project/neural-search/blob/main/src/main/java/org/opensearch/neuralsearch/plugin/NeuralSearch.java#L112). Do you have an idea why that hasn't been changed after you made the constructor private?

The NeuralQueryBuilder::new is mapped to public NeuralQueryBuilder(StreamInput in)
https://github.com/opensearch-project/neural-search/blob/main/src/main/java/org/opensearch/neuralsearch/query/NeuralQueryBuilder.java#L117

Approved by mistake

…earch into support_for_builder_in_neural_query_builder

Signed-off-by: will-hwang <[email protected]>

…_in_neural_query_builder

Signed-off-by: will-hwang <[email protected]>

martin-gaievski

Looks good to me

* add support for builder constructor in neural query builder Signed-off-by: will-hwang <[email protected]> * create custom builder class to enforce valid neural query builder instantiation Signed-off-by: will-hwang <[email protected]> * refactor code to remove duplicate Signed-off-by: will-hwang <[email protected]> * include new constructor in qa packages Signed-off-by: will-hwang <[email protected]> * refactor code to remove unnecessary code Signed-off-by: will-hwang <[email protected]> * fix bug in neural query builder instantiation Signed-off-by: will-hwang <[email protected]> --------- Signed-off-by: will-hwang <[email protected]> (cherry picked from commit 2ecd32c)

…ch-project#1047) * add support for builder constructor in neural query builder Signed-off-by: will-hwang <[email protected]> * create custom builder class to enforce valid neural query builder instantiation Signed-off-by: will-hwang <[email protected]> * refactor code to remove duplicate Signed-off-by: will-hwang <[email protected]> * include new constructor in qa packages Signed-off-by: will-hwang <[email protected]> * refactor code to remove unnecessary code Signed-off-by: will-hwang <[email protected]> * fix bug in neural query builder instantiation Signed-off-by: will-hwang <[email protected]> --------- Signed-off-by: will-hwang <[email protected]> (cherry picked from commit 2ecd32c)

…ch-project#1047) * add support for builder constructor in neural query builder Signed-off-by: will-hwang <[email protected]> * create custom builder class to enforce valid neural query builder instantiation Signed-off-by: will-hwang <[email protected]> * refactor code to remove duplicate Signed-off-by: will-hwang <[email protected]> * include new constructor in qa packages Signed-off-by: will-hwang <[email protected]> * refactor code to remove unnecessary code Signed-off-by: will-hwang <[email protected]> * fix bug in neural query builder instantiation Signed-off-by: will-hwang <[email protected]> --------- Signed-off-by: will-hwang <[email protected]> (cherry picked from commit 2ecd32c) Signed-off-by: will-hwang <[email protected]>

…ilder (#1084) * add support for builder constructor in neural query builder (#1047) Signed-off-by: will-hwang <[email protected]>

* add support for builder constructor in neural query builder Signed-off-by: will-hwang <[email protected]> * create custom builder class to enforce valid neural query builder instantiation Signed-off-by: will-hwang <[email protected]> * refactor code to remove duplicate Signed-off-by: will-hwang <[email protected]> * include new constructor in qa packages Signed-off-by: will-hwang <[email protected]> * refactor code to remove unnecessary code Signed-off-by: will-hwang <[email protected]> * fix bug in neural query builder instantiation Signed-off-by: will-hwang <[email protected]> --------- Signed-off-by: will-hwang <[email protected]>

will-hwang requested review from heemin32, navneet1v, VijayanB, vamshin, jmazanec15, naveentatikonda, junqiu-lei, martin-gaievski, sean-zheng-amazon, model-collapse, zane-neo, vibrantvarun, zhichao-aws, yuye-aws and minalsha as code owners December 30, 2024 23:35

bzhangam reviewed Dec 30, 2024

View reviewed changes

will-hwang force-pushed the support_for_builder_in_neural_query_builder branch from 344c2ef to b98e6d0 Compare January 1, 2025 00:07

bzhangam reviewed Jan 2, 2025

View reviewed changes

src/main/java/org/opensearch/neuralsearch/query/NeuralQueryBuilder.java Outdated Show resolved Hide resolved

bzhangam reviewed Jan 2, 2025

View reviewed changes

src/main/java/org/opensearch/neuralsearch/query/NeuralQueryBuilder.java Outdated Show resolved Hide resolved

will-hwang added 3 commits January 2, 2025 13:27

add support for builder constructor in neural query builder

cbba8e0

Signed-off-by: will-hwang <[email protected]>

create custom builder class to enforce valid neural query builder ins…

e4835f6

…tantiation Signed-off-by: will-hwang <[email protected]>

refactor code to remove duplicate

d8b7fe5

Signed-off-by: will-hwang <[email protected]>

will-hwang force-pushed the support_for_builder_in_neural_query_builder branch from 3b4fbf1 to d8b7fe5 Compare January 2, 2025 21:27

martin-gaievski reviewed Jan 3, 2025

View reviewed changes

include new constructor in qa packages

00c9923

Signed-off-by: will-hwang <[email protected]>

heemin32 previously approved these changes Jan 3, 2025

View reviewed changes

heemin32 added skip-changelog backport 2.x Label will add auto workflow to backport PR to 2.x branch labels Jan 3, 2025

will-hwang added 5 commits January 3, 2025 13:42

Merge branch 'main' of https://github.com/opensearch-project/neural-s…

3384ef4

…earch into support_for_builder_in_neural_query_builder

refactor code to remove unnecessary code

6aaffd2

Signed-off-by: will-hwang <[email protected]>

Merge remote-tracking branch 'upstream/main' into support_for_builder…

21c349c

…_in_neural_query_builder

resolving merge conflict

caf0ccc

fix bug in neural query builder instantiation

5042ea2

Signed-off-by: will-hwang <[email protected]>

martin-gaievski added the v2.19.0 label Jan 7, 2025

martin-gaievski approved these changes Jan 7, 2025

View reviewed changes

heemin32 approved these changes Jan 7, 2025

View reviewed changes

heemin32 merged commit 2ecd32c into opensearch-project:main Jan 7, 2025
41 checks passed

opensearch-trigger-bot bot mentioned this pull request Jan 7, 2025

[Backport 2.x] add support for builder constructor in neural query builder #1065

Closed

martin-gaievski mentioned this pull request Jan 8, 2025

Pagination in Hybrid query #1048

Merged

5 tasks

will-hwang mentioned this pull request Jan 9, 2025

[Backport 2.x] add support for builder constructor in neural query builder #1084

Merged

junqiu-lei pushed a commit that referenced this pull request Jan 9, 2025

[Backport 2.x] add support for builder constructor in neural query bu…

452db42

…ilder (#1084) * add support for builder constructor in neural query builder (#1047) Signed-off-by: will-hwang <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for builder constructor in neural query builder #1047

add support for builder constructor in neural query builder #1047

will-hwang commented Dec 30, 2024 •

edited

Loading

bzhangam Dec 30, 2024

bzhangam Dec 31, 2024

junqiu-lei Dec 31, 2024

will-hwang Jan 2, 2025

junqiu-lei commented Dec 31, 2024

martin-gaievski left a comment

martin-gaievski Jan 3, 2025

will-hwang Jan 3, 2025

martin-gaievski Jan 3, 2025

junqiu-lei Jan 3, 2025

junqiu-lei Jan 3, 2025 •

edited

Loading

will-hwang commented Jan 3, 2025

martin-gaievski commented Jan 3, 2025

will-hwang commented Jan 3, 2025

heemin32 commented Jan 3, 2025 •

edited

Loading

martin-gaievski left a comment

add support for builder constructor in neural query builder #1047

add support for builder constructor in neural query builder #1047

Conversation

will-hwang commented Dec 30, 2024 • edited Loading

Description

Related Issues

Check List

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

junqiu-lei commented Dec 31, 2024

martin-gaievski 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

junqiu-lei Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

will-hwang commented Jan 3, 2025

martin-gaievski commented Jan 3, 2025

will-hwang commented Jan 3, 2025

heemin32 commented Jan 3, 2025 • edited Loading

martin-gaievski left a comment

Choose a reason for hiding this comment

will-hwang commented Dec 30, 2024 •

edited

Loading

junqiu-lei Jan 3, 2025 •

edited

Loading

heemin32 commented Jan 3, 2025 •

edited

Loading