Skip to content

FORK: Allow all processing commands #129465

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
merged 11 commits into from
Jun 17, 2025
Merged

FORK: Allow all processing commands #129465

merged 11 commits into from
Jun 17, 2025

Conversation

@ioanatia
Copy link
Contributor

@ioanatia ioanatia commented Jun 16, 2025 •
edited
Loading

related: #126553

Allows all commands to be used in FORK branches.

We needed to make some adjustments to how we handle ) because otherwise using commands such as DROP in FORK branches caused errors such as:

line 4:40: token recognition error at: ')'
org.elasticsearch.xpack.esql.parser.ParsingException: line 4:40: token recognition error at: ')'
	at __randomizedtesting.SeedInfo.seed([73D239A9C4E69AD:69172FF2E651E43C]:0)
	at org.elasticsearch.xpack.esql.parser.EsqlParser$1.syntaxError(EsqlParser.java:198)
	at org.antlr.v4.runtime.ProxyErrorListener.syntaxError(ProxyErrorListener.java:41)
	at org.antlr.v4.runtime.Lexer.notifyListeners(Lexer.java:364)
	at org.antlr.v4.runtime.Lexer.nextToken(Lexer.java:144)
	at org.elasticsearch.xpack.esql.parser.EsqlParser$ParametrizedTokenSource.nextToken(EsqlParser.java:223)
	at org.antlr.v4.runtime.BufferedTokenStream.fetch(BufferedTokenStream.java:169)
	at org.antlr.v4.runtime.BufferedTokenStream.sync(BufferedTokenStream.java:152)
	at org.antlr.v4.runtime.BufferedTokenStream.consume(BufferedTokenStream.java:136)
	at org.antlr.v4.runtime.Parser.consume(Parser.java:571)
	at org.antlr.v4.runtime.Parser.match(Parser.java:205)
	at org.elasticsearch.xpack.esql.parser.EsqlBaseParser.identifierPattern(EsqlBaseParser.java:2254)
	at org.elasticsearch.xpack.esql.parser.EsqlBaseParser.qualifiedNamePattern(EsqlBaseParser.java:2056)
	at org.elasticsearch.xpack.esql.parser.EsqlBaseParser.qualifiedNamePatterns(EsqlBaseParser.java:2128)
	at org.elasticsearch.xpack.esql.parser.EsqlBaseParser.dropCommand(EsqlBaseParser.java:2849)
	at org.elasticsearch.xpack.esql.parser.EsqlBaseParser.processingCommand(EsqlBaseParser.java:618)
	at org.elasticsearch.xpack.esql.parser.EsqlBaseParser.forkSubQueryProcessingCommand(EsqlBaseParser.java:4224)
	at org.elasticsearch.xpack.esql.parser.EsqlBaseParser.forkSubQueryCommand(EsqlBaseParser.java:4171)
	at org.elasticsearch.xpack.esql.parser.EsqlBaseParser.forkSubQuery(EsqlBaseParser.java:4054)
	at org.elasticsearch.xpack.esql.parser.EsqlBaseParser.forkSubQueries(EsqlBaseParser.java:3994)
	at org.elasticsearch.xpack.esql.parser.EsqlBaseParser.forkCommand(EsqlBaseParser.java:3936)
	at org.elasticsearch.xpack.esql.parser.EsqlBaseParser.processingCommand(EsqlBaseParser.java:710)
	at org.elasticsearch.xpack.esql.parser.EsqlBaseParser.query(EsqlBaseParser.java:362)

@idegtiarenko provided this example to visualize the problem.
we need to make sure we pop the ) while we are still in PROJECT_MODE:

| FORK ( WHERE content:"fox" | DROP content)
^ pushMode(FORK_MODE)
       ^ pushMode(DEFAULT_MODE)
                             ^ popMode
                               ^ pushMode(PROJECT_MODE)
                                           ^ here we need to exit project mode. And also exit default mode to return to fork mode.

we needed to make these changes for all commands that can be part of FORK branches that don't use EXPRESSION_MODE (which was handled here).

@ioanatia ioanatia added >non-issue :Analytics/ES|QL AKA ESQL v9.1.0 Team:Search - Relevance The Search organization Search Relevance team labels Jun 16, 2025
ivancea
ivancea reviewed Jun 16, 2025
View reviewed changes
Copy link
Contributor

@ivancea ivancea left a comment

Choose a reason for hiding this comment

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

Looks good!

@ioanatia ioanatia marked this pull request as ready for review June 17, 2025 06:23
@elasticsearchmachine elasticsearchmachine added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) and removed Team:Search - Relevance The Search organization Search Relevance team labels Jun 17, 2025
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Jun 17, 2025

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

@ioanatia ioanatia requested review from alex-spies and ivancea June 17, 2025 06:24
@ioanatia ioanatia added the ES|QL-ui Impacts ES|QL UI label Jun 17, 2025
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Jun 17, 2025

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

@ioanatia ioanatia mentioned this pull request Jun 11, 2025
Closed
3 tasks
ChrisHegarty
ChrisHegarty approved these changes Jun 17, 2025
View reviewed changes
Copy link
Contributor

@ChrisHegarty ChrisHegarty left a comment

Choose a reason for hiding this comment

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

LGTM

x-pack/plugin/esql/src/main/antlr/EsqlBaseParser.g4
| changePointCommand
| completionCommand
| grokCommand
: processingCommand
Copy link
Contributor

@ChrisHegarty ChrisHegarty Jun 17, 2025

Choose a reason for hiding this comment

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

When we added fork initially, it was a goal to get to this point - well done!

@ioanatia ioanatia requested a review from idegtiarenko June 17, 2025 08:52
alex-spies
alex-spies approved these changes Jun 17, 2025
View reviewed changes
Copy link
Contributor

@alex-spies alex-spies left a comment

Choose a reason for hiding this comment

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

Looks good! Only minor comments from me.

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java
assertThat(dissect.parser().pattern(), equalTo("%{d} %{e} %{f}"));
}

public void testForkAllCommands() {
Copy link
Contributor

@alex-spies alex-spies Jun 17, 2025

Choose a reason for hiding this comment

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

Nice test! I think it'll get stale over time - but that's not a problem as long as fork is plugged into the generative testing with other commands.

@ioanatia ioanatia merged commit b496f11 into elastic:main Jun 17, 2025
27 checks passed
@ioanatia ioanatia deleted the fork_grammar branch June 17, 2025 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@idegtiarenko idegtiarenko idegtiarenko approved these changes

@ivancea ivancea ivancea approved these changes

@ChrisHegarty ChrisHegarty ChrisHegarty approved these changes

@alex-spies alex-spies alex-spies approved these changes

Assignees

No one assigned

Labels

:Analytics/ES|QL AKA ESQL ES|QL-ui Impacts ES|QL UI >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v9.1.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ioanatia @elasticsearchmachine @idegtiarenko @ivancea @ChrisHegarty @alex-spies