[ES|QL] Remove command option definitions#215425
Merged
drewdaemon merged 24 commits intoelastic:mainfrom Mar 21, 2025
Merged
Conversation
Contributor
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Public APIs missing comments
Page load bundle
Unknown metric groupsAPI count
Unreferenced deprecated APIs
History
|
Contributor
|
Pinging @elastic/kibana-esql (Team:ESQL) |
stratoula
approved these changes
Mar 21, 2025
Contributor
stratoula
left a comment
There was a problem hiding this comment.
This is a very nice cleanup 👏
Contributor
|
Starting backport for target branches: 8.x https://github.com/elastic/kibana/actions/runs/13990332729 |
Contributor
💔 All backports failed
Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation |
This was referenced Mar 21, 2025
Contributor
Author
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
drewdaemon
added a commit
to drewdaemon/kibana
that referenced
this pull request
Mar 21, 2025
## 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
drewdaemon
added a commit
that referenced
this pull request
Mar 24, 2025
# Backport This will backport the following commits from `main` to `8.x`: - [[ES|QL] Remove command option definitions (#215425)](#215425) <!--- Backport version: 9.6.6 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport) <!--BACKPORT [{"author":{"name":"Drew Tate","email":"drew.tate@elastic.co"},"sourceCommit":{"committedDate":"2025-03-21T11:04:33Z","message":"[ES|QL] Remove command option definitions (#215425)\n\n## Summary\n\nThis PR removes the declarative objects that were meant to describe the\nbehavior of \"options\" (see details section below if you don't know what\nI'm talking about). **It does not remove \"options\" as a concept from our\nAST.** \"Option\" is probably the wrong name for the subcommands in the\nAST but, at the moment, it is working fine how it is.\n\nHere is a list of what these definitions were being used for and where I\nended up.\n\n| Use | How it worked | What I did |\n\n|---------------------------------------------------------------------|---------------|------------|\n| To generate command declarations for display in suggestions menu | It\nhad some complex logic to try to construct a declaration string from the\ninformation in the `signature` property | I replaced this with\nstatically declared declaration strings on the command definitions. I\ntook most of them directly from our docs. They are a better result than\nthe autogenerated stuff |\n| To build the `METADATA` suggestion | the definition was passed into\n`buildOptionDefinition` | I declared the `METADATA` suggestion\nstatically in the `FROM` autocomplete code. |\n| To check for field correctness in `METADATA` | This logic lived in the\noption definition's `validate` method | I moved it to the `FROM`\ncommand's validate method |\n| To validate the type of the value assigned to `APPEND_SEPARATOR` in\n`DISSECT` | This logic lived in the option definition's `validate`\nmethod | I moved it to the `DISSECT` command's validate method |\n| To check if the left side of the equals sign in `DISSECT` is\n\"APPEND_SEPARATOR | In most cases, the parser catches stuff like this,\nbut for some reason `DISSECT`'s grammar is very loose so we have been\nstepping in with our own validation (maybe we should suggest changing\nthis). This was the only case that was triggering the \"Unknown option\"\nmessage. | I moved it to the `DISSECT` command's validate method |\n| To prevent default column validation for `METADATA` | This was the\nonly true use of the `skipCommonValidation` property which would prevent\nthe validator trying to find metadata fields in the standard field list\n| I inserted an option name check directly into the validation code.\nIt's not a good long-term solution, but it is actually an improvement\nsince the former code pretended to be general but was actually just for\n`METADATA`. At least now it is clear what the exception is for. |\n| To filter functions and operators that are available after `BY` |\nFunction definitions sometimes declare that they are supported in a `by`\nstatement. The validator checks if the function does. | This didn't\nchange. The option nodes in the AST are still there and we are still\nrelying on the `supportedCommands` and `supportedOptions` properties in\nthe function definitions. |\n\n#### Pictures\n\n<img width=\"859\" alt=\"Screenshot 2025-03-20 at 1 47 36 PM\"\nsrc=\"https://github.com/user-attachments/assets/3bd3c3c6-6066-466e-b33b-9444ab58670a\"\n/>\n\n_New, statically-defined declarations_\n\n<img width=\"783\" alt=\"Screenshot 2025-03-20 at 2 12 28 PM\"\nsrc=\"https://github.com/user-attachments/assets/94550b25-5da9-4c82-9586-11b3515debd7\"\n/>\n\n_In cases besides `APPEND_SEPARATOR`, incorrect keywords produce syntax\nerrors._\n\n<img width=\"700\" alt=\"Screenshot 2025-03-20 at 2 09 05 PM\"\nsrc=\"https://github.com/user-attachments/assets/de1a23f4-2509-4c6e-84ec-a807e96b65a5\"\n/>\n\n_Didn't break the `APPEND_SEPARATOR` datatype validation_\n\n<img width=\"791\" alt=\"Screenshot 2025-03-20 at 2 03 28 PM\"\nsrc=\"https://github.com/user-attachments/assets/169aaa15-52f3-4d22-ab77-26a560cd9359\"\n/>\n\n_Didn't break `METADATA` fields validation_\n\n### Checklist\n\nCheck the PR satisfies following conditions. \n\nReviewers should verify this PR satisfies this list as well.\n\n- [x] Any text added follows [EUI's writing\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\nsentence case text and includes [i18n\nsupport](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)\n- [x]\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\nwas added for features that require explanation or tutorials\n- [x] [Unit or functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere updated or added to match the most common scenarios\n\n### Background/details\n\nTill now, \"options\" have been a concept in our code. Their definition\nisn't clear, but it essentially comes down to any capitalized keyword\nafter the command name. For example `STATS... >BY<`, `DISSECT...\n>APPEND_SEPARATOR<`. You could think of them as roughly subcommands or\nsubstatements.\n\nThere was a hope that commands would be uniform enough that these\n\"options\" would deserve to be their own special first-class citizen. But\nthey break conceptually...\n\nFor example `APPEND_SEPARATOR` is not a keyword with an expression after\nit... it is a variable `APPEND_SEPARATOR=\":\"`... or filtering in\nstats.... `STATS AVG(bytes) >WHERE<` .... so is WHERE an option now?\n\n`FORK` will break this even further.\n\nSo, we are moving the architecture to allow for complexity and variance\namong the commands. Command-specific logic will have the final say in\nhow autocomplete and validation work for anything with that command.","sha":"b7854a8759ca91255fe318c8d7a33b91996bf990","branchLabelMapping":{"^v9.1.0$":"main","^v8.19.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Feature:ES|QL","Team:ESQL","backport:version","v9.1.0","v8.19.0"],"title":"[ES|QL] Remove command option definitions","number":215425,"url":"https://github.com/elastic/kibana/pull/215425","mergeCommit":{"message":"[ES|QL] Remove command option definitions (#215425)\n\n## Summary\n\nThis PR removes the declarative objects that were meant to describe the\nbehavior of \"options\" (see details section below if you don't know what\nI'm talking about). **It does not remove \"options\" as a concept from our\nAST.** \"Option\" is probably the wrong name for the subcommands in the\nAST but, at the moment, it is working fine how it is.\n\nHere is a list of what these definitions were being used for and where I\nended up.\n\n| Use | How it worked | What I did |\n\n|---------------------------------------------------------------------|---------------|------------|\n| To generate command declarations for display in suggestions menu | It\nhad some complex logic to try to construct a declaration string from the\ninformation in the `signature` property | I replaced this with\nstatically declared declaration strings on the command definitions. I\ntook most of them directly from our docs. They are a better result than\nthe autogenerated stuff |\n| To build the `METADATA` suggestion | the definition was passed into\n`buildOptionDefinition` | I declared the `METADATA` suggestion\nstatically in the `FROM` autocomplete code. |\n| To check for field correctness in `METADATA` | This logic lived in the\noption definition's `validate` method | I moved it to the `FROM`\ncommand's validate method |\n| To validate the type of the value assigned to `APPEND_SEPARATOR` in\n`DISSECT` | This logic lived in the option definition's `validate`\nmethod | I moved it to the `DISSECT` command's validate method |\n| To check if the left side of the equals sign in `DISSECT` is\n\"APPEND_SEPARATOR | In most cases, the parser catches stuff like this,\nbut for some reason `DISSECT`'s grammar is very loose so we have been\nstepping in with our own validation (maybe we should suggest changing\nthis). This was the only case that was triggering the \"Unknown option\"\nmessage. | I moved it to the `DISSECT` command's validate method |\n| To prevent default column validation for `METADATA` | This was the\nonly true use of the `skipCommonValidation` property which would prevent\nthe validator trying to find metadata fields in the standard field list\n| I inserted an option name check directly into the validation code.\nIt's not a good long-term solution, but it is actually an improvement\nsince the former code pretended to be general but was actually just for\n`METADATA`. At least now it is clear what the exception is for. |\n| To filter functions and operators that are available after `BY` |\nFunction definitions sometimes declare that they are supported in a `by`\nstatement. The validator checks if the function does. | This didn't\nchange. The option nodes in the AST are still there and we are still\nrelying on the `supportedCommands` and `supportedOptions` properties in\nthe function definitions. |\n\n#### Pictures\n\n<img width=\"859\" alt=\"Screenshot 2025-03-20 at 1 47 36 PM\"\nsrc=\"https://github.com/user-attachments/assets/3bd3c3c6-6066-466e-b33b-9444ab58670a\"\n/>\n\n_New, statically-defined declarations_\n\n<img width=\"783\" alt=\"Screenshot 2025-03-20 at 2 12 28 PM\"\nsrc=\"https://github.com/user-attachments/assets/94550b25-5da9-4c82-9586-11b3515debd7\"\n/>\n\n_In cases besides `APPEND_SEPARATOR`, incorrect keywords produce syntax\nerrors._\n\n<img width=\"700\" alt=\"Screenshot 2025-03-20 at 2 09 05 PM\"\nsrc=\"https://github.com/user-attachments/assets/de1a23f4-2509-4c6e-84ec-a807e96b65a5\"\n/>\n\n_Didn't break the `APPEND_SEPARATOR` datatype validation_\n\n<img width=\"791\" alt=\"Screenshot 2025-03-20 at 2 03 28 PM\"\nsrc=\"https://github.com/user-attachments/assets/169aaa15-52f3-4d22-ab77-26a560cd9359\"\n/>\n\n_Didn't break `METADATA` fields validation_\n\n### Checklist\n\nCheck the PR satisfies following conditions. \n\nReviewers should verify this PR satisfies this list as well.\n\n- [x] Any text added follows [EUI's writing\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\nsentence case text and includes [i18n\nsupport](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)\n- [x]\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\nwas added for features that require explanation or tutorials\n- [x] [Unit or functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere updated or added to match the most common scenarios\n\n### Background/details\n\nTill now, \"options\" have been a concept in our code. Their definition\nisn't clear, but it essentially comes down to any capitalized keyword\nafter the command name. For example `STATS... >BY<`, `DISSECT...\n>APPEND_SEPARATOR<`. You could think of them as roughly subcommands or\nsubstatements.\n\nThere was a hope that commands would be uniform enough that these\n\"options\" would deserve to be their own special first-class citizen. But\nthey break conceptually...\n\nFor example `APPEND_SEPARATOR` is not a keyword with an expression after\nit... it is a variable `APPEND_SEPARATOR=\":\"`... or filtering in\nstats.... `STATS AVG(bytes) >WHERE<` .... so is WHERE an option now?\n\n`FORK` will break this even further.\n\nSo, we are moving the architecture to allow for complexity and variance\namong the commands. Command-specific logic will have the final say in\nhow autocomplete and validation work for anything with that command.","sha":"b7854a8759ca91255fe318c8d7a33b91996bf990"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/215425","number":215425,"mergeCommit":{"message":"[ES|QL] Remove command option definitions (#215425)\n\n## Summary\n\nThis PR removes the declarative objects that were meant to describe the\nbehavior of \"options\" (see details section below if you don't know what\nI'm talking about). **It does not remove \"options\" as a concept from our\nAST.** \"Option\" is probably the wrong name for the subcommands in the\nAST but, at the moment, it is working fine how it is.\n\nHere is a list of what these definitions were being used for and where I\nended up.\n\n| Use | How it worked | What I did |\n\n|---------------------------------------------------------------------|---------------|------------|\n| To generate command declarations for display in suggestions menu | It\nhad some complex logic to try to construct a declaration string from the\ninformation in the `signature` property | I replaced this with\nstatically declared declaration strings on the command definitions. I\ntook most of them directly from our docs. They are a better result than\nthe autogenerated stuff |\n| To build the `METADATA` suggestion | the definition was passed into\n`buildOptionDefinition` | I declared the `METADATA` suggestion\nstatically in the `FROM` autocomplete code. |\n| To check for field correctness in `METADATA` | This logic lived in the\noption definition's `validate` method | I moved it to the `FROM`\ncommand's validate method |\n| To validate the type of the value assigned to `APPEND_SEPARATOR` in\n`DISSECT` | This logic lived in the option definition's `validate`\nmethod | I moved it to the `DISSECT` command's validate method |\n| To check if the left side of the equals sign in `DISSECT` is\n\"APPEND_SEPARATOR | In most cases, the parser catches stuff like this,\nbut for some reason `DISSECT`'s grammar is very loose so we have been\nstepping in with our own validation (maybe we should suggest changing\nthis). This was the only case that was triggering the \"Unknown option\"\nmessage. | I moved it to the `DISSECT` command's validate method |\n| To prevent default column validation for `METADATA` | This was the\nonly true use of the `skipCommonValidation` property which would prevent\nthe validator trying to find metadata fields in the standard field list\n| I inserted an option name check directly into the validation code.\nIt's not a good long-term solution, but it is actually an improvement\nsince the former code pretended to be general but was actually just for\n`METADATA`. At least now it is clear what the exception is for. |\n| To filter functions and operators that are available after `BY` |\nFunction definitions sometimes declare that they are supported in a `by`\nstatement. The validator checks if the function does. | This didn't\nchange. The option nodes in the AST are still there and we are still\nrelying on the `supportedCommands` and `supportedOptions` properties in\nthe function definitions. |\n\n#### Pictures\n\n<img width=\"859\" alt=\"Screenshot 2025-03-20 at 1 47 36 PM\"\nsrc=\"https://github.com/user-attachments/assets/3bd3c3c6-6066-466e-b33b-9444ab58670a\"\n/>\n\n_New, statically-defined declarations_\n\n<img width=\"783\" alt=\"Screenshot 2025-03-20 at 2 12 28 PM\"\nsrc=\"https://github.com/user-attachments/assets/94550b25-5da9-4c82-9586-11b3515debd7\"\n/>\n\n_In cases besides `APPEND_SEPARATOR`, incorrect keywords produce syntax\nerrors._\n\n<img width=\"700\" alt=\"Screenshot 2025-03-20 at 2 09 05 PM\"\nsrc=\"https://github.com/user-attachments/assets/de1a23f4-2509-4c6e-84ec-a807e96b65a5\"\n/>\n\n_Didn't break the `APPEND_SEPARATOR` datatype validation_\n\n<img width=\"791\" alt=\"Screenshot 2025-03-20 at 2 03 28 PM\"\nsrc=\"https://github.com/user-attachments/assets/169aaa15-52f3-4d22-ab77-26a560cd9359\"\n/>\n\n_Didn't break `METADATA` fields validation_\n\n### Checklist\n\nCheck the PR satisfies following conditions. \n\nReviewers should verify this PR satisfies this list as well.\n\n- [x] Any text added follows [EUI's writing\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\nsentence case text and includes [i18n\nsupport](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)\n- [x]\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\nwas added for features that require explanation or tutorials\n- [x] [Unit or functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere updated or added to match the most common scenarios\n\n### Background/details\n\nTill now, \"options\" have been a concept in our code. Their definition\nisn't clear, but it essentially comes down to any capitalized keyword\nafter the command name. For example `STATS... >BY<`, `DISSECT...\n>APPEND_SEPARATOR<`. You could think of them as roughly subcommands or\nsubstatements.\n\nThere was a hope that commands would be uniform enough that these\n\"options\" would deserve to be their own special first-class citizen. But\nthey break conceptually...\n\nFor example `APPEND_SEPARATOR` is not a keyword with an expression after\nit... it is a variable `APPEND_SEPARATOR=\":\"`... or filtering in\nstats.... `STATS AVG(bytes) >WHERE<` .... so is WHERE an option now?\n\n`FORK` will break this even further.\n\nSo, we are moving the architecture to allow for complexity and variance\namong the commands. Command-specific logic will have the final say in\nhow autocomplete and validation work for anything with that command.","sha":"b7854a8759ca91255fe318c8d7a33b91996bf990"}},{"branch":"8.x","label":"v8.19.0","branchLabelMappingKey":"^v8.19.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT-->
JoseLuisGJ
pushed a commit
to JoseLuisGJ/kibana
that referenced
this pull request
Mar 24, 2025
## 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.
cqliu1
pushed a commit
to cqliu1/kibana
that referenced
this pull request
Mar 31, 2025
## 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.
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
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.
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.
signaturepropertyMETADATAsuggestionbuildOptionDefinitionMETADATAsuggestion statically in theFROMautocomplete code.METADATAvalidatemethodFROMcommand's validate methodAPPEND_SEPARATORinDISSECTvalidatemethodDISSECTcommand's validate methodDISSECTis "APPEND_SEPARATORDISSECT'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.DISSECTcommand's validate methodMETADATAskipCommonValidationproperty which would prevent the validator trying to find metadata fields in the standard field listMETADATA. At least now it is clear what the exception is for.BYbystatement. The validator checks if the function does.supportedCommandsandsupportedOptionsproperties in the function definitions.Pictures
New, statically-defined declarations
In cases besides
APPEND_SEPARATOR, incorrect keywords produce syntax errors.Didn't break the
APPEND_SEPARATORdatatype validationDidn't break
METADATAfields validationChecklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.
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_SEPARATORis not a keyword with an expression after it... it is a variableAPPEND_SEPARATOR=":"... or filtering in stats....STATS AVG(bytes) >WHERE<.... so is WHERE an option now?FORKwill 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.