ESQL: Split grouping functions based on their EVAL-ability #126597

bpintea · 2025-04-10T12:16:19Z

This splits the grouping functions in two: those that can be evaluated independently through the EVAL operator (BUCKET) and those that don't (like those that that are evaluated through an agg operator, CATEGORIZE).

Closes #124608

This splits the grouping functions in two groups: those that can be evaluated independently through the EVAL operator (BUCKET) and those that don't (like those that that are evaluated through an agg operator, CATEGORIZE).

elasticsearchmachine · 2025-04-10T12:16:44Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

Copilot

Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.

Files not reviewed (1)

x-pack/plugin/esql/qa/testFixtures/src/main/resources/categorize.csv-spec: Language not supported

Comments suppressed due to low confidence (1)

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/grouping/Categorize.java:39

[nitpick] Switching Categorize to extend NonEvaluatableGroupingFunction clarifies its intended usage. Please ensure that both documentation and error messages consistently reflect this change.

public class Categorize extends GroupingFunction.NonEvaluatableGroupingFunction {

...gin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java

bpintea · 2025-04-10T12:34:26Z

...asticsearch/xpack/esql/optimizer/rules/logical/ReplaceAggregateNestedExpressionWithEval.java

                        groupingChanged = true;
-                        var fieldAs = new Alias(as.source(), as.name(), cat.field(), null, true);


The nested expression alias name shouldn't be the same as the grouping function alias name.

bpintea · 2025-04-10T12:34:29Z

...asticsearch/xpack/esql/optimizer/rules/logical/ReplaceAggregateNestedExpressionWithEval.java

-                        var fieldAs = new Alias(as.source(), as.name(), cat.field(), null, true);
-                        var fieldAttr = fieldAs.toAttribute();
-                        evals.add(fieldAs);
-                        evalNames.put(fieldAs.name(), fieldAttr);


This should not be needed, as the expression provided to the grouping function can't show up stand-alone / aliased in the aggs.

bpintea · 2025-04-10T12:35:32Z

.../elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceAggregateAggExpressionWithEval.java

        AttributeMap.Builder<Expression> aliasesBuilder = AttributeMap.builder();
-        aggregate.forEachExpressionUp(Alias.class, a -> aliasesBuilder.put(a.toAttribute(), a.child()));


This iteration can be combined with the one below, so much more as they do complementary things on each iteration.

ivancea

LGTM, thanks for doing this refactor!

ivancea · 2025-04-10T13:10:16Z

...rc/main/java/org/elasticsearch/xpack/esql/expression/function/grouping/GroupingFunction.java

@@ -45,4 +41,20 @@ public BiConsumer<LogicalPlan, Failures> postAnalysisPlanVerification() {
        };
    }

+    public abstract static non-sealed class NonEvaluatableGroupingFunction extends GroupingFunction {


Nit: Should this add a final foldable()=false?

Maybe, but not sure. We can't fold these functions through EvaluatorMapper / with EVAL's operator, but we might maybe with another agg operator?

Gotcha. With the "evaluated outside of an aggregation" docs you added, it's also clear what this means specifically.

I still feel like a foldable()=true could lead to some rule doing something nasty, as "foldable()" doesn't specify "inside an eval" in any way, it's just foldable or not. But I can't think of a grouping causing trouble here, so maybe when it appears, we can do whatever we must

I still feel like a foldable()=true could lead to some rule doing something nasty

Indeed. The foldable() is false by default so one would have to actively switch it to true, which will indeed become immediately problematic.

ivancea · 2025-04-11T09:33:11Z

...asticsearch/xpack/esql/optimizer/rules/logical/ReplaceAggregateNestedExpressionWithEval.java

                        groupingChanged = true;
-                        var fieldAs = new Alias(as.source(), as.name(), cat.field(), null, true);


ivancea · 2025-04-11T10:05:01Z

...asticsearch/xpack/esql/optimizer/rules/logical/ReplaceAggregateNestedExpressionWithEval.java

+        GroupingFunction.NonEvaluatableGroupingFunction gf,
+        List<Alias> evals
+    ) {
+        int[] counter = new int[] { 0 };


I think we don't need this wrapped here (?)

Correct, 🙏 .

ivancea · 2025-04-11T10:24:45Z

...asticsearch/xpack/esql/optimizer/rules/logical/ReplaceAggregateNestedExpressionWithEval.java

+        List<Expression> newChildren = new ArrayList<>(gf.children().size());
+
+        for (Expression ex : gf.children()) {
+            if (ex instanceof Attribute == false) { // TODO: foldables shouldn't require eval'ing either


What is this TODO about? Are NonEvaluatableGroupingFunction expected to be potentially foldable?

I think we shouldn't need to have to transform a ... BY CATEGORIZE("value") into EVAL ref = "value" | ... BY CATEGORIZE(ref)

ivancea · 2025-04-11T10:25:17Z

...asticsearch/xpack/esql/optimizer/rules/logical/ReplaceAggregateNestedExpressionWithEval.java

+        // TODO: rm me
+        // af.children().forEach(e -> e.forEachDown(AggregateFunction.class, unused -> foundNestedAggs.set(Boolean.TRUE)));


ivancea · 2025-04-11T10:38:57Z

...rc/main/java/org/elasticsearch/xpack/esql/expression/function/grouping/GroupingFunction.java

@@ -45,4 +41,20 @@ public BiConsumer<LogicalPlan, Failures> postAnalysisPlanVerification() {
        };
    }

+    public abstract static non-sealed class NonEvaluatableGroupingFunction extends GroupingFunction {


Should we add some javadocs here? Maybe mentioning that "it's (currently?) used for stateful grouping functions, which make them effectively non-evaluatable before the agg, so its value can't be used inside the aggs".

Even if it has some high level examples, I think it will be useful for whoever has to reuse them in the future

Yes, added some docs.

bpintea · 2025-04-11T14:19:21Z

Thanks Ivan!

elasticsearchmachine · 2025-04-11T14:21:29Z

💔 Backport failed

Status	Branch	Result
❌	8.x	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 126597

…126715) This splits the grouping functions in two: those that can be evaluated independently through the EVAL operator (`BUCKET`) and those that don't (like those that that are evaluated through an agg operator, `CATEGORIZE`). Closes #124608 (cherry picked from commit 9784e0e)

Split grouping functions based on their EVAL-ability

8b91fbc

This splits the grouping functions in two groups: those that can be evaluated independently through the EVAL operator (BUCKET) and those that don't (like those that that are evaluated through an agg operator, CATEGORIZE).

bpintea added >refactoring auto-backport Automatically create backport pull requests when merged :Analytics/ES|QL AKA ESQL v8.19.0 v9.0.1 labels Apr 10, 2025

bpintea requested review from costin, ivancea and Copilot April 10, 2025 12:16

elasticsearchmachine added v9.1.0 Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels Apr 10, 2025

Copilot AI reviewed Apr 10, 2025

View reviewed changes

...gin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java Show resolved Hide resolved

bpintea and others added 2 commits April 10, 2025 14:17

Revert unintentional change

a35e7bc

[CI] Auto commit changes from spotless

fe92dca

bpintea commented Apr 10, 2025

View reviewed changes

Merge branch 'main' into refactor/grouping_funcs

8d0e6fa

ivancea approved these changes Apr 11, 2025

View reviewed changes

bpintea added 2 commits April 11, 2025 14:48

Review suggestions

1162f17

Merge branch 'main' into refactor/grouping_funcs

07777f8

bpintea removed the v9.0.1 label Apr 11, 2025

ivancea approved these changes Apr 11, 2025

View reviewed changes

bpintea merged commit 9784e0e into elastic:main Apr 11, 2025
17 checks passed

bpintea deleted the refactor/grouping_funcs branch April 11, 2025 14:19

elasticsearchmachine added the backport pending label Apr 11, 2025

bpintea removed the backport pending label Apr 14, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: Split grouping functions based on their EVAL-ability #126597

ESQL: Split grouping functions based on their EVAL-ability #126597

bpintea commented Apr 10, 2025

elasticsearchmachine commented Apr 10, 2025

Copilot AI left a comment

bpintea Apr 10, 2025

ivancea Apr 11, 2025

bpintea Apr 10, 2025

bpintea Apr 10, 2025

ivancea left a comment

ivancea Apr 10, 2025

bpintea Apr 11, 2025

ivancea Apr 11, 2025

bpintea Apr 11, 2025

ivancea Apr 11, 2025

ivancea Apr 11, 2025

bpintea Apr 11, 2025

ivancea Apr 11, 2025

bpintea Apr 11, 2025

ivancea Apr 11, 2025

bpintea Apr 11, 2025

ivancea Apr 11, 2025

bpintea Apr 11, 2025

bpintea commented Apr 11, 2025

elasticsearchmachine commented Apr 11, 2025

		groupingChanged = true;
		var fieldAs = new Alias(as.source(), as.name(), cat.field(), null, true);

		AttributeMap.Builder<Expression> aliasesBuilder = AttributeMap.builder();
		aggregate.forEachExpressionUp(Alias.class, a -> aliasesBuilder.put(a.toAttribute(), a.child()));

		// TODO: rm me
		// af.children().forEach(e -> e.forEachDown(AggregateFunction.class, unused -> foundNestedAggs.set(Boolean.TRUE)));

ESQL: Split grouping functions based on their EVAL-ability #126597

ESQL: Split grouping functions based on their EVAL-ability #126597

Conversation

bpintea commented Apr 10, 2025

elasticsearchmachine commented Apr 10, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivancea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpintea commented Apr 11, 2025

elasticsearchmachine commented Apr 11, 2025

💔 Backport failed