ESQL: Remove duplicated nested commands #123085
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
elasticsearchmachine
added
the
Team:Analytics
|
Hi @costin, I've created a changelog YAML for you. |
Fork grammar duplicated nested command declaration causing additional lexing to occur resulting in invalid field name declaration Relates to elastic#121948
costin
force-pushed
the
esql/remove-nested-commands
branch
from
3fc4d36 to
c5e5c67
Compare
costin
commented
Feb 21, 2025
Comment on lines
-226
to
-227
| NESTED_SORT : {this.isDevVersion()}? SORT -> type(SORT); | ||
| NESTED_LIMIT : {this.isDevVersion()}? LIMIT -> type(LIMIT); |
There was a problem hiding this comment.
These cause parsing error since commands don't need to be nested. WHERE is declared here for the STATS avg() WHERE declaration -WHERE inside a command.
costin
commented
Feb 21, 2025
| @@ -678,8 +677,8 @@ INSIST_MULTILINE_COMMENT : MULTILINE_COMMENT -> channel(HIDDEN); | |||
| // | |||
| mode FORK_MODE; | |||
| FORK_LP : LP -> type(LP), pushMode(DEFAULT_MODE); | |||
| FORK_RP : RP -> type(RP), popMode; | |||
There was a problem hiding this comment.
The popup mode should already be completed by the sub fork, otherwise the mode stack is not completed, causing non-first subqueries to fail (since parsing is executed in a different mode).
costin
requested review from
astefan,
bpintea,
ioanatia,
ChrisHegarty and
fang-xing-esql
and removed request for
bpintea
|
Hi @costin, I've created a changelog YAML for you. |
| @@ -3085,7 +3085,28 @@ public void testInvalidFork() { | |||
| "line 1:20: mismatched input 'FORK' expecting {'limit', 'sort', 'where'}" | |||
| ); | |||
| expectError("FROM foo* | FORK ( x+1 ) ( WHERE y>2 )", "line 1:20: mismatched input 'x+1' expecting {'limit', 'sort', 'where'}"); | |||
| expectError("FROM foo* | FORK ( LIMIT 10 ) ( y+2 )", "line 1:33: mismatched input 'y' expecting {'limit', 'sort', 'where'}"); | |||
| expectError("FROM foo* | FORK ( LIMIT 10 ) ( y+2 )", "line 1:33: mismatched input 'y+2' expecting {'limit', 'sort', 'where'}"); | |||
There was a problem hiding this comment.
Good catch. I dunno why this escaped the CI testing in the PR that added the test scenario! ? (I'll check this separately)
| pr: 123085 | ||
| summary: Remove duplicated nested commands | ||
| area: ES|QL | ||
| type: bug |
There was a problem hiding this comment.
definitely a bug, but it should not escape into a published artifact or release.
ChrisHegarty
approved these changes
Feb 21, 2025
|
I updated the branch with main in order to pickup #122586 ( which I see causing failures in this PR ) |
costin
added a commit
that referenced
this pull request
Mar 15, 2025
Due to recent grammar changes made ( token to no longer be reported by its text rather by his internal token name. Due to the use of pushMode, the symbol is not treated as a literal rather as a symbol. To address this, the parser listener looks at the error message and changes the message before returning it to the user. Replace hacky regex approach with Vocabulary substitution (not as pluggable as it could be yet much better) Fix #124145 Relates #123085 #121948 Co-authored-by: Alexander Spies <alexander.spies@elastic.co>
omricohenn
pushed a commit
to omricohenn/elasticsearch
that referenced
this pull request
Mar 28, 2025
Due to recent grammar changes made ( token to no longer be reported by its text rather by his internal token name. Due to the use of pushMode, the symbol is not treated as a literal rather as a symbol. To address this, the parser listener looks at the error message and changes the message before returning it to the user. Replace hacky regex approach with Vocabulary substitution (not as pluggable as it could be yet much better) Fix elastic#124145 Relates elastic#123085 elastic#121948 Co-authored-by: Alexander Spies <alexander.spies@elastic.co>
nik9000
added a commit
to nik9000/elasticsearch
that referenced
this pull request
May 1, 2025
Due to recent grammar changes made ( token to no longer be reported by its text rather by his internal token name. Due to the use of pushMode, the symbol is not treated as a literal rather as a symbol. To address this, the parser listener looks at the error message and changes the message before returning it to the user. Fix elastic#124145 Relates elastic#123085 elastic#121948
nik9000
added a commit
that referenced
this pull request
May 1, 2025
Due to recent grammar changes made ( token to no longer be reported by its text rather by his internal token name. Due to the use of pushMode, the symbol is not treated as a literal rather as a symbol. To address this, the parser listener looks at the error message and changes the message before returning it to the user. Fix #124145 Relates #123085 #121948
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rework fork nested declaration to properly pop the mode stack otherwise, parsing gets stuck inside expression mode
and removed duplicated commands that caused lexing errors.
Without this fix, queries that previously were working such as:
FROM index | STATS avg(max.limit)failed.Relates to #121948