KNN Query visit_percentage

[john-wagster]

Updating the query interface for knn to include visit_percentage

Usage example with a basic knn query (note this PR updates retrievers as well). This is setup such that visit_percentage overrides num_candidates for bbq_disk:

curl -XPUT --header 'Content-Type: application/json' "http://localhost:9200/test" -d '{
  "mappings": {
    "properties": {
       "image-vector": {
        "type": "dense_vector",
        "dims": 3,
        "similarity": "l2_norm",
        "index_options": {
          "type": "bbq_disk"
        }
      }
    }
  }
}'

seq 1 1 | xargs -I % -P1 curl -XPOST --header 'Content-Type: application/json' "http://localhost:9200/test/_doc?refresh" -d '
    { "image-vector": [0.0127, 0.1230, 0.3929] }
'

curl -XGET --header 'Content-Type: application/json' "http://localhost:9200/test/_search" -d '{
  "query" : {
    "knn": {
      "field": "image-vector",
      "query_vector": [0.0127, 0.1230, 0.3929],
      "k": 10,
      "visit_percentage": 10.0
    }
  }
}' | python -mjson.tool

[github-actions]

🔍 Preview links for changed docs

• docs/reference/elasticsearch/mapping-reference/dense-vector.md
• docs/reference/elasticsearch/rest-apis/retrievers/knn-retriever.md
• docs/reference/query-languages/query-dsl/query-dsl-knn-query.md

[john-wagster]

since we didn't add these in a previous PR I just added them here but could be convinced to pull docs out to a separate PR

[john-wagster]

@benwtrent how do you feel about the docs going in with the belief we are going to yank out the feature flag immediately here. Would you be more comfortable with me pulling all of these out to a separate PR?

[benwtrent]

I read through again, looking good. Just need to test null cases.

I left some comments on ones that I found, but then I figured, eh, that is too many comments. So I stopped.

Basically, anything that is a "random input" deal, should test the null case. Additionally, the dense vector type tests should verify that when the value is passed, it actually is applied to the query.

The "refresh" popped up on the screen in the middle of the commenting. Sorry if any of them are on an old commit :(

[john-wagster]

@benwtrent

Basically, anything that is a "random input" deal, should test the null case. Additionally, the dense vector type tests should verify that when the value is passed, it actually is applied to the query.

I think I got all of them? I'm double checking here and reviewing tests in general. But I believe I got fixed up what you saw. I appreciate the feedback too; that was super helpful.

Still curious if you think I should punt the docs update out of this PR or not given that stuff is still behind a feature flag or if you feel like we'll take the flag out shortly and it won't matter.

[benwtrent]

Let's remove the docs changes

[john-wagster]

docs changes pulled out to here: #134082

[benwtrent]

I would just make sure that the new tests you added/mutated all pass with various seeds.

I would also maybe do some release tests verification locally to make sure you handled the feature flag.

I am particularly wondering about things like the more general "RandomSearchRequestGenerator" change and if adding the value, then the generated object gets serialized to xcontent, and then parsed (which doesn't get the field if the flag isn't set) will be missing the object.

I don't know if we have tests that do that, but I know that by default release tests do not run in PRs.

[john-wagster]

I've tried to run all of the tests multiple times and on several seeds and not seen any problems. Tried breaking RandomSearchRequestGenerator and not been able to do that. Tried turning the feature flag on and off and behavior seems to be ok. bwc tests seem to run ok (although to be fair configuring some of this from a release vs snapshot standpoint is confusing). I think this is good.