Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactored HybridQueryPhaseSearcherTests to remove knn specific classes #977

Merged
merged 2 commits into from
Nov 8, 2024
Merged

Refactored HybridQueryPhaseSearcherTests to remove knn specific classes #977

merged 2 commits into from
Nov 8, 2024

Conversation

owaiskazi19
Copy link
Member

Description

Refactored HybridQueryPhaseSearcherTests to remove knn specific classes

Related Issues

Resolves ##859

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.

@martin-gaievski
Copy link
Member

Code looks good to me, thanks for contribution @owaiskazi19

I do see other tests where we do have dependency on the same knn classes: https://github.com/opensearch-project/neural-search/blob/main/src/test/java/org/opensearch/neuralsearch/query/HybridQueryTests.java and https://github.com/opensearch-project/neural-search/blob/main/src/test/java/org/opensearch/neuralsearch/query/HybridQueryBuilderTests.java.

Did you look into those tests as well, can we do similar refactoring?

@martin-gaievski martin-gaievski added the backport 2.x Label will add auto workflow to backport PR to 2.x branch label Nov 7, 2024
@owaiskazi19
Copy link
Member Author

Did you look into those tests as well, can we do similar refactoring?

Handled HybridQueryTests. HybridQueryBuilderTests.java is tightly coupled with knn and might need another issue of it's own.

yuye-aws
yuye-aws approved these changes Nov 8, 2024
View reviewed changes
Copy link
Member

@yuye-aws yuye-aws left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

martin-gaievski
martin-gaievski approved these changes Nov 8, 2024
View reviewed changes
Copy link
Member

@martin-gaievski martin-gaievski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thank you

@martin-gaievski martin-gaievski merged commit 71017e6 into opensearch-project:main Nov 8, 2024
35 of 37 checks passed
@minalsha
Copy link
Collaborator

minalsha commented Nov 8, 2024

Did you look into those tests as well, can we do similar refactoring?

Handled HybridQueryTests. HybridQueryBuilderTests.java is tightly coupled with knn and might need another issue of it's own.

@owaiskazi19 could you please create a tracking issue for this if it doesn't exist? Thanks

opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 8, 2024
@owaiskazi19 @github-actions
Refactored HybridQueryPhaseSearcherTests to remove knn specific class…
d9e322e 
…es (#977)

* Refactored HybridQueryPhaseSearcherTests to remove knn specific classes

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

* Refactored HybridQueryTests

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

---------

Signed-off-by: Owais <[email protected]>
(cherry picked from commit 71017e6)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Nov 8, 2024
Merged
@owaiskazi19 owaiskazi19 mentioned this pull request Nov 8, 2024
Open
@owaiskazi19
Copy link
Member Author

@owaiskazi19 could you please create a tracking issue for this if it doesn't exist? Thanks

#979

martin-gaievski pushed a commit that referenced this pull request Nov 8, 2024
@opensearch-trigger-bot @owaiskazi19
Refactored HybridQueryPhaseSearcherTests to remove knn specific class…
abaa3d8 
…es (#977) (#978)

* Refactored HybridQueryPhaseSearcherTests to remove knn specific classes

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

Co-authored-by: Owais Kazi <[email protected]>
martin-gaievski pushed a commit that referenced this pull request Nov 18, 2024
@owaiskazi19 @martin-gaievski
Refactored HybridQueryPhaseSearcherTests to remove knn specific class…
9b8c3e4 
…es (#977)

* Refactored HybridQueryPhaseSearcherTests to remove knn specific classes

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

* Refactored HybridQueryTests

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

---------

Signed-off-by: Owais <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martin-gaievski martin-gaievski martin-gaievski approved these changes

@yuye-aws yuye-aws yuye-aws approved these changes

@heemin32 heemin32 Awaiting requested review from heemin32 heemin32 is a code owner

@navneet1v navneet1v Awaiting requested review from navneet1v navneet1v is a code owner

@VijayanB VijayanB Awaiting requested review from VijayanB VijayanB is a code owner

@vamshin vamshin Awaiting requested review from vamshin vamshin is a code owner

@jmazanec15 jmazanec15 Awaiting requested review from jmazanec15 jmazanec15 is a code owner

@naveentatikonda naveentatikonda Awaiting requested review from naveentatikonda naveentatikonda is a code owner

@junqiu-lei junqiu-lei Awaiting requested review from junqiu-lei junqiu-lei is a code owner

@sean-zheng-amazon sean-zheng-amazon Awaiting requested review from sean-zheng-amazon sean-zheng-amazon is a code owner

@model-collapse model-collapse Awaiting requested review from model-collapse model-collapse is a code owner

@zane-neo zane-neo Awaiting requested review from zane-neo zane-neo is a code owner

@vibrantvarun vibrantvarun Awaiting requested review from vibrantvarun vibrantvarun is a code owner

@zhichao-aws zhichao-aws Awaiting requested review from zhichao-aws zhichao-aws is a code owner

@minalsha minalsha Awaiting requested review from minalsha minalsha is a code owner

Assignees
No one assigned
Labels
backport 2.x Label will add auto workflow to backport PR to 2.x branch skip-changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@owaiskazi19 @martin-gaievski @minalsha @yuye-aws