Support for post filter in hybrid query

[vibrantvarun]

Description

This PR adds Support for Post filter in hybrid query.

The crux of the logic implemented in this PR is when the search request has post filter query in that, then instead of returning plain HybridTopDocCollector Object we would be returning FilteredCollectorObject which will have HybridTopDocCollector object wrapped underneath it.

Also This PR has unit tests and Integration tests for the post_filter scenario.

Testing example on local machine

1. Create index

http://localhost:9200/movies

{
  "settings": {
    "index": {
      "number_of_shards": 1,
      "number_of_replicas": 0
    },
    "index.search.concurrent_segment_search.enabled": false
  },
  "mappings": {
    "properties": {
      "name": {
        "type": "text"
      },
      "stock":{
        "type": "integer"
      }
    }
  }
}

2. Create pipeline

http://localhost:9200/_search/pipeline/nlp-search-pipeline

{
  "description": "Post processor for hybrid search",
  "phase_results_processors": [
    {
      "normalization-processor": {
        "normalization": {
          "technique": "min_max"
        },
        "combination": {
          "technique": "arithmetic_mean",
          "parameters": {
            "weights": [
              0.3,
              0.6,
              0.1
            ]
          }
        }
      }
    }
  ]
}

3. Add documents

{
  "name": "Dunes part 1",
  "stock":2
}

{
  "name": "Dunes part 2",
  "stock":26
}

{
  "name": "Mission Impossible 1",
  "stock":234
}

{
  "name": "Mission Impossible 2",
  "stock":238
}

{
  "name": "Avengers",
  "stock":300
}

4. search without post_filter

http://localhost:9200/movies/_search?search_pipeline=nlp-search-pipeline

{
   "query": {
      "hybrid":{
          "queries":[
              { 
                "match":{
                    "name": "mission"
                }
              },
              {
                "term":{
                    "name":{
                        "value":"part"
                    }
                }
              },
              {
              "range": {
                 "stock": {
                      "gte": 200,
                      "lte": 400
                  }
                }
              }
          ]
      }

  }
}


{
    "took": 12,
    "timed_out": false,
    "_shards": {
        "total": 1,
        "successful": 1,
        "skipped": 0,
        "failed": 0
    },
    "hits": {
        "total": {
            "value": 5,
            "relation": "eq"
        },
        "max_score": 0.70000005,
        "hits": [
            {
                "_index": "movies",
                "_id": "2",
                "_score": 0.70000005,
                "_source": {
                    "name": "Mission Impossible 1",
                    "stock": 234
                }
            },
            {
                "_index": "movies",
                "_id": "3",
                "_score": 0.70000005,
                "_source": {
                    "name": "Mission Impossible 2",
                    "stock": 238
                }
            },
            {
                "_index": "movies",
                "_id": "4",
                "_score": 0.4,
                "_source": {
                    "name": "Avengers",
                    "stock": 300
                }
            },
            {
                "_index": "movies",
                "_id": "1",
                "_score": 0.3,
                "_source": {
                    "name": "Dunes part 1",
                    "stock": 2
                }
            },
            {
                "_index": "movies",
                "_id": "5",
                "_score": 0.3,
                "_source": {
                    "name": "Dunes part 2",
                    "stock": 26
                }
            }
        ]
    }
}

5. Search with post_filter

{
   "query": {
      "hybrid":{
          "queries":[
              { 
                "match":{
                    "name": "mission"
                }
              },
              {
                "term":{
                    "name":{
                        "value":"part"
                    }
                }
              },
              {
              "range": {
                 "stock": {
                      "gte": 200,
                      "lte": 400
                  }
                }
              }
          ]
      }

  },
  "post_filter":{
      "range": {
                 "stock": {
                      "gte": 230,
                      "lte": 400
                  }
       }
  }
}

{
    "took": 12,
    "timed_out": false,
    "_shards": {
        "total": 1,
        "successful": 1,
        "skipped": 0,
        "failed": 0
    },
    "hits": {
        "total": {
            "value": 3,
            "relation": "eq"
        },
        "max_score": 0.70000005,
        "hits": [
            {
                "_index": "movies",
                "_id": "2",
                "_score": 0.70000005,
                "_source": {
                    "name": "Mission Impossible 1",
                    "stock": 234
                }
            },
            {
                "_index": "movies",
                "_id": "3",
                "_score": 0.70000005,
                "_source": {
                    "name": "Mission Impossible 2",
                    "stock": 238
                }
            },
            {
                "_index": "movies",
                "_id": "4",
                "_score": 0.4,
                "_source": {
                    "name": "Avengers",
                    "stock": 300
                }
            }
        ]
    }
}

Issues Resolved

508

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• 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.

[martin-gaievski]

Overall code looks good, left just few comments

[navneet1v]

we should make this parameter as Optional optionalFilterWeight.

[vibrantvarun]

If i do that then in newCollector method

 if (optionalFilterWeight.isEmpty()) {
            return hybridcollector;
        }

This code breaks with below error

{
    "error": {
        "root_cause": [
            {
                "type": "null_pointer_exception",
                "reason": "Cannot invoke \"java.util.Optional.isEmpty()\" because \"this.optionalFilterWeight\" is null"
            }
        ],
        "type": "search_phase_execution_exception",
        "reason": "all shards failed",
        "phase": "query",
        "grouped": true,
        "failed_shards": [
            {
                "shard": 0,
                "index": "movies",
                "node": "MpUK-BFmRRiigVIII84uIQ",
                "reason": {
                    "type": "null_pointer_exception",
                    "reason": "Cannot invoke \"java.util.Optional.isEmpty()\" because \"this.optionalFilterWeight\" is null"
                }
            }
        ],
        "caused_by": {
            "type": "null_pointer_exception",
            "reason": "Cannot invoke \"java.util.Optional.isEmpty()\" because \"this.optionalFilterWeight\" is null",
            "caused_by": {
                "type": "null_pointer_exception",
                "reason": "Cannot invoke \"java.util.Optional.isEmpty()\" because \"this.optionalFilterWeight\" is null"
            }
        }
    },
    "status": 500
}

[martin-gaievski]

@vibrantvarun did you construct optional with ofNullable method?

[vibrantvarun]

yes. @navneet1v and me hopped on call and determined that it is kind of potiential bug and not the right practice to use Optional in class variables.

[martin-gaievski]

wonder what is the potential bug, it's not a blocker for this PR but using optional definitely isn't a bug

[vibrantvarun]

@navneet1v would you like to answer martin question

[navneet1v]

It was not a bug. The implementation was not correct due to which above exception happened.

but overall when we used optional on class member the IDE was adding warnings. Hence I suggested we can skip that. It was more of a coding practice and not a serious issue

[navneet1v]

Overall code looks good to me. Just resolve the comments and then its ready to be shipped.

[navneet1v]

@vibrantvarun lets ensure the GH actions are successful.

[codecov]

Codecov Report

Attention: Patch coverage is 76.92308% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 82.54%. Comparing base (759a971) to head (82e204f).

Files	Patch %	Lines
...ralsearch/search/query/HybridCollectorManager.java	76.92%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #633      +/-   ##
============================================
- Coverage     82.57%   82.54%   -0.03%     
- Complexity      656      659       +3     
============================================
  Files            52       52              
  Lines          2055     2063       +8     
  Branches        328      332       +4     
============================================
+ Hits           1697     1703       +6     
  Misses          212      212              
- Partials        146      148       +2     

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