Support distance type radius search for Lucene engine

[junqiu-lei]

Description

This PR introduces radius search capability to the Lucene engine, leveraging a new distance parameter. This feature allows users to perform searches within a specified radius, supporting L2 Square and Cosine Similarity metrics, similar to the existing top-k search functionality.

For now for other unsupported engines, the query request will return 400 response with unsupported information.

For detailed distance metric explanations, see OpenSearch k-NN documentation.

Example radius search workflow:

// Create index with L2 space type
curl --location --request PUT 'http://localhost:9200/target-index-lucene-l2' \
--header 'Content-Type: application/json' \
--data '{
  "settings": {
    "number_of_shards": 3,
    "number_of_replicas": 1,
    "index.knn": true
  },
  "mappings": {
    "properties": {
      "my_vector1": {
        "type": "knn_vector",
        "dimension": 2,
        "method": {
            "name": "hnsw",
            "space_type": "l2",
            "engine": "lucene",
            "parameters": {
              "ef_construction": 128,
              "m": 24
            }
          }
      }
    }
  }
}'

// Ingest data
curl --location 'http://localhost:9200/_bulk' \
--header 'Content-Type: application/json' \
--data '{"index": {"_index": "target-index-lucene-l2", "_id": "1"}}
{"my_vector1": [7.0, 8.2], "price": 4.4}
{"index": {"_index": "target-index-lucene-l2", "_id": "2"}}
{"my_vector1": [9.0, 6.4], "price": 14.2}
{"index": {"_index": "target-index-lucene-l2", "_id": "3"}}
{"my_vector1": [3.5, 6.0], "price": 19.1}
{"index": {"_index": "target-index-lucene-l2", "_id": "4"}}
{"my_vector1": [1.5, 9.8], "price": 1.2}
{"index": {"_index": "target-index-lucene-l2", "_id": "5"}}
{"my_vector1": [3.3, 2.3], "price": 16.5}

// Perform radius search with L2 squared distance 10
curl --location --request GET 'http://localhost:9200/target-index-lucene-l2/_search' \
--header 'Content-Type: application/json' \
--data '{
  "query": {
    "knn": {
      "my_vector1": {
        "vector": [7.0, 8.5],
        "distance": 10
      }
    }
  }
}'

// Radius search results
{
    "took": 318,
    "timed_out": false,
    "_shards": {
        "total": 3,
        "successful": 3,
        "skipped": 0,
        "failed": 0
    },
    "hits": {
        "total": {
            "value": 2,
            "relation": "eq"
        },
        "max_score": 0.91743106,
        "hits": [
            {
                "_index": "target-index-lucene-l2",
                "_id": "1",
                "_score": 0.91743106,
                "_source": {
                    "my_vector1": [
                        7.0,
                        8.2
                    ],
                    "price": 4.4
                }
            },
            {
                "_index": "target-index-lucene-l2",
                "_id": "2",
                "_score": 0.106269926,
                "_source": {
                    "my_vector1": [
                        9.0,
                        6.4
                    ],
                    "price": 14.2
                }
            }
        ]
    }
}

Issues Resolved

#814

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.

[codecov]

Codecov Report

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

Project coverage is 85.00%. Comparing base (fe774f7) to head (d7d5a83).
Report is 2 commits behind head on feature/radius-search.

Files	Patch %	Lines
...rg/opensearch/knn/index/query/KNNQueryBuilder.java	81.92%	7 Missing and 8 partials ⚠️
...rg/opensearch/knn/index/query/KNNQueryFactory.java	60.00%	3 Missing and 1 partial ⚠️
...rg/opensearch/knn/index/query/RNNQueryFactory.java	84.61%	3 Missing and 1 partial ⚠️
...g/opensearch/knn/index/query/BaseQueryFactory.java	86.95%	3 Missing ⚠️
...java/org/opensearch/knn/index/query/KNNWeight.java	82.35%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@                     Coverage Diff                     @@
##             feature/radius-search    #1498      +/-   ##
===========================================================
- Coverage                    85.07%   85.00%   -0.08%     
- Complexity                    1278     1311      +33     
===========================================================
  Files                          167      170       +3     
  Lines                         5207     5342     +135     
  Branches                       493      512      +19     
===========================================================
+ Hits                          4430     4541     +111     
- Misses                         570      584      +14     
- Partials                       207      217      +10     

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

[VijayanB]

may be time to refactor to Builder pattern?

[junqiu-lei]

Good point, let me check.

[jmazanec15]

I would say make radius last parameter for consistency. In terms of using Builder pattern for optional patterns (filter and radius I believe) that makes sense.

[junqiu-lei]

after introducing radius, neither k or radius will be required

[jmazanec15]

But cant we switch to builder pattern still?

[junqiu-lei]

Updated to use builder pattern in new code.

[jmazanec15]

But for setting max results returned, couldnt k be used? How are we limiting outside of this?

[jmazanec15]

But cant we switch to builder pattern still?

[jmazanec15]

On second thought, we should have a separate factory that is called for Radial Queries. Putting k and radius in same create method is confusing. There should be a RadialQueryFactory that gets called if we need a radial nearest neighbor query and KNNQueryFactory gets called if we need a KNNQuery. @VijayanB what do you think?

[junqiu-lei]

Since all current files and class name are originally from KNN, I'm not sure if we want a more generic name and pattern from "KNN" to "VectorQuery" in whole codebase, but this is a long term refactoring I think. For now I prefer reuse the existing factory without much change.

[VijayanB]

@jmazanec15 Good point. I like your suggestion

[junqiu-lei]

@jmazanec15 @VijayanB Added BaseQueryFactory as parent class and added RNNQueryFactory for radials search.

[VijayanB]

@jmazanec15 Do you see any potential backward incompatibility if we change k from int to integer?

[junqiu-lei]

We're still using int for k in KNNQueryBuilder

[VijayanB]

remove yet.

[junqiu-lei]

Why do we need remove? This is used to validate engines if support radius search in knn.

[VijayanB]

sorry, i meant word "yet"

[junqiu-lei]

Got it, updated

[VijayanB]

Do we need to expose this? Why not pass engine and return true if supported else otherwise?

[junqiu-lei]

I used same pattern from other knn features.

k-NN/src/main/java/org/opensearch/knn/index/util/KNNEngine.java

Lines 82 to 107 in dad43c6

	/**
	* Get the engine from the path.
	*
	* @param path to be checked
	* @return KNNEngine corresponding to path
	*/
	public static KNNEngine getEngineNameFromPath(String path) {
	if (path.endsWith(KNNEngine.NMSLIB.getExtension()) || path.endsWith(KNNEngine.NMSLIB.getCompoundExtension())) {
	return KNNEngine.NMSLIB;
	}
	if (path.endsWith(KNNEngine.FAISS.getExtension()) || path.endsWith(KNNEngine.FAISS.getCompoundExtension())) {
	return KNNEngine.FAISS;
	}
	throw new IllegalArgumentException("No engine matches the path's suffix");
	}
	/**
	* Returns all engines that create custom segment files.
	*
	* @return Set of all engines that create custom segment files.
	*/
	public static Set<KNNEngine> getEnginesThatCreateCustomSegmentFiles() {
	return CUSTOM_SEGMENT_FILE_ENGINES;
	}

[VijayanB]

I don't see need to expose , can we update it to accept engine as parameter and decide whether is it supported or not?

[junqiu-lei]

Updated to directly use set ENGINES_SUPPORTING_RADIAL_SEARCH

[VijayanB]

@junqiu-lei Your description says it supports only for lucene engine. Can you add additional description on what happens if it is used for nmslib or fais?

[VijayanB]

flip if condition

[junqiu-lei]

updated

[VijayanB]

can you create local instance? chaining makes it hard to debug and read

[junqiu-lei]

updated

[VijayanB]

can we use import static?

[junqiu-lei]

updated

[VijayanB]

move common fields to line 276 and set either k or distance inside if block

[junqiu-lei]

Updated

[junqiu-lei]

@junqiu-lei Your description says it supports only for lucene engine. Can you add additional description on what happens if it is used for nmslib or fais?

Sure

[VijayanB]

@jmazanec15 is it correct to call it as Radius nearest neighbor?

[VijayanB]

may be VectorSearchRadialQuery ?

[junqiu-lei]

Ref for the nearest neighbors search names: https://en.wikipedia.org/wiki/Nearest_neighbor_search#Variants

[VijayanB]

i don't think you need to update . You can do like 285

[junqiu-lei]

Updated

[VijayanB]

i think what you need is a Single Factory class or method that will return KNNQuery ( instance of Radial or KNN based on parameter )

[junqiu-lei]

The motivation of this refactor is come from #1498 (comment)

[VijayanB]

Approving it since pending questions are not blocker and can be refactored in later PR.