Adding aggregations in hybrid query #630

martin-gaievski · 2024-03-12T00:15:41Z

Description

Adding aggregations to hybrid query.
Implementation is based on design RFC . Big chunk of implementation is done under #624, in this PR we mainly:

changing way of getting total hits, it's number of unique documents from all sub-queries. in today's implementation it's max of results from individual sub-queries
adding base integ test for 1) metric, bucket and pipeline aggressions and 2) with and without concurrent search (as there was an overlap in initial implementation of hybrid query)

Issues Resolved

#509

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.

martin-gaievski · 2024-03-12T00:22:35Z

CHANGELOG.md

@@ -8,19 +8,17 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 ### Enhancements
 ### Bug Fixes
 - Fix async actions are left in neural_sparse query ([#438](https://github.com/opensearch-project/neural-search/pull/438))
- Fixed exception for case when Hybrid query being wrapped into bool query ([#490](https://github.com/opensearch-project/neural-search/pull/490))
- Hybrid query and nested type fields ([#498](https://github.com/opensearch-project/neural-search/pull/498))


those PRs were released in 2.12, doing cleanup

martin-gaievski · 2024-03-12T00:23:58Z

CHANGELOG.md

 - Fix typo for sparse encoding processor factory([#578](https://github.com/opensearch-project/neural-search/pull/578))
 - Add non-null check for queryBuilder in NeuralQueryEnricherProcessor ([#615](https://github.com/opensearch-project/neural-search/pull/615))
 ### Infrastructure
 ### Documentation
 ### Maintenance
- Added support for jdk-21 ([#500](https://github.com/opensearch-project/neural-search/pull/500)))


same as 2 lines above, this is part of 2.12

codecov · 2024-03-12T00:25:27Z

Codecov Report

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

Project coverage is 82.57%. Comparing base (c9cdcc1) to head (b785198).

Files	Patch %	Lines
.../opensearch/neuralsearch/util/HybridQueryUtil.java	71.42%	0 Missing and 4 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #630      +/-   ##
============================================
- Coverage     82.70%   82.57%   -0.13%     
- Complexity      650      656       +6     
============================================
  Files            51       52       +1     
  Lines          2053     2055       +2     
  Branches        329      328       -1     
============================================
- Hits           1698     1697       -1     
  Misses          212      212              
- Partials        143      146       +3

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

martin-gaievski · 2024-03-12T00:54:41Z

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

@@ -63,7 +63,7 @@ public void testCombination_whenMultipleSubqueriesResultsAndDefaultMethod_thenSc
        assertNotNull(queryTopDocs);
        assertEquals(3, queryTopDocs.size());

-        assertEquals(3, queryTopDocs.get(0).getScoreDocs().size());
+        assertEquals(5, queryTopDocs.get(0).getScoreDocs().size());


this is needed because of the change in a way we count total hits

Signed-off-by: Martin Gaievski <[email protected]>

src/main/java/org/opensearch/neuralsearch/processor/combination/ScoreCombiner.java

src/main/java/org/opensearch/neuralsearch/util/HybridQueryUtil.java

src/test/java/org/opensearch/neuralsearch/query/HybridQueryIT.java

navneet1v

Minor comments. Overall code looks good to me.

Signed-off-by: Martin Gaievski <[email protected]>

opensearch-trigger-bot · 2024-03-12T17:04:56Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-630-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 f04c058fc5ab193342c583cf820cd6cb72be42ea
# Push it to GitHub
git push --set-upstream origin backport/backport-630-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-630-to-2.x.

vibrantvarun · 2024-03-12T17:06:57Z

LGTM-

* Adding aggregations in hybrid query Signed-off-by: Martin Gaievski <[email protected]> (cherry picked from commit f04c058)

martin-gaievski added Features Introduces a new unit of functionality that satisfies a requirement backport 2.x Label will add auto workflow to backport PR to 2.x branch v2.13.0 labels Mar 12, 2024

martin-gaievski force-pushed the aggregations_in_hybrid_query branch 2 times, most recently from 57491e1 to 235761d Compare March 12, 2024 00:21

martin-gaievski commented Mar 12, 2024

View reviewed changes

martin-gaievski force-pushed the aggregations_in_hybrid_query branch 3 times, most recently from fe4dfae to 09852a1 Compare March 12, 2024 00:53

martin-gaievski commented Mar 12, 2024

View reviewed changes

martin-gaievski force-pushed the aggregations_in_hybrid_query branch from 09852a1 to 801e58e Compare March 12, 2024 00:55

Adding aggregations in hybrid query

0622927

Signed-off-by: Martin Gaievski <[email protected]>

martin-gaievski force-pushed the aggregations_in_hybrid_query branch from 801e58e to 0622927 Compare March 12, 2024 01:28

martin-gaievski marked this pull request as ready for review March 12, 2024 01:38

martin-gaievski requested review from heemin32, navneet1v, VijayanB, vamshin, jmazanec15, naveentatikonda, junqiu-lei, sean-zheng-amazon, model-collapse, zane-neo, ylwu-amzn, jngz-es and vibrantvarun as code owners March 12, 2024 01:38

navneet1v reviewed Mar 12, 2024

View reviewed changes

src/main/java/org/opensearch/neuralsearch/processor/combination/ScoreCombiner.java Show resolved Hide resolved

navneet1v reviewed Mar 12, 2024

View reviewed changes

src/main/java/org/opensearch/neuralsearch/util/HybridQueryUtil.java Show resolved Hide resolved

navneet1v reviewed Mar 12, 2024

View reviewed changes

src/test/java/org/opensearch/neuralsearch/query/HybridQueryIT.java Outdated Show resolved Hide resolved

navneet1v reviewed Mar 12, 2024

View reviewed changes

Refactor integ tests and main code

b785198

Signed-off-by: Martin Gaievski <[email protected]>

navneet1v approved these changes Mar 12, 2024

View reviewed changes

jmazanec15 approved these changes Mar 12, 2024

View reviewed changes

martin-gaievski merged commit f04c058 into opensearch-project:main Mar 12, 2024
58 of 60 checks passed

martin-gaievski mentioned this pull request Mar 12, 2024

[Backport 2.x] Adding aggregations in hybrid query (#630) #631

Merged

martin-gaievski added a commit that referenced this pull request Mar 12, 2024

Adding aggregations in hybrid query (#630) (#631)

4043f9f

* Adding aggregations in hybrid query Signed-off-by: Martin Gaievski <[email protected]> (cherry picked from commit f04c058)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding aggregations in hybrid query #630

Adding aggregations in hybrid query #630

martin-gaievski commented Mar 12, 2024

martin-gaievski Mar 12, 2024

martin-gaievski Mar 12, 2024 •

edited

Loading

codecov bot commented Mar 12, 2024 •

edited

Loading

martin-gaievski Mar 12, 2024

navneet1v left a comment

opensearch-trigger-bot bot commented Mar 12, 2024

vibrantvarun commented Mar 12, 2024

Adding aggregations in hybrid query #630

Adding aggregations in hybrid query #630

Conversation

martin-gaievski commented Mar 12, 2024

Description

Issues Resolved

Check List

martin-gaievski Mar 12, 2024

Choose a reason for hiding this comment

martin-gaievski Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Mar 12, 2024 • edited Loading

Codecov Report

martin-gaievski Mar 12, 2024

Choose a reason for hiding this comment

navneet1v left a comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Mar 12, 2024

vibrantvarun commented Mar 12, 2024

martin-gaievski Mar 12, 2024 •

edited

Loading

codecov bot commented Mar 12, 2024 •

edited

Loading