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

[FEATURE] Refactor HybridQuery*Tests to decouple types from K-NN library #859

Closed
chishui opened this issue Aug 6, 2024 · 2 comments
Closed
Assignees
Labels
good first issue Good for newcomers help wanted Extra attention is needed Refactoring Improve the design, structure, and implementation while preserving its functionality

Comments

@chishui
Copy link
Contributor

chishui commented Aug 6, 2024

Is your feature request related to a problem?

HybridQueryPhaseSearcherTests has direct references to types in K-NN library, and it has broken the build several times recently as k-NN repo keeps refactoring their code. While a feature request was made to k-NN repo asking guarantee for backward compatibility, but there's no mechanism set up yet.

Broken test:

  1. https://github.com/opensearch-project/neural-search/actions/runs/10258762961/job/28382116242?pr=852
  2. https://github.com/opensearch-project/neural-search/actions/runs/9904116192/job/27602462802?pr=832

What solution would you like?

Can we refactor HybridQueryPhaseSearcherTests to make it not directly refer to those k-NN types?

What alternatives have you considered?

k-NN repo to have a mechanism to ensure backward compatibility.

Do you have any additional context?

NA

@chishui chishui changed the title [FEATURE] Refactor HybridQueryPhaseSearcherTests to decouple types from K-NN library [FEATURE] Refactor HybridQueryPhaseSearcherTests/HybridQueryTests to decouple types from K-NN library Aug 6, 2024
@chishui chishui changed the title [FEATURE] Refactor HybridQueryPhaseSearcherTests/HybridQueryTests to decouple types from K-NN library [FEATURE] Refactor HybridQuery*Tests to decouple types from K-NN library Aug 6, 2024
@martin-gaievski
Copy link
Member

We should be able to refactor HybridQueryPhaseSearcherTests and get rid of KNN specific classes, in those test cases knn query isn't required and can be replaced by other core query.
But there are other tests where it's not trivial, like HybridQueryTests, where knn query is a must

@martin-gaievski martin-gaievski added Refactoring Improve the design, structure, and implementation while preserving its functionality good first issue Good for newcomers help wanted Extra attention is needed and removed untriaged enhancement labels Aug 7, 2024
@chishui
Copy link
Contributor Author

chishui commented Aug 8, 2024

neural-search build failed due to another k-NN update.

org.opensearch.neuralsearch.query.HybridQueryBuilderTests.testDoToQuery_whenOneSubquery_thenBuildSuccessfully(HybridQueryBuilderTests.java:102)
  2> REPRODUCE WITH: ./gradlew ':test' --tests "org.opensearch.neuralsearch.query.HybridQueryBuilderTests.testDoToQuery_whenMultipleSubqueries_thenBuildSuccessfully" -Dtests.seed=157CC6D10AE9F68D -Dtests.security.manager=false -Dtests.locale=to-Latn-TO -Dtests.timezone=GMT0 -Druntime.java=21
  2> java.lang.NullPointerException: Cannot invoke "org.opensearch.cluster.service.ClusterService.getClusterSettings()" because "this.clusterService" is null
        at __randomizedtesting.SeedInfo.seed([157CC6D10AE9F68D:542142C44FAC5854]:0)
        at org.opensearch.knn.index.KNNSettings.getSettingValue(KNNSettings.java:346)
        at org.opensearch.knn.common.featureflags.KNNFeatureFlags.isKnnQueryRewriteEnabled(KNNFeatureFlags.java:44)
        at org.opensearch.knn.index.query.KNNQueryFactory.create(KNNQueryFactory.java:129)
        at org.opensearch.knn.index.query.KNNQueryBuilder.doToQuery(KNNQueryBuilder.java:475)
        at org.opensearch.index.query.AbstractQueryBuilder.toQuery(AbstractQueryBuilder.java:117)
        at org.opensearch.neuralsearch.query.HybridQueryBuilder.lambda$toQueries$0(HybridQueryBuilder.java:292)
        at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
        at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1708)
        at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
        at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
        at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:921)
        at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
        at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:682)
        at org.opensearch.neuralsearch.query.HybridQueryBuilder.toQueries(HybridQueryBuilder.java:296)
        at org.opensearch.neuralsearch.query.HybridQueryBuilder.doToQuery(HybridQueryBuilder.java:112)
        at 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed Refactoring Improve the design, structure, and implementation while preserving its functionality
Projects
None yet
Development

No branches or pull requests

3 participants