Skip to content

Commit 5a24930

Browse files
authored
[9.1] ESQL: Fix ReplaceAliasingEvalWithProject in case of shadowing (#137025) (#137316)
* ESQL: Fix ReplaceAliasingEvalWithProject in case of shadowing (#137025) Fix #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 * Remove accidentally committed test from other PR * Fix tests
1 parent 4b8538e commit 5a24930

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
@@ -50,6 +50,7 @@
5050
import org.elasticsearch.xpack.esql.action.EsqlQueryResponse;
5151
import org.elasticsearch.xpack.esql.analysis.EnrichResolution;
5252
import org.elasticsearch.xpack.esql.analysis.Verifier;
53+
import org.elasticsearch.xpack.esql.core.expression.Alias;
5354
import org.elasticsearch.xpack.esql.core.expression.Attribute;
5455
import org.elasticsearch.xpack.esql.core.expression.Expression;
5556
import org.elasticsearch.xpack.esql.core.expression.FieldAttribute;
@@ -63,11 +64,13 @@
6364
import org.elasticsearch.xpack.esql.core.type.DataType;
6465
import org.elasticsearch.xpack.esql.core.type.EsField;
6566
import org.elasticsearch.xpack.esql.core.util.DateUtils;
67+
import org.elasticsearch.xpack.esql.core.util.Holder;
6668
import org.elasticsearch.xpack.esql.core.util.StringUtils;
6769
import org.elasticsearch.xpack.esql.expression.function.EsqlFunctionRegistry;
6870
import org.elasticsearch.xpack.esql.expression.function.scalar.string.regex.RLike;
6971
import org.elasticsearch.xpack.esql.expression.function.scalar.string.regex.WildcardLike;
7072
import org.elasticsearch.xpack.esql.expression.predicate.Range;
73+
import org.elasticsearch.xpack.esql.expression.predicate.operator.arithmetic.Mul;
7174
import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.Equals;
7275
import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.GreaterThan;
7376
import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.GreaterThanOrEqual;
@@ -123,6 +126,7 @@
123126
import java.util.Map;
124127
import java.util.Set;
125128
import java.util.TreeMap;
129+
import java.util.function.Predicate;
126130
import java.util.jar.JarInputStream;
127131
import java.util.zip.ZipEntry;
128132

@@ -197,6 +201,27 @@ public static GreaterThanOrEqual greaterThanOrEqualOf(Expression left, Expressio
197201
return new GreaterThanOrEqual(EMPTY, left, right, ESTestCase.randomZone());
198202
}
199203

204+
public static FieldAttribute findFieldAttribute(LogicalPlan plan, String name) {
205+
return findFieldAttribute(plan, name, (unused) -> true);
206+
}
207+
208+
public static FieldAttribute findFieldAttribute(LogicalPlan plan, String name, Predicate<EsRelation> inThisRelation) {
209+
Holder<FieldAttribute> result = new Holder<>();
210+
plan.forEachDown(EsRelation.class, relation -> {
211+
if (inThisRelation.test(relation) == false) {
212+
return;
213+
}
214+
for (Attribute attr : relation.output()) {
215+
if (attr.name().equals(name)) {
216+
assertNull("Multiple matching field attributes found", result.get());
217+
result.set((FieldAttribute) attr);
218+
return;
219+
}
220+
}
221+
});
222+
return result.get();
223+
}
224+
200225
public static FieldAttribute getFieldAttribute() {
201226
return getFieldAttribute("a");
202227
}
@@ -239,6 +264,14 @@ public static ReferenceAttribute referenceAttribute(String name, DataType type)
239264
return new ReferenceAttribute(EMPTY, name, type);
240265
}
241266

267+
public static Alias alias(String name, Expression child) {
268+
return new Alias(EMPTY, name, child);
269+
}
270+
271+
public static Mul mul(Expression left, Expression right) {
272+
return new Mul(EMPTY, left, right);
273+
}
274+
242275
public static Range rangeOf(Expression value, Expression lower, boolean includeLower, Expression upper, boolean includeUpper) {
243276
return new Range(EMPTY, value, lower, includeLower, upper, includeUpper, randomZone());
244277
}

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
@@ -788,3 +788,28 @@ from firewall_logs,threat_list,airports
788788
dsjbDWegyVkg:boolean | name:integer | nhIWDevxEtU:null | iPIbTFddDK:integer | rUvqtgSl:integer
789789
false | 1 | null | 1 | 1
790790
;
791+
792+
// Bugfixes for ReplaceAliasingEvalWithProject not accounting for shadowing
793+
chainOfAliases
794+
required_capability: fix_replace_aliasing_eval_with_project_shadowing
795+
796+
row x = 1
797+
| eval y = x, z = y, x = 2, y = 3
798+
| keep *
799+
;
800+
801+
z:integer | x:integer | y:integer
802+
1 | 2 | 3
803+
;
804+
805+
shadowingOfAliases
806+
required_capability: fix_replace_aliasing_eval_with_project_shadowing
807+
808+
row x = 1
809+
| eval y = "foo", z = x, x = 9
810+
| rename y as a
811+
;
812+
813+
a:keyword | z:integer | x:integer
814+
foo | 1 | 9
815+
;

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
@@ -1280,7 +1280,17 @@ public enum Cap {
12801280
* e.g. STATS SUM(1) WHERE x==3 is replaced by
12811281
* STATS MV_SUM(const)*COUNT(*) WHERE x == 3.
12821282
*/
1283-
STATS_WITH_FILTERED_SURROGATE_FIXED;
1283+
STATS_WITH_FILTERED_SURROGATE_FIXED,
1284+
1285+
/**
1286+
* {@link org.elasticsearch.xpack.esql.optimizer.rules.logical.ReplaceAliasingEvalWithProject} did not fully account for shadowing.
1287+
* https://github.com/elastic/elasticsearch/issues/137019.
1288+
*/
1289+
FIX_REPLACE_ALIASING_EVAL_WITH_PROJECT_SHADOWING,
1290+
1291+
// Last capability should still have a comma for fewer merge conflicts when adding new ones :)
1292+
// This comment prevents the semicolon from being on the previous capability when Spotless formats the file.
1293+
;
12841294

12851295
private final boolean enabled;
12861296

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)