From c0779832f8d4d44084c89520799467da2cabcb59 Mon Sep 17 00:00:00 2001 From: Alexander Spies Date: Thu, 23 Oct 2025 14:38:26 +0200 Subject: [PATCH 1/7] Fix shadowing in ReplaceAliasingEvalWithProject --- .../ReplaceAliasingEvalWithProject.java | 66 +++++++++++++------ 1 file changed, 46 insertions(+), 20 deletions(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAliasingEvalWithProject.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAliasingEvalWithProject.java index bd506862108ee..072c86b2214de 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAliasingEvalWithProject.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAliasingEvalWithProject.java @@ -20,7 +20,11 @@ import org.elasticsearch.xpack.esql.rule.Rule; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; +import java.util.Set; + +import static org.elasticsearch.xpack.esql.optimizer.rules.logical.TemporaryNameUtils.locallyUniqueTemporaryName; /** * Replace aliasing evals (eval x=a) with a projection which can be further combined / simplified. @@ -50,50 +54,72 @@ public LogicalPlan apply(LogicalPlan logicalPlan) { }); } + // TODO: test cases + // - chain of aliases pointing to field from previous commands + // - chain of aliases pointing to field from eval + // in both cases: also include cases where subsequent fields will shadow either the origin attribute, or an attribute somewhere in the + // middle of the chain. + // Also include cases where the shadowing alias is used in other subsequent fields, so temporarily renaming it needs to be propagated. private LogicalPlan rule(Eval eval) { LogicalPlan plan = eval; - // holds simple aliases such as b = a, c = b, d = c - AttributeMap.Builder basicAliasesBuilder = AttributeMap.builder(); - // same as above but keeps the original expression - AttributeMap.Builder basicAliasSourcesBuilder = AttributeMap.builder(); + // Mostly, holds simple aliases from the eval, such as b = a, c = b, d = c, so we can resolve them in subsequent eval fields + AttributeMap.Builder renamesToPropagate = AttributeMap.builder(); + // the aliases for the final projection - mostly, same as above but holds the final aliases rather than the original attributes + AttributeMap.Builder projectionAliases = AttributeMap.builder(); + // The names of attributes that are required to perform the aliases in a subsequent projection - if the next eval field + // shadows one of these names, the subsequent projection won't work, so we need to perform a temporary rename. + Set namesRequiredForProjectionAliases = new HashSet<>(); - List keptFields = new ArrayList<>(); + List newEvalFields = new ArrayList<>(); var fields = eval.fields(); for (int i = 0, size = fields.size(); i < size; i++) { Alias field = fields.get(i); - Expression child = field.child(); var attribute = field.toAttribute(); - // put the aliases in a separate map to separate the underlying resolve from other aliases - if (child instanceof Attribute) { - basicAliasesBuilder.put(attribute, child); - basicAliasSourcesBuilder.put(attribute, field); + // propagate all previous aliases into the current field + field = (Alias) field.transformUp(e -> renamesToPropagate.build().resolve(e, e)); + Expression child = field.child(); + + if (child instanceof Attribute renamedAttribute) { + // Basic renaming - let's do that in the subsequent projection + renamesToPropagate.put(attribute, renamedAttribute); + projectionAliases.put(attribute, field); + namesRequiredForProjectionAliases.add(renamedAttribute.name()); } else { - // be lazy and start replacing name aliases only if needed - if (basicAliasesBuilder.build().size() > 0) { - // update the child through the field - field = (Alias) field.transformUp(e -> basicAliasesBuilder.build().resolve(e, e)); + // not a basic renaming, needs to remain in the eval + + // The field may shadow one of the attributes that we will need to correctly perform the subsequent projection. + // If so, rename it in the eval! + if (namesRequiredForProjectionAliases.contains(field.name())) { + Alias newField = new Alias(field.source(), locallyUniqueTemporaryName(field.name()), field.child(), null, true); + Alias reRenamedField = new Alias(field.source(), field.name(), newField.toAttribute(), field.id(), field.synthetic()); + projectionAliases.put(field.toAttribute(), reRenamedField); + // the renaming also needs to be propagated to eval fields to the right + renamesToPropagate.put(field.toAttribute(), newField.toAttribute()); + + field = newField; } - keptFields.add(field); + + newEvalFields.add(field); } } // at least one alias encountered, move it into a project - if (basicAliasesBuilder.build().size() > 0) { + if (renamesToPropagate.build().size() > 0) { // preserve the eval output (takes care of shadowing and order) but replace the basic aliases List projections = new ArrayList<>(eval.output()); - var basicAliasSources = basicAliasSourcesBuilder.build(); + var projectionAliasesMap = projectionAliases.build(); // replace the removed aliases with their initial definition - however use the output to preserve the shadowing for (int i = projections.size() - 1; i >= 0; i--) { NamedExpression project = projections.get(i); - projections.set(i, basicAliasSources.getOrDefault(project, project)); + projections.set(i, projectionAliasesMap.getOrDefault(project, project)); } LogicalPlan child = eval.child(); - if (keptFields.size() > 0) { + if (newEvalFields.size() > 0) { // replace the eval with just the kept fields - child = new Eval(eval.source(), eval.child(), keptFields); + child = new Eval(eval.source(), eval.child(), newEvalFields); } // put the projection in place plan = new Project(eval.source(), child, projections); From c8221e35c8a4bd14e18f6c2159de49462c084b1f Mon Sep 17 00:00:00 2001 From: Alexander Spies Date: Thu, 23 Oct 2025 14:53:42 +0200 Subject: [PATCH 2/7] Add csv tests --- .../src/main/resources/eval.csv-spec | 25 +++++++++++++++++++ .../xpack/esql/action/EsqlCapabilities.java | 7 ++++++ 2 files changed, 32 insertions(+) diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/eval.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/eval.csv-spec index a151636b47f18..bcce79c00ce10 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/eval.csv-spec +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/eval.csv-spec @@ -805,3 +805,28 @@ year:integer 12 12 ; + +// Bugfixes for ReplaceAliasingEvalWithProject not accounting for shadowing +chainOfAliases +required_capability: fix_replace_aliasing_eval_with_project_shadowing + +row x = 1 +| eval y = x, z = y, x = 2, y = 3 +| keep * +; + +z:integer | x:integer | y:integer +1 | 2 | 3 +; + +shadowingOfAliases +required_capability: fix_replace_aliasing_eval_with_project_shadowing + +row x = 1 +| eval y = "foo", z = x, x = 9 +| rename y as a +; + +a:keyword | z:integer | x:integer +foo | 1 | 9 +; diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java index 84a5103200a14..bc9b9f1841f6f 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java @@ -1558,6 +1558,13 @@ public enum Cap { * Temporarily forbid the use of an explicit or implicit LIMIT before INLINE STATS. */ FORBID_LIMIT_BEFORE_INLINE_STATS(INLINE_STATS.enabled), + + /** + * {@link org.elasticsearch.xpack.esql.optimizer.rules.logical.ReplaceAliasingEvalWithProject} did not fully account for shadowing. + * https://github.com/elastic/elasticsearch/issues/137019. + */ + FIX_REPLACE_ALIASING_EVAL_WITH_PROJECT_SHADOWING, + // Last capability should still have a comma for fewer merge conflicts when adding new ones :) // This comment prevents the semicolon from being on the previous capability when Spotless formats the file. ; From 52e122f418c54c64918dd2c18f4fe96ccad6028a Mon Sep 17 00:00:00 2001 From: Alexander Spies Date: Thu, 23 Oct 2025 14:56:16 +0200 Subject: [PATCH 3/7] Update docs/changelog/137025.yaml --- docs/changelog/137025.yaml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 docs/changelog/137025.yaml diff --git a/docs/changelog/137025.yaml b/docs/changelog/137025.yaml new file mode 100644 index 0000000000000..8a6dbb43f9c17 --- /dev/null +++ b/docs/changelog/137025.yaml @@ -0,0 +1,6 @@ +pr: 137025 +summary: Fix `ReplaceAliasingEvalWithProject` in case of shadowing +area: ES|QL +type: bug +issues: + - 137019 From ddea63d9d14761b91cebc397da2734a8d8783f7e Mon Sep 17 00:00:00 2001 From: Alexander Spies Date: Fri, 24 Oct 2025 13:39:31 +0200 Subject: [PATCH 4/7] Add tests --- .../xpack/esql/EsqlTestUtils.java | 23 ++ .../ReplaceAliasingEvalWithProject.java | 6 - .../ReplaceAliasingEvalWithProjectTests.java | 242 ++++++++++++++++++ 3 files changed, 265 insertions(+), 6 deletions(-) create mode 100644 x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAliasingEvalWithProjectTests.java diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java b/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java index acf4492bcc1aa..697ab136d4a96 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java @@ -82,6 +82,7 @@ import org.elasticsearch.xpack.esql.core.type.DataType; import org.elasticsearch.xpack.esql.core.type.EsField; import org.elasticsearch.xpack.esql.core.util.DateUtils; +import org.elasticsearch.xpack.esql.core.util.Holder; import org.elasticsearch.xpack.esql.core.util.StringUtils; import org.elasticsearch.xpack.esql.expression.function.EsqlFunctionRegistry; import org.elasticsearch.xpack.esql.expression.function.scalar.spatial.StGeohash; @@ -90,6 +91,7 @@ import org.elasticsearch.xpack.esql.expression.function.scalar.string.regex.RLike; import org.elasticsearch.xpack.esql.expression.function.scalar.string.regex.WildcardLike; import org.elasticsearch.xpack.esql.expression.predicate.Range; +import org.elasticsearch.xpack.esql.expression.predicate.operator.arithmetic.Mul; import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.Equals; import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.GreaterThan; import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.GreaterThanOrEqual; @@ -229,6 +231,19 @@ public static GreaterThanOrEqual greaterThanOrEqualOf(Expression left, Expressio return new GreaterThanOrEqual(EMPTY, left, right, ESTestCase.randomZone()); } + public static FieldAttribute findFieldAttribute(LogicalPlan plan, String name) { + Holder result = new Holder<>(); + plan.forEachDown(EsRelation.class, relation -> { + for (Attribute attr : relation.output()) { + if (attr.name().equals(name)) { + result.set((FieldAttribute) attr); + return; + } + } + }); + return result.get(); + } + public static FieldAttribute getFieldAttribute() { return getFieldAttribute("a"); } @@ -271,6 +286,14 @@ public static ReferenceAttribute referenceAttribute(String name, DataType type) return new ReferenceAttribute(EMPTY, name, type); } + public static Alias alias(String name, Expression child) { + return new Alias(EMPTY, name, child); + } + + public static Mul mul(Expression left, Expression right) { + return new Mul(EMPTY, left, right); + } + public static Range rangeOf(Expression value, Expression lower, boolean includeLower, Expression upper, boolean includeUpper) { return new Range(EMPTY, value, lower, includeLower, upper, includeUpper, randomZone()); } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAliasingEvalWithProject.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAliasingEvalWithProject.java index 072c86b2214de..7a09f908e63a6 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAliasingEvalWithProject.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAliasingEvalWithProject.java @@ -54,12 +54,6 @@ public LogicalPlan apply(LogicalPlan logicalPlan) { }); } - // TODO: test cases - // - chain of aliases pointing to field from previous commands - // - chain of aliases pointing to field from eval - // in both cases: also include cases where subsequent fields will shadow either the origin attribute, or an attribute somewhere in the - // middle of the chain. - // Also include cases where the shadowing alias is used in other subsequent fields, so temporarily renaming it needs to be propagated. private LogicalPlan rule(Eval eval) { LogicalPlan plan = eval; diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAliasingEvalWithProjectTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAliasingEvalWithProjectTests.java new file mode 100644 index 0000000000000..6885a84cc764e --- /dev/null +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAliasingEvalWithProjectTests.java @@ -0,0 +1,242 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +package org.elasticsearch.xpack.esql.optimizer.rules.logical; + +import org.elasticsearch.xpack.esql.core.expression.Expression; +import org.elasticsearch.xpack.esql.optimizer.AbstractLogicalPlanOptimizerTests; +import org.elasticsearch.xpack.esql.plan.logical.Eval; +import org.elasticsearch.xpack.esql.plan.logical.Project; + +import java.util.ArrayList; +import java.util.List; + +import static org.elasticsearch.xpack.esql.EsqlTestUtils.alias; +import static org.elasticsearch.xpack.esql.EsqlTestUtils.as; +import static org.elasticsearch.xpack.esql.EsqlTestUtils.assertEqualsIgnoringIds; +import static org.elasticsearch.xpack.esql.EsqlTestUtils.findFieldAttribute; +import static org.elasticsearch.xpack.esql.EsqlTestUtils.mul; +import static org.elasticsearch.xpack.esql.EsqlTestUtils.of; +import static org.hamcrest.Matchers.startsWith; + +public class ReplaceAliasingEvalWithProjectTests extends AbstractLogicalPlanOptimizerTests { + /** + * EsqlProject[[emp_no{f}#18, salary{f}#23, emp_no{f}#18 AS emp_no2#7, salary2{r}#10, emp_no{f}#18 AS emp_no3#13, salary3{r}#16]] + * \_Eval[[salary{f}#23 * 2[INTEGER] AS salary2#10, salary2{r}#10 * 3[INTEGER] AS salary3#16]] + * \_Limit[1000[INTEGER],false,false] + * \_EsRelation[test][_meta_field{f}#24, emp_no{f}#18, first_name{f}#19, ..] + */ + public void testSimple() { + // Rule only kicks in if there's a Project or Aggregate above, so we add a KEEP * + var plan = plan(""" + from test + | KEEP emp_no, salary + | EVAL emp_no2 = emp_no, salary2 = 2*salary, emp_no3 = emp_no2, salary3 = 3*salary2 + | KEEP * + """); + + var project = as(plan, Project.class); + var eval = as(project.child(), Eval.class); + + var empNo = findFieldAttribute(plan, "emp_no"); + var salary = findFieldAttribute(plan, "salary"); + + var salary2 = alias("salary2", mul(salary, of(2))); + var salary3 = alias("salary3", mul(salary2.toAttribute(), of(3))); + + List expectedEvalFields = new ArrayList<>(); + expectedEvalFields.add(salary2); + expectedEvalFields.add(salary3); + assertEqualsIgnoringIds(expectedEvalFields, eval.fields()); + + List expectedProjections = new ArrayList<>(); + expectedProjections.add(empNo); + expectedProjections.add(salary.toAttribute()); + expectedProjections.add(alias("emp_no2", empNo)); + expectedProjections.add(salary2.toAttribute()); + expectedProjections.add(alias("emp_no3", empNo)); + expectedProjections.add(salary3.toAttribute()); + assertEqualsIgnoringIds(expectedProjections, project.projections()); + } + + /** + * EsqlProject[[salary{f}#23, emp_no{f}#18 AS emp_no2#7, $$emp_no$temp_name$29{r}#30 AS emp_no#10, + * emp_no{f}#18 AS emp_no3#13, salary3{r}#16]] + * \_Eval[[salary{f}#23 * 2[INTEGER] AS $$emp_no$temp_name$29#30, $$emp_no$temp_name$29{r$}#30 * 2[INTEGER] AS salary3#16]] + * \_Limit[1000[INTEGER],false,false] + * \_EsRelation[test][_meta_field{f}#24, emp_no{f}#18, first_name{f}#19, ..] + */ + public void testNonAliasShadowingAliasedAttribute() { + // Rule only kicks in if there's a Project or Aggregate above, so we add a KEEP * + var plan = plan(randomFrom(""" + from test + | KEEP emp_no, salary + | EVAL emp_no2 = emp_no, emp_no = 2*salary, emp_no3 = emp_no2, salary3 = 2*emp_no + | KEEP * + """)); + + var project = as(plan, Project.class); + var eval = as(project.child(), Eval.class); + + var empNo = findFieldAttribute(plan, "emp_no"); + var salary = findFieldAttribute(plan, "salary"); + + assertEquals(2, eval.fields().size()); + + var empNoTempName = eval.fields().get(0); + assertThat(empNoTempName.name(), startsWith("$$emp_no$temp_name$")); + assertEqualsIgnoringIds(mul(salary, of(2)), empNoTempName.child()); + + var salary3 = alias("salary3", mul(empNoTempName.toAttribute(), of(2))); + assertEqualsIgnoringIds(salary3, eval.fields().get(1)); + + List expectedProjections = new ArrayList<>(); + expectedProjections.add(salary); + expectedProjections.add(alias("emp_no2", empNo)); + expectedProjections.add(alias("emp_no", empNoTempName.toAttribute())); + expectedProjections.add(alias("emp_no3", empNo)); + expectedProjections.add(salary3.toAttribute()); + assertEqualsIgnoringIds(expectedProjections, project.projections()); + } + + /** + * EsqlProject[[emp_no{f}#18, salary{f}#23, emp_no{f}#18 AS emp_no3#10, emp_no2{r}#13, salary3{r}#16]] + * \_Eval[[salary{f}#23 * 2[INTEGER] AS emp_no2#13, emp_no2{r}#13 * 3[INTEGER] AS salary3#16]] + * \_Limit[1000[INTEGER],false,false] + * \_EsRelation[test][_meta_field{f}#24, emp_no{f}#18, first_name{f}#19, ..] + */ + public void testNonAliasShadowingAliasOfAliasedAttribute() { + // Rule only kicks in if there's a Project or Aggregate above, so we add a KEEP * + var plan = plan(""" + from test + | KEEP emp_no, salary + | EVAL emp_no2 = emp_no, emp_no3 = emp_no2, emp_no2 = 2*salary, salary3 = 3*emp_no2 + | KEEP * + """); + + var project = as(plan, Project.class); + var eval = as(project.child(), Eval.class); + + var empNo = findFieldAttribute(plan, "emp_no"); + var salary = findFieldAttribute(plan, "salary"); + + var empNo2 = alias("emp_no2", mul(salary, of(2))); + var salary3 = alias("salary3", mul(empNo2.toAttribute(), of(3))); + assertEqualsIgnoringIds(List.of(empNo2, salary3), eval.fields()); + + List expectedProjections = new ArrayList<>(); + expectedProjections.add(empNo); + expectedProjections.add(salary); + expectedProjections.add(alias("emp_no3", empNo)); + expectedProjections.add(empNo2.toAttribute()); + expectedProjections.add(salary3.toAttribute()); + assertEqualsIgnoringIds(expectedProjections, project.projections()); + } + + /** + * EsqlProject[[emp_no{f}#24, salary{f}#29, emp_no{f}#24 AS emp_no3#13, salary{f}#29 AS salary3#16, + * salary{f}#29 AS emp_no2#19, emp_no{f}#24 AS salary2#22]] + * \_Limit[1000[INTEGER],false,false] + * \_EsRelation[test][_meta_field{f}#30, emp_no{f}#24, first_name{f}#25, ..] + */ + public void testAliasShadowingOtherAlias() { + // Rule only kicks in if there's a Project or Aggregate above, so we add a KEEP * + var plan = plan(""" + from test + | KEEP emp_no, salary + | EVAL emp_no2 = emp_no, salary2 = salary, emp_no3 = emp_no2, salary3 = salary2, emp_no2 = salary, salary2 = emp_no + | KEEP * + """); + + var project = as(plan, Project.class); + + var empNo = findFieldAttribute(plan, "emp_no"); + var salary = findFieldAttribute(plan, "salary"); + + List expectedProjections = new ArrayList<>(); + expectedProjections.add(empNo); + expectedProjections.add(salary); + expectedProjections.add(alias("emp_no3", empNo)); + expectedProjections.add(alias("salary3", salary)); + expectedProjections.add(alias("emp_no2", salary)); + expectedProjections.add(alias("salary2", empNo)); + assertEqualsIgnoringIds(expectedProjections, project.projections()); + } + + /** + * EsqlProject[[salary{f}#22, salary2{r}#6, salary2{r}#6 AS aliased_salary2#9, salary3{r}#12, salary2{r}#6 AS twice_aliased_salary2#15]] + * \_Eval[[salary{f}#22 * 2[INTEGER] AS salary2#6, salary2{r}#6 * 3[INTEGER] AS salary3#12]] + * \_Limit[1000[INTEGER],false,false] + * \_EsRelation[test][_meta_field{f}#23, emp_no{f}#17, first_name{f}#18, ..] + */ + public void testAliasForNonAlias() { + // Rule only kicks in if there's a Project or Aggregate above, so we add a KEEP * + var plan = plan(""" + from test + | KEEP salary + | EVAL salary2 = 2*salary, aliased_salary2 = salary2, salary3 = 3*salary2, twice_aliased_salary2 = aliased_salary2 + | KEEP * + """); + + var project = as(plan, Project.class); + var eval = as(project.child(), Eval.class); + + var salary = findFieldAttribute(plan, "salary"); + + var salary2 = alias("salary2", mul(salary, of(2))); + var salary3 = alias("salary3", mul(salary2.toAttribute(), of(3))); + assertEqualsIgnoringIds(List.of(salary2, salary3), eval.fields()); + + List expectedProjections = new ArrayList<>(); + expectedProjections.add(salary); + expectedProjections.add(salary2.toAttribute()); + expectedProjections.add(alias("aliased_salary2", salary2.toAttribute())); + expectedProjections.add(salary3.toAttribute()); + expectedProjections.add(alias("twice_aliased_salary2", salary2.toAttribute())); + assertEqualsIgnoringIds(expectedProjections, project.projections()); + } + + /** + * EsqlProject[[salary{f}#25, salary2{r}#6 AS aliased_salary2#9, $$salary2$temp_name$31{r$}#32 AS salary2#12, salary3{r}#15, + * salary3{r}#15 AS salary4#18]] + * \_Eval[[salary{f}#25 * 2[INTEGER] AS salary2#6, salary2{r}#6 * 3[INTEGER] AS $$salary2$temp_name$31#32, + * $$salary2$temp_name$31{r$}#32 * 4[INTEGER] AS salary3#15]] + * \_Limit[1000[INTEGER],false,false] + * \_EsRelation[test][_meta_field{f}#26, emp_no{f}#20, first_name{f}#21, ..] + */ + public void testAliasForShadowedNonAlias() { + // Rule only kicks in if there's a Project or Aggregate above, so we add a KEEP * + var plan = plan(""" + from test + | KEEP salary + | EVAL salary2 = 2*salary, aliased_salary2 = salary2, salary2 = 3*salary2, salary3 = 4*salary2, salary4 = salary3 + | KEEP * + """); + + var project = as(plan, Project.class); + var eval = as(project.child(), Eval.class); + + var salary = findFieldAttribute(plan, "salary"); + + assertEquals(3, eval.fields().size()); + var salary2 = alias("salary2", mul(salary, of(2))); + assertEqualsIgnoringIds(salary2, eval.fields().get(0)); + var salary2TempName = eval.fields().get(1); + assertThat(salary2TempName.name(), startsWith("$$salary2$temp_name$")); + assertEqualsIgnoringIds(mul(salary2.toAttribute(), of(3)), salary2TempName.child()); + var salary3 = alias("salary3", mul(salary2TempName.toAttribute(), of(4))); + assertEqualsIgnoringIds(salary3, eval.fields().get(2)); + + List expectedProjections = new ArrayList<>(); + expectedProjections.add(salary); + expectedProjections.add(alias("aliased_salary2", salary2.toAttribute())); + expectedProjections.add(alias("salary2", salary2TempName.toAttribute())); + expectedProjections.add(salary3.toAttribute()); + expectedProjections.add(alias("salary4", salary3.toAttribute())); + assertEqualsIgnoringIds(expectedProjections, project.projections()); + } +} From ee04fe2981c4e81d7cc4223057e17b626a579ccc Mon Sep 17 00:00:00 2001 From: Alexander Spies Date: Mon, 27 Oct 2025 17:12:51 +0100 Subject: [PATCH 5/7] Bogdan's review --- .../xpack/esql/EsqlTestUtils.java | 1 + .../ReplaceAliasingEvalWithProject.java | 30 +++++++++---------- .../ReplaceAliasingEvalWithProjectTests.java | 12 ++++++++ 3 files changed, 28 insertions(+), 15 deletions(-) diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java b/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java index 614bd7177874c..efdad6340574c 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java @@ -237,6 +237,7 @@ public static FieldAttribute findFieldAttribute(LogicalPlan plan, String name) { plan.forEachDown(EsRelation.class, relation -> { for (Attribute attr : relation.output()) { if (attr.name().equals(name)) { + assertNull("Multiple matching field attributes found", result.get()); result.set((FieldAttribute) attr); return; } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAliasingEvalWithProject.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAliasingEvalWithProject.java index 7a09f908e63a6..b5ed67da6b762 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAliasingEvalWithProject.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAliasingEvalWithProject.java @@ -68,11 +68,11 @@ private LogicalPlan rule(Eval eval) { List newEvalFields = new ArrayList<>(); var fields = eval.fields(); - for (int i = 0, size = fields.size(); i < size; i++) { - Alias field = fields.get(i); - var attribute = field.toAttribute(); + for (Alias alias : fields) { // propagate all previous aliases into the current field - field = (Alias) field.transformUp(e -> renamesToPropagate.build().resolve(e, e)); + Alias field = (Alias) alias.transformUp(e -> renamesToPropagate.build().resolve(e, e)); + String name = field.name(); + Attribute attribute = field.toAttribute(); Expression child = field.child(); if (child instanceof Attribute renamedAttribute) { @@ -80,21 +80,21 @@ private LogicalPlan rule(Eval eval) { renamesToPropagate.put(attribute, renamedAttribute); projectionAliases.put(attribute, field); namesRequiredForProjectionAliases.add(renamedAttribute.name()); - } else { - // not a basic renaming, needs to remain in the eval - + } else if (namesRequiredForProjectionAliases.contains(name)) { + // Not a basic renaming, needs to remain in the eval. // The field may shadow one of the attributes that we will need to correctly perform the subsequent projection. // If so, rename it in the eval! - if (namesRequiredForProjectionAliases.contains(field.name())) { - Alias newField = new Alias(field.source(), locallyUniqueTemporaryName(field.name()), field.child(), null, true); - Alias reRenamedField = new Alias(field.source(), field.name(), newField.toAttribute(), field.id(), field.synthetic()); - projectionAliases.put(field.toAttribute(), reRenamedField); - // the renaming also needs to be propagated to eval fields to the right - renamesToPropagate.put(field.toAttribute(), newField.toAttribute()); - field = newField; - } + Alias newField = new Alias(field.source(), locallyUniqueTemporaryName(name), child, null, true); + Attribute newAttribute = newField.toAttribute(); + Alias reRenamedField = new Alias(field.source(), name, newAttribute, field.id(), field.synthetic()); + projectionAliases.put(attribute, reRenamedField); + // the renaming also needs to be propagated to eval fields to the right + renamesToPropagate.put(attribute, newAttribute); + newEvalFields.add(newField); + } else { + // still not a basic renaming, but no risk of shadowing newEvalFields.add(field); } } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAliasingEvalWithProjectTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAliasingEvalWithProjectTests.java index 6885a84cc764e..9f544e0e263f2 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAliasingEvalWithProjectTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAliasingEvalWithProjectTests.java @@ -25,10 +25,12 @@ public class ReplaceAliasingEvalWithProjectTests extends AbstractLogicalPlanOptimizerTests { /** + * 
{@code
      * EsqlProject[[emp_no{f}#18, salary{f}#23, emp_no{f}#18 AS emp_no2#7, salary2{r}#10, emp_no{f}#18 AS emp_no3#13, salary3{r}#16]]
      * \_Eval[[salary{f}#23 * 2[INTEGER] AS salary2#10, salary2{r}#10 * 3[INTEGER] AS salary3#16]]
      *   \_Limit[1000[INTEGER],false,false]
      *     \_EsRelation[test][_meta_field{f}#24, emp_no{f}#18, first_name{f}#19, ..]
+     * }
*/ public void testSimple() { // Rule only kicks in if there's a Project or Aggregate above, so we add a KEEP * @@ -64,11 +66,13 @@ public void testSimple() { } /** + * 
{@code
      * EsqlProject[[salary{f}#23, emp_no{f}#18 AS emp_no2#7, $$emp_no$temp_name$29{r}#30 AS emp_no#10,
      *  emp_no{f}#18 AS emp_no3#13, salary3{r}#16]]
      * \_Eval[[salary{f}#23 * 2[INTEGER] AS $$emp_no$temp_name$29#30, $$emp_no$temp_name$29{r$}#30 * 2[INTEGER] AS salary3#16]]
      *   \_Limit[1000[INTEGER],false,false]
      *     \_EsRelation[test][_meta_field{f}#24, emp_no{f}#18, first_name{f}#19, ..]
+     * }
*/ public void testNonAliasShadowingAliasedAttribute() { // Rule only kicks in if there's a Project or Aggregate above, so we add a KEEP * @@ -104,10 +108,12 @@ public void testNonAliasShadowingAliasedAttribute() { } /** + * 
{@code
      * EsqlProject[[emp_no{f}#18, salary{f}#23, emp_no{f}#18 AS emp_no3#10, emp_no2{r}#13, salary3{r}#16]]
      * \_Eval[[salary{f}#23 * 2[INTEGER] AS emp_no2#13, emp_no2{r}#13 * 3[INTEGER] AS salary3#16]]
      *   \_Limit[1000[INTEGER],false,false]
      *     \_EsRelation[test][_meta_field{f}#24, emp_no{f}#18, first_name{f}#19, ..]
+     * }
*/ public void testNonAliasShadowingAliasOfAliasedAttribute() { // Rule only kicks in if there's a Project or Aggregate above, so we add a KEEP * @@ -138,10 +144,12 @@ public void testNonAliasShadowingAliasOfAliasedAttribute() { } /** + * 
{@code
      * EsqlProject[[emp_no{f}#24, salary{f}#29, emp_no{f}#24 AS emp_no3#13, salary{f}#29 AS salary3#16,
      *  salary{f}#29 AS emp_no2#19, emp_no{f}#24 AS salary2#22]]
      * \_Limit[1000[INTEGER],false,false]
      *   \_EsRelation[test][_meta_field{f}#30, emp_no{f}#24, first_name{f}#25, ..]
+     * }
*/ public void testAliasShadowingOtherAlias() { // Rule only kicks in if there's a Project or Aggregate above, so we add a KEEP * @@ -168,10 +176,12 @@ public void testAliasShadowingOtherAlias() { } /** + * 
{@code
      * EsqlProject[[salary{f}#22, salary2{r}#6, salary2{r}#6 AS aliased_salary2#9, salary3{r}#12, salary2{r}#6 AS twice_aliased_salary2#15]]
      * \_Eval[[salary{f}#22 * 2[INTEGER] AS salary2#6, salary2{r}#6 * 3[INTEGER] AS salary3#12]]
      *   \_Limit[1000[INTEGER],false,false]
      *     \_EsRelation[test][_meta_field{f}#23, emp_no{f}#17, first_name{f}#18, ..]
+     * }
*/ public void testAliasForNonAlias() { // Rule only kicks in if there's a Project or Aggregate above, so we add a KEEP * @@ -201,12 +211,14 @@ public void testAliasForNonAlias() { } /** + * 
{@code
      * EsqlProject[[salary{f}#25, salary2{r}#6 AS aliased_salary2#9, $$salary2$temp_name$31{r$}#32 AS salary2#12, salary3{r}#15,
      *  salary3{r}#15 AS salary4#18]]
      * \_Eval[[salary{f}#25 * 2[INTEGER] AS salary2#6, salary2{r}#6 * 3[INTEGER] AS $$salary2$temp_name$31#32,
      *  $$salary2$temp_name$31{r$}#32 * 4[INTEGER] AS salary3#15]]
      *   \_Limit[1000[INTEGER],false,false]
      *     \_EsRelation[test][_meta_field{f}#26, emp_no{f}#20, first_name{f}#21, ..]
+     * }
*/ public void testAliasForShadowedNonAlias() { // Rule only kicks in if there's a Project or Aggregate above, so we add a KEEP * From b836a6a8eab435b342d7dc2564fb22032e26f25c Mon Sep 17 00:00:00 2001 From: Alexander Spies Date: Tue, 28 Oct 2025 10:05:33 +0100 Subject: [PATCH 6/7] Improve comment --- .../optimizer/rules/logical/ReplaceAliasingEvalWithProject.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAliasingEvalWithProject.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAliasingEvalWithProject.java index b5ed67da6b762..f12f05961def3 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAliasingEvalWithProject.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAliasingEvalWithProject.java @@ -83,7 +83,7 @@ private LogicalPlan rule(Eval eval) { } else if (namesRequiredForProjectionAliases.contains(name)) { // Not a basic renaming, needs to remain in the eval. // The field may shadow one of the attributes that we will need to correctly perform the subsequent projection. - // If so, rename it in the eval! + // So, rename it in the eval! Alias newField = new Alias(field.source(), locallyUniqueTemporaryName(name), child, null, true); Attribute newAttribute = newField.toAttribute(); From ec09fa16a9c2b59bf91974b85cf8ede721a72716 Mon Sep 17 00:00:00 2001 From: Alexander Spies Date: Tue, 28 Oct 2025 18:16:29 +0100 Subject: [PATCH 7/7] Add more tests --- .../xpack/esql/EsqlTestUtils.java | 8 + .../ReplaceAliasingEvalWithProjectTests.java | 250 +++++++++++++++++- 2 files changed, 253 insertions(+), 5 deletions(-) diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java b/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java index efdad6340574c..8b2838a47f8e3 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java @@ -158,6 +158,7 @@ import java.util.Map; import java.util.Set; import java.util.TreeMap; +import java.util.function.Predicate; import java.util.jar.JarInputStream; import java.util.zip.ZipEntry; @@ -233,8 +234,15 @@ public static GreaterThanOrEqual greaterThanOrEqualOf(Expression left, Expressio } public static FieldAttribute findFieldAttribute(LogicalPlan plan, String name) { + return findFieldAttribute(plan, name, (unused) -> true); + } + + public static FieldAttribute findFieldAttribute(LogicalPlan plan, String name, Predicate inThisRelation) { Holder result = new Holder<>(); plan.forEachDown(EsRelation.class, relation -> { + if (inThisRelation.test(relation) == false) { + return; + } for (Attribute attr : relation.output()) { if (attr.name().equals(name)) { assertNull("Multiple matching field attributes found", result.get()); diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAliasingEvalWithProjectTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAliasingEvalWithProjectTests.java index 9f544e0e263f2..0c204a4687a7b 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAliasingEvalWithProjectTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAliasingEvalWithProjectTests.java @@ -7,9 +7,12 @@ package org.elasticsearch.xpack.esql.optimizer.rules.logical; +import org.elasticsearch.index.IndexMode; import org.elasticsearch.xpack.esql.core.expression.Expression; import org.elasticsearch.xpack.esql.optimizer.AbstractLogicalPlanOptimizerTests; +import org.elasticsearch.xpack.esql.plan.logical.Aggregate; import org.elasticsearch.xpack.esql.plan.logical.Eval; +import org.elasticsearch.xpack.esql.plan.logical.Limit; import org.elasticsearch.xpack.esql.plan.logical.Project; import java.util.ArrayList; @@ -34,12 +37,18 @@ public class ReplaceAliasingEvalWithProjectTests extends AbstractLogicalPlanOpti */ public void testSimple() { // Rule only kicks in if there's a Project or Aggregate above, so we add a KEEP * - var plan = plan(""" - from test + var plan = plan(randomFrom(""" + FROM test | KEEP emp_no, salary | EVAL emp_no2 = emp_no, salary2 = 2*salary, emp_no3 = emp_no2, salary3 = 3*salary2 | KEEP * - """); + """, """ + FROM test + | KEEP emp_no, salary + | EVAL emp_no2 = emp_no, salary2 = 2*salary + | EVAL emp_no3 = emp_no2, salary3 = 3*salary2 + | KEEP * + """)); var project = as(plan, Project.class); var eval = as(project.child(), Eval.class); @@ -57,12 +66,153 @@ public void testSimple() { List expectedProjections = new ArrayList<>(); expectedProjections.add(empNo); - expectedProjections.add(salary.toAttribute()); + expectedProjections.add(salary); + expectedProjections.add(alias("emp_no2", empNo)); + expectedProjections.add(salary2.toAttribute()); + expectedProjections.add(alias("emp_no3", empNo)); + expectedProjections.add(salary3.toAttribute()); + assertEqualsIgnoringIds(expectedProjections, project.projections()); + } + + /** + * 
{@code
+     * EsqlProject[[emp_no{f}#19, salary{f}#35, emp_no{f}#19 AS emp_no2#8, salary2{r}#11, emp_no{f}#19 AS emp_no3#14, salary3{r}#17]]
+     * \_Eval[[salary{f}#35 * 2[INTEGER] AS salary2#11, salary2{r}#11 * 3[INTEGER] AS salary3#17]]
+     *   \_Limit[1000[INTEGER],true,false]
+     *     \_Join[LEFT,[emp_no{f}#19],[emp_no{f}#30],null]
+     *       |_Limit[1000[INTEGER],false,false]
+     *       | \_EsRelation[test][_meta_field{f}#25, emp_no{f}#19, first_name{f}#20, ..]
+     *       \_EsRelation[test_lookup][LOOKUP][emp_no{f}#30, salary{f}#35]
+     * }
+ */ + public void testSimpleFieldFromLookup() { + // Rule only kicks in if there's a Project or Aggregate above, so we add a KEEP * + var plan = plan(""" + FROM test + | LOOKUP JOIN test_lookup ON emp_no + | KEEP emp_no, salary + | EVAL emp_no2 = emp_no, salary2 = 2*salary, emp_no3 = emp_no2, salary3 = 3*salary2 + | KEEP * + """); + + var project = as(plan, Project.class); + var eval = as(project.child(), Eval.class); + + // the final emp_no is from the main index, the final salary from the lookup index + var empNo = findFieldAttribute(plan, "emp_no", relation -> relation.indexMode() == IndexMode.STANDARD); + var salary = findFieldAttribute(plan, "salary", relation -> relation.indexMode() == IndexMode.LOOKUP); + + var salary2 = alias("salary2", mul(salary, of(2))); + var salary3 = alias("salary3", mul(salary2.toAttribute(), of(3))); + + List expectedEvalFields = new ArrayList<>(); + expectedEvalFields.add(salary2); + expectedEvalFields.add(salary3); + assertEqualsIgnoringIds(expectedEvalFields, eval.fields()); + // assert using id to ensure we're pointing to the right salary + assertEquals(eval.fields().get(0).child(), mul(salary, of(2))); + + List expectedProjections = new ArrayList<>(); + expectedProjections.add(empNo); + expectedProjections.add(salary); expectedProjections.add(alias("emp_no2", empNo)); expectedProjections.add(salary2.toAttribute()); expectedProjections.add(alias("emp_no3", empNo)); expectedProjections.add(salary3.toAttribute()); assertEqualsIgnoringIds(expectedProjections, project.projections()); + // assert using id to ensure we're pointing to the right emp_no and salary + assertEquals(project.projections().get(0), empNo); + assertEquals(project.projections().get(1), salary); + } + + /** + * 
{@code
+     * EsqlProject[[emp_no{f}#24 AS emp_no2#7, salary{f}#29 AS salary2#10, emp_no{f}#24 AS emp_no3#13, emp_no{f}#24 AS salary#16,
+     *  salary{f}#29 AS salary3#19, salary{f}#29 AS emp_no#22]]
+     * \_Limit[1000[INTEGER],false,false]
+     *   \_EsRelation[test][_meta_field{f}#30, emp_no{f}#24, first_name{f}#25, ..]
+     * }
+ */ + public void testOnlyAliases() { + // Rule only kicks in if there's a Project or Aggregate above, so we add a KEEP * + var plan = plan(""" + from test + | KEEP emp_no, salary + | EVAL emp_no2 = emp_no, salary2 = salary, emp_no3 = emp_no2, salary = emp_no3, salary3 = salary2, emp_no = salary3 + | KEEP * + """); + + var project = as(plan, Project.class); + var limit = as(project.child(), Limit.class); + + var empNo = findFieldAttribute(plan, "emp_no"); + var salary = findFieldAttribute(plan, "salary"); + + List expectedProjections = new ArrayList<>(); + expectedProjections.add(alias("emp_no2", empNo)); + expectedProjections.add(alias("salary2", salary)); + expectedProjections.add(alias("emp_no3", empNo)); + expectedProjections.add(alias("salary", empNo)); + expectedProjections.add(alias("salary3", salary)); + expectedProjections.add(alias("emp_no", salary)); + assertEqualsIgnoringIds(expectedProjections, project.projections()); + } + + /** + * 
{@code
+     * EsqlProject[[emp_no{f}#26 AS b#21, emp_no{f}#26 AS a#24]]
+     * \_Limit[1000[INTEGER],false,false]
+     *   \_EsRelation[test][_meta_field{f}#32, emp_no{f}#26, first_name{f}#27, ..]
+     * }
+ */ + public void testAliasLoopTwoVars() { + // Rule only kicks in if there's a Project or Aggregate above, so we add a KEEP * + var plan = plan(""" + from test + | KEEP emp_no + | RENAME emp_no AS a + | EVAL b = a, a = b, b = a, a = b, b = a, a = b + | KEEP * + """); + + var project = as(plan, Project.class); + var limit = as(project.child(), Limit.class); + + var empNo = findFieldAttribute(plan, "emp_no"); + + List expectedProjections = new ArrayList<>(); + expectedProjections.add(alias("b", empNo)); + expectedProjections.add(alias("a", empNo)); + assertEqualsIgnoringIds(expectedProjections, project.projections()); + } + + /** + * 
{@code
+     * EsqlProject[[emp_no{f}#17 AS b#9, emp_no{f}#17 AS c#12, emp_no{f}#17 AS a#15]]
+     * \_Limit[1000[INTEGER],false,false]
+     *   \_EsRelation[test][_meta_field{f}#23, emp_no{f}#17, first_name{f}#18, ..]
+     * }
+ */ + public void testAliasLoopThreeVars() { + // Rule only kicks in if there's a Project or Aggregate above, so we add a KEEP * + var plan = plan(""" + from test + | KEEP emp_no + | RENAME emp_no AS a + | EVAL b = a, c = b, a = c + | KEEP * + """); + + var project = as(plan, Project.class); + var limit = as(project.child(), Limit.class); + + var empNo = findFieldAttribute(plan, "emp_no"); + + List expectedProjections = new ArrayList<>(); + expectedProjections.add(alias("b", empNo)); + expectedProjections.add(alias("c", empNo)); + expectedProjections.add(alias("a", empNo)); + assertEqualsIgnoringIds(expectedProjections, project.projections()); } /** @@ -93,7 +243,7 @@ public void testNonAliasShadowingAliasedAttribute() { var empNoTempName = eval.fields().get(0); assertThat(empNoTempName.name(), startsWith("$$emp_no$temp_name$")); - assertEqualsIgnoringIds(mul(salary, of(2)), empNoTempName.child()); + assertEquals(mul(salary, of(2)), empNoTempName.child()); var salary3 = alias("salary3", mul(empNoTempName.toAttribute(), of(2))); assertEqualsIgnoringIds(salary3, eval.fields().get(1)); @@ -107,6 +257,96 @@ public void testNonAliasShadowingAliasedAttribute() { assertEqualsIgnoringIds(expectedProjections, project.projections()); } + /** + * 
{@code
+     * Limit[1000[INTEGER],false,false]
+     * \_Aggregate[[$$emp_no$temp_name$33{r$}#34, emp_no{f}#22, salary{f}#27, salary3{r}#16],
+     *      [$$emp_no$temp_name$33{r$}#34 AS emp_no#10, emp_no{f}#22 AS emp_no2#7, emp_no{f}#22 AS emp_no3#13, salary{f}#27, salary3{r}#16]]
+     *   \_Eval[[salary{f}#27 * 2[INTEGER] AS $$emp_no$temp_name$33#34, $$emp_no$temp_name$33{r$}#34 * 2[INTEGER] AS salary3#1
+     * 6]]
+     *     \_EsRelation[test][_meta_field{f}#28, emp_no{f}#22, first_name{f}#23, ..]
+     * }
+ */ + public void testNonAliasShadowingAliasedAttributeWithAgg() { + // Rule only kicks in if there's a Project or Aggregate above, so we add a KEEP * + var plan = plan(randomFrom(""" + from test + | KEEP emp_no, salary + | EVAL emp_no2 = emp_no, emp_no = 2*salary, emp_no3 = emp_no2, salary3 = 2*emp_no + | STATS BY emp_no, emp_no2, emp_no3, salary, salary3 + """)); + + var limit = as(plan, Limit.class); + var agg = as(limit.child(), Aggregate.class); + var eval = as(agg.child(), Eval.class); + + var empNo = findFieldAttribute(plan, "emp_no"); + var salary = findFieldAttribute(plan, "salary"); + + assertEquals(2, eval.fields().size()); + + var empNoTempName = eval.fields().get(0); + assertThat(empNoTempName.name(), startsWith("$$emp_no$temp_name$")); + assertEquals(mul(salary, of(2)), empNoTempName.child()); + + var salary3 = alias("salary3", mul(empNoTempName.toAttribute(), of(2))); + assertEqualsIgnoringIds(salary3, eval.fields().get(1)); + + List expectedAggregates = new ArrayList<>(); + expectedAggregates.add(alias("emp_no", empNoTempName.toAttribute())); + expectedAggregates.add(alias("emp_no2", empNo)); + expectedAggregates.add(alias("emp_no3", empNo)); + expectedAggregates.add(salary); + expectedAggregates.add(salary3.toAttribute()); + assertEqualsIgnoringIds(expectedAggregates, agg.aggregates()); + } + + /** + * 
{@code
+     * EsqlProject[[salary{f}#35, emp_no{f}#30 AS emp_no2#22, $$id$temp_name$41{r$}#42 AS emp_no#25, emp_no{f}#30 AS emp_no3#28,
+     *  salary3{r}#19]]
+     * \_Eval[[salary{f}#35 * 2[INTEGER] AS $$id$temp_name$41#42, $$id$temp_name$41{r$}#42 * 2[INTEGER] AS salary3#19]]
+     *   \_Limit[1000[INTEGER],false,false]
+     *     \_EsRelation[test][_meta_field{f}#36, emp_no{f}#30, first_name{f}#31, ..]
+     * }
+ */ + public void testNonAliasShadowingAliasedAttributeWithRename() { + // Rule only kicks in if there's a Project or Aggregate above, so we add a KEEP * + var plan = plan(randomFrom(""" + from test + | KEEP emp_no, salary + | RENAME emp_no AS id + | EVAL id2 = id, id = 2*salary, id3 = id2, salary3 = 2*id + | RENAME id2 AS emp_no2, id AS emp_no, id3 AS emp_no3 + | KEEP * + """)); + + var project = as(plan, Project.class); + var eval = as(project.child(), Eval.class); + + var empNo = findFieldAttribute(plan, "emp_no"); + var salary = findFieldAttribute(plan, "salary"); + + assertEquals(2, eval.fields().size()); + + var idTempName = eval.fields().get(0); + // The renaming to $$id$temp_name$ is not strictly necessary. If the eval pushdown past the first rename happened before the + // alias extraction, the eval wouldn't shadow any input attributes anymore. + assertThat(idTempName.name(), startsWith("$$id$temp_name$")); + assertEquals(mul(salary, of(2)), idTempName.child()); + + var salary3 = alias("salary3", mul(idTempName.toAttribute(), of(2))); + assertEqualsIgnoringIds(salary3, eval.fields().get(1)); + + List expectedProjections = new ArrayList<>(); + expectedProjections.add(salary); + expectedProjections.add(alias("emp_no2", empNo)); + expectedProjections.add(alias("emp_no", idTempName.toAttribute())); + expectedProjections.add(alias("emp_no3", empNo)); + expectedProjections.add(salary3.toAttribute()); + assertEqualsIgnoringIds(expectedProjections, project.projections()); + } + /** * 
{@code
      * EsqlProject[[emp_no{f}#18, salary{f}#23, emp_no{f}#18 AS emp_no3#10, emp_no2{r}#13, salary3{r}#16]]