search

opensearch-project / neural-search

Plugin that adds dense neural retrieval into the OpenSearch ecosytem
Apache License 2.0
57 stars 58 forks source link

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

Closed conggguan closed 1 month ago

conggguan commented 1 month ago

Description

This change implement for #646

Feature support query

Issues Resolved

Resolve #646

Check List

documentation-website issue

https://github.com/opensearch-project/documentation-website/issues/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.

codecov[bot] commented 1 month ago

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.

:exclamation: 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 :warning:
...earch/processor/NeuralSparseTwoPhaseProcessor.java 89.88% 2 Missing and 7 partials :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

conggguan commented 1 month ago

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?

martin-gaievski commented 1 month ago

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.