[ES|QL] Add support for `RRF` #221349

sddonne · 2025-05-23T10:11:46Z

Summary

Considerations

`commandsSuggestionsAfter`

The RRF command must be shown only immediately after a FORK command. To achieve this, the proposed solution is to keep the hidden flag set to true for the lifetime of the command, and a new property commandsSuggestionsAfter has been added to the CommandDefinition interface, following the same concept as fieldsSuggestionsAfter, this new method lets each command to alter the commands suggested after them, in this case, it's used by FORK to add the RRF command to the list. (If the approach is approved by the team, will add documentation about it.)

Warning

This means that RRF will be activated when the FORK command does. Thoughts?

`RRF` may change in the future

@ioanatia has stated that the command may change in the future as it's not fully implemented yet.

Licence check

Will be addressed at #216791

Screen.Recording.2025-05-23.at.12.42.15.mov

New command support checklist

AST support

Make sure that the new command is in the local Kibana grammar definition. The ANTLR lexer and parser files are updated every Monday from the source definition of the language at Elasticsearch (via a manually merged, automatically generated PR).
Create a factory for generating the new node. The new node should satisfy the ESQLCommand<Name> interface. If the syntax of your command cannot be decomposed only in parameters, you can hold extra information by extending the ESQLCommand interface. I.E., check the Rerank command.
While ANTLR is parsing the text query, we create our own AST by using onExit listeners at kbn-esql-ast/src/parser/esql_ast_builder_listener.ts. Implement the onExit<COMMAND_NAME> method based on the interface autogenerated by ANTLR and push the new node into the AST.
Create unit tests checking that the correct AST nodes are generated when parsing your command.
Add a dedicated visitor callback for the new command.
Verify that the Walker API can visit the new node.
Verify that the Synth API can construct the new node.

Validating that the command works well when prettifying the query

Validate that the prettifier works correctly.
Adjust the basic pretty printer and the wrapping pretty printer if needed.
Add unit tests validating that the queries are correctly formatted (even if no adjustment has been done).

Creating the command definition

Add the definition of the new command at kbn-esql-validation-autocomplete/src/definitions/commands.ts.

Adding the corresponding client-side validations

Add a custom validation if needed.
Add tests checking the behavior of the validation following this guide.

Adding the autocomplete suggestions

Add the suggestions to be shown when positioned at the new command.
- Create a new folder at kbn-esql-validation-autocomplete/src/autocomplete/commands for your command.
- Export a suggest function that should return an array of suggestions and set it up into the command definition.
- Optionally, we must filter or incorporate fields suggestions after a command is executed, this is supported by adding the fieldsSuggestionsAfter method to the command definition. Read this documentation to understand how it works.
If the new command must be suggested only in particular situations, modify the corresponding suggestions to make it possible.
Add tests following this guide.

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The release_note:breaking label should be applied in these situations.
Flaky Test Runner was used on any tests changed
The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines

Identify risks

Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging.

See some risk examples
...

elasticmachine · 2025-05-23T11:41:49Z

Pinging @elastic/kibana-esql (Team:ESQL)

sddonne · 2025-05-23T11:44:11Z

@elasticmachine merge upstream

stratoula

Looks great, added some comments.

Also RRF is on snapshot releases (which means we are not ready to suggest them). With your changes it is going to appear and we don't want it.

I am not sure if utilizing the hidden flag is a correct decision here. Ideally I want to have hidden true if it is still on snaphsots and find another way to hide it when it makes sense only after specific commands.

cc @drewdaemon

stratoula · 2025-05-23T11:58:51Z

src/platform/packages/shared/kbn-esql-ast/src/parser/__tests__/rrf.test.ts

+import { parse } from '../parser';
+
+describe('RRF', () => {
+  describe('correctly formatted', () => {


2 describe methods are unecessary when you only have one test. But I think you should add some tests for errors too

✅ commit 80655cef7dd31f8c20a5da366432993d5c7801df

stratoula · 2025-05-23T12:03:05Z

src/platform/packages/shared/kbn-esql-validation-autocomplete/src/__tests__/helpers.ts

@@ -13,6 +13,14 @@ import { ESQLFieldWithMetadata } from '../validation/types';
 import { fieldTypes } from '../definitions/types';
 import { ESQLCallbacks } from '../shared/types';

+export const metadataFields: ESQLFieldWithMetadata[] = [


We have already declared the metadataFields here src/platform/packages/shared/kbn-esql-validation-autocomplete/src/tests/helpers.ts so no need to re-declare them here

@stratoula couldn't find the tests folder, and the file in tests one is this same file 🤔

stratoula · 2025-05-23T12:08:51Z

...bn-esql-validation-autocomplete/src/autocomplete/commands/fork/commands_suggestions_after.ts

+import { getCommandAutocompleteDefinitions } from '../../complete_items';
+import { SuggestionRawDefinition } from '../../types';
+
+export function commandsSuggestionsAfter(suggestions: SuggestionRawDefinition[]) {


Instead of passing the existing suggestions and add RRF you can change this method to return the extra commands and just populate them here [src/autocomplete/autocomplete.ts](https://github.com/elastic/kibana/pull/221349/files#diff-40ae50a3a9b3ffeaf11039070a8f527dc27264a985ba6378ac7c4b86e626ec55)

I think it will be a bit cleaner.

So this should be

return [ ...getCommandAutocompleteDefinitions([ { ...(getCommandDefinition('rrf') as CommandDefinition<string>), hidden: false, }, ]), ]

stratoula · 2025-05-23T12:09:46Z

src/platform/packages/shared/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.ts

+    const lastCommand = root.commands[root.commands.length - 1];
+    const lastCommandDefinition = getCommandDefinition(lastCommand.name);
+
+    if (lastCommandDefinition.commandsSuggestionsAfter) {


Check my other comment. Let's change the commandsSuggestionsAfter to only return the extra suggestions. So you won't need to reassign here, just push to the existing suggestions. I think it is cleaner

stratoula · 2025-05-23T12:11:20Z

...form/packages/shared/kbn-esql-validation-autocomplete/src/autocomplete/commands/rrf/index.ts

+import type { SuggestionRawDefinition } from '../../types';
+import { pipeCompleteItem } from '../../complete_items';
+
+export function suggest(_params: CommandSuggestParams<'rrf'>): SuggestionRawDefinition[] {


Suggested change

export function suggest(_params: CommandSuggestParams<'rrf'>): SuggestionRawDefinition[] {

export function suggest(): SuggestionRawDefinition[] {

I don't think that this is needed, right?

✅ 2130458

stratoula · 2025-05-23T12:12:18Z

src/platform/packages/shared/kbn-esql-validation-autocomplete/src/definitions/commands.ts

@@ -69,6 +70,8 @@ import {
  suggest as suggestForRename,
  fieldsSuggestionsAfter as fieldsSuggestionsAfterRename,
 } from '../autocomplete/commands/rename';
+import { suggest as suggestForRrf } from '../autocomplete/commands/rrf';


Theoretically as we return only the pipe we don't need the extra folder, we can just add the code immediately here. @drewdaemon wdyt?

It's planned to be added some customizations to the command, so at some point it'll become more complex. elastic/elasticsearch#123389

This will be an internal thing and not something that will change the RRF syntax if I understand correctly

Yes just confirmed with ES

@stratoula I do lean toward keeping it this way because it might be confusing to see every command but one when I open the autocomplete/commands directory. limit is another very small and simple suggest function, but I think it is nice to have consistency.

That said, I really have no strong feelings here. I can see an argument for going the other way, too. WDYT

stratoula · 2025-05-23T12:20:23Z

...atform/packages/shared/kbn-esql-validation-autocomplete/src/validation/commands/rrf/index.ts

+  return {
+    location: command.location,
+    text: i18n.translate('kbn-esql-validation-autocomplete.esql.validation.rrfMissingMetadata', {
+      defaultMessage: `[RRF] {metadataField} metadata must be selected in the FROM command.`,


Not sure about the text 🤔

I would like something more obvious that it is in FROM .... METADATA option.

@andreadelrio it appears here, any suggestions?

stratoula · 2025-05-23T12:22:08Z

...atform/packages/shared/kbn-esql-validation-autocomplete/src/validation/commands/rrf/index.ts

+      values: { metadataField },
+    }),
+    type: 'error',
+    code: `rrfMissing${fieldName}Metadata`,


You dont need a different code per field.

Suggested change

code: `rrfMissing${fieldName}Metadata`,

code: `rrfMissingMetadataField`,

✅ 37b15c7

into esql/rrf/ast-parsing

elasticmachine · 2025-05-23T16:23:49Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 37b15c7

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`esql`	202	205	+3
`lists`	430	433	+3
`securitySolution`	7400	7403	+3
`stackAlerts`	256	259	+3
`unifiedSearch`	364	367	+3
total			+15

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/esql-validation-autocomplete`	163	165	+2

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`kbnUiSharedDeps-srcJs`	3.7MB	3.7MB	+1.6KB

Unknown metric groups

API count

id	before	after	diff
`@kbn/esql-validation-autocomplete`	184	187	+3

History

💔 Build #302023 failed 0779540

drewdaemon · 2025-05-23T17:59:53Z

I am not sure if utilizing the hidden flag is a correct decision here. Ideally I want to have hidden true if it is still on snaphsots and find another way to hide it when it makes sense only after specific commands.

This makes me wonder again if we should be treating RRF-after-FORK as a special case instead of trying to build a generic system for this.

I don't think it's a bad system @sddonne has set up here, but the autocomplete refactor was supposed to fix the anti-pattern of adding "generic" properties to the command definitions to support quirks only found in a single command. If we just handled RRF as a one-off in the top-level suggest method it might

be more clear what is actually going on
avoid growing the command definition interface
avoid using hidden in this new way

sddonne added 11 commits May 20, 2025 15:07

feat: add rrf node

e1d67fb

feat: add rff ast parser tests

83b0e3e

feat: add rrf to visitor api

e3b0da1

feat: add tests for rrf pretty printer

084f04b

feat: add new rff command and suggestions

d19f0bd

feat: only display RRF command after a FORK

06a937e

feat: only display RRF command after FORK

832a5fa

feat: add tests for rrf autocomplete

39d8b2d

feat: add rrf validations and autocomplete tests

36dd3c4

feat: add validations for rrf command

fac9e5c

fix: typo

89b6516

sddonne added Feature:ES|QL ES|QL related features in Kibana Team:ESQL ES|QL related features in Kibana backport:skip This commit does not require backporting v9.1.0 release_note:skip Skip the PR/issue when compiling release notes labels May 23, 2025

sddonne marked this pull request as ready for review May 23, 2025 11:41

sddonne requested a review from a team as a code owner May 23, 2025 11:41

sddonne requested review from drewdaemon, stratoula and vadimkibana May 23, 2025 11:42

Merge branch 'main' into esql/rrf/ast-parsing

0779540

stratoula reviewed May 23, 2025

View reviewed changes

sddonne added 4 commits May 23, 2025 15:59

add failure test to rrf command parse

80655ce

Merge branch 'esql/rrf/ast-parsing' of https://github.com/sddonne/kibana

36fa32d

into esql/rrf/ast-parsing

fix: parameter not needed

2130458

unify metada error codes

37b15c7

	export function suggest(_params: CommandSuggestParams<'rrf'>): SuggestionRawDefinition[] {
	export function suggest(): SuggestionRawDefinition[] {

	code: `rrfMissing${fieldName}Metadata`,
	code: `rrfMissingMetadataField`,

[ES|QL] Add support for RRF #221349

Are you sure you want to change the base?

[ES|QL] Add support for RRF #221349

Conversation

sddonne commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Considerations

commandsSuggestionsAfter

RRF may change in the future

Licence check

New command support checklist

AST support

Validating that the command works well when prettifying the query

Creating the command definition

Adding the corresponding client-side validations

Adding the autocomplete suggestions

Checklist

Identify risks

Uh oh!

elasticmachine commented May 23, 2025

Uh oh!

sddonne commented May 23, 2025

Uh oh!

stratoula left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sddonne May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented May 23, 2025

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Public APIs missing comments

Page load bundle

API count

History

Uh oh!

drewdaemon commented May 23, 2025

Uh oh!

Uh oh!

[ES|QL] Add support for `RRF` #221349

[ES|QL] Add support for `RRF` #221349

sddonne commented May 23, 2025 •

edited

Loading

`commandsSuggestionsAfter`

`RRF` may change in the future

sddonne May 23, 2025 •

edited

Loading