Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ES|QL] Fix some validation misconfiguration #177783

Merged
merged 10 commits into from
Mar 7, 2024

Conversation

dej611
Copy link
Contributor

@dej611 dej611 commented Feb 26, 2024

Summary

Related issue #177699

Fix variables logic for expressions at stats by ... level.
Fix validation logic for agg functions within eval or where scope.
Fix validation and autocomplete logic for nested quoted expressions

  • i.e.
from index | eval round(numberField) + 1 | eval `round(numberField) + 1` + 1 | eval ```round(numberField) + 1`` + 1` + 1 | eval ```````round(numberField) + 1```` + 1`` + 1` + 1 | eval ```````````````round(numberField) + 1```````` + 1```` + 1`` + 1` + 1 | keep ```````````````````````````````round(numberField) + 1```````````````` + 1```````` + 1```` + 1`` + 1`
  • updated count_distinct agg definition to have the precision second optional param.

Checklist

@dej611 dej611 added Team:Visualizations Visualization editors, elastic-charts and infrastructure release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Feature:ES|QL ES|QL related features in Kibana v8.13.0 v8.14.0 labels Feb 26, 2024
@dej611
Copy link
Contributor Author

dej611 commented Feb 26, 2024

/ci

@dej611 dej611 marked this pull request as ready for review February 26, 2024 16:03
@dej611 dej611 requested a review from a team as a code owner February 26, 2024 16:03
@elasticmachine
Copy link
Contributor

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

@stratoula
Copy link
Contributor

/ci

@stratoula
Copy link
Contributor

So far so good but if you do something like that it also errors out while it should not

image

@dej611
Copy link
Contributor Author

dej611 commented Mar 4, 2024

So far so good but if you do something like that it also errors out while it should not

image

I think that is fixable, but unsure about the timing here.
If we're still in time for 8.13 I propose to fix it as a follow up, while if we're already in the .1 patch area I'll merge it here.

@stratoula
Copy link
Contributor

We have till the 13th of March, why not fix it here? It is related with the bug you are trying to fix in this PR

@dej611
Copy link
Contributor Author

dej611 commented Mar 6, 2024

/ci

@dej611
Copy link
Contributor Author

dej611 commented Mar 6, 2024

/ci

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

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.9MB 2.9MB -153.0B

History

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

testSuggestions(
`from a_index | eval round(numberField) + 1 | eval \`round(numberField) + 1\` + 1 | eval \`\`\`round(numberField) + 1\`\` + 1\` + 1 | eval \`\`\`\`\`\`\`round(numberField) + 1\`\`\`\` + 1\`\` + 1\` + 1 | eval \`\`\`\`\`\`\`\`\`\`\`\`\`\`\`round(numberField) + 1\`\`\`\`\`\`\`\` + 1\`\`\`\` + 1\`\` + 1\` + 1 | ${command} `,
[
...getFieldNamesByType('any'),
Copy link
Contributor

@stratoula stratoula Mar 7, 2024

Choose a reason for hiding this comment

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

😄

Copy link
Contributor

@stratoula stratoula 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 now! LGTM!

@dej611 dej611 merged commit cad276f into elastic:main Mar 7, 2024
16 checks passed
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.13 Backport failed because of merge conflicts

You might need to backport the following PRs to 8.13:
- [Profiling] Fix default value of profiling settings (#177716)

Manual backport

To create the backport manually run:

node scripts/backport --pr 177783

Questions ?

Please refer to the Backport tool documentation

@drewdaemon
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.13

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

drewdaemon pushed a commit to drewdaemon/kibana that referenced this pull request Mar 7, 2024
## Summary

Related issue elastic#177699

Fix variables logic for expressions at `stats by ...` level.
Fix validation logic for agg functions within `eval` or `where` scope.
Fix validation and autocomplete logic for nested quoted expressions
  * i.e.
  ```
from index | eval round(numberField) + 1 | eval `round(numberField) + 1`
+ 1 | eval ```round(numberField) + 1`` + 1` + 1 | eval
```````round(numberField) + 1```` + 1`` + 1` + 1 | eval
```````````````round(numberField) + 1```````` + 1```` + 1`` + 1` + 1 |
keep ```````````````````````````````round(numberField) +
1```````````````` + 1```````` + 1```` + 1`` + 1`
  ```
* updated `count_distinct` agg definition to have the `precision` second
optional param.

### Checklist

- [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

---------

Co-authored-by: Stratoula Kalafateli <[email protected]>
(cherry picked from commit cad276f)

# Conflicts:
#	packages/kbn-monaco/src/esql/lib/ast/validation/esql_validation_meta_tests.json
drewdaemon added a commit that referenced this pull request Mar 26, 2024
# Backport

This will backport the following commits from `main` to `8.13`:
- [[ES|QL] Fix some validation misconfiguration
(#177783)](#177783)

<!--- Backport version: 8.9.8 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Marco
Liberati","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-03-07T12:57:27Z","message":"[ES|QL]
Fix some validation misconfiguration (#177783)\n\n##
Summary\r\n\r\nRelated issue #177699\r\n\r\nFix variables logic for
expressions at `stats by ...` level.\r\nFix validation logic for agg
functions within `eval` or `where` scope.\r\nFix validation and
autocomplete logic for nested quoted expressions\r\n * i.e. \r\n
```\r\nfrom index | eval round(numberField) + 1 | eval
`round(numberField) + 1`\r\n+ 1 | eval ```round(numberField) + 1`` + 1`
+ 1 | eval\r\n```````round(numberField) + 1```` + 1`` + 1` + 1 |
eval\r\n```````````````round(numberField) + 1```````` + 1```` + 1`` + 1`
+ 1 |\r\nkeep ```````````````````````````````round(numberField)
+\r\n1```````````````` + 1```````` + 1```` + 1`` + 1`\r\n ```\r\n*
updated `count_distinct` agg definition to have the `precision`
second\r\noptional param.\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Stratoula Kalafateli
<[email protected]>","sha":"cad276fcbd9fe5b13c18f08fd01bcf0c802b7ec9","branchLabelMapping":{"^v8.14.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Visualizations","release_note:skip","backport:prev-minor","Feature:ES|QL","v8.13.0","v8.14.0"],"number":177783,"url":"https://github.com/elastic/kibana/pull/177783","mergeCommit":{"message":"[ES|QL]
Fix some validation misconfiguration (#177783)\n\n##
Summary\r\n\r\nRelated issue #177699\r\n\r\nFix variables logic for
expressions at `stats by ...` level.\r\nFix validation logic for agg
functions within `eval` or `where` scope.\r\nFix validation and
autocomplete logic for nested quoted expressions\r\n * i.e. \r\n
```\r\nfrom index | eval round(numberField) + 1 | eval
`round(numberField) + 1`\r\n+ 1 | eval ```round(numberField) + 1`` + 1`
+ 1 | eval\r\n```````round(numberField) + 1```` + 1`` + 1` + 1 |
eval\r\n```````````````round(numberField) + 1```````` + 1```` + 1`` + 1`
+ 1 |\r\nkeep ```````````````````````````````round(numberField)
+\r\n1```````````````` + 1```````` + 1```` + 1`` + 1`\r\n ```\r\n*
updated `count_distinct` agg definition to have the `precision`
second\r\noptional param.\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Stratoula Kalafateli
<[email protected]>","sha":"cad276fcbd9fe5b13c18f08fd01bcf0c802b7ec9"}},"sourceBranch":"main","suggestedTargetBranches":["8.13"],"targetPullRequestStates":[{"branch":"8.13","label":"v8.13.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.14.0","labelRegex":"^v8.14.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/177783","number":177783,"mergeCommit":{"message":"[ES|QL]
Fix some validation misconfiguration (#177783)\n\n##
Summary\r\n\r\nRelated issue #177699\r\n\r\nFix variables logic for
expressions at `stats by ...` level.\r\nFix validation logic for agg
functions within `eval` or `where` scope.\r\nFix validation and
autocomplete logic for nested quoted expressions\r\n * i.e. \r\n
```\r\nfrom index | eval round(numberField) + 1 | eval
`round(numberField) + 1`\r\n+ 1 | eval ```round(numberField) + 1`` + 1`
+ 1 | eval\r\n```````round(numberField) + 1```` + 1`` + 1` + 1 |
eval\r\n```````````````round(numberField) + 1```````` + 1```` + 1`` + 1`
+ 1 |\r\nkeep ```````````````````````````````round(numberField)
+\r\n1```````````````` + 1```````` + 1```` + 1`` + 1`\r\n ```\r\n*
updated `count_distinct` agg definition to have the `precision`
second\r\noptional param.\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Stratoula Kalafateli
<[email protected]>","sha":"cad276fcbd9fe5b13c18f08fd01bcf0c802b7ec9"}}]}]
BACKPORT-->

Co-authored-by: Marco Liberati <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
@mistic
Copy link
Member

mistic commented Mar 27, 2024

This PR didn't make it into the latest BC. Updating the labels.

@mistic mistic added v8.13.1 and removed v8.13.0 labels Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Feature:ES|QL ES|QL related features in Kibana release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.13.1 v8.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants