Fixed exception for case when Hybrid query being wrapped into bool query

[martin-gaievski]

Description

There is a runtime NullPointer exception in case hybrid query is:

• wrapped into Bool query
  AND
• for one of the sub-queries in hybrid query there are no matches.

It's not directly related to the fact that hybrid is wrapped into some other query. That case needs to be discussed separately, my current opinion is - we should not allow execution of such query, hybrid must be the top level query. This particular PR is focused on a runtime exception, and does not affect or lock us on a particular approach for a wrapped hybrid query case.

For instance if we send following search request:

GET /index-test/_search?search_pipeline=nlp-search-pipeline

{
    "query": {
        "bool": {
            "should": [
                {
                    "hybrid": {
                        "queries": [
                            {
                                "match": {
                                    "name": {
                                        "query": "wild west"
                                    }
                                }
                            },
                            {
                                "match": {
                                    "name": {
                                        "query": "team"
                                    }
                                }
                            }
                        ]
                    }
                },
                {
                    "match": {
                        "name": {
                            "query": "university"
                        }
                    }
                }
            ]
        }
    }
}

Response looks something like one below:

{
    "error": {
        "root_cause": [
            {
                "type": "null_pointer_exception",
                "reason": "Cannot invoke \"org.apache.lucene.search.Scorer.docID()\" because \"scorer\" is null"
            }
        ],
        "type": "search_phase_execution_exception",
        "reason": "all shards failed",
        "phase": "query",
        "grouped": true,
        "failed_shards": [
            {
                "shard": 0,
                "index": "index-test",
                "node": "Ibk3JAobTkO8VygwSsXZ0g",
                "reason": {
                    "type": "null_pointer_exception",
                    "reason": "Cannot invoke \"org.apache.lucene.search.Scorer.docID()\" because \"scorer\" is null"
                }
            }
        ],
        "caused_by": {
            "type": "null_pointer_exception",
            "reason": "Cannot invoke \"org.apache.lucene.search.Scorer.docID()\" because \"scorer\" is null",
            "caused_by": {
                "type": "null_pointer_exception",
                "reason": "Cannot invoke \"org.apache.lucene.search.Scorer.docID()\" because \"scorer\" is null"
            }
        }
    },
    "status": 500
}

this is what is the server logs:

[2023-11-14T11:35:06,517][WARN ][r.suppressed             ] [integTest-0] path: /index-test/_search, params: {search_pipeline=nlp-search-pipeline, index=index-test}
org.opensearch.action.search.SearchPhaseExecutionException: all shards failed
        at org.opensearch.action.search.AbstractSearchAsyncAction.onPhaseFailure(AbstractSearchAsyncAction.java:708) [opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
...
Caused by: org.opensearch.OpenSearchException$3: Cannot invoke "org.apache.lucene.search.Scorer.docID()" because "scorer" is null
        at org.opensearch.OpenSearchException.guessRootCauses(OpenSearchException.java:708) ~[opensearch-core-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
        at org.opensearch.action.search.AbstractSearchAsyncAction.executeNextPhase(AbstractSearchAsyncAction.java:378) [opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
        ... 23 more
Caused by: java.lang.NullPointerException: Cannot invoke "org.apache.lucene.search.Scorer.docID()" because "scorer" is null
        at org.opensearch.neuralsearch.query.HybridQueryScorer.lambda$getMaxScore$0(HybridQueryScorer.java:88) ~[?:?]
        at java.base/java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:178) ~[?:?]
        at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1625) ~[?:?]
        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.reduce(ReferencePipeline.java:662) ~[?:?]
        at java.base/java.util.stream.ReferencePipeline.max(ReferencePipeline.java:698) ~[?:?]
        at org.opensearch.neuralsearch.query.HybridQueryScorer.getMaxScore(HybridQueryScorer.java:94) ~[?:?]

Expected result is that we shouldn't have any exceptions, and individual scores are summarized based on the rules of wrapping Bool query. Below is example of how successful response may look like:

{
    "took": 36,
    "timed_out": false,
    "_shards": {
        "total": 1,
        "successful": 1,
        "skipped": 0,
        "failed": 0
    },
    "hits": {
        "total": {
            "value": 4,
            "relation": "eq"
        },
        "max_score": 4.3169837,
        "hits": [
            {
                "_index": "index-test",
                "_id": "1",
                "_score": 4.3169837,
                "_source": {
                    "name": "A West Virginia university women 's basketball team , officials , and a small gathering of fans are in a West Virginia arena .",
                    "id": "4319130149.jpg"
                }
            },
            {
                "_index": "index-test",
                "_id": "2",
                "_score": 0.58093566,
                "_source": {
                    "name": "A wild animal races across an uncut field with a minimal amount of trees .",
                    "id": "1775029934.jpg"
                }
            },
            {
                "_index": "index-test",
                "_id": "5",
                "_score": 0.55228686,
                "_source": {
                    "name": "A rodeo cowboy , wearing a cowboy hat , is being thrown off of a wild white horse .",
                    "id": "2691147709.jpg"
                }
            },
            {
                "_index": "index-test",
                "_id": "4",
                "_score": 0.53899646,
                "_source": {
                    "name": "A man who is riding a wild horse in the rodeo is very near to falling off .",
                    "id": "4427058951.jpg"
                }
            }
        ]
    }
}

Check List

• All tests pass
• Commits are signed as per the DCO using --signoff

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.

[codecov]

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (00c5589) 84.37% compared to head (f262695) 84.65%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
...h/neuralsearch/query/NeuralSparseQueryBuilder.java	81.25%	0 Missing and 3 partials ⚠️
...ensearch/neuralsearch/query/HybridQueryScorer.java	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #490      +/-   ##
============================================
+ Coverage     84.37%   84.65%   +0.27%     
- Complexity      498      508      +10     
============================================
  Files            40       40              
  Lines          1491     1505      +14     
  Branches        228      234       +6     
============================================
+ Hits           1258     1274      +16     
+ Misses          133      128       -5     
- Partials        100      103       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[navneet1v]

@martin-gaievski can you add the exception signature in the PR which we are getting?

[navneet1v]

@martin-gaievski can you add what is the expected outcome with this change when a hybrid query is inside a bool query?

[navneet1v]

@martin-gaievski the integration tests are also failing. If they are flaky lets fix them.

[martin-gaievski]

@martin-gaievski can you add the exception signature in the PR which we are getting?

I've updated PR header with the error response and server log entry

[martin-gaievski]

@martin-gaievski can you add what is the expected outcome with this change when a hybrid query is inside a bool query?

Yes, sure, I've updated PR with a sample of expected response. Basically scores should be summarized based on Bool clauses.

[martin-gaievski]

@martin-gaievski the integration tests are also failing. If they are flaky lets fix them.

Ack, fixed the test.

now it passes with the same seed:

neural-search % ./gradlew ':test' --tests "org.opensearch.neuralsearch.query.HybridQueryWeightTests.testScorerIterator_whenExecuteQuery_thenScorerIteratorSuccessful" -Dtests.seed=9073359D6B181E6E
=======================================
OpenSearch Build Hamster says Hello!
  Gradle Version        : 8.1.1
  OS Info               : Mac OS X 13.5.2 (x86_64)
  JDK Version           : 17 (Amazon Corretto JDK)
  JAVA_HOME             : /Users/gaievski/.sdkman/candidates/java/17.0.5-amzn
  Random Testing Seed   : 9073359D6B181E6E
  In FIPS 140 mode      : false
=======================================

> Task :compileTestJava
Note: /Users/user/dev/opensearch/neural-search/src/test/java/org/opensearch/neuralsearch/query/HybridQueryWeightTests.java uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.

BUILD SUCCESSFUL in 14s
9 actionable tasks: 2 executed, 7 up-to-date

[sam-herman]

Do we also have a task to not allow such queries in the future?

[martin-gaievski]

Yes, this is the general plan, but there are cases when it's not that easy. For instance, when nested field is present in the index mapping then the query will be wrapped into the Bool query where hybrid will be one of the child clause. We're investigating possible options.

[navneet1v]

@martin-gaievski can you link the github issue here?

[martin-gaievski]

sure, this is a link to an issue #466

[opensearch-trigger-bot]

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-490-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 b3c73bdaae28629d9be4128afcf549c790070887
# Push it to GitHub
git push --set-upstream origin backport/backport-490-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-490-to-2.x.