Skip to content

Commit bd71a29

Browse files
authored
[8.19] ESQL: Fix ReplaceAliasingEvalWithProject in case of shadowing (elastic#137025) (elastic#137318)
* ESQL: Fix ReplaceAliasingEvalWithProject in case of shadowing (elastic#137025) Fix elastic#137019: a bug that happened when the Eval has (non-aliasing) fields that happen to overwrite the attributes that we try to alias in a subsequent Project. (cherry picked from commit 386b156) # Conflicts: # x-pack/plugin/esql/qa/testFixtures/src/main/resources/eval.csv-spec # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java * Fix tests
1 parent 69c5e92 commit bd71a29

File tree

6 files changed

+607
-24
lines changed

6 files changed

+607
-24
lines changed

docs/changelog/137025.yaml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
pr: 137025
2+
summary: Fix `ReplaceAliasingEvalWithProject` in case of shadowing
3+
area: ES|QL
4+
type: bug
5+
issues:
6+
- 137019

x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@
5151
import org.elasticsearch.xpack.esql.action.EsqlQueryResponse;
5252
import org.elasticsearch.xpack.esql.analysis.EnrichResolution;
5353
import org.elasticsearch.xpack.esql.analysis.Verifier;
54+
import org.elasticsearch.xpack.esql.core.expression.Alias;
5455
import org.elasticsearch.xpack.esql.core.expression.Attribute;
5556
import org.elasticsearch.xpack.esql.core.expression.Expression;
5657
import org.elasticsearch.xpack.esql.core.expression.FieldAttribute;
@@ -64,11 +65,13 @@
6465
import org.elasticsearch.xpack.esql.core.type.DataType;
6566
import org.elasticsearch.xpack.esql.core.type.EsField;
6667
import org.elasticsearch.xpack.esql.core.util.DateUtils;
68+
import org.elasticsearch.xpack.esql.core.util.Holder;
6769
import org.elasticsearch.xpack.esql.core.util.StringUtils;
6870
import org.elasticsearch.xpack.esql.expression.function.EsqlFunctionRegistry;
6971
import org.elasticsearch.xpack.esql.expression.function.scalar.string.regex.RLike;
7072
import org.elasticsearch.xpack.esql.expression.function.scalar.string.regex.WildcardLike;
7173
import org.elasticsearch.xpack.esql.expression.predicate.Range;
74+
import org.elasticsearch.xpack.esql.expression.predicate.operator.arithmetic.Mul;
7275
import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.Equals;
7376
import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.GreaterThan;
7477
import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.GreaterThanOrEqual;
@@ -124,6 +127,7 @@
124127
import java.util.Map;
125128
import java.util.Set;
126129
import java.util.TreeMap;
130+
import java.util.function.Predicate;
127131
import java.util.jar.JarInputStream;
128132
import java.util.zip.ZipEntry;
129133

@@ -200,6 +204,27 @@ public static GreaterThanOrEqual greaterThanOrEqualOf(Expression left, Expressio
200204
return new GreaterThanOrEqual(EMPTY, left, right, ESTestCase.randomZone());
201205
}
202206

207+
public static FieldAttribute findFieldAttribute(LogicalPlan plan, String name) {
208+
return findFieldAttribute(plan, name, (unused) -> true);
209+
}
210+
211+
public static FieldAttribute findFieldAttribute(LogicalPlan plan, String name, Predicate<EsRelation> inThisRelation) {
212+
Holder<FieldAttribute> result = new Holder<>();
213+
plan.forEachDown(EsRelation.class, relation -> {
214+
if (inThisRelation.test(relation) == false) {
215+
return;
216+
}
217+
for (Attribute attr : relation.output()) {
218+
if (attr.name().equals(name)) {
219+
assertNull("Multiple matching field attributes found", result.get());
220+
result.set((FieldAttribute) attr);
221+
return;
222+
}
223+
}
224+
});
225+
return result.get();
226+
}
227+
203228
public static FieldAttribute getFieldAttribute() {
204229
return getFieldAttribute("a");
205230
}
@@ -242,6 +267,14 @@ public static ReferenceAttribute referenceAttribute(String name, DataType type)
242267
return new ReferenceAttribute(EMPTY, name, type);
243268
}
244269

270+
public static Alias alias(String name, Expression child) {
271+
return new Alias(EMPTY, name, child);
272+
}
273+
274+
public static Mul mul(Expression left, Expression right) {
275+
return new Mul(EMPTY, left, right);
276+
}
277+
245278
public static Range rangeOf(Expression value, Expression lower, boolean includeLower, Expression upper, boolean includeUpper) {
246279
return new Range(EMPTY, value, lower, includeLower, upper, includeUpper, randomZone());
247280
}

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

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -752,3 +752,28 @@ g:integer | max:integer
752752
12 | 2
753753
12 | 2
754754
;
755+
756+
// Bugfixes for ReplaceAliasingEvalWithProject not accounting for shadowing
757+
chainOfAliases
758+
required_capability: fix_replace_aliasing_eval_with_project_shadowing
759+
760+
row x = 1
761+
| eval y = x, z = y, x = 2, y = 3
762+
| keep *
763+
;
764+
765+
z:integer | x:integer | y:integer
766+
1 | 2 | 3
767+
;
768+
769+
shadowingOfAliases
770+
required_capability: fix_replace_aliasing_eval_with_project_shadowing
771+
772+
row x = 1
773+
| eval y = "foo", z = x, x = 9
774+
| rename y as a
775+
;
776+
777+
a:keyword | z:integer | x:integer
778+
foo | 1 | 9
779+
;

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1064,7 +1064,17 @@ public enum Cap {
10641064
* e.g. STATS SUM(1) WHERE x==3 is replaced by
10651065
* STATS MV_SUM(const)*COUNT(*) WHERE x == 3.
10661066
*/
1067-
STATS_WITH_FILTERED_SURROGATE_FIXED;
1067+
STATS_WITH_FILTERED_SURROGATE_FIXED,
1068+
1069+
/**
1070+
* {@link org.elasticsearch.xpack.esql.optimizer.rules.logical.ReplaceAliasingEvalWithProject} did not fully account for shadowing.
1071+
* https://github.com/elastic/elasticsearch/issues/137019.
1072+
*/
1073+
FIX_REPLACE_ALIASING_EVAL_WITH_PROJECT_SHADOWING,
1074+
1075+
// Last capability should still have a comma for fewer merge conflicts when adding new ones :)
1076+
// This comment prevents the semicolon from being on the previous capability when Spotless formats the file.
1077+
;
10681078

10691079
private final boolean enabled;
10701080

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAliasingEvalWithProject.java

Lines changed: 43 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,11 @@
2020
import org.elasticsearch.xpack.esql.rule.Rule;
2121

2222
import java.util.ArrayList;
23+
import java.util.HashSet;
2324
import java.util.List;
25+
import java.util.Set;
26+
27+
import static org.elasticsearch.xpack.esql.optimizer.rules.logical.TemporaryNameUtils.locallyUniqueTemporaryName;
2428

2529
/**
2630
* Replace aliasing evals (eval x=a) with a projection which can be further combined / simplified.
@@ -53,47 +57,63 @@ public LogicalPlan apply(LogicalPlan logicalPlan) {
5357
private LogicalPlan rule(Eval eval) {
5458
LogicalPlan plan = eval;
5559

56-
// holds simple aliases such as b = a, c = b, d = c
57-
AttributeMap.Builder<Expression> basicAliasesBuilder = AttributeMap.builder();
58-
// same as above but keeps the original expression
59-
AttributeMap.Builder<NamedExpression> basicAliasSourcesBuilder = AttributeMap.builder();
60+
// Mostly, holds simple aliases from the eval, such as b = a, c = b, d = c, so we can resolve them in subsequent eval fields
61+
AttributeMap.Builder<Expression> renamesToPropagate = AttributeMap.builder();
62+
// the aliases for the final projection - mostly, same as above but holds the final aliases rather than the original attributes
63+
AttributeMap.Builder<NamedExpression> projectionAliases = AttributeMap.builder();
64+
// The names of attributes that are required to perform the aliases in a subsequent projection - if the next eval field
65+
// shadows one of these names, the subsequent projection won't work, so we need to perform a temporary rename.
66+
Set<String> namesRequiredForProjectionAliases = new HashSet<>();
6067

61-
List<Alias> keptFields = new ArrayList<>();
68+
List<Alias> newEvalFields = new ArrayList<>();
6269

6370
var fields = eval.fields();
64-
for (int i = 0, size = fields.size(); i < size; i++) {
65-
Alias field = fields.get(i);
71+
for (Alias alias : fields) {
72+
// propagate all previous aliases into the current field
73+
Alias field = (Alias) alias.transformUp(e -> renamesToPropagate.build().resolve(e, e));
74+
String name = field.name();
75+
Attribute attribute = field.toAttribute();
6676
Expression child = field.child();
67-
var attribute = field.toAttribute();
68-
// put the aliases in a separate map to separate the underlying resolve from other aliases
69-
if (child instanceof Attribute) {
70-
basicAliasesBuilder.put(attribute, child);
71-
basicAliasSourcesBuilder.put(attribute, field);
77+
78+
if (child instanceof Attribute renamedAttribute) {
79+
// Basic renaming - let's do that in the subsequent projection
80+
renamesToPropagate.put(attribute, renamedAttribute);
81+
projectionAliases.put(attribute, field);
82+
namesRequiredForProjectionAliases.add(renamedAttribute.name());
83+
} else if (namesRequiredForProjectionAliases.contains(name)) {
84+
// Not a basic renaming, needs to remain in the eval.
85+
// The field may shadow one of the attributes that we will need to correctly perform the subsequent projection.
86+
// So, rename it in the eval!
87+
88+
Alias newField = new Alias(field.source(), locallyUniqueTemporaryName(name), child, null, true);
89+
Attribute newAttribute = newField.toAttribute();
90+
Alias reRenamedField = new Alias(field.source(), name, newAttribute, field.id(), field.synthetic());
91+
projectionAliases.put(attribute, reRenamedField);
92+
// the renaming also needs to be propagated to eval fields to the right
93+
renamesToPropagate.put(attribute, newAttribute);
94+
95+
newEvalFields.add(newField);
7296
} else {
73-
// be lazy and start replacing name aliases only if needed
74-
if (basicAliasesBuilder.build().size() > 0) {
75-
// update the child through the field
76-
field = (Alias) field.transformUp(e -> basicAliasesBuilder.build().resolve(e, e));
77-
}
78-
keptFields.add(field);
97+
// still not a basic renaming, but no risk of shadowing
98+
newEvalFields.add(field);
7999
}
80100
}
81101

82102
// at least one alias encountered, move it into a project
83-
if (basicAliasesBuilder.build().size() > 0) {
103+
if (renamesToPropagate.build().size() > 0) {
84104
// preserve the eval output (takes care of shadowing and order) but replace the basic aliases
85105
List<NamedExpression> projections = new ArrayList<>(eval.output());
86-
var basicAliasSources = basicAliasSourcesBuilder.build();
106+
var projectionAliasesMap = projectionAliases.build();
87107
// replace the removed aliases with their initial definition - however use the output to preserve the shadowing
88108
for (int i = projections.size() - 1; i >= 0; i--) {
89109
NamedExpression project = projections.get(i);
90-
projections.set(i, basicAliasSources.getOrDefault(project, project));
110+
projections.set(i, projectionAliasesMap.getOrDefault(project, project));
91111
}
92112

93113
LogicalPlan child = eval.child();
94-
if (keptFields.size() > 0) {
114+
if (newEvalFields.size() > 0) {
95115
// replace the eval with just the kept fields
96-
child = new Eval(eval.source(), eval.child(), keptFields);
116+
child = new Eval(eval.source(), eval.child(), newEvalFields);
97117
}
98118
// put the projection in place
99119
plan = new Project(eval.source(), child, projections);

0 commit comments

Comments
 (0)