[Feature] Neural Sparse Query Two Phase Search pipeline #747

conggguan · 2024-05-14T05:21:07Z

Description

This change implement for #646

Enhance the speed of neuralsparse query by two-phase.
Now support top-level, boolean and boost compound query; for other compound query will degrade into origin logic and speed.

Feature support query

NeuralSparseQuery
NeuralSparseQuery nested in BoostQuery
NeuralSparseQuery nested in BooleanQuery

Issues Resolved

Resolve #646

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

documentation-website issue

opensearch-project/documentation-website#7289

BWC PR

On the way...

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Signed-off-by: conggguan <[email protected]>

…rocess funciton. Signed-off-by: conggguan <[email protected]>

…uilder. Signed-off-by: conggguan <[email protected]>

martin-gaievski

General comments for this PR:

please think of simpler and more optimal low level design, logic is a bit overcomplicated
formatting, please put calls under if or else conditions into a braces even if it's one liner

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

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

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

martin-gaievski · 2024-05-17T02:32:16Z

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

+        float baseBoost
+    ) {
+        baseBoost *= queryBuilder.boost();
+        neuralSparseQueryBuilderFloatMap.put(queryBuilder.getCopyNeuralSparseQueryBuilderForTwoPhase(ratio), baseBoost);


Another comment - seems we're creating copy of the NeuralSparceQueryBuilder every time we reach here. In worse case how many times we can hit this place? Do we really need a new copy every time?

Indeed, consolidating all query tokens to construct a single NeuralSparseQueryBuilder is a viable strategy. This approach could potentially increase the overall efficiency of the queries, but it might also complicate the logic. Queries involving a large number of NeuralSparseQuery components may not be very common. Therefore, do you think make a more complicate but more effective logic is essential?

if can keep correctness and improve performance I would vote for such approach. Also that aligns with one of the PRs goals which is improve query latency. Is it possible for you to run a benchmark to see how much latency improvement we can get if we reuse a single object?

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

codecov · 2024-05-20T03:44:08Z

Codecov Report

Attention: Patch coverage is 79.02098% with 30 lines in your changes are missing coverage. Please review.

Project coverage is 84.53%. Comparing base (7c54c86) to head (a93c8cd).
Report is 7 commits behind head on main.

❗ Current head a93c8cd differs from pull request most recent head a53966c

Please upload reports for the commit a53966c to get more accurate results.

Files	Patch %	Lines
...h/neuralsearch/query/NeuralSparseQueryBuilder.java	60.37%	14 Missing and 7 partials ⚠️
...earch/processor/NeuralSparseTwoPhaseProcessor.java	89.88%	2 Missing and 7 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #747      +/-   ##
============================================
- Coverage     85.02%   84.53%   -0.50%     
- Complexity      790      807      +17     
============================================
  Files            60       61       +1     
  Lines          2430     2534     +104     
  Branches        410      427      +17     
============================================
+ Hits           2066     2142      +76     
- Misses          202      220      +18     
- Partials        162      172      +10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: conggguan <[email protected]>

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

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

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

zhichao-aws · 2024-05-27T08:26:56Z

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

+        if (queryBuilder instanceof BoolQueryBuilder) {
+            BoolQueryBuilder boolQueryBuilder = (BoolQueryBuilder) queryBuilder;
+            float updatedBoost = baseBoost * boolQueryBuilder.boost();
+            for (QueryBuilder subQuery : boolQueryBuilder.should()) {


In last PR we also supported BoostQuery, and use clause.isScoring() to determine whether to support the boolean clause. Are they same here?

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

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

Signed-off-by: conggguan <[email protected]>

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

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

Signed-off-by: conggguan <[email protected]>

zhichao-aws

lgtm

zhichao-aws · 2024-05-30T05:03:01Z

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

        if (queryTokensSupplier != null) {
            builder.append(queryTokensSupplier.get());
        }
+        if (twoPhaseSharedQueryToken != null) {


We don't need to do the null check here

Signed-off-by: conggguan <[email protected]>

martin-gaievski

We need a bwc for this change, are we covered by anything that already exists or we need a new test?

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

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

conggguan · 2024-06-02T04:38:03Z

We need a bwc for this change, are we covered by anything that already exists or we need a new test?

I think we need a BWC test, and it would be better to perform it after this code is merged. Currently, I can't build a BWC test to invoke the code from this PR since it hasn't been merged yet. I will add a BWC test as soon as this PR is merged.

Is this a good solution?

Signed-off-by: conggguan <[email protected]>

…e search pipeline to neural sparse query builder. Signed-off-by: conggguan <[email protected]>

martin-gaievski · 2024-06-03T16:06:50Z

We need a bwc for this change, are we covered by anything that already exists or we need a new test?

I think we need a BWC test, and it would be better to perform it after this code is merged. Currently, I can't build a BWC test to invoke the code from this PR since it hasn't been merged yet. I will add a BWC test as soon as this PR is merged.

Is this a good solution?

That works, although I'm not sure what issue you're facing as bwc should be able to use code from active PR. Please make sure the PR with BWC is merged in the same release, which in case of this change is 2.15.

* Poc of pipeline Signed-off-by: conggguan <[email protected]> * Complete some settings for two phase pipeline. Signed-off-by: conggguan <[email protected]> * Change the implement of two-phase from QueryBuilderVistor to custom process funciton. Signed-off-by: conggguan <[email protected]> * Add It and fix some bug on the state of multy same neuralsparsequerybuilder. Signed-off-by: conggguan <[email protected]> * Simplify some logic, and correct some format. Signed-off-by: conggguan <[email protected]> * Optimize some format. Signed-off-by: conggguan <[email protected]> * Add some test case. Signed-off-by: conggguan <[email protected]> * Optimize some logic for zhichao-aws's comments. Signed-off-by: conggguan <[email protected]> * Optimize a line without application. Signed-off-by: conggguan <[email protected]> * Add some comments, remove some redundant lines, fix some format. Signed-off-by: conggguan <[email protected]> * Remove a redundant null check, fix a if format. Signed-off-by: conggguan <[email protected]> * Fix a typo for a comment, camelcase format for some variable. Signed-off-by: conggguan <[email protected]> * Add some comments to illustrate the influence of the modify on 2-phase search pipeline to neural sparse query builder. Signed-off-by: conggguan <[email protected]> --------- Signed-off-by: conggguan <[email protected]> Signed-off-by: conggguan <[email protected]> (cherry picked from commit 2b21110)

* Poc of pipeline Signed-off-by: conggguan <[email protected]> * Complete some settings for two phase pipeline. Signed-off-by: conggguan <[email protected]> * Change the implement of two-phase from QueryBuilderVistor to custom process funciton. Signed-off-by: conggguan <[email protected]> * Add It and fix some bug on the state of multy same neuralsparsequerybuilder. Signed-off-by: conggguan <[email protected]> * Simplify some logic, and correct some format. Signed-off-by: conggguan <[email protected]> * Optimize some format. Signed-off-by: conggguan <[email protected]> * Add some test case. Signed-off-by: conggguan <[email protected]> * Optimize some logic for zhichao-aws's comments. Signed-off-by: conggguan <[email protected]> * Optimize a line without application. Signed-off-by: conggguan <[email protected]> * Add some comments, remove some redundant lines, fix some format. Signed-off-by: conggguan <[email protected]> * Remove a redundant null check, fix a if format. Signed-off-by: conggguan <[email protected]> * Fix a typo for a comment, camelcase format for some variable. Signed-off-by: conggguan <[email protected]> * Add some comments to illustrate the influence of the modify on 2-phase search pipeline to neural sparse query builder. Signed-off-by: conggguan <[email protected]> --------- Signed-off-by: conggguan <[email protected]> Signed-off-by: conggguan <[email protected]> (cherry picked from commit 2b21110) Co-authored-by: conggguan <[email protected]>

conggguan added 3 commits May 10, 2024 23:50

Poc of pipeline

3d2bd18

Signed-off-by: conggguan <[email protected]>

Complete some settings for two phase pipeline.

b8ef828

Signed-off-by: conggguan <[email protected]>

Change the implement of two-phase from QueryBuilderVistor to custom p…

f678e93

…rocess funciton. Signed-off-by: conggguan <[email protected]>

github-actions bot added the Features Introduces a new unit of functionality that satisfies a requirement label May 14, 2024

conggguan changed the title ~~Search pipeline~~ [Feature] Neural Sparse Query Two Phase Search pipeline May 14, 2024

Add It and fix some bug on the state of multy same neuralsparsequeryb…

9a1d52c

…uilder. Signed-off-by: conggguan <[email protected]>

conggguan force-pushed the search-pipeline branch from 75a33fd to 9a1d52c Compare May 15, 2024 08:55

conggguan marked this pull request as ready for review May 16, 2024 02:38

conggguan 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 16, 2024 02:38

zhichao-aws added the v2.15.0 label May 16, 2024

zhichao-aws assigned conggguan May 16, 2024

martin-gaievski reviewed May 17, 2024

View reviewed changes

conggguan force-pushed the search-pipeline branch 2 times, most recently from fd3d634 to 0a7bd74 Compare May 20, 2024 03:36

conggguan force-pushed the search-pipeline branch from 0a7bd74 to 91f6a25 Compare May 20, 2024 04:21

Add some test case.

f190834

Signed-off-by: conggguan <[email protected]>

zhichao-aws reviewed May 27, 2024

View reviewed changes

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

Optimize some logic for zhichao-aws's comments.

5ee07d1

Signed-off-by: conggguan <[email protected]>

conggguan force-pushed the search-pipeline branch 3 times, most recently from 5fa1e74 to 7846f5d Compare May 27, 2024 10:33

Merge branch 'main' into search-pipeline

a9adb72

Signed-off-by: conggguan <[email protected]>

conggguan force-pushed the search-pipeline branch from 7846f5d to a9adb72 Compare May 27, 2024 10:59

Optimize a line without application.

0f5eab9

Signed-off-by: conggguan <[email protected]>

zhichao-aws reviewed May 28, 2024

View reviewed changes

src/main/java/org/opensearch/neuralsearch/processor/NeuralSparseTwoPhaseProcessor.java Outdated Show resolved Hide resolved

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

conggguan force-pushed the search-pipeline branch from 12ce9ce to c5d0e99 Compare May 28, 2024 07:11

Add some comments, remove some redundant lines, fix some format.

25edb27

Signed-off-by: conggguan <[email protected]>

conggguan force-pushed the search-pipeline branch from c5d0e99 to 25edb27 Compare May 28, 2024 07:18

zhichao-aws approved these changes May 30, 2024

View reviewed changes

zhichao-aws reviewed May 30, 2024

View reviewed changes

Remove a redundant null check, fix a if format.

61cac40

Signed-off-by: conggguan <[email protected]>

martin-gaievski reviewed Jun 2, 2024

View reviewed changes

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

src/main/java/org/opensearch/neuralsearch/processor/NeuralSparseTwoPhaseProcessor.java Outdated Show resolved Hide resolved

conggguan added 2 commits June 2, 2024 12:39

Fix a typo for a comment, camelcase format for some variable.

83abb31

Signed-off-by: conggguan <[email protected]>

Add some comments to illustrate the influence of the modify on 2-phas…

a53966c

…e search pipeline to neural sparse query builder. Signed-off-by: conggguan <[email protected]>

martin-gaievski approved these changes Jun 3, 2024

View reviewed changes

zhichao-aws merged commit 2b21110 into opensearch-project:main Jun 4, 2024
71 checks passed

zhichao-aws added the backport 2.x Label will add auto workflow to backport PR to 2.x branch label Jun 4, 2024

opensearch-trigger-bot bot mentioned this pull request Jun 4, 2024

[Backport 2.x] [Feature] Neural Sparse Query Two Phase Search pipeline #771

Merged

conggguan mentioned this pull request Jun 4, 2024

[DOC] Add Doc for new feature 2-phase search on NeuralSparseQuery opensearch-project/documentation-website#7289

Closed

4 tasks

conggguan mentioned this pull request Jun 6, 2024

Neural sparse query two-phase search processor's bwc test #777

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Neural Sparse Query Two Phase Search pipeline #747

[Feature] Neural Sparse Query Two Phase Search pipeline #747

conggguan commented May 14, 2024 •

edited

Loading

martin-gaievski left a comment

martin-gaievski May 17, 2024

conggguan May 20, 2024

martin-gaievski May 21, 2024

codecov bot commented May 20, 2024 •

edited

Loading

zhichao-aws May 27, 2024

zhichao-aws left a comment

zhichao-aws May 30, 2024

martin-gaievski left a comment

conggguan commented Jun 2, 2024

martin-gaievski commented Jun 3, 2024

[Feature] Neural Sparse Query Two Phase Search pipeline #747

[Feature] Neural Sparse Query Two Phase Search pipeline #747

Conversation

conggguan commented May 14, 2024 • edited Loading

Description

Feature support query

Issues Resolved

Check List

documentation-website issue

BWC PR

martin-gaievski left a comment

Choose a reason for hiding this comment

martin-gaievski May 17, 2024

Choose a reason for hiding this comment

conggguan May 20, 2024

Choose a reason for hiding this comment

martin-gaievski May 21, 2024

Choose a reason for hiding this comment

codecov bot commented May 20, 2024 • edited Loading

Codecov Report

zhichao-aws May 27, 2024

Choose a reason for hiding this comment

zhichao-aws left a comment

Choose a reason for hiding this comment

zhichao-aws May 30, 2024

Choose a reason for hiding this comment

martin-gaievski left a comment

Choose a reason for hiding this comment

conggguan commented Jun 2, 2024

martin-gaievski commented Jun 3, 2024

conggguan commented May 14, 2024 •

edited

Loading

codecov bot commented May 20, 2024 •

edited

Loading