ESQL: Remove aliasing inside Eval #100238

costin · 2023-10-04T02:54:32Z

Evals that introduce aliased can be simplified by extracting them into a
project (and thus signaling there's no underlying processing).

The following eval:
eval x = a + 1, y = x, z = y + 1, y = z, w = y + 1
can be converted into:
eval x = a + 1, z = a + 1 + 1, w = a + 1 + 1 + 1 | project x, z, z as y, w

Fix #100174

elasticsearchmachine · 2023-10-04T02:54:57Z

Pinging @elastic/es-ql (Team:QL)

elasticsearchmachine · 2023-10-04T02:54:57Z

Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL)

costin · 2023-10-04T02:57:25Z

...ql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizerTests.java

-     *         \_FieldExtractExec[salary{f}#20]
-     *           \_EsQueryExec[test], query[{"esql_single_value":{"field":"emp_no","next":{"range":{"emp_no":{"lt":10050,"boost":1.0}}}}}]
-     *             [_doc{f}#42], limit[], sort[] estimatedRowSize[16]
+     *     \_EsStatsQueryExec[test], stats[Stat[name=salary, type=COUNT, query={


This is a great example on how this PR helps - since the eval goes away, there's no field extraction needed.

costin · 2023-10-04T03:00:18Z

Depending on the test runs, I'd like to get this PR in 8.11 since several queries with aggs end up simplified (see the extra tests).
Currently there's only one Csv test that fails due to the one channel per layout check which is triggered by projection vs the eval since the block is not copied but referred as is.

costin · 2023-10-04T03:00:49Z

...k/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java

+            Limiter.ONCE,
+            new SubstituteSurrogates(),
+            new ReplaceRegexMatch(),
+            new ReplaceAliasingEvalWithProject()


The nice thing about the current state of the rule is that it only gets applied once.

costin · 2023-10-04T03:01:37Z

...gin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java

-        assertThat(aggregate.aggregates(), hasSize(1));
-        var alias = as(aggregate.aggregates().get(0), Alias.class);
-        var count = as(alias.child(), Count.class);
-        var eval = as(aggregate.child(), Eval.class);
-        assertThat(eval.fields(), hasSize(1));
-        var field = as(eval.fields().get(0), Alias.class);
-        assertThat(field.name(), is("x"));
-        var source = as(eval.child(), EsRelation.class);


Note how the intermediate evals were removed.

costin · 2023-10-04T03:01:51Z

...gin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java

-        var project = as(plan, Project.class);
-        var eval = as(project.child(), Eval.class);
-        var limit = as(eval.child(), Limit.class);


Evals that introduce aliased can be simplified by extracting them into a project (and thus signaling there's no underlying processing). The following eval: eval x = a + 1, y = x, z = y + 1, y = z, w = y + 1 can be converted into: eval x = a + 1, z = a + 1 + 1, w = a + 1 + 1 | project x, z, z as y, w Fix elastic#100174

astefan · 2023-10-04T12:18:46Z

Your description is slightly wrong:

The following eval:
eval x = a + 1, y = x, z = y + 1, y = z, w = y + 1
can be converted into:
eval x = a + 1, z = a + 1 + 1, w = a + 1 + 1 | project x, z, z as y, w

it should be (notice w = a + 1 + 1 + 1 | project ...)

The following eval:
eval x = a + 1, y = x, z = y + 1, y = z, w = y + 1
can be converted into:
eval x = a + 1, z = a + 1 + 1, w = a + 1 + 1 + 1 | project x, z, z as y, w

astefan

LGTM, especially the simplifications (and performance improvements) it's adding to queries where evaluation of expressions is not performed when not actually needed.

I have concerns regarding the Layout check that has been commented, combined with the bug I referenced. I am not sure anymore why the check has been added in the first place. I've seen this check being commented in the past as well. Is there an issue waiting to be fixed/implemented for this check to stay as is?

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/Layout.java

x-pack/plugin/esql/qa/testFixtures/src/main/resources/rename.csv-spec

...k/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java

astefan

LGTM

bpintea

Nice.

bpintea · 2023-10-05T08:19:47Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/Layout.java

-                    if (prev != null) {
-                        throw new IllegalArgumentException("Name [" + id + "] is on two channels [" + prev + "] and [" + next + "]");
-                    }
+                    // Do allow multiple name to point to the same channel - see https://github.com/elastic/elasticsearch/pull/100238


Suggested change

// Do allow multiple name to point to the same channel - see https://github.com/elastic/elasticsearch/pull/100238

// Do allow multiple name to point to the same channel - see https://github.com/elastic/elasticsearch/pull/100299

elasticsearchmachine · 2023-10-05T16:48:00Z

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

elasticsearchmachine · 2023-10-05T16:50:06Z

💚 Backport successful

Status	Branch	Result
✅	8.11

Evals that introduce aliased can be simplified by extracting them into a project (and thus signaling there's no underlying processing). The following eval: eval x = a + 1, y = x, z = y + 1, y = z, w = y + 1 can be converted into: eval x = a + 1, z = a + 1 + 1, w = a + 1 + 1 | project x, z, z as y, w Fix elastic#100174 Fix elastic#100050

nik9000 · 2023-10-05T17:44:29Z

I think this started failing #100356

* ESQL: Remove aliasing inside Eval (#100238) Evals that introduce aliased can be simplified by extracting them into a project (and thus signaling there's no underlying processing). The following eval: eval x = a + 1, y = x, z = y + 1, y = z, w = y + 1 can be converted into: eval x = a + 1, z = a + 1 + 1, w = a + 1 + 1 | project x, z, z as y, w Fix #100174 Fix #100050 * Incorporate #100357 * Temporarily disable failing test Relates #100365 (cherry picked from commit 0ef4da2) * ESQL: Temporarily disable huge concat tests (#100352) We're working on these and we have a plan! (cherry picked from commit acca114) --------- Co-authored-by: Nik Everett <[email protected]>

costin added :Analytics/ES|QL AKA ESQL v8.11.0 labels Oct 4, 2023

costin requested review from luigidellaquila, astefan, bpintea and alex-spies October 4, 2023 02:54

costin self-assigned this Oct 4, 2023

elasticsearchmachine added the Team:QL (Deprecated) Meta label for query languages team label Oct 4, 2023

costin force-pushed the fix/eval-as-alias-before-agg branch from defb7bd to 5ec7954 Compare October 4, 2023 02:56

costin commented Oct 4, 2023

View reviewed changes

costin force-pushed the fix/eval-as-alias-before-agg branch from 5ec7954 to c7ccdfc Compare October 4, 2023 03:03

costin added 3 commits October 3, 2023 21:40

Fix checkstyle

7386144

Allow a channel to be mapped to multiple ids

256e42d

Spotless again

e314f16

mattc58 added v8.12.0 and removed v8.11.0 labels Oct 4, 2023

astefan reviewed Oct 4, 2023

View reviewed changes

costin mentioned this pull request Oct 4, 2023

ESQL: Allow multiple columns to refer to the same channel/block #100299

Closed

Incorporate feedback

c4457d9

costin force-pushed the fix/eval-as-alias-before-agg branch from 247fb29 to c4457d9 Compare October 4, 2023 20:08

astefan approved these changes Oct 5, 2023

View reviewed changes

bpintea approved these changes Oct 5, 2023

View reviewed changes

bpintea reviewed Oct 5, 2023

View reviewed changes

costin added >bug v8.11.0 auto-backport-and-merge labels Oct 5, 2023

Update docs/changelog/100238.yaml

3f5693b

costin merged commit ad0a266 into elastic:main Oct 5, 2023

costin deleted the fix/eval-as-alias-before-agg branch October 5, 2023 16:48

costin mentioned this pull request Oct 5, 2023

[8.11] ESQL: Remove aliasing inside Eval (#100238) #100353

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: Remove aliasing inside Eval #100238

ESQL: Remove aliasing inside Eval #100238

costin commented Oct 4, 2023 •

edited

Loading

elasticsearchmachine commented Oct 4, 2023

elasticsearchmachine commented Oct 4, 2023

costin Oct 4, 2023

costin commented Oct 4, 2023

costin Oct 4, 2023

costin Oct 4, 2023

costin Oct 4, 2023

astefan commented Oct 4, 2023

astefan left a comment •

edited

Loading

astefan left a comment

bpintea left a comment

bpintea Oct 5, 2023

elasticsearchmachine commented Oct 5, 2023

elasticsearchmachine commented Oct 5, 2023

nik9000 commented Oct 5, 2023

	// Do allow multiple name to point to the same channel - see https://github.com/elastic/elasticsearch/pull/100238
	// Do allow multiple name to point to the same channel - see https://github.com/elastic/elasticsearch/pull/100299

ESQL: Remove aliasing inside Eval #100238

ESQL: Remove aliasing inside Eval #100238

Conversation

costin commented Oct 4, 2023 • edited Loading

elasticsearchmachine commented Oct 4, 2023

elasticsearchmachine commented Oct 4, 2023

costin Oct 4, 2023

Choose a reason for hiding this comment

costin commented Oct 4, 2023

costin Oct 4, 2023

Choose a reason for hiding this comment

costin Oct 4, 2023

Choose a reason for hiding this comment

costin Oct 4, 2023

Choose a reason for hiding this comment

astefan commented Oct 4, 2023

astefan left a comment • edited Loading

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

bpintea left a comment

Choose a reason for hiding this comment

bpintea Oct 5, 2023

Choose a reason for hiding this comment

elasticsearchmachine commented Oct 5, 2023

elasticsearchmachine commented Oct 5, 2023

💚 Backport successful

nik9000 commented Oct 5, 2023

costin commented Oct 4, 2023 •

edited

Loading

astefan left a comment •

edited

Loading