Skip to content
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

ESQL: Break up antlr #122075

Merged
merged 1 commit into from
Feb 24, 2025
Merged

ESQL: Break up antlr #122075

merged 1 commit into from
Feb 24, 2025

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Feb 7, 2025 •
edited
Loading

This breaks the antlr lexer and parser into a few files rather than two giant files. It's much easier to read them this way.

@nik9000
Copy link
Member Author

nik9000 commented Feb 7, 2025

I'm not sure if we want this. I want this, but it conflicts with a few things we mention in the ANTLR comments - that we want to prevent shifting around the token ids if possible - this makes that much much less likely because everything is ordered by its include.

costin
costin approved these changes Feb 10, 2025
View reviewed changes
Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

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

LGTM - we can't guarantee the token ids anyway since we're expanding existing modes (instead of adding them at then end).
Breaking down the lexer is going to help with contributions - we should do the same for the parser.

@nik9000 nik9000 marked this pull request as ready for review February 10, 2025 15:19
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Feb 10, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@nik9000
Copy link
Member Author

nik9000 commented Feb 10, 2025

I've merged main into this to pickup some conflicts. The kibana folks will have to give this one a shot before I merge this. @drewdaemon or @stratoula will approve it when they are ready for it.

@nik9000 nik9000 changed the title ESQL: Break up antlr lexer ESQL: Break up antlr Feb 10, 2025
luigidellaquila
luigidellaquila approved these changes Feb 11, 2025
View reviewed changes
Copy link
Contributor

@luigidellaquila luigidellaquila left a comment

Choose a reason for hiding this comment

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

❤️

@stratoula stratoula mentioned this pull request Feb 11, 2025
Closed
@drewdaemon
Copy link
Contributor

I will open a PR on the Kibana side to take this update into account!

drewdaemon
drewdaemon approved these changes Feb 14, 2025
View reviewed changes
Copy link
Contributor

@drewdaemon drewdaemon left a comment

Choose a reason for hiding this comment

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

Let's go ahead with the merge!

@fang-xing-esql fang-xing-esql added auto-backport Automatically create backport pull requests when merged >refactoring test-release Trigger CI checks against release build labels Feb 20, 2025
@fang-xing-esql fang-xing-esql added the ES|QL-ui Impacts ES|QL UI label Feb 24, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/kibana-esql (ES|QL-ui)

@fang-xing-esql fang-xing-esql merged commit a792334 into elastic:main Feb 24, 2025
18 checks passed
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
❌ 9.0 Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 122075

@fang-xing-esql fang-xing-esql mentioned this pull request Feb 24, 2025
Merged
@fang-xing-esql
Copy link
Member

💚 All backports created successfully

Status Branch Result
✅ 9.0

Questions ?

Please refer to the Backport tool documentation

fang-xing-esql pushed a commit to fang-xing-esql/Elasticsearch that referenced this pull request Feb 24, 2025
@nik9000
pick up main (elastic#122075)
05822ab 
Co-authored-by: Fang Xing <[email protected]>
(cherry picked from commit a792334)

# Conflicts:
#	x-pack/plugin/esql/src/main/antlr/EsqlBaseLexer.g4
#	x-pack/plugin/esql/src/main/antlr/EsqlBaseLexer.tokens
#	x-pack/plugin/esql/src/main/antlr/EsqlBaseParser.g4
#	x-pack/plugin/esql/src/main/antlr/EsqlBaseParser.tokens
#	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
elasticsearchmachine pushed a commit that referenced this pull request Feb 24, 2025
@fang-xing-esql @nik9000
pick up main (#122075) (#123312)
2de65d0 
Co-authored-by: Fang Xing <[email protected]>
(cherry picked from commit a792334)

# Conflicts:
#	x-pack/plugin/esql/src/main/antlr/EsqlBaseLexer.g4
#	x-pack/plugin/esql/src/main/antlr/EsqlBaseLexer.tokens
#	x-pack/plugin/esql/src/main/antlr/EsqlBaseParser.g4
#	x-pack/plugin/esql/src/main/antlr/EsqlBaseParser.tokens
#	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

Co-authored-by: Nik Everett <[email protected]>
drewdaemon
drewdaemon reviewed Mar 4, 2025
View reviewed changes
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseLexer.java
Comment on lines +142 to +143
"'+'", "'-'", "'*'", "'/'", "'%'", "'{'", "'}'", null, null, "']'", null,
"')'", null, null, null, null, null, "'metadata'", null, null, null,
Copy link
Contributor

Choose a reason for hiding this comment

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

@nik9000 sorry for the late-stage comment, but it's a little curious to me that we lost the literal names for LP and OPENING_BRACKET. It changes the syntax errors, for example

row var = 1 not in

used to result in

mismatched input '<EOF>' expecting '('

but now says

mismatched input '<EOF>' expecting LP

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wrong... this regression was actually in 21845ad

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drewdaemon drewdaemon drewdaemon approved these changes

@costin costin costin approved these changes

@luigidellaquila luigidellaquila luigidellaquila approved these changes

Assignees

@fang-xing-esql fang-xing-esql

Labels
:Analytics/ES|QL AKA ESQL auto-backport Automatically create backport pull requests when merged backport pending ES|QL-ui Impacts ES|QL UI >non-issue >refactoring Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) test-release Trigger CI checks against release build v9.0.0 v9.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nik9000 @elasticsearchmachine @drewdaemon @fang-xing-esql @costin @luigidellaquila