search

opensearch-project / neural-search

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

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

Open chishui opened 1 month ago

chishui commented 1 month ago

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

martin-gaievski commented 1 month ago

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

chishui commented 1 month ago

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