Skip to content

Commit 7bc6749

Browse files
authored
ESQL: Fix EsqlNodeSubclassTest failure from mutating (#125503) (#125514)
We shouldn't mutate LogicalPlan.child().output() (or just LogicalPlan.output()); we did so in RENAME, but only in EsqlNodeSubclassTest, making them flaky. >The PR at #122250 seems to have created a flaky test failure in `EsqlNodeSubclassTests`. Local runs with `-Dtests.iters=100` lead to about two dozen failures in over 70k tests run. This is not a high failure rate, but still requires addressing. > >The single line added to the Analyzer by that PR causes an `UnsupportedOperationException` on attempting to mutate an immutable collection when running `EsqlNodeSubclassTests`. It turns out that this code path comes from `Rename.output()` which is only called in test scenarios. So a quick fix is to copy the child output into a mutable collection.
1 parent 5082f32 commit 7bc6749

File tree

2 files changed

+12
-1
lines changed

2 files changed

+12
-1
lines changed

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -443,6 +443,8 @@ protected LogicalPlan rule(LogicalPlan plan, AnalyzerContext context) {
443443
}
444444
final List<Attribute> childrenOutput = new ArrayList<>();
445445

446+
// Gather all the children's output in case of non-unary plans; even for unaries, we need to copy because we may mutate this to
447+
// simplify resolution of e.g. RENAME.
446448
for (LogicalPlan child : plan.children()) {
447449
var output = child.output();
448450
childrenOutput.addAll(output);
@@ -893,6 +895,10 @@ private LogicalPlan resolveRename(Rename rename, List<Attribute> childrenOutput)
893895
return new EsqlProject(rename.source(), rename.child(), projections);
894896
}
895897

898+
/**
899+
* This will turn a {@link Rename} into an equivalent {@link Project}.
900+
* Can mutate {@code childrenOutput}; hand this a copy if you want to avoid mutation.
901+
*/
896902
public static List<NamedExpression> projectionsForRename(Rename rename, List<Attribute> childrenOutput, Logger logger) {
897903
List<NamedExpression> projections = new ArrayList<>(childrenOutput);
898904

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

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import org.elasticsearch.xpack.esql.core.tree.Source;
1919
import org.elasticsearch.xpack.esql.expression.function.UnsupportedAttribute;
2020

21+
import java.util.ArrayList;
2122
import java.util.List;
2223
import java.util.Objects;
2324

@@ -47,7 +48,11 @@ public List<Alias> renamings() {
4748
@Override
4849
public List<Attribute> output() {
4950
// Normally shouldn't reach here, as Rename only exists before resolution.
50-
List<NamedExpression> projectionsAfterResolution = ResolveRefs.projectionsForRename(this, this.child().output(), null);
51+
List<NamedExpression> projectionsAfterResolution = ResolveRefs.projectionsForRename(
52+
this,
53+
new ArrayList<>(this.child().output()),
54+
null
55+
);
5156

5257
return Expressions.asAttributes(projectionsAfterResolution);
5358
}

0 commit comments

Comments
 (0)