Skip to content

[ES|QL] Escape keywords in identifier priting #230491

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.

Sign up for GitHub

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

Jump to bottom
Merged
vadimkibana merged 2 commits into from
Aug 5, 2025
Merged

[ES|QL] Escape keywords in identifier priting #230491

vadimkibana merged 2 commits into from
Aug 5, 2025

Conversation

@vadimkibana
Copy link
Contributor

@vadimkibana vadimkibana commented Aug 5, 2025 •
edited by kibanamachine
Loading

Summary

The LeafPrinter escapes identifiers only when needed, for example, when they contain special characters. However, it must also escape ES|QL keywords, which it did not. Fixes a bug where the LeafPrinter would not escape identifiers which match ES|QL keywords.

Checklist

@vadimkibana vadimkibana requested a review from a team as a code owner August 5, 2025 06:49
@vadimkibana vadimkibana added bug Fixes for quality problems that affect the customer experience review release_note:skip Skip the PR/issue when compiling release notes Feature:ES|QL ES|QL related features in Kibana Team:ESQL ES|QL related features in Kibana t// backport:version Backport to applied version labels v9.2.0 v9.1.1 v8.19.1 labels Aug 5, 2025
@elasticmachine
Copy link
Contributor

elasticmachine commented Aug 5, 2025

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

@vadimkibana vadimkibana enabled auto-merge (squash) August 5, 2025 06:51
@elasticmachine
Copy link
Contributor

elasticmachine commented Aug 5, 2025

💚 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
onechat 595.5KB 707.6KB +112.0KB
securitySolution 10.3MB 10.3MB +472.0B
total +112.5KB

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.8MB 3.8MB +483.0B

@vadimkibana vadimkibana merged commit 04973b1 into elastic:main Aug 5, 2025
12 checks passed
@kibanamachine
Copy link
Contributor

kibanamachine commented Aug 5, 2025

Starting backport for target branches: 8.19, 9.1

https://github.com/elastic/kibana/actions/runs/16748013192

@kibanamachine
Copy link
Contributor

kibanamachine commented Aug 5, 2025

💔 All backports failed

Status Branch Result
❌ 8.19 Backport failed because of merge conflicts
❌ 9.1 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 230491

Questions ?

Please refer to the Backport tool documentation

bartoval
bartoval approved these changes Aug 5, 2025
View reviewed changes
Copy link
Contributor

@bartoval bartoval left a comment

Choose a reason for hiding this comment

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

✅

src/platform/packages/shared/kbn-esql-ast/src/parser/constants.ts

export const SOURCE_COMMANDS = new Set<string>(['FROM', 'ROW', 'SHOW', 'TS', 'EXPLAIN']);

export const PROCESSING_COMMANDS = new Set<string>([
Copy link
Contributor

@bartoval bartoval Aug 5, 2025

Choose a reason for hiding this comment

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

Maybe I am missing something but I was wondering if it wouldn't be better to try to have a single source of truth, perhaps using information in our esqlCommandRegistry

example:

export const SOURCE_COMMANDS = .....

export const PROCESSING_COMMANDS = new Set<string>(
  esqlCommandRegistry.getAllCommandNames()
    .map(name => name.toUpperCase())
    .filter(name => !SOURCE_COMMANDS.has(name)) // or just map all commands and remove this line
);
export const COMMANDS = ....;```

Copy link
Contributor

@stratoula stratoula Aug 5, 2025

Choose a reason for hiding this comment

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

yes this is a good idea. I was also wondering if we can get this info from ANTLR

Copy link
Contributor Author

@vadimkibana vadimkibana Aug 5, 2025

Choose a reason for hiding this comment

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

Maybe we can extract the keywords from the lexer, I'll take a look. Loading the whole command registry only for command keywords seems too expensive, and should be handled on the lower-level somewhere in the parser, imo.

Copy link
Contributor

@stratoula stratoula Aug 5, 2025

Choose a reason for hiding this comment

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

Agreed

@kibanamachine kibanamachine mentioned this pull request Aug 5, 2025
Merged
1 task
delanni pushed a commit to delanni/kibana that referenced this pull request Aug 5, 2025
@vadimkibana @delanni
[ES|QL] Escape keywords in identifier priting (elastic#230491)
1b5c344 
## Summary

The `LeafPrinter` escapes identifiers only when needed, for example,
when they contain special characters. However, it must also escape ES|QL
**keywords**, which it did not. Fixes a bug where the `LeafPrinter`
would not escape identifiers which match ES|QL keywords.


### 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
This was referenced Aug 6, 2025
Merged
Merged
@vadimkibana
Copy link
Contributor Author

vadimkibana commented Aug 6, 2025

💚 All backports created successfully

Status Branch Result
✅ 9.1
✅ 8.19

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

Questions ?

Please refer to the Backport tool documentation

vadimkibana added a commit to vadimkibana/kibana that referenced this pull request Aug 6, 2025
@vadimkibana
[ES|QL] Escape keywords in identifier priting (elastic#230491)
c3ae943 
## Summary

The `LeafPrinter` escapes identifiers only when needed, for example,
when they contain special characters. However, it must also escape ES|QL
**keywords**, which it did not. Fixes a bug where the `LeafPrinter`
would not escape identifiers which match ES|QL keywords.

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

(cherry picked from commit 04973b1)

# Conflicts:
#	src/platform/packages/shared/kbn-esql-ast/src/parser/constants.ts
vadimkibana added a commit to vadimkibana/kibana that referenced this pull request Aug 6, 2025
@vadimkibana
[ES|QL] Escape keywords in identifier priting (elastic#230491)
3075191 
## Summary

The `LeafPrinter` escapes identifiers only when needed, for example,
when they contain special characters. However, it must also escape ES|QL
**keywords**, which it did not. Fixes a bug where the `LeafPrinter`
would not escape identifiers which match ES|QL keywords.

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

(cherry picked from commit 04973b1)

# Conflicts:
#	src/platform/packages/shared/kbn-esql-ast/src/parser/constants.ts
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Aug 7, 2025
@kibanamachine
Copy link
Contributor

kibanamachine commented Aug 7, 2025

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.
cc: @vadimkibana

vadimkibana added a commit that referenced this pull request Aug 7, 2025
@vadimkibana
[9.1] [ES|QL] Escape keywords in identifier priting (#230491) (#230715)
126fd75 
# Backport

This will backport the following commits from `main` to `9.1`:
- [[ES|QL] Escape keywords in identifier priting
(#230491)](#230491)

<!--- Backport version: 10.0.1 -->

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

<!--BACKPORT [{"author":{"name":"Vadim
Kibana","email":"82822460+vadimkibana@users.noreply.github.com"},"sourceCommit":{"committedDate":"2025-08-05T10:53:34Z","message":"[ES|QL]
Escape keywords in identifier priting (#230491)\n\n## Summary\n\nThe
`LeafPrinter` escapes identifiers only when needed, for example,\nwhen
they contain special characters. However, it must also escape
ES|QL\n**keywords**, which it did not. Fixes a bug where the
`LeafPrinter`\nwould not escape identifiers which match ES|QL
keywords.\n\n\n### Checklist\n\n- [x] [Unit or
functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere
updated or added to match the most common
scenarios","sha":"04973b1c8de46f84d47f5946a27be7c425e0f3e8","branchLabelMapping":{"^v9.2.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","review","release_note:skip","Feature:ES|QL","Team:ESQL","backport:version","v9.2.0","v9.1.1","v8.19.1"],"title":"[ES|QL]
Escape keywords in identifier
priting","number":230491,"url":"https://github.com/elastic/kibana/pull/230491","mergeCommit":{"message":"[ES|QL]
Escape keywords in identifier priting (#230491)\n\n## Summary\n\nThe
`LeafPrinter` escapes identifiers only when needed, for example,\nwhen
they contain special characters. However, it must also escape
ES|QL\n**keywords**, which it did not. Fixes a bug where the
`LeafPrinter`\nwould not escape identifiers which match ES|QL
keywords.\n\n\n### Checklist\n\n- [x] [Unit or
functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere
updated or added to match the most common
scenarios","sha":"04973b1c8de46f84d47f5946a27be7c425e0f3e8"}},"sourceBranch":"main","suggestedTargetBranches":["9.1","8.19"],"targetPullRequestStates":[{"branch":"main","label":"v9.2.0","branchLabelMappingKey":"^v9.2.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/230491","number":230491,"mergeCommit":{"message":"[ES|QL]
Escape keywords in identifier priting (#230491)\n\n## Summary\n\nThe
`LeafPrinter` escapes identifiers only when needed, for example,\nwhen
they contain special characters. However, it must also escape
ES|QL\n**keywords**, which it did not. Fixes a bug where the
`LeafPrinter`\nwould not escape identifiers which match ES|QL
keywords.\n\n\n### Checklist\n\n- [x] [Unit or
functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere
updated or added to match the most common
scenarios","sha":"04973b1c8de46f84d47f5946a27be7c425e0f3e8"}},{"branch":"9.1","label":"v9.1.1","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.19","label":"v8.19.1","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->
stratoula added a commit that referenced this pull request Aug 7, 2025
@vadimkibana @stratoula
[8.19] [ES|QL] Escape keywords in identifier priting (#230491) (#230716)
3d567fc 
# Backport

This will backport the following commits from `main` to `8.19`:
- [[ES|QL] Escape keywords in identifier priting
(#230491)](#230491)

<!--- Backport version: 10.0.1 -->

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

<!--BACKPORT [{"author":{"name":"Vadim
Kibana","email":"82822460+vadimkibana@users.noreply.github.com"},"sourceCommit":{"committedDate":"2025-08-05T10:53:34Z","message":"[ES|QL]
Escape keywords in identifier priting (#230491)\n\n## Summary\n\nThe
`LeafPrinter` escapes identifiers only when needed, for example,\nwhen
they contain special characters. However, it must also escape
ES|QL\n**keywords**, which it did not. Fixes a bug where the
`LeafPrinter`\nwould not escape identifiers which match ES|QL
keywords.\n\n\n### Checklist\n\n- [x] [Unit or
functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere
updated or added to match the most common
scenarios","sha":"04973b1c8de46f84d47f5946a27be7c425e0f3e8","branchLabelMapping":{"^v9.2.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","review","release_note:skip","Feature:ES|QL","Team:ESQL","backport:version","v9.2.0","v9.1.1","v8.19.1"],"title":"[ES|QL]
Escape keywords in identifier
priting","number":230491,"url":"https://github.com/elastic/kibana/pull/230491","mergeCommit":{"message":"[ES|QL]
Escape keywords in identifier priting (#230491)\n\n## Summary\n\nThe
`LeafPrinter` escapes identifiers only when needed, for example,\nwhen
they contain special characters. However, it must also escape
ES|QL\n**keywords**, which it did not. Fixes a bug where the
`LeafPrinter`\nwould not escape identifiers which match ES|QL
keywords.\n\n\n### Checklist\n\n- [x] [Unit or
functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere
updated or added to match the most common
scenarios","sha":"04973b1c8de46f84d47f5946a27be7c425e0f3e8"}},"sourceBranch":"main","suggestedTargetBranches":["9.1","8.19"],"targetPullRequestStates":[{"branch":"main","label":"v9.2.0","branchLabelMappingKey":"^v9.2.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/230491","number":230491,"mergeCommit":{"message":"[ES|QL]
Escape keywords in identifier priting (#230491)\n\n## Summary\n\nThe
`LeafPrinter` escapes identifiers only when needed, for example,\nwhen
they contain special characters. However, it must also escape
ES|QL\n**keywords**, which it did not. Fixes a bug where the
`LeafPrinter`\nwould not escape identifiers which match ES|QL
keywords.\n\n\n### Checklist\n\n- [x] [Unit or
functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere
updated or added to match the most common
scenarios","sha":"04973b1c8de46f84d47f5946a27be7c425e0f3e8"}},{"branch":"9.1","label":"v9.1.1","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.19","label":"v8.19.1","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Aug 7, 2025
@wildemat wildemat mentioned this pull request Aug 7, 2025
Closed
10 tasks
@mistic mistic added v9.1.2 and removed v9.1.1 labels Aug 7, 2025
NicholasPeretti pushed a commit to NicholasPeretti/kibana that referenced this pull request Aug 18, 2025
@vadimkibana @NicholasPeretti
[ES|QL] Escape keywords in identifier priting (elastic#230491)
5f7eaf1 
## Summary

The `LeafPrinter` escapes identifiers only when needed, for example,
when they contain special characters. However, it must also escape ES|QL
**keywords**, which it did not. Fixes a bug where the `LeafPrinter`
would not escape identifiers which match ES|QL keywords.


### 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stratoula stratoula stratoula approved these changes

@bartoval bartoval bartoval approved these changes

Assignees

No one assigned

Labels

backport:version Backport to applied version labels bug Fixes for quality problems that affect the customer experience Feature:ES|QL ES|QL related features in Kibana release_note:skip Skip the PR/issue when compiling release notes review Team:ESQL ES|QL related features in Kibana t// v8.19.1 v9.1.2 v9.2.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@vadimkibana @elasticmachine @kibanamachine @stratoula @bartoval @mistic