[ES|QL] Sorting accepts expressions#181916
Conversation
|
/ci |
1 similar comment
|
/ci |
|
Pinging @elastic/kibana-esql (Team:ESQL) |
stratoula
left a comment
There was a problem hiding this comment.
Just a code review for now
|
|
||
| // enrich with assignment has some special rules who are handled somewhere else | ||
| const canHaveAssignments = ['eval', 'stats', 'row'].includes(command.name); | ||
| const canHaveFunctions = canHaveAssignments || command.name === 'sort'; |
There was a problem hiding this comment.
Can you add a comment here showcasing what we mean with canHaveFunctions? The Where can have functions too so this variable name confuses me a bit 🙈
There was a problem hiding this comment.
Oof. Good point. I will look into what code the where command is hitting...
There was a problem hiding this comment.
The where command's parameter is of type boolean so it hits a different branch. I have updated the code here to be hopefully less confusing (LMK what you think).
However, this whole piece of logic deserves a second look. I find it strange that canHaveAssignments is being used to determine whether functions should be shown.
Since this PR is aimed at 8.14 and I don't want to disturb the universe, I will revisit in a follow-up effort.
There was a problem hiding this comment.
yy sounds fair and thanx for the change, def less confusing!
|
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Page load bundle
History
To update your PR or re-run it, just comment with: |
## Summary `SORT` accepts expressions as of elastic/elasticsearch#107158. This PR updates the validator and autocompletion engine to account for this improvement. ### Checklist - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added — looks like docs haven't been added for this feature yet. I opened the discussion with ES team ([ref](elastic/elasticsearch#107158 (comment))) - [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 <efstratia.kalafateli@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 5ba6a39)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
# Backport This will backport the following commits from `main` to `8.14`: - [[ES|QL] Sorting accepts expressions (#181916)](#181916) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Drew Tate","email":"drew.tate@elastic.co"},"sourceCommit":{"committedDate":"2024-05-01T17:32:37Z","message":"[ES|QL] Sorting accepts expressions (#181916)\n\n## Summary\r\n\r\n`SORT` accepts expressions as of\r\nhttps://github.com/elastic/elasticsearch/pull/107158.\r\n\r\nThis PR updates the validator and autocompletion engine to account for\r\nthis improvement.\r\n\r\n### Checklist\r\n\r\n- [ ]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas added — looks like docs haven't been added for this feature yet. I\r\nopened the discussion with ES team\r\n([ref](https://github.com/elastic/elasticsearch/pull/107158#issuecomment-2080063533))\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 <efstratia.kalafateli@elastic.co>\r\nCo-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>","sha":"5ba6a399f23282603011332b997044fc485fb270","branchLabelMapping":{"^v8.15.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","backport:prev-minor","Feature:ES|QL","v8.14.0","Team:ESQL","v8.15.0"],"title":"[ES|QL] Sorting accepts expressions","number":181916,"url":"https://github.com/elastic/kibana/pull/181916","mergeCommit":{"message":"[ES|QL] Sorting accepts expressions (#181916)\n\n## Summary\r\n\r\n`SORT` accepts expressions as of\r\nhttps://github.com/elastic/elasticsearch/pull/107158.\r\n\r\nThis PR updates the validator and autocompletion engine to account for\r\nthis improvement.\r\n\r\n### Checklist\r\n\r\n- [ ]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas added — looks like docs haven't been added for this feature yet. I\r\nopened the discussion with ES team\r\n([ref](https://github.com/elastic/elasticsearch/pull/107158#issuecomment-2080063533))\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 <efstratia.kalafateli@elastic.co>\r\nCo-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>","sha":"5ba6a399f23282603011332b997044fc485fb270"}},"sourceBranch":"main","suggestedTargetBranches":["8.14"],"targetPullRequestStates":[{"branch":"8.14","label":"v8.14.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.15.0","branchLabelMappingKey":"^v8.15.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/181916","number":181916,"mergeCommit":{"message":"[ES|QL] Sorting accepts expressions (#181916)\n\n## Summary\r\n\r\n`SORT` accepts expressions as of\r\nhttps://github.com/elastic/elasticsearch/pull/107158.\r\n\r\nThis PR updates the validator and autocompletion engine to account for\r\nthis improvement.\r\n\r\n### Checklist\r\n\r\n- [ ]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas added — looks like docs haven't been added for this feature yet. I\r\nopened the discussion with ES team\r\n([ref](https://github.com/elastic/elasticsearch/pull/107158#issuecomment-2080063533))\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 <efstratia.kalafateli@elastic.co>\r\nCo-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>","sha":"5ba6a399f23282603011332b997044fc485fb270"}}]}] BACKPORT--> Co-authored-by: Drew Tate <drew.tate@elastic.co>
* master: (1654 commits) Bump ejs from 3.1.9 to 3.1.10 Don't render exceptions flyout if data is loading (elastic#181588) Enable value list modal (elastic#181593) skip flaky suite (elastic#181777) skip failing test suite (elastic#182263) [Mappings Editor] Disable _source field in serverless (elastic#181712) [data.search] Fix unhandled promise rejections (elastic#181785) [Fleet] Fix logic for detecting first time Elastic Agent users (elastic#182214) [ML] Decouple data_visualizer from MapEmbeddable (elastic#181928) [ES|QL] Sorting accepts expressions (elastic#181916) [ML] Single Metric Viewer: ensures chart displays correctly when opening from a job annotation (elastic#182176) Adding optional Description field to Roles APIs (elastic#182039) Upgrade Markdown-it to 14.1.0 (elastic#182244) Bump xml-crypto from 5.0.0 to 6.0.0 [DOCS] Fix docs and screenshots for rule creation changes (elastic#181925) Update dependency elastic-apm-node to ^4.5.3 (main) (elastic#182236) [Obs AI Assistant] register alert details context in observability plugin (elastic#181501) Add `@typescript-eslint/no-floating-promises` (elastic#181456) [Playground] Propagate Error message into FE (elastic#182201) [ES|QL] Rename the setting to a more generic one and move to the general section (elastic#182074) ...
Summary
SORTaccepts expressions as of elastic/elasticsearch#107158.This PR updates the validator and autocompletion engine to account for this improvement.
Checklist