From cf402b43513c51e55d0b3c1b8a30bba9ec2b678f Mon Sep 17 00:00:00 2001 From: Bogdan Pintea Date: Tue, 18 Nov 2025 22:11:14 +0100 Subject: [PATCH 1/5] Drop PropagateInlineEvals optimizer rule This drops the PropagateInlineEvals rule that moves an Eval (or part of) it from the RHS of an InlineJoin to the LHS of it. Namely, the evaluation of the groups. This can be done directly in the ReplaceAggregateNestedExpressionWithEval rule, that creates these evaluations in the first place. This rule is now InlineJoins aware. --- .../src/main/resources/inlinestats.csv-spec | 16 ++-- .../esql/optimizer/LogicalPlanOptimizer.java | 4 - .../rules/logical/PropagateInlineEvals.java | 92 ------------------- ...laceAggregateNestedExpressionWithEval.java | 73 ++++++++++++--- .../local/PushExpressionsToFieldLoad.java | 8 +- .../esql/plan/logical/join/InlineJoin.java | 7 +- .../esql/plan/logical/join/StubRelation.java | 23 ++++- .../optimizer/LogicalPlanOptimizerTests.java | 54 +++++++++++ 8 files changed, 149 insertions(+), 128 deletions(-) delete mode 100644 x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PropagateInlineEvals.java diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/inlinestats.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/inlinestats.csv-spec index 267eec949dcb5..b3ed1830ab039 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/inlinestats.csv-spec +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/inlinestats.csv-spec @@ -1118,7 +1118,7 @@ required_capability: inline_stats FROM employees | KEEP emp_no, languages, gender, last_name | WHERE gender IS NOT NULL -| INLINE STATS max_lang = MAX(languages), min_lang = MIN(languages) BY f = left(last_name, 1), gender +| INLINE STATS max_lang = MAX(languages), min_lang = MIN(languages) BY f = LEFT(last_name, 1), gender | SORT last_name DESC | LIMIT 8 ; @@ -2675,14 +2675,14 @@ FROM employees stdDevFilter required_capability: inline_stats FROM employees -| inline stats greater_than = STD_DEV(salary_change) WHERE languages > 3 -, less_than = STD_DEV(salary_change) WHERE languages <= 3 -, salary = STD_DEV(salary * 2) -, count = COUNT(*) BY gender +| INLINE STATS greater_than = STD_DEV(salary_change) WHERE languages > 3 + , less_than = STD_DEV(salary_change) WHERE languages <= 3 + , salary = STD_DEV(salary * 2) + , count = COUNT(*) BY gender | EVAL greater_than = ROUND(greater_than, 5) -, less_than = ROUND(less_than, 5) -, salary = ROUND(salary, 5) -| keep emp_no, gender, languages, *than, salary, count + , less_than = ROUND(less_than, 5) + , salary = ROUND(salary, 5) +| KEEP emp_no, gender, languages, *than, salary, count | SORT emp_no asc | limit 10 ; diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java index 020694210e327..9b09bd0ee0560 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java @@ -29,7 +29,6 @@ import org.elasticsearch.xpack.esql.optimizer.rules.logical.PropagateEmptyRelation; import org.elasticsearch.xpack.esql.optimizer.rules.logical.PropagateEquals; import org.elasticsearch.xpack.esql.optimizer.rules.logical.PropagateEvalFoldables; -import org.elasticsearch.xpack.esql.optimizer.rules.logical.PropagateInlineEvals; import org.elasticsearch.xpack.esql.optimizer.rules.logical.PropagateNullable; import org.elasticsearch.xpack.esql.optimizer.rules.logical.PropgateUnmappedFields; import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneColumns; @@ -155,9 +154,6 @@ protected static Batch substitutions() { // after translating metric aggregates, we need to replace surrogate substitutions and nested expressions again. new SubstituteSurrogateAggregations(), new ReplaceAggregateNestedExpressionWithEval(), - // this one needs to be placed before ReplaceAliasingEvalWithProject, so that any potential aliasing eval (eval x = y) - // is not replaced with a Project before the eval to be copied on the left hand side of an InlineJoin - new PropagateInlineEvals(), new ReplaceRegexMatch(), new ReplaceTrivialTypeConversions(), new ReplaceAliasingEvalWithProject(), diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PropagateInlineEvals.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PropagateInlineEvals.java deleted file mode 100644 index f518322c4f924..0000000000000 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PropagateInlineEvals.java +++ /dev/null @@ -1,92 +0,0 @@ -/* - * 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.Alias; -import org.elasticsearch.xpack.esql.core.expression.Expression; -import org.elasticsearch.xpack.esql.core.expression.ReferenceAttribute; -import org.elasticsearch.xpack.esql.plan.logical.Aggregate; -import org.elasticsearch.xpack.esql.plan.logical.Eval; -import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan; -import org.elasticsearch.xpack.esql.plan.logical.join.InlineJoin; -import org.elasticsearch.xpack.esql.plan.logical.join.StubRelation; - -import java.util.ArrayList; -import java.util.LinkedHashMap; -import java.util.List; -import java.util.Map; - -import static org.elasticsearch.xpack.esql.plan.logical.join.InlineJoin.replaceStub; -import static org.elasticsearch.xpack.esql.plan.logical.join.StubRelation.computeOutput; - -/** - * Replace any evaluation from the inlined aggregation side (right side) to the left side (source) to perform the matching. - * In INLINE STATS m = MIN(x) BY a + b the right side contains STATS m = MIN(X) BY a + b. - * As the grouping key is used to perform the join, the evaluation required for creating it has to be copied to the left side - * as well. - */ -public class PropagateInlineEvals extends OptimizerRules.OptimizerRule { - - @Override - protected LogicalPlan rule(InlineJoin plan) { - // check if there's any grouping that uses a reference on the right side - // if so, look for the source until finding a StubReference - // then copy those on the left side as well - LogicalPlan left = plan.left(); - LogicalPlan right = plan.right(); - - // grouping references - List groupingAlias = new ArrayList<>(); - // TODO: replace this with AttributeSet - Map groupingRefs = new LinkedHashMap<>(); - - // perform only one iteration that does two things - // first checks any aggregate that declares expressions inside the grouping - // second that checks any found references to collect their declaration - right = right.transformDown(p -> { - if (p instanceof Aggregate aggregate) { - // collect references - for (Expression g : aggregate.groupings()) { - if (g instanceof ReferenceAttribute ref) { - groupingRefs.put(ref.name(), ref); - } - } - } - - if (groupingRefs.isEmpty()) { - return p; - } - - // find their declaration and remove it - if (p instanceof Eval eval) { - List fields = eval.fields(); - List remainingEvals = new ArrayList<>(fields.size()); - for (Alias f : fields) { - // TODO: look into identifying refs by their NameIds instead - if (groupingRefs.remove(f.name()) != null) { - groupingAlias.add(f); - } else { - remainingEvals.add(f); - } - } - if (remainingEvals.size() != fields.size()) { - // if all fields are moved, replace the eval - p = remainingEvals.size() == 0 ? eval.child() : new Eval(eval.source(), eval.child(), remainingEvals); - } - } - return p; - }); - - // copy found evals on the left side - if (groupingAlias.size() > 0) { - left = new Eval(plan.source(), plan.left(), groupingAlias); - } - // replace the old stub with the new out to capture the new output - return plan.replaceChildren(left, replaceStub(new StubRelation(right.source(), computeOutput(right, left)), right)); - } -} diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAggregateNestedExpressionWithEval.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAggregateNestedExpressionWithEval.java index 830220d3f8dbd..cf63688ad699f 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAggregateNestedExpressionWithEval.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAggregateNestedExpressionWithEval.java @@ -7,6 +7,7 @@ package org.elasticsearch.xpack.esql.optimizer.rules.logical; +import org.elasticsearch.core.Nullable; import org.elasticsearch.xpack.esql.core.expression.Alias; import org.elasticsearch.xpack.esql.core.expression.Attribute; import org.elasticsearch.xpack.esql.core.expression.Expression; @@ -18,6 +19,10 @@ import org.elasticsearch.xpack.esql.plan.logical.Aggregate; import org.elasticsearch.xpack.esql.plan.logical.Eval; import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan; +import org.elasticsearch.xpack.esql.plan.logical.UnaryPlan; +import org.elasticsearch.xpack.esql.plan.logical.join.InlineJoin; +import org.elasticsearch.xpack.esql.plan.logical.join.StubRelation; +import org.elasticsearch.xpack.esql.rule.Rule; import java.util.ArrayList; import java.util.HashMap; @@ -34,14 +39,44 @@ * becomes * {@code EVAL `a + 1` = a + 1, `x % 2` = x % 2 | INLINE STATS SUM(`a+1`_ref) BY `x % 2`_ref} */ -public final class ReplaceAggregateNestedExpressionWithEval extends OptimizerRules.OptimizerRule { +public final class ReplaceAggregateNestedExpressionWithEval extends Rule { @Override - protected LogicalPlan rule(Aggregate aggregate) { - List evals = new ArrayList<>(); + public LogicalPlan apply(LogicalPlan plan) { + return plan.transformDown(p -> switch (p) { + case InlineJoin inlineJoin -> rule(inlineJoin); + // aggs having a StubRelation child are handled in the InlineJoin case above + case Aggregate agg -> isInlineStats(agg) ? agg : rule(agg, null); + default -> p; + }); + } + + private static boolean isInlineStats(Aggregate aggregate) { + var child = aggregate.child(); + while (child instanceof UnaryPlan unary) { + child = unary.child(); + } + return child instanceof StubRelation; + } + + /** + * The InlineJoin will perform the join on the groupings, so any expressions used within the group part of the aggs will be performed + * on the left side. The expressions used within the aggregates part of the aggs will remain on the right side. + */ + private static LogicalPlan rule(InlineJoin inlineJoin) { + Holder evalHolder = new Holder<>(null); + LogicalPlan newRight = inlineJoin.right().transformDown(Aggregate.class, agg -> rule(agg, evalHolder)); + Eval eval = evalHolder.get(); + return eval != null + ? new InlineJoin(inlineJoin.source(), eval.replaceChild(inlineJoin.left()), newRight, inlineJoin.config()) + : inlineJoin.replaceRight(newRight); + } + + private static LogicalPlan rule(Aggregate aggregate, @Nullable Holder evalHolder) { Map evalNames = new HashMap<>(); Map groupingAttributes = new HashMap<>(); List newGroupings = new ArrayList<>(aggregate.groupings()); + List groupsEvals = new ArrayList<>(newGroupings.size()); boolean groupingChanged = false; // start with the groupings since the aggs might reuse/reference them @@ -52,7 +87,7 @@ protected LogicalPlan rule(Aggregate aggregate) { // for non-evaluable grouping functions, replace their nested expressions with attributes and extract the expression out // into an eval (added later below) if (asChild instanceof GroupingFunction.NonEvaluatableGroupingFunction gf) { - Expression newGroupingFunction = transformNonEvaluatableGroupingFunction(gf, evals); + Expression newGroupingFunction = transformNonEvaluatableGroupingFunction(gf, groupsEvals); if (newGroupingFunction != gf) { groupingChanged = true; newGroupings.set(i, as.replaceChild(newGroupingFunction)); @@ -61,7 +96,7 @@ protected LogicalPlan rule(Aggregate aggregate) { // Move the alias into an eval and replace it with its attribute. groupingChanged = true; var attr = as.toAttribute(); - evals.add(as); + groupsEvals.add(as); evalNames.put(as.name(), attr); newGroupings.set(i, attr); if (asChild instanceof GroupingFunction.EvaluatableGroupingFunction gf) { @@ -74,10 +109,11 @@ protected LogicalPlan rule(Aggregate aggregate) { Holder aggsChanged = new Holder<>(false); List aggs = aggregate.aggregates(); List newAggs = new ArrayList<>(aggs.size()); + List aggsEvals = new ArrayList<>(aggs.size()); // map to track common expressions Map expToAttribute = new HashMap<>(); - for (Alias a : evals) { + for (Alias a : groupsEvals) { expToAttribute.put(a.child().canonical(), a.toAttribute()); } @@ -102,7 +138,7 @@ protected LogicalPlan rule(Aggregate aggregate) { // look for the aggregate function var replaced = child.transformUp( AggregateFunction.class, - af -> transformAggregateFunction(af, expToAttribute, evals, counter, aggsChanged) + af -> transformAggregateFunction(af, expToAttribute, aggsEvals, counter, aggsChanged) ); // replace any evaluatable grouping functions with their references pointing to the added synthetic eval replaced = replaced.transformDown(GroupingFunction.EvaluatableGroupingFunction.class, gf -> { @@ -118,12 +154,27 @@ protected LogicalPlan rule(Aggregate aggregate) { newAggs.add(a); } - if (evals.size() > 0) { + if (groupingChanged || aggsChanged.get()) { + List rightEvals; + if (evalHolder != null) { // this is an INLINE STATS scenario, group evals go to the LHS + if (groupsEvals.size() > 0) { + var eval = new Eval(aggregate.source(), aggregate.child(), groupsEvals); + evalHolder.set(eval); + + // update the StubRelation to include the refs that'll come from (to be added to) the LHS Eval + aggregate = (Aggregate) aggregate.transformDown(StubRelation.class, sr -> sr.extendWith(eval)); + } + rightEvals = aggsEvals; // aggs evals remain on the RHS + } else { // this is a regular STATS scenario, all evals remain on the RHS + rightEvals = groupsEvals; + rightEvals.addAll(aggsEvals); + } + // add the RHS Eval if needed + var aggChild = rightEvals.size() > 0 ? new Eval(aggregate.source(), aggregate.child(), rightEvals) : aggregate.child(); + var groupings = groupingChanged ? newGroupings : aggregate.groupings(); var aggregates = aggsChanged.get() ? newAggs : aggregate.aggregates(); - - var newEval = new Eval(aggregate.source(), aggregate.child(), evals); - aggregate = aggregate.with(newEval, groupings, aggregates); + aggregate = aggregate.with(aggChild, groupings, aggregates); } return aggregate; diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/local/PushExpressionsToFieldLoad.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/local/PushExpressionsToFieldLoad.java index bec1fcbcd5ef2..d6d4814e29554 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/local/PushExpressionsToFieldLoad.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/local/PushExpressionsToFieldLoad.java @@ -113,12 +113,6 @@ private static Expression replaceFieldsForFieldTransformations( new NameId(), true ); - Attribute.IdIgnoringWrapper key = newFunctionAttr.ignoreId(); - if (addedAttrs.containsKey(key)) { - return addedAttrs.get(key); - } - - addedAttrs.put(key, newFunctionAttr); - return newFunctionAttr; + return addedAttrs.computeIfAbsent(newFunctionAttr.ignoreId(), k -> newFunctionAttr); } } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/InlineJoin.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/InlineJoin.java index f4cdcf28bb8e3..e03a7f2899b92 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/InlineJoin.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/InlineJoin.java @@ -55,10 +55,11 @@ public class InlineJoin extends Join { ); /** - * Replaces the source of the target plan with a stub preserving the output of the source plan. + * Replaces the source of the {@code destination} plan with a stub, preserving the output from the {@code target} plan, which + * the stub substitutes (or theoretically points to). */ - public static LogicalPlan stubSource(UnaryPlan sourcePlan, LogicalPlan target) { - return sourcePlan.replaceChild(new StubRelation(sourcePlan.source(), StubRelation.computeOutput(sourcePlan, target))); + public static LogicalPlan stubSource(UnaryPlan destination, LogicalPlan target) { + return destination.replaceChild(StubRelation.of(destination, target)); } /** diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/StubRelation.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/StubRelation.java index 770fcf33df90f..820c050508036 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/StubRelation.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/StubRelation.java @@ -16,6 +16,7 @@ import org.elasticsearch.xpack.esql.io.stream.PlanStreamInput; import org.elasticsearch.xpack.esql.plan.logical.LeafPlan; import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan; +import org.elasticsearch.xpack.esql.plan.logical.UnaryPlan; import java.io.IOException; import java.util.ArrayList; @@ -25,6 +26,7 @@ import java.util.Set; import static java.util.Collections.emptyList; +import static org.elasticsearch.xpack.esql.expression.NamedExpressions.mergeOutputAttributes; /** * Synthetic {@link LogicalPlan} used by the planner that the child plan is referred elsewhere. @@ -46,13 +48,28 @@ public StubRelation(Source source, List output) { this.output = output; } + /** + * Produces a StubRelation whose output is the merger of the {@code sourcePlan}'s output and {@code destinationPlan}'s referenced + * attributes. + */ + public static StubRelation of(UnaryPlan destinationPlan, LogicalPlan sourcePlan) { + return new StubRelation(destinationPlan.source(), computeOutput(destinationPlan, sourcePlan)); + } + + /** + * Produces a new StubRelation whose output is the merger of this StubRelation's output and the {@code sourcePlan}'s output. + */ + public StubRelation extendWith(LogicalPlan sourcePlan) { + return new StubRelation(source(), mergeOutputAttributes(sourcePlan.output(), output)); + } + /* * The output of a StubRelation must also include any synthetic attributes referenced by the source plan (union types is a great * example of those attributes that has some special treatment throughout the planning phases, especially in the EsRelation). */ - public static List computeOutput(LogicalPlan source, LogicalPlan target) { - Set stubRelationOutput = new LinkedHashSet<>(target.output()); - stubRelationOutput.addAll(source.references().stream().filter(Attribute::synthetic).toList()); + private static List computeOutput(LogicalPlan destinationPlan, LogicalPlan sourcePlan) { + Set stubRelationOutput = new LinkedHashSet<>(sourcePlan.output()); + stubRelationOutput.addAll(destinationPlan.references().stream().toList()); return new ArrayList<>(stubRelationOutput); } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java index c34bcf078de9d..7cf7c2d6ffdb7 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java @@ -5685,6 +5685,60 @@ public void testInlineStatsNestedExpressionsInGroups() { var stub = as(agg.child(), StubRelation.class); } + /* + * Limit[1000[INTEGER],false,false] + * \_InlineJoin[LEFT,[emo{r}#15, gender{f}#20, languages{f}#21],[emo{r}#15, gender{r}#20, languages{r}#21]] + * |_EsqlProject[[emp_no{f}#18, gender{f}#20, languages{f}#21, salary{f}#23, emo{r}#15]] + * | \_Eval[[emp_no{f}#18 % 2[INTEGER] AS emo#15]] + * | \_EsRelation[test][_meta_field{f}#24, emp_no{f}#18, first_name{f}#19, ..] + * \_Project[[avg{r}#12, emo{r}#15, gender{f}#20, languages{f}#21]] + * \_Eval[[$$SUM$$$AVG$AVG(salary)_+_2$0$0{r$}#30 / $$COUNT$$$AVG$AVG(salary)_+_2$0$1{r$}#31 AS $$AVG$AVG(salary)_+_2$0#29, + * $$AVG$AVG(salary)_+_2$0{r$}#29 + 2[INTEGER] AS avg#12]] + * \_Aggregate[[emo{r}#15, gender{f}#20, languages{f}#21],[SUM(salary{f}#23,true[BOOLEAN],PT0S[TIME_DURATION], + * compensated[KEYWORD]) AS $$SUM$$$AVG$AVG(salary)_+_2$0$0#30, COUNT(salary{f}#23,true[BOOLEAN],PT0S[TIME_DURATION]) AS + * $$COUNT$$$AVG$AVG(salary)_+_2$0$1#31, emo{r}#15, gender{f}#20, languages{f}#21]] + * \_StubRelation[[emp_no{f}#18, gender{f}#20, languages{f}#21, salary{f}#23, emo{r}#15]] + */ + public void testInlineStatsNestedAndShaddowingExpressions() { + var query = """ + FROM test + | KEEP emp_no, gender, languages, salary + | INLINE STATS languages = MV_AVG(languages + 1), avg = AVG(salary) + 2 BY emo = emp_no % 2, gender, languages + """; + if (releaseBuildForInlineStats(query)) { + return; + } + var plan = optimizedPlan(query); + var limit = as(plan, Limit.class); + var inline = as(limit.child(), InlineJoin.class); + + // Left side of the join + var leftProj = as(inline.left(), EsqlProject.class); + assertThat(Expressions.names(leftProj.projections()), containsInAnyOrder("emp_no", "gender", "languages", "salary", "emo")); + var evalLeft = as(leftProj.child(), Eval.class); + assertThat(Expressions.names(evalLeft.fields()), is(List.of("emo"))); + var mod = as(evalLeft.fields().get(0).child(), Mod.class); + assertThat(Expressions.name(mod.left()), is("emp_no")); + assertThat(mod.right().toString(), is("2")); + as(evalLeft.child(), EsRelation.class); + + // Right side of the join + var rightProj = as(inline.right(), Project.class); + assertThat(Expressions.names(rightProj.projections()), containsInAnyOrder("avg", "emo", "gender", "languages")); + var evalRight = as(rightProj.child(), Eval.class); + assertThat(evalRight.fields(), hasSize(2)); // one for AVG surrogate, one for `+ 2` + var agg = as(evalRight.child(), Aggregate.class); + assertThat(agg.groupings(), hasSize(3)); + assertThat(Expressions.names(agg.aggregates()), hasSize(5)); // 2 for avg, 1 for mv_avg, plus 2 groupings + var stub = as(agg.child(), StubRelation.class); + assertThat(Expressions.names(stub.output()), contains("emp_no", "gender", "languages", "salary", "emo")); + + assertWarnings( + "No limit defined, adding default limit of [1000]", + "Line 3:16: Field 'languages' shadowed by field at line 3:102" + ); + } + // if non-null, the `query` must have "INLINE STATS" capitalized public static boolean releaseBuildForInlineStats(@Nullable String query) { if (EsqlCapabilities.Cap.INLINE_STATS.isEnabled() == false) { From 9f25f2baba784f2efdf2c84b140d48334e7e83d5 Mon Sep 17 00:00:00 2001 From: Bogdan Pintea Date: Wed, 19 Nov 2025 08:55:15 +0100 Subject: [PATCH 2/5] Update docs/changelog/138270.yaml --- docs/changelog/138270.yaml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 docs/changelog/138270.yaml diff --git a/docs/changelog/138270.yaml b/docs/changelog/138270.yaml new file mode 100644 index 0000000000000..da2da7570d67d --- /dev/null +++ b/docs/changelog/138270.yaml @@ -0,0 +1,6 @@ +pr: 138270 +summary: Drop `PropagateInlineEvals` optimizer rule +area: ES|QL +type: enhancement +issues: + - 124754 From b23ddc60620e9345f73f1de2a84a784c9057664e Mon Sep 17 00:00:00 2001 From: elasticsearchmachine Date: Wed, 19 Nov 2025 08:05:20 +0000 Subject: [PATCH 3/5] [CI] Auto commit changes from spotless --- .../xpack/esql/optimizer/LogicalPlanOptimizerTests.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java index 7cf7c2d6ffdb7..8cf2b008bac38 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java @@ -5733,10 +5733,7 @@ public void testInlineStatsNestedAndShaddowingExpressions() { var stub = as(agg.child(), StubRelation.class); assertThat(Expressions.names(stub.output()), contains("emp_no", "gender", "languages", "salary", "emo")); - assertWarnings( - "No limit defined, adding default limit of [1000]", - "Line 3:16: Field 'languages' shadowed by field at line 3:102" - ); + assertWarnings("No limit defined, adding default limit of [1000]", "Line 3:16: Field 'languages' shadowed by field at line 3:102"); } // if non-null, the `query` must have "INLINE STATS" capitalized From 642302a6c31b53ea6ba0236e91d685d230b14e86 Mon Sep 17 00:00:00 2001 From: Bogdan Pintea Date: Mon, 15 Dec 2025 18:19:20 +0100 Subject: [PATCH 4/5] small refactorings, added more comments --- ...laceAggregateNestedExpressionWithEval.java | 25 +++++++++++++------ 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAggregateNestedExpressionWithEval.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAggregateNestedExpressionWithEval.java index cf63688ad699f..0be0a27b76064 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAggregateNestedExpressionWithEval.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAggregateNestedExpressionWithEval.java @@ -45,12 +45,16 @@ public final class ReplaceAggregateNestedExpressionWithEval extends Rule switch (p) { case InlineJoin inlineJoin -> rule(inlineJoin); - // aggs having a StubRelation child are handled in the InlineJoin case above + // aggs having a StubRelation child are handled by the InlineJoin case above, only deal with the "stand-alone" Aggregate here. case Aggregate agg -> isInlineStats(agg) ? agg : rule(agg, null); default -> p; }); } + /** + * Returns {@code true} if the Aggregate has a {@code StubRelation} as (grand)child, meaning it is under a {@code InlineJoin}, i.e., + * part of an {@code INLINE STATS}. + */ private static boolean isInlineStats(Aggregate aggregate) { var child = aggregate.child(); while (child instanceof UnaryPlan unary) { @@ -60,8 +64,8 @@ private static boolean isInlineStats(Aggregate aggregate) { } /** - * The InlineJoin will perform the join on the groupings, so any expressions used within the group part of the aggs will be performed - * on the left side. The expressions used within the aggregates part of the aggs will remain on the right side. + * The InlineJoin will perform the join on the groupings, so any expressions used within the group part of the Aggregate should be + * performed on the left side of the join. The expressions used within the aggregates part of the Aggregate will remain on the right. */ private static LogicalPlan rule(InlineJoin inlineJoin) { Holder evalHolder = new Holder<>(null); @@ -72,10 +76,12 @@ private static LogicalPlan rule(InlineJoin inlineJoin) { : inlineJoin.replaceRight(newRight); } - private static LogicalPlan rule(Aggregate aggregate, @Nullable Holder evalHolder) { + private static LogicalPlan rule(Aggregate aggregate, @Nullable Holder evalForIJHolder) { Map evalNames = new HashMap<>(); Map groupingAttributes = new HashMap<>(); List newGroupings = new ArrayList<>(aggregate.groupings()); + // Evaluations needed for expressions within the groupings + // "| STATS c = COUNT(*) BY a + 1" --> "| EVAL `a + 1` = a + 1 | STATS s = COUNT(*) BY `a + 1`_ref" List groupsEvals = new ArrayList<>(newGroupings.size()); boolean groupingChanged = false; @@ -109,6 +115,9 @@ private static LogicalPlan rule(Aggregate aggregate, @Nullable Holder eval Holder aggsChanged = new Holder<>(false); List aggs = aggregate.aggregates(); List newAggs = new ArrayList<>(aggs.size()); + // Evaluations needed for expressions within the aggs + // "| STATS s = SUM(a + 1)" --> "| EVAL `a + 1` = a + 1 | STATS s = SUM(`a + 1`_ref)" + // (i.e. not outside, like `| STATS s = SUM(a) + 1`) List aggsEvals = new ArrayList<>(aggs.size()); // map to track common expressions @@ -156,20 +165,20 @@ private static LogicalPlan rule(Aggregate aggregate, @Nullable Holder eval if (groupingChanged || aggsChanged.get()) { List rightEvals; - if (evalHolder != null) { // this is an INLINE STATS scenario, group evals go to the LHS + if (evalForIJHolder != null) { // this is an INLINE STATS scenario, group evals go to the LHS if (groupsEvals.size() > 0) { var eval = new Eval(aggregate.source(), aggregate.child(), groupsEvals); - evalHolder.set(eval); + evalForIJHolder.set(eval); // update the StubRelation to include the refs that'll come from (to be added to) the LHS Eval aggregate = (Aggregate) aggregate.transformDown(StubRelation.class, sr -> sr.extendWith(eval)); } - rightEvals = aggsEvals; // aggs evals remain on the RHS + rightEvals = aggsEvals; // aggs evals that remain on the RHS, i.e. those that feed into the Aggregate } else { // this is a regular STATS scenario, all evals remain on the RHS rightEvals = groupsEvals; rightEvals.addAll(aggsEvals); } - // add the RHS Eval if needed + // add a RHS Eval if needed, going under the Aggregate var aggChild = rightEvals.size() > 0 ? new Eval(aggregate.source(), aggregate.child(), rightEvals) : aggregate.child(); var groupings = groupingChanged ? newGroupings : aggregate.groupings(); From 7e5d0c02bc738902ae8f9984d11889a861acbcf4 Mon Sep 17 00:00:00 2001 From: Bogdan Pintea Date: Mon, 15 Dec 2025 20:43:02 +0100 Subject: [PATCH 5/5] refactor Evals creating in own method --- ...laceAggregateNestedExpressionWithEval.java | 80 +++++++++++++------ 1 file changed, 56 insertions(+), 24 deletions(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAggregateNestedExpressionWithEval.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAggregateNestedExpressionWithEval.java index 0be0a27b76064..27b71aa98d4f5 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAggregateNestedExpressionWithEval.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAggregateNestedExpressionWithEval.java @@ -8,6 +8,7 @@ package org.elasticsearch.xpack.esql.optimizer.rules.logical; import org.elasticsearch.core.Nullable; +import org.elasticsearch.core.Tuple; import org.elasticsearch.xpack.esql.core.expression.Alias; import org.elasticsearch.xpack.esql.core.expression.Attribute; import org.elasticsearch.xpack.esql.core.expression.Expression; @@ -65,15 +66,22 @@ private static boolean isInlineStats(Aggregate aggregate) { /** * The InlineJoin will perform the join on the groupings, so any expressions used within the group part of the Aggregate should be - * performed on the left side of the join. The expressions used within the aggregates part of the Aggregate will remain on the right. + * executed on the left side of the join: they'll be part of LHS's output, and through the StubRelation, RHS's too. + * The expressions used within the aggregates part of the Aggregate will remain on the right: they'll only be used for computing the + * joined values (corresponding to the groups values). */ private static LogicalPlan rule(InlineJoin inlineJoin) { Holder evalHolder = new Holder<>(null); LogicalPlan newRight = inlineJoin.right().transformDown(Aggregate.class, agg -> rule(agg, evalHolder)); Eval eval = evalHolder.get(); - return eval != null - ? new InlineJoin(inlineJoin.source(), eval.replaceChild(inlineJoin.left()), newRight, inlineJoin.config()) - : inlineJoin.replaceRight(newRight); + if (eval != null) { + // update the StubRelation to include the refs that'll come from the LHS Eval (added next) + newRight = newRight.transformDown(StubRelation.class, sr -> sr.extendWith(eval)); + inlineJoin = new InlineJoin(inlineJoin.source(), eval.replaceChild(inlineJoin.left()), newRight, inlineJoin.config()); + } else { + inlineJoin = (InlineJoin) inlineJoin.replaceRight(newRight); + } + return inlineJoin; } private static LogicalPlan rule(Aggregate aggregate, @Nullable Holder evalForIJHolder) { @@ -117,7 +125,7 @@ private static LogicalPlan rule(Aggregate aggregate, @Nullable Holder eval List newAggs = new ArrayList<>(aggs.size()); // Evaluations needed for expressions within the aggs // "| STATS s = SUM(a + 1)" --> "| EVAL `a + 1` = a + 1 | STATS s = SUM(`a + 1`_ref)" - // (i.e. not outside, like `| STATS s = SUM(a) + 1`) + // (i.e. not outside, like `| STATS s = SUM(a) + 1`; those are handled by ReplaceAggregateAggExpressionWithEval) List aggsEvals = new ArrayList<>(aggs.size()); // map to track common expressions @@ -164,31 +172,55 @@ private static LogicalPlan rule(Aggregate aggregate, @Nullable Holder eval } if (groupingChanged || aggsChanged.get()) { - List rightEvals; - if (evalForIJHolder != null) { // this is an INLINE STATS scenario, group evals go to the LHS - if (groupsEvals.size() > 0) { - var eval = new Eval(aggregate.source(), aggregate.child(), groupsEvals); - evalForIJHolder.set(eval); - - // update the StubRelation to include the refs that'll come from (to be added to) the LHS Eval - aggregate = (Aggregate) aggregate.transformDown(StubRelation.class, sr -> sr.extendWith(eval)); - } - rightEvals = aggsEvals; // aggs evals that remain on the RHS, i.e. those that feed into the Aggregate - } else { // this is a regular STATS scenario, all evals remain on the RHS - rightEvals = groupsEvals; - rightEvals.addAll(aggsEvals); + var evals = evals(aggregate, groupsEvals, aggsEvals, evalForIJHolder != null); + if (evalForIJHolder != null) { + evalForIJHolder.set(evals.v1()); } - // add a RHS Eval if needed, going under the Aggregate - var aggChild = rightEvals.size() > 0 ? new Eval(aggregate.source(), aggregate.child(), rightEvals) : aggregate.child(); - - var groupings = groupingChanged ? newGroupings : aggregate.groupings(); - var aggregates = aggsChanged.get() ? newAggs : aggregate.aggregates(); - aggregate = aggregate.with(aggChild, groupings, aggregates); + aggregate = updateAggregate(aggregate, evals.v2(), groupingChanged ? newGroupings : null, aggsChanged.get() ? newAggs : null); } return aggregate; } + /** + * The evals that will go under the Aggregate: either all the evals collected, for "stand-alone" Aggregate, + * or only those needed for the aggregates (nested) expressions, for the Aggregate under InlineJoin. + * @return a Tuple of {@code Eval}s (LHS, RHS), either of which can be null if no evals are needed. In case the Aggregate is + * stand-alone, the RHS Eval will contain all evals, and the LHS will be null. + */ + private static Tuple evals(Aggregate aggregate, List groupsEvals, List aggsEvals, boolean isInlineStats) { + Eval lhs = null, rhs; + List subAggEvals; + + if (isInlineStats) { // this is an INLINE STATS scenario, group evals go to the LHS, aggs evals remain on the RHS + if (groupsEvals.size() > 0) { + lhs = new Eval(aggregate.source(), aggregate.child(), groupsEvals); // LHS evals + } + subAggEvals = aggsEvals; // RHS evals + } else { // this is a regular STATS scenario, place all evals under the Aggregate + subAggEvals = groupsEvals; + subAggEvals.addAll(aggsEvals); + } + + // add an Eval (if needed), going under the Aggregate + rhs = subAggEvals.size() > 0 ? new Eval(aggregate.source(), aggregate.child(), subAggEvals) : null; + + return Tuple.tuple(lhs, rhs); + } + + private static Aggregate updateAggregate( + Aggregate aggregate, + @Nullable LogicalPlan newChild, + @Nullable List newGroupings, + @Nullable List newAggs + ) { + var groupings = newGroupings != null ? newGroupings : aggregate.groupings(); + var aggregates = newAggs != null ? newAggs : aggregate.aggregates(); + var child = newChild != null ? newChild : aggregate.child(); + + return aggregate.with(child, groupings, aggregates); + } + private static Expression transformNonEvaluatableGroupingFunction( GroupingFunction.NonEvaluatableGroupingFunction gf, List evals