[8.x] [ES|QL] Remove command option definitions (#215425)#215542
Merged
drewdaemon merged 1 commit intoelastic:8.xfrom Mar 24, 2025
Merged
[8.x] [ES|QL] Remove command option definitions (#215425)#215542drewdaemon merged 1 commit intoelastic:8.xfrom
drewdaemon merged 1 commit intoelastic:8.xfrom
Conversation
## Summary This PR removes the declarative objects that were meant to describe the behavior of "options" (see details section below if you don't know what I'm talking about). **It does not remove "options" as a concept from our AST.** "Option" is probably the wrong name for the subcommands in the AST but, at the moment, it is working fine how it is. Here is a list of what these definitions were being used for and where I ended up. | Use | How it worked | What I did | |---------------------------------------------------------------------|---------------|------------| | To generate command declarations for display in suggestions menu | It had some complex logic to try to construct a declaration string from the information in the `signature` property | I replaced this with statically declared declaration strings on the command definitions. I took most of them directly from our docs. They are a better result than the autogenerated stuff | | To build the `METADATA` suggestion | the definition was passed into `buildOptionDefinition` | I declared the `METADATA` suggestion statically in the `FROM` autocomplete code. | | To check for field correctness in `METADATA` | This logic lived in the option definition's `validate` method | I moved it to the `FROM` command's validate method | | To validate the type of the value assigned to `APPEND_SEPARATOR` in `DISSECT` | This logic lived in the option definition's `validate` method | I moved it to the `DISSECT` command's validate method | | To check if the left side of the equals sign in `DISSECT` is "APPEND_SEPARATOR | In most cases, the parser catches stuff like this, but for some reason `DISSECT`'s grammar is very loose so we have been stepping in with our own validation (maybe we should suggest changing this). This was the only case that was triggering the "Unknown option" message. | I moved it to the `DISSECT` command's validate method | | To prevent default column validation for `METADATA` | This was the only true use of the `skipCommonValidation` property which would prevent the validator trying to find metadata fields in the standard field list | I inserted an option name check directly into the validation code. It's not a good long-term solution, but it is actually an improvement since the former code pretended to be general but was actually just for `METADATA`. At least now it is clear what the exception is for. | | To filter functions and operators that are available after `BY` | Function definitions sometimes declare that they are supported in a `by` statement. The validator checks if the function does. | This didn't change. The option nodes in the AST are still there and we are still relying on the `supportedCommands` and `supportedOptions` properties in the function definitions. | #### Pictures <img width="859" alt="Screenshot 2025-03-20 at 1 47 36 PM" src="https://github.com/user-attachments/assets/3bd3c3c6-6066-466e-b33b-9444ab58670a" /> _New, statically-defined declarations_ <img width="783" alt="Screenshot 2025-03-20 at 2 12 28 PM" src="https://github.com/user-attachments/assets/94550b25-5da9-4c82-9586-11b3515debd7" /> _In cases besides `APPEND_SEPARATOR`, incorrect keywords produce syntax errors._ <img width="700" alt="Screenshot 2025-03-20 at 2 09 05 PM" src="https://github.com/user-attachments/assets/de1a23f4-2509-4c6e-84ec-a807e96b65a5" /> _Didn't break the `APPEND_SEPARATOR` datatype validation_ <img width="791" alt="Screenshot 2025-03-20 at 2 03 28 PM" src="https://github.com/user-attachments/assets/169aaa15-52f3-4d22-ab77-26a560cd9359" /> _Didn't break `METADATA` fields validation_ ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [x] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### Background/details Till now, "options" have been a concept in our code. Their definition isn't clear, but it essentially comes down to any capitalized keyword after the command name. For example `STATS... >BY<`, `DISSECT... >APPEND_SEPARATOR<`. You could think of them as roughly subcommands or substatements. There was a hope that commands would be uniform enough that these "options" would deserve to be their own special first-class citizen. But they break conceptually... For example `APPEND_SEPARATOR` is not a keyword with an expression after it... it is a variable `APPEND_SEPARATOR=":"`... or filtering in stats.... `STATS AVG(bytes) >WHERE<` .... so is WHERE an option now? `FORK` will break this even further. So, we are moving the architecture to allow for complexity and variance among the commands. Command-specific logic will have the final say in how autocomplete and validation work for anything with that command. (cherry picked from commit b7854a8) # Conflicts: # src/platform/packages/shared/kbn-esql-validation-autocomplete/src/definitions/options.ts
3 tasks
Contributor
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Page load bundle
|
vadimkibana
approved these changes
Mar 24, 2025
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Backport
This will backport the following commits from
mainto8.x:Questions ?
Please refer to the Backport tool documentation