diff --git a/CHANGELOG.md b/CHANGELOG.md index f0d35c921ccd5..a22a2377ad3c6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Fix flaky test FieldDataLoadingIT.testIndicesFieldDataCacheSizeSetting ([#19571](https://github.com/opensearch-project/OpenSearch/pull/19571)) - Avoid primary shard failure caused by merged segment warmer exceptions ([#19436](https://github.com/opensearch-project/OpenSearch/pull/19436)) - Fix pull-based ingestion out-of-bounds offset scenarios and remove persisted offsets ([#19607](https://github.com/opensearch-project/OpenSearch/pull/19607)) +- Fix NPE of ScriptScoreQuery ([#19650](https://github.com/opensearch-project/OpenSearch/pull/19650)) ### Dependencies - Update to Gradle 9.1 ([#19575](https://github.com/opensearch-project/OpenSearch/pull/19575)) diff --git a/server/src/internalClusterTest/java/org/opensearch/search/query/ScriptScoreQueryIT.java b/server/src/internalClusterTest/java/org/opensearch/search/query/ScriptScoreQueryIT.java index 136ddce152f63..966b2d81e3fd1 100644 --- a/server/src/internalClusterTest/java/org/opensearch/search/query/ScriptScoreQueryIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/search/query/ScriptScoreQueryIT.java @@ -55,6 +55,7 @@ import java.util.function.Function; import static org.opensearch.index.query.QueryBuilders.boolQuery; +import static org.opensearch.index.query.QueryBuilders.idsQuery; import static org.opensearch.index.query.QueryBuilders.matchQuery; import static org.opensearch.index.query.QueryBuilders.scriptScoreQuery; import static org.opensearch.search.SearchService.CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING; @@ -217,4 +218,24 @@ public void testDisallowExpensiveQueries() throws Exception { assertAcked(client().admin().cluster().updateSettings(updateSettingsRequest).actionGet()); } } + + // test case added for issue https://github.com/opensearch-project/OpenSearch/issues/18446 + public void testScriptScoreNotExistingQuery() throws Exception { + assertAcked(prepareCreate("test-index").setMapping("field2", "type=double")); + client().prepareIndex("test-index").setId("1").setSource("field2", 1).get(); + refresh(); + indexRandomForConcurrentSearch("test-index"); + + Map params = new HashMap<>(); + params.put("param1", 0.1); + Script script = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "doc['field2'].value * param1", params); + QueryBuilder idsQuery = idsQuery().addIds("2"); + QueryBuilder scriptScoreQuery = scriptScoreQuery(idsQuery, script); + // Issue 18446 arises because of null Scorer returned from ScorerSupplier. + // However, Lucene prefers bulkScorer() instead of scorer() which doesn't trigger NPE as stated in issue 18446. + // As a result, we have to set profile to true to force the usage of scorer() instead of bulkScorer(), + // to make sure we test the intended code path + SearchResponse resp = client().prepareSearch("test-index").setQuery(scriptScoreQuery).setProfile(true).get(); + assertNoFailures(resp); + } } diff --git a/server/src/main/java/org/opensearch/common/lucene/search/function/ScriptScoreQuery.java b/server/src/main/java/org/opensearch/common/lucene/search/function/ScriptScoreQuery.java index 6a2b7ded93b4f..a835deef066e9 100644 --- a/server/src/main/java/org/opensearch/common/lucene/search/function/ScriptScoreQuery.java +++ b/server/src/main/java/org/opensearch/common/lucene/search/function/ScriptScoreQuery.java @@ -124,72 +124,45 @@ public Weight createWeight(IndexSearcher searcher, ScoreMode scoreMode, float bo Weight subQueryWeight = subQuery.createWeight(searcher, subQueryScoreMode, 1.0f); return new Weight(this) { + @Override public ScorerSupplier scorerSupplier(LeafReaderContext context) throws IOException { - final Weight weight = this; + ScorerSupplier subQueryScorerSupplier = subQueryWeight.scorerSupplier(context); + if (subQueryScorerSupplier == null) { + return null; + } + Weight weight = this; return new ScorerSupplier() { - private Scorer scorer; - private BulkScorer bulkScorer; - @Override public BulkScorer bulkScorer() throws IOException { if (minScore == null) { - final BulkScorer subQueryBulkScorer = subQueryWeight.bulkScorer(context); - if (subQueryBulkScorer == null) { - bulkScorer = null; - } else { - bulkScorer = new ScriptScoreBulkScorer( - subQueryBulkScorer, - subQueryScoreMode, - makeScoreScript(context), - boost - ); - } + final BulkScorer subQueryBulkScorer = subQueryScorerSupplier.bulkScorer(); + return new ScriptScoreBulkScorer(subQueryBulkScorer, subQueryScoreMode, makeScoreScript(context), boost); } else { - final Scorer scorer = get(Long.MAX_VALUE); - if (scorer != null) { - bulkScorer = new Weight.DefaultBulkScorer(scorer); - } + return super.bulkScorer(); } - - return bulkScorer; } @Override public Scorer get(long leadCost) throws IOException { - final Scorer subQueryScorer = subQueryWeight.scorer(context); - - if (subQueryScorer == null) { - scorer = null; - } else { - Scorer scriptScorer = new ScriptScorer( - weight, - makeScoreScript(context), - subQueryScorer, - subQueryScoreMode, - boost, - null - ); - if (minScore != null) { - scriptScorer = new MinScoreScorer(weight, scriptScorer, minScore); - } - scorer = scriptScorer; + Scorer subQueryScorer = subQueryScorerSupplier.get(leadCost); + Scorer scriptScorer = new ScriptScorer( + weight, + makeScoreScript(context), + subQueryScorer, + subQueryScoreMode, + boost, + null + ); + if (minScore != null) { + scriptScorer = new MinScoreScorer(weight, scriptScorer, minScore); } - - return scorer; + return scriptScorer; } @Override public long cost() { - if (scorer != null) { - return scorer.iterator().cost(); - } else if (bulkScorer != null) { - return bulkScorer.cost(); - } else { - // We have no prior knowledge of how many docs might match for any given query term, - // so we assume that all docs could be a match. - return Integer.MAX_VALUE; - } + return subQueryScorerSupplier.cost(); } }; } diff --git a/server/src/test/java/org/opensearch/search/query/ScriptScoreQueryTests.java b/server/src/test/java/org/opensearch/search/query/ScriptScoreQueryTests.java index 55c50b8cf854d..22086ba06de97 100644 --- a/server/src/test/java/org/opensearch/search/query/ScriptScoreQueryTests.java +++ b/server/src/test/java/org/opensearch/search/query/ScriptScoreQueryTests.java @@ -44,8 +44,10 @@ import org.apache.lucene.search.IndexSearcher; import org.apache.lucene.search.MatchAllDocsQuery; import org.apache.lucene.search.Query; +import org.apache.lucene.search.QueryVisitor; import org.apache.lucene.search.ScoreMode; import org.apache.lucene.search.Scorer; +import org.apache.lucene.search.ScorerSupplier; import org.apache.lucene.search.TwoPhaseIterator; import org.apache.lucene.search.Weight; import org.apache.lucene.store.Directory; @@ -216,6 +218,64 @@ public void testTwoPhaseIteratorDelegation() throws IOException { assertTrue("Expected to find at least one matching document", foundMatchingDoc); } + public void testNullScorerSupplier() throws IOException { + Script script = new Script("script using explain"); + ScoreScript.LeafFactory factory = newFactory(script, true, explanation -> { + assertNotNull(explanation); + explanation.set("this explains the score"); + return 1.0; + }); + + ScriptScoreQuery query = new ScriptScoreQuery(new NullScorerSupplierQuery(), script, factory, null, "index", 0, Version.CURRENT); + Weight weight = query.createWeight(searcher, ScoreMode.COMPLETE, 1.0f); + ScorerSupplier scorerSupplier = weight.scorerSupplier(null); + assertNull(scorerSupplier); + } + + private static class NullScorerSupplierQuery extends Query { + + @Override + public String toString(String field) { + return getClass().getSimpleName(); + } + + @Override + public void visit(QueryVisitor visitor) { + visitor.visitLeaf(this); + } + + @Override + public boolean equals(Object obj) { + return this == obj; + } + + @Override + public int hashCode() { + return 0; + } + + @Override + public Weight createWeight(IndexSearcher searcher, ScoreMode scoreMode, float boost) throws IOException { + return new Weight(this) { + + @Override + public Explanation explain(LeafReaderContext context, int doc) throws IOException { + throw new UnsupportedOperationException(); + } + + @Override + public ScorerSupplier scorerSupplier(LeafReaderContext context) throws IOException { + return null; + } + + @Override + public boolean isCacheable(LeafReaderContext ctx) { + return true; + } + }; + } + } + private ScoreScript.LeafFactory newFactory( Script script, boolean needsScore,