-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[ES|QL] Build function arguments suggestions from hints #246495
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
7d982a6
2e0ce65
f84d936
78e02b4
8a71197
4da15db
6249586
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -99,6 +99,13 @@ export const isParameterType = (str: string | undefined): str is FunctionParamet | |
| export const isReturnType = (str: string | FunctionParameterType): str is FunctionReturnType => | ||
| str !== 'unsupported' && (str === 'unknown' || str === 'any' || dataTypes.includes(str)); | ||
|
|
||
| export const parameterHintEntityTypes = ['inference_endpoint'] as const; | ||
| export type ParameterHintEntityType = (typeof parameterHintEntityTypes)[number]; | ||
| export interface ParameterHint { | ||
| entityType: ParameterHintEntityType; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's make it an enum instead
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a type that is used within the autogenerated files, we will have type errors in them if using enums. |
||
| constraints?: Record<string, string>; | ||
| } | ||
|
|
||
| export interface FunctionParameter { | ||
| name: string; | ||
| type: FunctionParameterType; | ||
|
|
@@ -128,6 +135,12 @@ export interface FunctionParameter { | |
| * This indicates that the parameter can accept multiple values, which will be passed as an array. | ||
| */ | ||
| supportsMultiValues?: boolean; | ||
|
|
||
| /** | ||
| * Provides information that is useful for getting parameter values from external sources. | ||
| * For example, an inference endpoint | ||
| */ | ||
| hint?: ParameterHint; | ||
| } | ||
|
|
||
| export interface ElasticsearchCommandDefinition { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,7 @@ | |
| */ | ||
|
|
||
| import { ControlTriggerSource, ESQLVariableType } from '@kbn/esql-types'; | ||
| import { uniq } from 'lodash'; | ||
| import { isEqual, uniq, uniqWith } from 'lodash'; | ||
| import { matchesSpecialFunction } from '../utils'; | ||
| import { shouldSuggestComma, type CommaContext } from '../comma_decision_engine'; | ||
| import type { ExpressionContext } from '../types'; | ||
|
|
@@ -21,6 +21,7 @@ import type { | |
| FunctionDefinition, | ||
| FunctionParameter, | ||
| FunctionParameterType, | ||
| ParameterHint, | ||
| } from '../../../../types'; | ||
| import { type ISuggestionItem } from '../../../../../registry/types'; | ||
| import { FULL_TEXT_SEARCH_FUNCTIONS } from '../../../../constants'; | ||
|
|
@@ -29,6 +30,7 @@ import { | |
| valuePlaceholderConstant, | ||
| defaultValuePlaceholderConstant, | ||
| } from '../../../../../registry/complete_items'; | ||
| import { parametersFromHintsMap } from '../../parameters_from_hints'; | ||
|
|
||
| type FunctionParamContext = NonNullable<ExpressionContext['options']['functionParameterContext']>; | ||
|
|
||
|
|
@@ -70,7 +72,7 @@ async function handleFunctionParameterContext( | |
| } | ||
|
|
||
| // Try exclusive suggestions first (COUNT(*), enum values) | ||
| const exclusiveSuggestions = tryExclusiveSuggestions(functionParamContext, ctx); | ||
| const exclusiveSuggestions = await tryExclusiveSuggestions(functionParamContext, ctx); | ||
|
|
||
| if (exclusiveSuggestions.length > 0) { | ||
| return exclusiveSuggestions; | ||
|
|
@@ -81,10 +83,10 @@ async function handleFunctionParameterContext( | |
| } | ||
|
|
||
| /** Try suggestions that are exclusive (if present, return only these) */ | ||
| function tryExclusiveSuggestions( | ||
| async function tryExclusiveSuggestions( | ||
| functionParamContext: FunctionParamContext, | ||
| ctx: ExpressionContext | ||
| ): ISuggestionItem[] { | ||
| ): Promise<ISuggestionItem[]> { | ||
| const { functionDefinition, paramDefinitions } = functionParamContext; | ||
| const { options } = ctx; | ||
|
|
||
|
|
@@ -95,11 +97,16 @@ function tryExclusiveSuggestions( | |
| Boolean(functionParamContext.hasMoreMandatoryArgs), | ||
| options.isCursorFollowedByComma ?? false | ||
| ); | ||
|
|
||
| if (enumItems.length > 0) { | ||
| return enumItems; | ||
| } | ||
|
|
||
| // Some parameters suggests special values that are deduced from the hints object provided by ES. | ||
| const itemsFromHints = await buildSuggestionsFromHints(paramDefinitions, ctx); | ||
| if (itemsFromHints.length > 0) { | ||
| return itemsFromHints; | ||
| } | ||
|
|
||
| return []; | ||
| } | ||
|
|
||
|
|
@@ -374,6 +381,23 @@ function buildEnumValueSuggestions( | |
| }); | ||
| } | ||
|
|
||
| async function buildSuggestionsFromHints( | ||
| paramDefinitions: FunctionParameter[], | ||
| ctx: ExpressionContext | ||
| ): Promise<ISuggestionItem[]> { | ||
| // Keep the hints that are unique by entityType + constraints | ||
| const hints: ParameterHint[] = uniqWith( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need the uniq?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's to make it more robust, signatures can be redundant with parameters as ES creates permutations with the parameters, so I don't want to create duplicated suggestions if the 'inference_id' param appears in multiple signatures. We don't have cases now, but we probably will not be notified if they change this signature to these cases, so better be prepared as it's just a line code. |
||
| paramDefinitions.flatMap(({ hint }) => hint ?? []), | ||
| (a, b) => a.entityType === b.entityType && isEqual(a.constraints, b.constraints) | ||
| ); | ||
|
|
||
| const results = await Promise.all( | ||
| hints.map((hint) => parametersFromHintsMap[hint.entityType]?.(hint, ctx) ?? []) | ||
| ); | ||
|
|
||
| return results.flat(); | ||
| } | ||
|
|
||
| /** Builds suggestions for constant-only literal parameters */ | ||
| function buildConstantOnlyLiteralSuggestions( | ||
| paramDefinitions: FunctionParameter[], | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the "Elastic License | ||
| * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
| * Public License v 1"; you may not use this file except in compliance with, at | ||
| * your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
| * License v3.0 only", or the "Server Side Public License, v 1". | ||
| */ | ||
|
|
||
| import type { InferenceTaskType } from '@elastic/elasticsearch/lib/api/types'; | ||
| import { i18n } from '@kbn/i18n'; | ||
| import type { ParameterHint, ParameterHintEntityType } from '../../..'; | ||
| import type { ISuggestionItem } from '../../../registry/types'; | ||
| import type { ExpressionContext } from './expressions/types'; | ||
| import { createInferenceEndpointToCompletionItem } from './helpers'; | ||
|
|
||
| /** | ||
| * For some parameters, ES gives as hints about the nature of it, that we use to provide | ||
| * custom autocompletion handlers. | ||
| */ | ||
| export const parametersFromHintsMap: Record< | ||
| ParameterHintEntityType, | ||
| (hint: ParameterHint, ctx: ExpressionContext) => Promise<ISuggestionItem[]> | ||
| > = { | ||
| ['inference_endpoint']: inferenceEndpointHandler, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need better types here otherwise everytime they add a hint we need to change this part.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh it was on purpose, it's a way of getting notified if they add a new hint, so we know we might want to add support to it, fixing the type error it's just adding I can add a export const parametersFromHintsMap: Partial<
Record<
ParameterHintEntityType,
(hint: ParameterHint, ctx: ExpressionContext) => Promise<ISuggestionItem[]>
>
>
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes it makes it too hardcoded. We need an architecture as we have for functions. So something that we dont need to bother every time they add something. So have that in mind
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you mean with hardcoded?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean that this is much simpler and better https://github.com/elastic/kibana/pull/246495/files#r2626487947 |
||
| }; | ||
|
|
||
| async function inferenceEndpointHandler( | ||
| hint: ParameterHint, | ||
| ctx: ExpressionContext | ||
| ): Promise<ISuggestionItem[]> { | ||
| if (hint.constraints?.task_type) { | ||
| const inferenceEnpoints = | ||
| ( | ||
| await ctx.callbacks?.getInferenceEndpoints?.( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should follow the other context logic and not having the callback here |
||
| hint.constraints?.task_type as InferenceTaskType | ||
| ) | ||
| )?.inferenceEndpoints || []; | ||
|
|
||
| return inferenceEnpoints.map(createInferenceEndpointToCompletionItem).map((item) => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need the 2 maps?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed in new branch 67a221e |
||
| return { | ||
| ...item, | ||
| detail: i18n.translate('kbn-esql-ast.esql.definitions.inferenceEndpointInFunction', { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where is it visible (I didn't test, can you provide a screenshot?)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok so this text is wrong but we can discuss after #246495 (comment) |
||
| defaultMessage: 'Inference endpoint used for this function', | ||
| }), | ||
| text: `"${item.text}"`, | ||
| }; | ||
| }); | ||
| } | ||
| return []; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2010,4 +2010,14 @@ describe('functions arg suggestions', () => { | |
| expect(labels).not.toContain(','); | ||
| }); | ||
| }); | ||
|
|
||
| describe('function parameter built from hint', () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of hardcoding them every time they add a hint lets create these tests automatically from the info ES sent to us
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I don't see it 🤔 💭, hints could vary a lot one from each other, some may need to mock new endpoints, etc. They tell us, hey this param is special, here it's a hint of how to create the suggestions for it, but each hint needs a different implementation from each other, so I'm not seeing how to make a generic test for all u.u
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You have the function from the hints, so I am hoping that we can create generic tests that we show suggestions for the hints without every time to have to hard code them (the less work we do for this the better, it means that our architecture can scale). In my mind it should be doable at least 😄
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I get your idea now. I liked this approach more because it test both the handler and the autocomplete routine integration all together, with less code.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes I prefer a generic solution and then yes specific tests for the handlers would be ok 👌 |
||
| it('suggests inference endpoints for TEXT_EMBEDDING function', async () => { | ||
| const { suggest } = await setup(); | ||
| const suggestions = await suggest('FROM index | EVAL result = TEXT_EMBEDDING("text", /'); | ||
| const labels = suggestions.map(({ label }) => label); | ||
|
|
||
| expect(labels).toEqual(['inference_1']); | ||
| }); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -273,6 +273,7 @@ async function getSuggestionsWithinCommandExpression( | |
| hasMinimumLicenseRequired, | ||
| canCreateLookupIndex: callbacks?.canCreateLookupIndex, | ||
| isServerless: callbacks?.isServerless, | ||
| getInferenceEndpoints: callbacks?.getInferenceEndpoints, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer the inferences to come on the context We are having them already at the context, it is confusing to have them at the callbacks.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My first idea was using the Command context yes, but did found some drawbacks: These are suggestions for functions, not commands, in the command context we check if the command is So the implementation is less straightforward: when creating each command context we would need to walk the command and check the signature of every function within the command to check if some argument has a hint, if it has one do the fetching of the data in case we need it later. This is in theory good for user perceived performance as we have the data before we need it, but adds extra parsing code early on in the code and fetches for data that may not be used, and IMO the difference in speed it's not really noticeable. In contrast the current implementation makes use of the signature detection that is already been used for other suggestions, so at that point of the flow it's easy to detect if we have hints in it and if we need to fetch for data, and when we do it's because it's needed. So, putting that on the table, there are some tradeoffs between solutions, I think the current one is simpler, adds less code to maintain and blends with the flow we use for suggesting within functions. Summarising I think it's a decision about: Let me know your thoughts with this new information, and if the directrice is to always go with option 1, which is totally fine to me btw :)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is problematic to have this call in 2 different places. It shows that something goes wrong with the architecture. As long as we have this information on the context, adding this also to the callbacks is wrong. We should stick with our current one I think (otherwise we need to re-architect this thing). So I think we should go with the more complex one as it aligns with our current architeture. Can you create another PR with this to see how this will be? If it doesnt look ok then we should think more thoroughly and re-architect!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will do 👍
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @stratoula what do you think about this implementation path -> #246736
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes I really like it!! 👏 |
||
| }, | ||
| context, | ||
| offset | ||
|
|
||

Uh oh!
There was an error while loading. Please reload this page.