ESQL - Add K mandatory param for KNN function#129763
ESQL - Add K mandatory param for KNN function#129763carlosdelest merged 21 commits intoelastic:mainfrom
Conversation
|
🔍 Preview links for changed docs: 🔔 The preview site may take up to 3 minutes to finish building. These links will become live once it completes. |
| - class: org.elasticsearch.entitlement.runtime.policy.FileAccessTreeTests | ||
| method: testWindowsAbsolutPathAccess | ||
| issue: https://github.com/elastic/elasticsearch/issues/129168 | ||
| - class: org.elasticsearch.xpack.esql.qa.multi_node.EsqlSpecIT |
There was a problem hiding this comment.
This test is removed in this PR as it's already being tested in all other tests
| // end::knn-function-result[] | ||
| ; | ||
|
|
||
| knnSearchWithKOption |
There was a problem hiding this comment.
Removed test - k is already added to all other tests
| description = "Floating point number used to decrease or increase the relevance scores of the query." | ||
| + "Defaults to 1.0." | ||
| ), | ||
| @MapParam.MapParamEntry( |
There was a problem hiding this comment.
k is removed as an option
| ); | ||
| } | ||
|
|
||
| public void testFullTextFunctionsConstantQuery() throws Exception { |
There was a problem hiding this comment.
Renamed as we're checking other params not being null now
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java
Show resolved
Hide resolved
# Conflicts: # docs/reference/query-languages/esql/kibana/definition/functions/knn.json # docs/reference/query-languages/esql/kibana/docs/functions/knn.md # muted-tests.yml # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/EsqlFunctionRegistry.java
…o non-issue/knn-k-param # Conflicts: # docs/reference/query-languages/esql/kibana/definition/functions/knn.json # muted-tests.yml # server/src/main/java/org/elasticsearch/TransportVersions.java
|
Hey @ioanatia @tteofili @kderusso I've removed the usage of TransportVersions by not serializing Once the query builder is created as part of the query rewriting, there is no need for storing k as it will be included in the query builder options, so we don't really need to serialize it to data nodes. I think this is better for not dealing with transport versions, but LMK otherwise 👍 |
🔍 Preview links for changed docs |
| - class: org.elasticsearch.packaging.test.DockerTests | ||
| method: test012SecurityCanBeDisabled | ||
| issue: https://github.com/elastic/elasticsearch/issues/116636 | ||
| - class: org.elasticsearch.index.shard.StoreRecoveryTests |
There was a problem hiding this comment.
bad merge? same as the other ones that are added?
There was a problem hiding this comment.
🤦 ouch. Sorry about that.
Opened #130523 to fix
| return TypeResolution.TYPE_RESOLVED; | ||
| } | ||
|
|
||
| return isType(k(), dt -> dt == INTEGER, sourceText(), THIRD, "integer").and(isFoldable(k(), sourceText(), THIRD)) |
There was a problem hiding this comment.
are we missing a test here on what would happen if k is not a constant (or not foldable)?
|
bwc tests fail - opened #130441 to fix |
Right now KNN function sets k as an option:
| WHERE KNN(vector, [0, 1, 2], {"k": 10})This is a problem as it makes k optional. Setting a default value makes no sense until we use LIMIT for setting k (#129353).
Until we use LIMIT, this PR makes k a mandatory parameter for KNN function:
The new function format will be:
| WHERE KNN(vector, [0, 1, 2], 10)In case users want to set num_candidates, they can do so via option:
| WHERE KNN(vector, [0, 1, 2], 10, {"num_candidates": 50})After #129353 is done, we can make this parameter optional and check that a LIMIT can be used to set K on behalf of the user. Users will be able to override the default K by setting it explicitly.
This PR removes a test for the k option, which makes no sense now, and closes the following related issues for this test:
Closes #129447
Closes #129512