Skip to content

[ES|QL] Wrap the fork subcommands inside the parens node#242369

Merged
bartoval merged 6 commits intoelastic:mainfrom
bartoval:fork_parens
Nov 20, 2025
Merged

[ES|QL] Wrap the fork subcommands inside the parens node#242369
bartoval merged 6 commits intoelastic:mainfrom
bartoval:fork_parens

Conversation

@bartoval
Copy link
Contributor

@bartoval bartoval commented Nov 10, 2025 •
edited
Loading

Summary

#242225

Now the fork command is wrapped inside a new node called parens, which changes the ast structure. So now the branches are children of parens.
@bartoval bartoval self-assigned this Nov 10, 2025
@bartoval bartoval requested a review from a team as a code owner November 10, 2025 06:48
@bartoval bartoval added release_note:enhancement backport:skip This PR does not require backporting Feature:ES|QL ES|QL related features in Kibana Team:ESQL ES|QL related features in Kibana t// v9.3.0 labels Nov 10, 2025
@elasticmachine
Copy link
Contributor

elasticmachine commented Nov 10, 2025

Pinging @elastic/kibana-esql (Team:ESQL)
@stratoula
Copy link
Contributor

stratoula commented Nov 10, 2025

Val I saw many tests failing, I will review when CI is happier 😄 but thanx for being so fast with this!
@bartoval
Copy link
Contributor Author

bartoval commented Nov 10, 2025

Val I saw many tests failing, I will review when CI is happier 😄 but thanx for being so fast with this!

yes, it's a constant pain
@bartoval bartoval requested a review from a team as a code owner November 10, 2025 11:31
@bartoval bartoval requested a review from machadoum November 10, 2025 11:31
bartoval
bartoval commented Nov 10, 2025
View reviewed changes
...ty_solution/public/entity_analytics/components/privileged_user_monitoring/queries/helpers.ts Outdated
import type { DataViewFieldMap } from '@kbn/data-views-plugin/common';
import { partition } from 'lodash/fp';
import type { ESQLProperNode } from '@kbn/esql-ast/src/types';
import type { ESQLForkParens, ESQLProperNode } from '@kbn/esql-ast/src/types';
Copy link
Contributor Author

@bartoval bartoval Nov 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

  1. Maybe I should create something backwards compatible utils that accepts branches 🤔

  2. or keep this fix is enough
@bartoval
Copy link
Contributor Author

bartoval commented Nov 10, 2025

⏳ Build in-progress, with failures

Failed CI Steps

Test Failures

History

cc @bartoval

never give up!
@bartoval
Copy link
Contributor Author

bartoval commented Nov 10, 2025

Val I saw many tests failing, I will review when CI is happier 😄 but thanx for being so fast with this!

The last commit is green but wasn't notified (probably because GitHub thinks I'm not worthy of joy)
stratoula
stratoula reviewed Nov 10, 2025
View reviewed changes
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.

This looks great to me, I also tested it and could not find any regressions. I would like though Vadim's 👀 . Do you mind to wait for him till he is back on Wed? 🙏
@bartoval
Copy link
Contributor Author

bartoval commented Nov 10, 2025

This looks great to me, I also tested it and could not find any regressions. I would like though Vadim's 👀 . Do you mind to wait for him till he is back on Wed? 🙏

👍🏼
vadimkibana
vadimkibana approved these changes Nov 12, 2025
View reviewed changes
Copy link
Contributor

@vadimkibana vadimkibana left a comment

Choose a reason for hiding this comment

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

LGTM
tiansivive
tiansivive requested changes Nov 17, 2025
View reviewed changes
Copy link
Contributor

@tiansivive tiansivive left a comment

Choose a reason for hiding this comment

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

Thanks for this! Just a couple quick comments from the EA team
@bartoval bartoval requested a review from tiansivive November 17, 2025 17:46
@elasticmachine
Copy link
Contributor

elasticmachine commented Nov 19, 2025

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/esql-ast 610 614 +4

Async chunks

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

id before after diff
discover 1.2MB 1.2MB +16.0B
onechat 422.2KB 422.2KB +18.0B
securitySolution 11.1MB 11.1MB +313.0B
total +347.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 4.0MB 4.0MB +828.0B
Unknown metric groups

API count

id before after diff
@kbn/esql-ast 801 805 +4

History

cc @bartoval
@bartoval
Copy link
Contributor Author

bartoval commented Nov 20, 2025

Hello @tiansivive , I've updated your requests and answered your questions. Do you think we can unlock this PR?
tiansivive
tiansivive approved these changes Nov 20, 2025
View reviewed changes
Copy link
Contributor

@tiansivive tiansivive left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM
@bartoval bartoval merged commit 28b7e62 into elastic:main Nov 20, 2025
12 checks passed
andrimal pushed a commit to andrimal/kibana that referenced this pull request Nov 20, 2025
@bartoval @andrimal
[ES|QL] Wrap the fork subcommands inside the parens node (elastic#242369
15ee5bc 
)

## Summary

elastic#242225

Now the fork command is wrapped inside a new node called `parens`, which
changes the ast structure. So now the branches are children of `parens`.
eokoneyo pushed a commit to eokoneyo/kibana that referenced this pull request Dec 2, 2025
@bartoval @eokoneyo
[ES|QL] Wrap the fork subcommands inside the parens node (elastic#242369
bd5aeca 
)

## Summary

elastic#242225

Now the fork command is wrapped inside a new node called `parens`, which
changes the ast structure. So now the branches are children of `parens`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting Feature:ES|QL ES|QL related features in Kibana release_note:enhancement Team:ESQL ES|QL related features in Kibana t// v9.3.0

5 participants

@bartoval @elasticmachine @stratoula @tiansivive @vadimkibana