Skip to content

[ES|QL] Add validation coverage for wildcards on commands#170014

Merged
dej611 merged 12 commits intoelastic:feature/esql-validationfrom
dej611:fix/fuzzy-columns-validation
Nov 1, 2023
Merged

[ES|QL] Add validation coverage for wildcards on commands#170014
dej611 merged 12 commits intoelastic:feature/esql-validationfrom
dej611:fix/fuzzy-columns-validation

Conversation

@dej611
Copy link
Copy Markdown
Contributor

@dej611 dej611 commented Oct 27, 2023

Summary

This PR extends the validation logic to cover also wildcase scenarios for various commands:

  • wildcards are now supported by drop and keep:
Screenshot 2023-10-27 at 12 21 19 Screenshot 2023-10-27 at 12 21 41
  • the special case of drop * is marked as error:
Screenshot 2023-10-27 at 12 21 12
  • a fuzzy search is performed over fields and variables for wildcard names, and errors are reported if no match is found:
Screenshot 2023-10-27 at 12 21 29
  • handled also the case of drop @timestamp with a warning:
Screenshot 2023-10-27 at 12 40 59

Notes:

  • by*tes *bytes and bytes* will all match bytes from my tests on ES, therefore the validation will make them pass as valid if there's a bytes field.

Checklist

Delete any items that are not applicable to this PR.

@dej611 dej611 added Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// release_note:skip Skip the PR/issue when compiling release notes Feature:ES|QL ES|QL related features in Kibana v8.12.0 labels Oct 27, 2023
@dej611 dej611 marked this pull request as ready for review October 30, 2023 08:09
@dej611 dej611 requested a review from a team as a code owner October 30, 2023 08:09
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/kibana-visualizations (Team:Visualizations)

@drewdaemon
Copy link
Copy Markdown
Contributor

All your examples work great! However, I get a validation error when dropping with the wildcard at the first, even though it successfully drops the field. Same thing happens if the wildcard is in the middle of the term.

Screen.Recording.2023-10-30.at.1.14.18.PM.mov

@dej611
Copy link
Copy Markdown
Contributor Author

dej611 commented Oct 31, 2023

Good findings. I've never seen such usage in general. I'll try to add them as well.

@dej611
Copy link
Copy Markdown
Contributor Author

dej611 commented Oct 31, 2023

@drewdaemon addressed the issues with b54e6f2

@drewdaemon drewdaemon added the ui-copy Review of UI copy with docs team is recommended label Oct 31, 2023
Copy link
Copy Markdown
Contributor

@drewdaemon drewdaemon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great! I had one question about the necessity of the skipCommonValidation flag.

I'm approving but I added the docs label because I think it would be nice to get their feedback on the validation messages.

params: [{ name: 'separator', type: 'string' }],
},
optional: true,
skipCommonValidation: true, // tell the validation engine to use only the validate function here
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason we can't just use the presence of the validate method to decide (i.e. a clean override)?

Copy link
Copy Markdown
Contributor Author

@dej611 dej611 Nov 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially that was the design, but then I rolled back to this other decision.
There are several reasons for this change:

  • shared validation code is quite a big surface and replicate it every time to just add a tiny little bit of extra ad-hoc validation doesn't make sense
  • therefore I want a way to add only this extra validation bit on top of the common one
  • ideally I would like to have a "positive" flag (something like commonValidation: true) but I've only few cases (one so far) where this extra bit is required and adding a "negative" ("skipCommonValidation") was the less intrusive one

Another design approach could be to rename validate as extraValidation to semantically avoid this flag + callback confusion.

@dej611
Copy link
Copy Markdown
Contributor Author

dej611 commented Nov 1, 2023

I'm approving but I added the docs label because I think it would be nice to get their feedback on the validation messages.

I've planned a copy review of all messages once all the validation has been merged into the feature branch.
That is because some messages depends on ES current error message copy while others are specific to Kibana and I need to flag them for the copy team.

@kibana-ci
Copy link
Copy Markdown

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
textBasedLanguages 149.4KB 149.4KB +1.0B

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 2.3MB 2.3MB +1.7KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@dej611 dej611 merged commit db639ea into elastic:feature/esql-validation Nov 1, 2023
dej611 added a commit that referenced this pull request Dec 7, 2023
## Summary

Fixes #166242 , #166876, #166173
, part of #166092, #166084

List of tasks:

* [x] AST work ( #166185 )
* [x] Basic validate work ( #166185 )
* [x] Hover feature ( #166185 )
* [x] Initial autocomplete work with new AST ( #166185 )
* Complete validation feature for MVP
  * [x] wildcards support ( #170014 )
  * [x] remote index validation support ( #171996 )
  * [x] wildcard support as `count` argument  ( #172054 )
  * [x] Aggressive caching for field queries:
* cache as much as possible the `FROM` queries - possible clear the
cache every 10/15 minutes?
    * do not fire a query when code == submitted code
* cache as much as possible the custom `FROM` built from `ENRICH`
policies - same clear policy as above
  * [x] Add unsupported fields warning messages
  * [x] Notify usage of `project` command with deprecation `warning`
* Complete autocomplete work ( #171664 )
  * [x] `stats`
  * [x] `where`
  * [x] `eval`
    * `math syntax`
  * [x] Aggressive cache for fields queries? ( #171866 )
  * [x] Fix when cursor is not at the end position ( #172060 ) 
* [x] Revisit copy messages
  * Label Kibana-only messages
* [x] Extend hover feature ( #171940 )
* [x] Disable editor query highlight for warnings ( #171968 )
* Fix editor highlight with new grammar
  * [x] on multi-line ( #172080 )
  * [x] for functions ( #172287 )

## Release notes

Enhance ES|QL query editing experience with client side validation.
Enhance ES|QL suggestions experience with more in context suggestions
leveraging field and variable types.
Show meta informations on ES|QL query hover on policy names.
Show function signature on ES|QL query hover on function text.


### Checklist

Delete any items that are not applicable to this PR.

- [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/packages/kbn-i18n/README.md)
- [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
- [x] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
Co-authored-by: Abdon Pijpelink <abdon.pijpelink@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:ES|QL ES|QL related features in Kibana release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// ui-copy Review of UI copy with docs team is recommended v8.12.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants