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.

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

Refactored HybridQueryPhaseSearcherTests to remove knn specific classes #977

Merged

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.

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

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
…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)
@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
…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
…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
Labels
backport 2.x Label will add auto workflow to backport PR to 2.x branch skip-changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants