Skip to content

[ES|QL] Sorting accepts expressions#181916

Merged
drewdaemon merged 7 commits intoelastic:mainfrom
drewdaemon:sorting-accepts-expressions
May 1, 2024
Merged

[ES|QL] Sorting accepts expressions#181916
drewdaemon merged 7 commits intoelastic:mainfrom
drewdaemon:sorting-accepts-expressions

Conversation

@drewdaemon
Copy link
Copy Markdown
Contributor

@drewdaemon drewdaemon commented Apr 26, 2024

Summary

SORT accepts expressions as of elastic/elasticsearch#107158.

This PR updates the validator and autocompletion engine to account for this improvement.

Checklist

  • Documentation was added — looks like docs haven't been added for this feature yet. I opened the discussion with ES team (ref)
  • Unit or functional tests were updated or added to match the most common scenarios

@drewdaemon drewdaemon added Team:ESQL ES|QL related features in Kibana t// backport:prev-minor labels Apr 26, 2024
@drewdaemon
Copy link
Copy Markdown
Contributor Author

/ci

1 similar comment
@drewdaemon
Copy link
Copy Markdown
Contributor Author

/ci

@drewdaemon drewdaemon marked this pull request as ready for review April 26, 2024 22:19
@drewdaemon drewdaemon requested a review from a team as a code owner April 26, 2024 22:19
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/kibana-esql (Team:ESQL)

@stratoula stratoula added Feature:ES|QL ES|QL related features in Kibana v8.14.0 v8.15.0 release_note:skip Skip the PR/issue when compiling release notes labels Apr 29, 2024
Copy link
Copy Markdown
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.

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';
Copy link
Copy Markdown
Contributor

@stratoula stratoula Apr 29, 2024

Choose a reason for hiding this comment

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

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 🙈

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oof. Good point. I will look into what code the where command is hitting...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

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.

yy sounds fair and thanx for the change, def less confusing!

@drewdaemon
Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

@drewdaemon drewdaemon requested a review from stratoula May 1, 2024 15:47
@kibana-ci
Copy link
Copy Markdown

💚 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 3.1MB 3.1MB +98.0B

History

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

@drewdaemon drewdaemon merged commit 5ba6a39 into elastic:main May 1, 2024
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request May 1, 2024
## 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)
@kibanamachine
Copy link
Copy Markdown
Contributor

💚 All backports created successfully

Status Branch Result
8.14

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

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request May 1, 2024
# 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>
TinLe added a commit to TinLe/kibana that referenced this pull request May 1, 2024
* 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)
  ...
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:ESQL ES|QL related features in Kibana t// v8.14.0 v8.15.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants