From 7320cd3c2b253718684eceebf1d327e6c9f8a75b Mon Sep 17 00:00:00 2001 From: Varun Jain Date: Tue, 23 Jan 2024 05:56:21 +0530 Subject: [PATCH 1/2] Fixing pulling of security plugin to run integ tests in secure opensearch cluster (#543) * Fixing security plugin archive Signed-off-by: Varun Jain --- .github/workflows/CI.yml | 13 +++++++++---- .../backwards_compatibility_tests_workflow.yml | 9 ++++++--- .github/workflows/test_security.yml | 3 ++- build.gradle | 11 +++++------ 4 files changed, 22 insertions(+), 14 deletions(-) diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index e3f749fac..3f89a3185 100644 --- a/.github/workflows/CI.yml +++ b/.github/workflows/CI.yml @@ -21,7 +21,8 @@ jobs: needs: Get-CI-Image-Tag strategy: matrix: - java: [11, 17, 21] + # Restricting java 21 to 21.0.1 due to ongoing bug in JDK 21.0.2 https://bugs.openjdk.org/browse/JDK-8323659. Once the fix https://github.com/opensearch-project/OpenSearch/pull/11968 get merged this change will be reverted. + java: [11, 17, 21.0.1] os: [ubuntu-latest] name: Gradle Check Linux @@ -33,6 +34,7 @@ jobs: # need to switch to root so that github actions can install runner binary on container without permission issues. options: --user root + steps: - name: Checkout neural-search uses: actions/checkout@v1 @@ -55,7 +57,8 @@ jobs: Check-neural-search-windows: strategy: matrix: - java: [11, 17, 21] + # Restricting java 21 to 21.0.1 due to ongoing bug in JDK 21.0.2 https://bugs.openjdk.org/browse/JDK-8323659. Once the fix https://github.com/opensearch-project/OpenSearch/pull/11968 get merged this change will be reverted. + java: [11, 17, 21.0.1] os: [windows-latest] name: Gradle Check Windows @@ -83,7 +86,8 @@ jobs: needs: Get-CI-Image-Tag strategy: matrix: - java: [11, 17, 21] + # Restricting java 21 to 21.0.1 due to ongoing bug in JDK 21.0.2 https://bugs.openjdk.org/browse/JDK-8323659. Once the fix https://github.com/opensearch-project/OpenSearch/pull/11968 get merged this change will be reverted. + java: [11, 17, 21.0.1] os: [ubuntu-latest] name: Pre-commit Linux @@ -117,7 +121,8 @@ jobs: Precommit-neural-search-windows: strategy: matrix: - java: [11, 17, 21] + # Restricting java 21 to 21.0.1 due to ongoing bug in JDK 21.0.2 https://bugs.openjdk.org/browse/JDK-8323659. Once the fix https://github.com/opensearch-project/OpenSearch/pull/11968 get merged this change will be reverted. + java: [11, 17, 21.0.1] os: [windows-latest] name: Pre-commit Windows diff --git a/.github/workflows/backwards_compatibility_tests_workflow.yml b/.github/workflows/backwards_compatibility_tests_workflow.yml index 32a71c5d0..9b34b6356 100644 --- a/.github/workflows/backwards_compatibility_tests_workflow.yml +++ b/.github/workflows/backwards_compatibility_tests_workflow.yml @@ -13,7 +13,8 @@ jobs: Restart-Upgrade-BWCTests-NeuralSearch: strategy: matrix: - java: [ 11, 17, 21 ] + # Restricting java 21 to 21.0.1 due to ongoing bug in JDK 21.0.2 https://bugs.openjdk.org/browse/JDK-8323659. Once the fix https://github.com/opensearch-project/OpenSearch/pull/11968 get merged this change will be reverted. + java: [ 11, 17, 21.0.1 ] os: [ubuntu-latest,windows-latest] bwc_version : ["2.9.0","2.10.0","2.11.0","2.12.0-SNAPSHOT"] opensearch_version : [ "3.0.0-SNAPSHOT" ] @@ -35,7 +36,8 @@ jobs: - name: Run NeuralSearch Restart-Upgrade BWC Tests from BWCVersion-${{ matrix.bwc_version }} to OpenSearch Version-${{ matrix.opensearch_version }} on ${{matrix.os}} run: | echo "Running restart-upgrade backwards compatibility tests ..." - ./gradlew :qa:restart-upgrade:testAgainstNewCluster -D'tests.bwc.version=${{ matrix.bwc_version }}' +# Disabling BWC tests due to ongoing build failure. https://github.com/opensearch-project/neural-search/issues/536 +# ./gradlew :qa:restart-upgrade:testAgainstNewCluster -D'tests.bwc.version=${{ matrix.bwc_version }}' Rolling-Upgrade-BWCTests-NeuralSearch: strategy: @@ -62,4 +64,5 @@ jobs: - name: Run NeuralSearch Rolling-Upgrade BWC Tests from BWCVersion-${{ matrix.bwc_version }} to OpenSearch Version-${{ matrix.opensearch_version }} on ${{matrix.os}} run: | echo "Running rolling-upgrade backwards compatibility tests ..." - ./gradlew :qa:rolling-upgrade:testRollingUpgrade -D'tests.bwc.version=${{ matrix.bwc_version }}' +# Disabling BWC tests due to ongoing build failure. https://github.com/opensearch-project/neural-search/issues/536 +# ./gradlew :qa:rolling-upgrade:testRollingUpgrade -D'tests.bwc.version=${{ matrix.bwc_version }}' diff --git a/.github/workflows/test_security.yml b/.github/workflows/test_security.yml index 5b7f9d210..163b8c220 100644 --- a/.github/workflows/test_security.yml +++ b/.github/workflows/test_security.yml @@ -20,7 +20,8 @@ jobs: integ-test-with-security-linux: strategy: matrix: - java: [11, 17, 21] + # Restricting java 21 to 21.0.1 due to ongoing bug in JDK 21.0.2 https://bugs.openjdk.org/browse/JDK-8323659. Once the fix https://github.com/opensearch-project/OpenSearch/pull/11968 get merged this change will be reverted. + java: [11, 17, 21.0.1] name: Run Integration Tests on Linux runs-on: ubuntu-latest diff --git a/build.gradle b/build.gradle index c4228d8be..c2dc3a317 100644 --- a/build.gradle +++ b/build.gradle @@ -66,7 +66,7 @@ ext { projectSubstitutions = [:] configureSecurityPlugin = { OpenSearchCluster cluster -> - configurations.zipArchive.asFileTree.each { + configurations.secureIntegTestPluginArchive.asFileTree.each { if(it.name.contains("opensearch-security")){ cluster.plugin(provider(new Callable() { @Override @@ -160,6 +160,7 @@ allprojects { configurations { zipArchive + secureIntegTestPluginArchive } tasks.register("preparePluginPathDirs") { @@ -231,7 +232,7 @@ dependencies { api "org.opensearch:opensearch:${opensearch_version}" zipArchive group: 'org.opensearch.plugin', name:'opensearch-knn', version: "${opensearch_build}" zipArchive group: 'org.opensearch.plugin', name:'opensearch-ml-plugin', version: "${opensearch_build}" - zipArchive group: 'org.opensearch.plugin', name:'opensearch-security', version: "${opensearch_build}" + secureIntegTestPluginArchive group: 'org.opensearch.plugin', name:'opensearch-security', version: "${opensearch_build}" compileOnly fileTree(dir: knnJarDirectory, include: '*.jar') api group: 'org.opensearch', name:'opensearch-ml-client', version: "${opensearch_build}" testFixturesImplementation "org.opensearch.test:framework:${opensearch_version}" @@ -337,8 +338,7 @@ testClusters.integTest { // Install K-NN/ml-commons plugins on the integTest cluster nodes except security configurations.zipArchive.asFileTree.each { - if(!it.name.contains("opensearch-security")) { - plugin(provider(new Callable(){ + plugin(provider(new Callable(){ @Override RegularFile call() throws Exception { return new RegularFile() { @@ -348,8 +348,7 @@ testClusters.integTest { } } } - })) - } + })) } // This installs our neural-search plugin into the testClusters From 98e5534929efb054e7b5d4382af46b0626ef1f50 Mon Sep 17 00:00:00 2001 From: Varun Jain Date: Tue, 23 Jan 2024 23:23:21 +0530 Subject: [PATCH 2/2] Enable support for default model id on HybridQueryBuilder (#541) * Enable support for default model id on HybridQueryBuilder Signed-off-by: Varun Jain * Adding tests and updating changelog.md Signed-off-by: Varun Jain * Optimizing code Signed-off-by: Varun Jain * modyfing the tests4 Signed-off-by: Varun Jain * Addressing Heemin comment Signed-off-by: Varun Jain --------- Signed-off-by: Varun Jain --- CHANGELOG.md | 1 + .../query/HybridQueryBuilder.java | 16 +++++++++++ .../NeuralQueryEnricherProcessorIT.java | 20 ++++++++++++++ .../query/HybridQueryBuilderTests.java | 8 ++++++ .../query/OpenSearchQueryTestCase.java | 27 +++++++++++++++---- 5 files changed, 67 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ede506f2b..922016575 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Bug Fixes - Fixing multiple issues reported in #497 ([#524](https://github.com/opensearch-project/neural-search/pull/524)) - Fix Flaky test reported in #433 ([#533](https://github.com/opensearch-project/neural-search/pull/533)) +- Enable support for default model id on HybridQueryBuilder ([#541](https://github.com/opensearch-project/neural-search/pull/541)) ### Infrastructure - BWC tests for Neural Search ([#515](https://github.com/opensearch-project/neural-search/pull/515)) - Github action to run integ tests in secure opensearch cluster ([#535](https://github.com/opensearch-project/neural-search/pull/535)) diff --git a/src/main/java/org/opensearch/neuralsearch/query/HybridQueryBuilder.java b/src/main/java/org/opensearch/neuralsearch/query/HybridQueryBuilder.java index b0104639b..aa4242c2e 100644 --- a/src/main/java/org/opensearch/neuralsearch/query/HybridQueryBuilder.java +++ b/src/main/java/org/opensearch/neuralsearch/query/HybridQueryBuilder.java @@ -13,6 +13,7 @@ import java.util.stream.Collectors; import org.apache.commons.lang.builder.EqualsBuilder; +import org.apache.lucene.search.BooleanClause.Occur; import org.apache.lucene.search.Query; import org.opensearch.common.lucene.search.Queries; import org.opensearch.core.ParseField; @@ -27,6 +28,7 @@ import org.opensearch.index.query.QueryShardContext; import org.opensearch.index.query.QueryShardException; import org.opensearch.index.query.Rewriteable; +import org.opensearch.index.query.QueryBuilderVisitor; import lombok.Getter; import lombok.NoArgsConstructor; @@ -295,4 +297,18 @@ private Collection toQueries(Collection queryBuilders, Quer }).filter(Objects::nonNull).collect(Collectors.toList()); return queries; } + + /** + * visit method to parse the HybridQueryBuilder by a visitor + */ + @Override + public void visit(QueryBuilderVisitor visitor) { + visitor.accept(this); + // getChildVisitor of NeuralSearchQueryVisitor return this. + // therefore any argument can be passed. Here we have used Occcur.MUST as an argument. + QueryBuilderVisitor subVisitor = visitor.getChildVisitor(Occur.MUST); + for (QueryBuilder subQueryBuilder : queries) { + subQueryBuilder.visit(subVisitor); + } + } } diff --git a/src/test/java/org/opensearch/neuralsearch/processor/NeuralQueryEnricherProcessorIT.java b/src/test/java/org/opensearch/neuralsearch/processor/NeuralQueryEnricherProcessorIT.java index 3c8adf5b3..b37db7c82 100644 --- a/src/test/java/org/opensearch/neuralsearch/processor/NeuralQueryEnricherProcessorIT.java +++ b/src/test/java/org/opensearch/neuralsearch/processor/NeuralQueryEnricherProcessorIT.java @@ -15,6 +15,7 @@ import org.junit.Before; import org.opensearch.common.settings.Settings; import org.opensearch.neuralsearch.BaseNeuralSearchIT; +import org.opensearch.neuralsearch.query.HybridQueryBuilder; import org.opensearch.neuralsearch.query.NeuralQueryBuilder; import com.google.common.primitives.Floats; @@ -62,6 +63,25 @@ public void testNeuralQueryEnricherProcessor_whenNoModelIdPassed_thenSuccess() { } + @SneakyThrows + public void testNeuralQueryEnricherProcessor_whenHybridQueryBuilderAndNoModelIdPassed_thenSuccess() { + initializeIndexIfNotExist(); + String modelId = getDeployedModelId(); + createSearchRequestProcessor(modelId, search_pipeline); + createPipelineProcessor(modelId, ingest_pipeline); + updateIndexSettings(index, Settings.builder().put("index.search.default_pipeline", search_pipeline)); + NeuralQueryBuilder neuralQueryBuilder = new NeuralQueryBuilder(); + neuralQueryBuilder.fieldName(TEST_KNN_VECTOR_FIELD_NAME_1); + neuralQueryBuilder.queryText("Hello World"); + neuralQueryBuilder.k(1); + HybridQueryBuilder hybridQueryBuilder = new HybridQueryBuilder(); + hybridQueryBuilder.add(neuralQueryBuilder); + Map response = search(index, hybridQueryBuilder, 2); + + assertFalse(response.isEmpty()); + + } + @SneakyThrows private void initializeIndexIfNotExist() { if (index.equals(NeuralQueryEnricherProcessorIT.index) && !indexExists(index)) { diff --git a/src/test/java/org/opensearch/neuralsearch/query/HybridQueryBuilderTests.java b/src/test/java/org/opensearch/neuralsearch/query/HybridQueryBuilderTests.java index 7bae96780..72ea8fa7b 100644 --- a/src/test/java/org/opensearch/neuralsearch/query/HybridQueryBuilderTests.java +++ b/src/test/java/org/opensearch/neuralsearch/query/HybridQueryBuilderTests.java @@ -20,6 +20,7 @@ import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.ArrayList; import java.util.function.Supplier; import org.apache.lucene.search.MatchNoDocsQuery; @@ -708,6 +709,13 @@ public void testBoost_whenDefaultBoostSet_thenBuildSuccessfully() { assertNotNull(hybridQueryBuilder); } + public void testVisit() { + HybridQueryBuilder hybridQueryBuilder = new HybridQueryBuilder().add(new NeuralQueryBuilder()).add(new NeuralSparseQueryBuilder()); + List visitedQueries = new ArrayList<>(); + hybridQueryBuilder.visit(createTestVisitor(visitedQueries)); + assertEquals(3, visitedQueries.size()); + } + private Map getInnerMap(Object innerObject, String queryName, String fieldName) { if (!(innerObject instanceof Map)) { fail("field name does not map to nested object"); diff --git a/src/test/java/org/opensearch/neuralsearch/query/OpenSearchQueryTestCase.java b/src/test/java/org/opensearch/neuralsearch/query/OpenSearchQueryTestCase.java index 97e1f7c5d..a1e8210e6 100644 --- a/src/test/java/org/opensearch/neuralsearch/query/OpenSearchQueryTestCase.java +++ b/src/test/java/org/opensearch/neuralsearch/query/OpenSearchQueryTestCase.java @@ -7,6 +7,14 @@ import static java.util.Collections.emptyMap; import static java.util.Collections.singletonMap; import static java.util.stream.Collectors.toList; +import org.apache.lucene.search.DocIdSetIterator; +import org.apache.lucene.search.Explanation; +import org.apache.lucene.search.Query; +import org.apache.lucene.search.Scorer; +import org.apache.lucene.search.Weight; +import org.apache.lucene.search.BooleanClause; +import org.opensearch.index.query.QueryBuilder; +import org.opensearch.index.query.QueryBuilderVisitor; import static org.opensearch.neuralsearch.settings.NeuralSearchSettings.NEURAL_SEARCH_HYBRID_SEARCH_DISABLED; import java.io.IOException; @@ -20,11 +28,6 @@ import org.apache.lucene.document.FieldType; import org.apache.lucene.document.TextField; import org.apache.lucene.index.LeafReaderContext; -import org.apache.lucene.search.DocIdSetIterator; -import org.apache.lucene.search.Explanation; -import org.apache.lucene.search.Query; -import org.apache.lucene.search.Scorer; -import org.apache.lucene.search.Weight; import org.opensearch.Version; import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.common.CheckedConsumer; @@ -230,4 +233,18 @@ public float getMaxScore(int upTo) { protected static void initFeatureFlags() { System.setProperty(NEURAL_SEARCH_HYBRID_SEARCH_DISABLED.getKey(), "true"); } + + protected static QueryBuilderVisitor createTestVisitor(List visitedQueries) { + return new QueryBuilderVisitor() { + @Override + public void accept(QueryBuilder qb) { + visitedQueries.add(qb); + } + + @Override + public QueryBuilderVisitor getChildVisitor(BooleanClause.Occur occur) { + return this; + } + }; + } }