ESQL - Add K mandatory param for KNN function #129763
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
carlosdelest
added
Team:Analytics
|
🔍 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. |
carlosdelest
commented
Jun 20, 2025
| @@ -511,9 +511,6 @@ tests: | |||
| - 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
| @@ -29,31 +29,12 @@ chartreuse | [127.0, 255.0, 0.0] | |||
| // end::knn-function-result[] | |||
| ; | |||
|
|
|||
| knnSearchWithKOption | |||
There was a problem hiding this comment.
Removed test - k is already added to all other tests
| @@ -100,14 +109,6 @@ public Knn( | |||
| 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
| @@ -2172,24 +2173,25 @@ private void checkFullTextFunctionNullArgs(String functionInvocation, String arg | |||
| ); | |||
| } | |||
|
|
|||
| public void testFullTextFunctionsConstantQuery() throws Exception { | |||
There was a problem hiding this comment.
Renamed as we're checking other params not being null now
7 tasks
elasticsearchmachine
removed
Team:Search Relevance
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
carlosdelest
requested review from
tteofili,
afoucret,
ioanatia and
ChrisHegarty
elasticsearchmachine
added
the
serverless-linked
tteofili
approved these changes
Jun 23, 2025
kderusso
approved these changes
Jun 23, 2025
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java
Show resolved
Hide resolved
ioanatia
removed
auto-backport
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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