[ES|QL] Fix nested editor marker leaks in autocomplete and introduce a shared autocomplete parsing boundary#260618
Conversation
26d90f2 to
e53d9bd
Compare
|
I was thinking about a more ambitious long-term idea (not for this PR): Instead of using markers and cleaning them up later in Kibana, @elastic/esql could handle incomplete input directly in the AST. For example, Using location instead of Before that, we should verify that parser locations are reliable with incomplete input. There are already some issues, like LIKE / NOT resolving to unknown, and trailing commas resolving only to very broad nodes. |
| if (Array.isArray(arg)) { | ||
| return arg.filter(isNotMarkerNodeOrArray).map(mapToNonMarkerNode); | ||
| } | ||
|
|
There was a problem hiding this comment.
Here the missing part of the task
| @@ -85,7 +84,7 @@ function getPosition( | |||
| return { position: CompletionPosition.AFTER_COMMAND }; | |||
| } | |||
|
|
|||
| const expressionRoot = prompt?.text !== EDITOR_MARKER ? prompt : undefined; | |||
There was a problem hiding this comment.
I tested this by hand, I found no side effects
e53d9bd to
a315d3c
Compare
| @@ -181,8 +181,7 @@ export async function autocomplete( | |||
|
|
|||
| case 'after_where': { | |||
| const whereFn = command.args[command.args.length - 1] as ESQLFunction; | |||
| // TODO do we still need this check? | |||
| const expressionRoot = isMarkerNode(whereFn.args[1]) ? undefined : whereFn.args[1]!; | |||
| const expressionRoot = whereFn.args[1]; | |||
There was a problem hiding this comment.
I did some testing by hand but the unit tests already covered it well
| } | ||
|
|
||
| /** Parses the query and resolves the cursor context (command, option, node). */ | ||
| export function getAutocompleteCursorContext(fullText: string, offset: number) { |
There was a problem hiding this comment.
It’s a weak API in the sense that it’s only used in one place at runtime and in tests. I could create a separate API for findAutocompleteAstPosition, but I’d rather not introduce more APIs and risk adding confusion.
|
Pinging @elastic/kibana-esql (Team:ESQL) |
stratoula
left a comment
There was a problem hiding this comment.
This looks very nice, can you fix the regex issue that was marked? Other than that I really like this
|
|
||
| // After `KEY BY field,`, suggest the next KEY BY field. | ||
| const KEY_BY_TRAILING_COMMA_REGEX = /\bkey\s+by(?:\s+\S+,)+\s*$/i; | ||
|
|
|
|
||
| const functionExpression = expressionRoot as ESQLFunction; | ||
| const paramContext = buildExpressionFunctionParameterContext(functionExpression, context); | ||
| const startingNewParam = STARTING_NEW_PARAM_REGEX.test(innerText); |
There was a problem hiding this comment.
This regex is duplicated. in_function.ts is STARTING_NEW_PARAM_REGEX and stats/autocomplete.ts uses inline /,\s*$/
We could create a shared utility.
💚 Build Succeeded
Metrics [docs]Module Count
Page load bundle
History
cc @bartoval |
Summary
Closes #215186
Disclaimer : This PR restarts following the last comment on the issue. So I’m trying to clean up the ESQL commands as a consumers, without venturing into suicide missions.
RERANK ... ON col0 =.