-
Notifications
You must be signed in to change notification settings - Fork 25.2k
[ES|QL] Double parameter markers for identifiers #122459
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] Double parameter markers for identifiers #122459
Conversation
Hi @fang-xing-esql, I've created a changelog YAML for you. |
Pinging @elastic/es-analytical-engine (Team:Analytics) |
Pinging @elastic/kibana-esql (ES|QL-ui) |
@elasticmachine update branch |
@elasticmachine update branch |
@@ -145,16 +145,23 @@ identifier | |||
identifierPattern | |||
: ID_PATTERN | |||
| parameter | |||
| {this.isDevVersion()}? doubleParameter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which is the plan for keeping this only on snapshot releases? I am going to work on it immediately when this is merged and ideally we want to release it asap. Does it make sense for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which is the plan for keeping this only on snapshot releases? I am going to work on it immediately when this is merged and ideally we want to release it asap. Does it make sense for you?
I'll test and try to merge this(??
) without being behind snapshot, and keep the old approach(?
) coexist until Kibana switch to ??
successfully. Hope it will make it easier for Kibana.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally 🙌
You can also merge with snapshot, give me time to create the PR (which means that I will also test the feature) and when I am ready to merge I can ping you to remove the snaphsot. I am also fine with this approch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for the extensive tests
💔 Backport failed
You can use sqren/backport to manually backport by running |
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
* double parameter markers for identifiers (cherry picked from commit bd81312) # Conflicts: # x-pack/plugin/esql/src/main/antlr/EsqlBaseLexer.tokens # x-pack/plugin/esql/src/main/antlr/EsqlBaseParser.tokens # x-pack/plugin/esql/src/main/antlr/lexer/Enrich.g4 # x-pack/plugin/esql/src/main/antlr/lexer/Expression.g4 # x-pack/plugin/esql/src/main/antlr/lexer/MvExpand.g4 # x-pack/plugin/esql/src/main/antlr/lexer/Project.g4 # x-pack/plugin/esql/src/main/antlr/lexer/Rename.g4 # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseLexer.interp # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseLexer.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseParser.interp # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseParser.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseParserBaseListener.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseParserBaseVisitor.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseParserListener.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseParserVisitor.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java
…evel (#213916) ## Summary Closes #209731 This PR is based on the change made here elastic/elasticsearch#122459 The main difference is that: - Functions and fields should now be added as ?? (instead of ?) - The payload to ES is the same regardless if you send a value or a field/function In order to accommodate this the following changes were made: - Now the variable name in the control form displays the ? or ?? (it didnt display them before) <img width="428" alt="image" src="https://github.com/user-attachments/assets/1381ba4a-591c-47f2-af93-30d54fe7a639" /> - The previous created charts with the old format are bwc (this means that they should load correctly when you checkout in this PR (a helper function has been created to ensure it)  ### Release notes Now the fields / functions variables are being described with ?? in the query. The values variables use ? as before. ### Checklist - [ ] [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 - [ ] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
…evel (elastic#213916) ## Summary Closes elastic#209731 This PR is based on the change made here elastic/elasticsearch#122459 The main difference is that: - Functions and fields should now be added as ?? (instead of ?) - The payload to ES is the same regardless if you send a value or a field/function In order to accommodate this the following changes were made: - Now the variable name in the control form displays the ? or ?? (it didnt display them before) <img width="428" alt="image" src="https://github.com/user-attachments/assets/1381ba4a-591c-47f2-af93-30d54fe7a639" /> - The previous created charts with the old format are bwc (this means that they should load correctly when you checkout in this PR (a helper function has been created to ensure it)  ### Release notes Now the fields / functions variables are being described with ?? in the query. The values variables use ? as before. ### Checklist - [ ] [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 - [ ] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (cherry picked from commit b477afb) # Conflicts: # src/platform/plugins/shared/data/common/search/expressions/esql.ts
…uery level (#213916) (#216329) # Backport This will backport the following commits from `main` to `8.x`: - [[ES|QL] Distinguish the functions/fields vs the values on the query level (#213916)](#213916) <!--- Backport version: 9.6.6 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport) <!--BACKPORT [{"author":{"name":"Stratoula Kalafateli","email":"[email protected]"},"sourceCommit":{"committedDate":"2025-03-28T15:34:37Z","message":"[ES|QL] Distinguish the functions/fields vs the values on the query level (#213916)\n\n## Summary\n\nCloses https://github.com/elastic/kibana/issues/209731\n\nThis PR is based on the change made here\nhttps://github.com/elastic/elasticsearch/pull/122459\n\nThe main difference is that:\n\n- Functions and fields should now be added as ?? (instead of ?)\n- The payload to ES is the same regardless if you send a value or a\nfield/function\n\n\nIn order to accommodate this the following changes were made:\n\n- Now the variable name in the control form displays the ? or ?? (it\ndidnt display them before)\n<img width=\"428\" alt=\"image\"\nsrc=\"https://github.com/user-attachments/assets/1381ba4a-591c-47f2-af93-30d54fe7a639\"\n/>\n\n- The previous created charts with the old format are bwc (this means\nthat they should load correctly when you checkout in this PR (a helper\nfunction has been created to ensure it)\n\n\n\n\n\n### Release notes\nNow the fields / functions variables are being described with ?? in the\nquery. The values variables use ? as before.\n\n### Checklist\n- [ ] [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\n- [ ] The PR description includes the appropriate Release Notes section,\nand the correct `release_note:*` label is applied per the\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"b477afb783c5d242c8eb57fe43016a5ca7c06a7d","branchLabelMapping":{"^v9.1.0$":"main","^v8.19.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:breaking","Feature:ES|QL","Team:ESQL","backport:version","v9.1.0","v8.19.0"],"title":"[ES|QL] Distinguish the functions/fields vs the values on the query level","number":213916,"url":"https://github.com/elastic/kibana/pull/213916","mergeCommit":{"message":"[ES|QL] Distinguish the functions/fields vs the values on the query level (#213916)\n\n## Summary\n\nCloses https://github.com/elastic/kibana/issues/209731\n\nThis PR is based on the change made here\nhttps://github.com/elastic/elasticsearch/pull/122459\n\nThe main difference is that:\n\n- Functions and fields should now be added as ?? (instead of ?)\n- The payload to ES is the same regardless if you send a value or a\nfield/function\n\n\nIn order to accommodate this the following changes were made:\n\n- Now the variable name in the control form displays the ? or ?? (it\ndidnt display them before)\n<img width=\"428\" alt=\"image\"\nsrc=\"https://github.com/user-attachments/assets/1381ba4a-591c-47f2-af93-30d54fe7a639\"\n/>\n\n- The previous created charts with the old format are bwc (this means\nthat they should load correctly when you checkout in this PR (a helper\nfunction has been created to ensure it)\n\n\n\n\n\n### Release notes\nNow the fields / functions variables are being described with ?? in the\nquery. The values variables use ? as before.\n\n### Checklist\n- [ ] [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\n- [ ] The PR description includes the appropriate Release Notes section,\nand the correct `release_note:*` label is applied per the\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"b477afb783c5d242c8eb57fe43016a5ca7c06a7d"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/213916","number":213916,"mergeCommit":{"message":"[ES|QL] Distinguish the functions/fields vs the values on the query level (#213916)\n\n## Summary\n\nCloses https://github.com/elastic/kibana/issues/209731\n\nThis PR is based on the change made here\nhttps://github.com/elastic/elasticsearch/pull/122459\n\nThe main difference is that:\n\n- Functions and fields should now be added as ?? (instead of ?)\n- The payload to ES is the same regardless if you send a value or a\nfield/function\n\n\nIn order to accommodate this the following changes were made:\n\n- Now the variable name in the control form displays the ? or ?? (it\ndidnt display them before)\n<img width=\"428\" alt=\"image\"\nsrc=\"https://github.com/user-attachments/assets/1381ba4a-591c-47f2-af93-30d54fe7a639\"\n/>\n\n- The previous created charts with the old format are bwc (this means\nthat they should load correctly when you checkout in this PR (a helper\nfunction has been created to ensure it)\n\n\n\n\n\n### Release notes\nNow the fields / functions variables are being described with ?? in the\nquery. The values variables use ? as before.\n\n### Checklist\n- [ ] [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\n- [ ] The PR description includes the appropriate Release Notes section,\nand the correct `release_note:*` label is applied per the\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"b477afb783c5d242c8eb57fe43016a5ca7c06a7d"}},{"branch":"8.x","label":"v8.19.0","branchLabelMappingKey":"^v8.19.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> --------- Co-authored-by: kibanamachine <[email protected]>
…evel (elastic#213916) ## Summary Closes elastic#209731 This PR is based on the change made here elastic/elasticsearch#122459 The main difference is that: - Functions and fields should now be added as ?? (instead of ?) - The payload to ES is the same regardless if you send a value or a field/function In order to accommodate this the following changes were made: - Now the variable name in the control form displays the ? or ?? (it didnt display them before) <img width="428" alt="image" src="https://github.com/user-attachments/assets/1381ba4a-591c-47f2-af93-30d54fe7a639" /> - The previous created charts with the old format are bwc (this means that they should load correctly when you checkout in this PR (a helper function has been created to ensure it)  ### Release notes Now the fields / functions variables are being described with ?? in the query. The values variables use ? as before. ### Checklist - [ ] [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 - [ ] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
#115061 introduces the support of (named)parameters for identifiers - field and function names with a single parameter marker
?
inES|QL
queries, however it requires users to provide the context whether the provided parameter is avalue
,identifier
orpattern
in theparams
string in a request to remove ambiguity, as they are all represented by?
and there are multiple places in theES|QL
syntax where several of these variations are valid options, that causes ambiguity in the query language.This PR use double parameter markers -
??
to represent field or function name inES|QL
queries, so that theparams
string can be simplified, and users don't have to annotate a field or function name withidentifier
.For now, this PR doesn't remove the old syntax of a single
?
for identifier just for backward compatibility, and??
is under snapshot, both syntax coexist so that Kibana have time to adapt to??
. This is related to elastic/kibana#209731.There are three variations of
??
, similar with?
, it can be a named parameter, a positional parameter or an anonymous parameter, below are some examples to show the usage of??
with different variations.named parameters -
??
followed by a named defined inparams
positional parameters -
??
followed by an integer number indicating the position of the parameter inparams
anonymous parameters - just a
??