ES|QL: Add grammar for SET instruction #134029

luigidellaquila · 2025-09-03T08:37:28Z

Adding grammar for a SET instruction (not properly a command) to pass settings to a query

SET setting1="value1", setting2="value2"; FROM idx | WHERE foo > 4

luigidellaquila · 2025-09-03T08:44:40Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/QuerySettings.java

+import java.util.List;
+import java.util.Objects;
+
+public class QuerySettings {


I'm not treating this as a command or as part of the AST in general (ie. it's not a LogicalPlan or a Node).
It's on purpose, we don't want it to be part of the query (for now at least), so it doesn't need resolution/serialization/traversal for now.

Ah, I'm not calling it Set. There is only one Set in Java.
Suggestions for a better name are welcome of course

elasticsearchmachine · 2025-09-03T09:18:27Z

Hi @luigidellaquila, I've created a changelog YAML for you.

luigidellaquila · 2025-09-03T09:23:20Z

x-pack/plugin/esql/src/main/antlr/EsqlBaseParser.g4

+    : {this.isDevVersion()}? setCommand+ singleStatement EOF
+    | singleStatement EOF
+    ;


This will become

statements : setCommand* singleStatement EOF ;

as soon as it's out of dev mode

elasticsearchmachine · 2025-09-03T10:14:06Z

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

idegtiarenko · 2025-09-03T11:07:04Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/QuerySettings.java

+
+public class QuerySettings {
+
+    private final List<Alias> fields;


I wonder if it is worth storing it in a name->value map? I suspect it will be accessed by name most of the times

I just realized we might want to store multiple settings with same name in order to be able to resolve correct one as well as warn about others. Probably list is fine as long as we have an accessor by setting name as well as a flat structure.

Thanks @idegtiarenko, I added a higher level method EsqlQuery.setting(name) that encapsulates the logic for value masking (last one wins).
I also refactored the tests a bit, now they should be more readable.

idegtiarenko · 2025-09-03T11:14:03Z

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/ParsingTests.java

+        assertThat(query.settings().size(), is(1));
+        assertThat(query.settings().get(0).fields().size(), is(1));
+        assertThat(query.settings().get(0).fields().get(0).name(), is("foo"));
+        assertThat(query.settings().get(0).fields().get(0).child().fold(FoldContext.small()), is(BytesRefs.toBytesRef("bar")));


This looks a bit too complex with 2 nested lists. Is it possible to collect query settings into a flat structure?
I imagine usage could become much simpler if we have something like assertThat(query.settings().setting("foo").fold(..), is(..))

I simplified this, both with an API method and with some helper methods in the test class, so that I can both check the correctness of the high level logic and the small details in the parsed objects.

…nto esql/project_routing

luigidellaquila · 2025-09-04T09:44:16Z

Following up on the off-line discussions, we have two options here:

only allow a single field per SET: SET foo=1;
allow multiple fields in the same SET: SET foo=1, bar=2, baz=3;

In both cases we'll allow multiple SET statements in the same query, eg. the following will be valid also with the second option.

SET foo=1, bar=2;
SET baz=3;
FROM idx | WHERE x > 3

My understanding is that Kibana will use one field per SET, but as a user I tend to prefer a single SET and a comma separated list of field=value. And it's consistent with EVAL, so I guess it's what users will expect in general.

The PR now implements the second option (multiple fields in the same SET, multiple SET in the same query), but we can easily add limitations if we think it's too much.

@quackaplop WDYT?

idegtiarenko

👍 from my side.
I believe this change could be merged once we have confirmation on multiple key-value pairs per SET

quackaplop · 2025-09-05T08:39:12Z

only allow a single field per SET

We should only allow a single field per set. We can consider expanding it further, but we don't need this now

quackaplop · 2025-09-05T08:40:18Z

x-pack/plugin/esql/src/main/antlr/EsqlBaseParser.g4

+    : SET setFields SEMICOLON
+    ;
+
+setFields


Pretty sure we do not need this one

luigidellaquila · 2025-09-05T08:59:11Z

Thanks @quackaplop, I simplified the grammar and the code in general, now we only allow a single field per SET

x-pack/plugin/esql/src/main/antlr/lexer/Expression.g4

x-pack/plugin/esql/src/main/antlr/lexer/Set.g4

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/EsqlQuery.java

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java

…nto esql/project_routing

quackaplop · 2025-09-05T11:03:39Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/QuerySetting.java

+
+import java.util.Objects;
+
+public class QuerySetting {


Do we want to add toString here?

quackaplop · 2025-09-05T11:09:25Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/EsqlStatement.java

+import java.util.List;
+
+public record EsqlStatement(LogicalPlan plan, List<QuerySetting> settings) {
+    /**


Do we want to add toString here mainly for reporting? Also, how do we feel about this sitting under "plan.logical", given that it is actually not deriving from LogicalPlan?

how do we feel about this sitting under "plan.logical", given that it is actually not deriving from LogicalPlan

I don't have a strong opinion on this, I put it there just because it gets parsed together with the logical plan, but "logically" it's a different thing. Let me move it to a separate package

quackaplop · 2025-09-05T11:09:45Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java

+    private EsqlStatement parse(String query, QueryParams params) {
+        var parsed = new EsqlParser().createQuery(query, params, planTelemetry, configuration);
+        if (LOGGER.isDebugEnabled()) {
+            LOGGER.debug("Parsed logical plan:\n{}", parsed.plan());


...and maybe using toString here instead?

luigidellaquila · 2025-09-08T06:58:55Z

Thanks for the feedback @idegtiarenko @quackaplop, if there are no further observations I'll go on and merge

…SET and INLINESTATS commands) (#134358) Three PRs have just been merged that cause failures in `test-release`, and a third PR changes ES|QL railroad diagrams for another recently released change to `KNN` (also a snapshot-only feature). * #132729 - `DecayTests` has over 300 failures * #134029 - `ParsingTests` had six new failures * #124725 - Failures in `VerifierTests` and `OptimizerVerificationTests` This PR attempts to fix all in one, because if we do this in separate PRs they will fail due to the failures cause by the other PRs.

…SET and INLINESTATS commands) (elastic#134358) Three PRs have just been merged that cause failures in `test-release`, and a third PR changes ES|QL railroad diagrams for another recently released change to `KNN` (also a snapshot-only feature). * elastic#132729 - `DecayTests` has over 300 failures * elastic#134029 - `ParsingTests` had six new failures * elastic#124725 - Failures in `VerifierTests` and `OptimizerVerificationTests` This PR attempts to fix all in one, because if we do this in separate PRs they will fail due to the failures cause by the other PRs.

## Summary Closes #233942 Implements parsing for `SET` pseudo-commands, which come before the main query. ``` SET key1 = 123; SET key2 = "asdf"; FROM index | LIMIT 123 ``` In `query` AST node introduces `header` section, which contain AST nodes before the main command. Although each `SET` instruction can now have a single key-value pair, it is implemented such that it will be able to store a list of those pairs. The reason is because in Elasticsearch team first implemented it to hold multiple key-value pairs, then changed to store only one, further they commented ([see thread](elastic/elasticsearch#134029)) that they may add support for a list of key-value pairs to `SET` back. ### 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

## Summary Closes elastic#233942 Implements parsing for `SET` pseudo-commands, which come before the main query. ``` SET key1 = 123; SET key2 = "asdf"; FROM index | LIMIT 123 ``` In `query` AST node introduces `header` section, which contain AST nodes before the main command. Although each `SET` instruction can now have a single key-value pair, it is implemented such that it will be able to store a list of those pairs. The reason is because in Elasticsearch team first implemented it to hold multiple key-value pairs, then changed to store only one, further they commented ([see thread](elastic/elasticsearch#134029)) that they may add support for a list of key-value pairs to `SET` back. ### 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

luigidellaquila added 2 commits September 2, 2025 19:29

Grammar for SET

d82b8c3

Parsing tests

5f3edb7

elasticsearchmachine added the v9.2.0 label Sep 3, 2025

luigidellaquila commented Sep 3, 2025

View reviewed changes

luigidellaquila added :Analytics/ES|QL AKA ESQL >enhancement labels Sep 3, 2025

Update docs/changelog/134029.yaml

12c2601

luigidellaquila commented Sep 3, 2025

View reviewed changes

Merge branch 'main' into esql/project_routing

1b97d03

luigidellaquila changed the title ~~ES|QL: Add SET instruction~~ ES|QL: Add grammar for SET instruction Sep 3, 2025

luigidellaquila marked this pull request as ready for review September 3, 2025 10:13

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Sep 3, 2025

idegtiarenko reviewed Sep 3, 2025

View reviewed changes

luigidellaquila added 6 commits September 3, 2025 15:03

setting(name) method and more readable tests

63142e0

Merge remote-tracking branch 'luigidellaquila/esql/project_routing' i…

337334d

…nto esql/project_routing

Merge branch 'main' into esql/project_routing

b879a93

Merge branch 'main' into esql/project_routing

3fcb48d

Merge branch 'main' into esql/project_routing

e601dca

Merge branch 'main' into esql/project_routing

9bfaf0c

stratoula mentioned this pull request Sep 4, 2025

[ES|QL] Supports SET instruction elastic/kibana#233942

Open

Merge branch 'main' into esql/project_routing

007613b

idegtiarenko approved these changes Sep 4, 2025

View reviewed changes

quackaplop reviewed Sep 5, 2025

View reviewed changes

Simplify

dd25ccb

luigidellaquila added 2 commits September 5, 2025 11:07

Negative test with multiple fields per SET

bfc598e

Merge branch 'main' into esql/project_routing

ca4c27e

quackaplop reviewed Sep 5, 2025

View reviewed changes

x-pack/plugin/esql/src/main/antlr/lexer/Expression.g4 Show resolved Hide resolved

quackaplop reviewed Sep 5, 2025

View reviewed changes

luigidellaquila added 2 commits September 5, 2025 12:05

Implement review suggestions

df93221

Merge remote-tracking branch 'luigidellaquila/esql/project_routing' i…

4a6a67a

…nto esql/project_routing

quackaplop reviewed Sep 5, 2025

View reviewed changes

luigidellaquila added 3 commits September 5, 2025 13:34

toString() and refactoring

368a53c

Merge branch 'main' into esql/project_routing

857186e

method names

60450d3

luigidellaquila merged commit 177a56a into elastic:main Sep 9, 2025
33 checks passed

craigtaverner mentioned this pull request Sep 9, 2025

Fix failing release tests (Require capability for DECAY function and SET and INLINESTATS commands) #134358

Merged

rjernst pushed a commit to rjernst/elasticsearch that referenced this pull request Sep 9, 2025

ES|QL: Add grammar for SET instruction (elastic#134029)

1866b2b

Kubik42 pushed a commit to Kubik42/elasticsearch that referenced this pull request Sep 9, 2025

ES|QL: Add grammar for SET instruction (elastic#134029)

f1210ac

vadimkibana mentioned this pull request Oct 1, 2025

[ES|QL] SET header instruction parsing elastic/kibana#236460

Merged

1 task


		public class QuerySettings {

		private final List<Alias> fields;

ES|QL: Add grammar for SET instruction #134029

ES|QL: Add grammar for SET instruction #134029

Uh oh!

Conversation

luigidellaquila commented Sep 3, 2025

Uh oh!

luigidellaquila Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Sep 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Sep 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luigidellaquila commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

idegtiarenko left a comment

Choose a reason for hiding this comment

Uh oh!

quackaplop commented Sep 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luigidellaquila commented Sep 5, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luigidellaquila Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luigidellaquila commented Sep 8, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

luigidellaquila Sep 3, 2025 •

edited

Loading

luigidellaquila commented Sep 4, 2025 •

edited

Loading

luigidellaquila Sep 5, 2025 •

edited

Loading