ESQL: Fix missing refs due to pruning renamed grouping columns #107328

alex-spies · 2024-04-10T17:08:14Z

Fix #107083, more specifically this remaining case.
Close #107166.

In some cases, CombineProjections prunes grouping columns if the column was only renamed. E.g.

...
| EVAL x = y, foo = bar
| STATS min(foo) BY x

// This is optimized to something equivalent to:

...
// EVAL pruned as it only renames vars
STATS `min(foo)` = min(bar) BY x

Notice how the x is still in the groupings. Confusingly, the query still got executed correctly in most cases.

This PR propagates the renaming x = y into the groupings. To be correct, we have to avoid propagating the same reference attribute twice into the grouping (otherwise layout errors occur), so this PR also prunes duplicate groupings:

...
| EVAL x = y, z = y
| STATS BY x, z

// This becomes something equivalent to

...
| STATS BY x = y, z = y
// and the corresponding Aggregate plan only has y in its groupings,
// and `x = y` and `z = y` in its aggregates.

elasticsearchmachine · 2024-04-10T17:08:38Z

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

elasticsearchmachine · 2024-04-10T17:08:38Z

Hi @alex-spies, I've created a changelog YAML for you.

alex-spies · 2024-04-11T07:19:13Z

@elasticmachine update branch

costin

Good catch.

costin · 2024-04-15T06:51:29Z

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

@@ -432,7 +432,7 @@ public void testCombineProjectionWithAggregationAndEval() {
        var limit = as(plan, Limit.class);
        var agg = as(limit.child(), Aggregate.class);
        assertThat(Expressions.names(agg.aggregates()), contains("s", "last_name", "first_name", "k"));
-        assertThat(Expressions.names(agg.groupings()), contains("last_name", "first_name", "k"));
+        assertThat(Expressions.names(agg.groupings()), contains("last_name", "first_name"));
    }


costin · 2024-04-15T06:56:04Z

...k/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java

+            for (Expression expr : upperGroupings) {
+                // All substitutions happen before; groupings must be attributes at this point.
+                Attribute attr = (Attribute) expr;
+                replaced.add((Attribute) aliases.resolve(attr, attr));


This will lead to a CCE - if there's always an attribute inside the alias (what about c = a + 1), make the aliases be a AttributeMap<Attribute>

This shouldn't cause CCE in practice, but I can use an AttributeMap<Attribute>. (Still need to cast the expressions under the projection's aliases to attributes, then.)

(I'd prefer not having to cast at all, but our Aggs/Projections don't adequately represent the fact that we don't have general expressions here, but attributes.)

...k/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java

costin · 2024-04-15T06:59:11Z

...k/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java

@@ -482,6 +487,27 @@ private static List<NamedExpression> combineProjections(
            return replaced;
        }

+        private static List<Expression> combineUpperGroupingsAndLowerProjections(


This method looks quite similar to combineProjections() which is the reused inside projectAggregations().
Have you looked into reusing them on groupings?

I initially wanted to re-use this, but I need to make some assumptions (in the form of casting expressions) that are only true if the upper expressions are groupings. I can probably still combine these, but that might become confusing to read.

bpintea

Lgtm.

bpintea · 2024-04-16T15:42:34Z

...k/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java

+                        a.source(),
+                        p.child(),
+                        // After substitutions groupings can only contain attributes.
+                        combineUpperGroupingsAndLowerProjections((List<? extends Attribute>) (List<?>) a.groupings(), p.projections()),


Suggested change

combineUpperGroupingsAndLowerProjections((List<? extends Attribute>) (List<?>) a.groupings(), p.projections()),

combineUpperGroupingsAndLowerProjections((List<? extends Attribute>) a.groupings(), p.projections()),

Ah, sorry, this suggestion is wrong.
Why the double cast though and not call the combining function with a List<Expression> and cast downstream, since groupings are expected as attributes? Same cast as for the projection list.

Yeah, the double casting is ugly indeed, but necessary as long as we have a list.

Initial version cast further downstream, but moved that up and required Attribute instead of Expression per Costin's request.

In case our assumptions about the type end up being wrong, it's going to blow up no matter what and roughly in the same spot; IMHO the way to improve this properly is to bake the invariant that groupings are attributes into our types; but that requires a refactoring that is currently not possible due to reliance on the ql project.

Nevermind, explicitly casting each grouping now per Andrei's suggestion.

astefan

Left a suggestion. Otherwise LGTM

...k/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java

Co-authored-by: Andrei Stefan <astefan@users.noreply.github.com>

elasticsearchmachine · 2024-04-17T14:06:14Z

💔 Backport failed

Status	Branch	Result
❌	8.13	Commit could not be cherrypicked due to conflicts

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

alex-spies · 2024-04-18T08:23:03Z

💚 All backports created successfully

Status	Branch	Result
✅	8.13

Questions ?

Please refer to the Backport tool documentation

…ic#107328) Sometimes, CombineProjections does not correctly update an aggregation's groupings when combining with a preceding projection. Fix this by resolving any aliases used in the groupings and de-duplicating them. --------- Co-authored-by: Andrei Stefan <astefan@users.noreply.github.com> (cherry picked from commit adaa476) # Conflicts: # x-pack/plugin/esql/qa/testFixtures/src/main/resources/stats.csv-spec # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java

…) (#107599) Sometimes, CombineProjections does not correctly update an aggregation's groupings when combining with a preceding projection. Fix this by resolving any aliases used in the groupings and de-duplicating them. --------- Co-authored-by: Andrei Stefan <astefan@users.noreply.github.com> (cherry picked from commit adaa476) # Conflicts: # x-pack/plugin/esql/qa/testFixtures/src/main/resources/stats.csv-spec # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java

alex-spies added 2 commits April 10, 2024 18:24

Fix leftover references to pruned columns in aggs

5d81019

Update LogicalPlanOptimizerTests

56e8f79

alex-spies added >bug :Analytics/ES|QL AKA ESQL v8.14.0 v8.13.3 labels Apr 10, 2024

alex-spies requested a review from costin April 10, 2024 17:08

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Apr 10, 2024

alex-spies added 2 commits April 10, 2024 19:08

Update docs/changelog/107328.yaml

d5ff7e9

Add simple test for duplicate removal

be4bd17

Merge branch 'main' into esql-fix-missing-refs-in-aggs

31bc504

alex-spies added the auto-backport Automatically create backport pull requests when merged label Apr 11, 2024

alex-spies mentioned this pull request Apr 12, 2024

ESQL: median, count and count_distinct over constants #107414

Merged

alex-spies requested review from astefan, luigidellaquila and bpintea April 15, 2024 06:29

costin reviewed Apr 15, 2024

View reviewed changes

costin requested review from fang-xing-esql and removed request for luigidellaquila April 15, 2024 07:01

Address feedback

e904a41

alex-spies requested a review from costin April 16, 2024 14:41

bpintea approved these changes Apr 16, 2024

View reviewed changes

astefan approved these changes Apr 17, 2024

View reviewed changes

...k/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java Outdated Show resolved Hide resolved

alex-spies commented Apr 17, 2024

View reviewed changes

...k/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java Outdated Show resolved Hide resolved

alex-spies and others added 2 commits April 17, 2024 15:07

Apply Andrei's suggestion

75ec8cf

Co-authored-by: Andrei Stefan <astefan@users.noreply.github.com>

Improve comment

7c519c8

alex-spies merged commit adaa476 into elastic:main Apr 17, 2024
14 checks passed

alex-spies deleted the esql-fix-missing-refs-in-aggs branch April 17, 2024 14:04

elasticsearchmachine added the backport pending label Apr 17, 2024

alex-spies mentioned this pull request Apr 18, 2024

[8.13] ESQL: Fix missing refs due to pruning renamed grouping columns (#107328) #107599

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: Fix missing refs due to pruning renamed grouping columns #107328

ESQL: Fix missing refs due to pruning renamed grouping columns #107328

alex-spies commented Apr 10, 2024

elasticsearchmachine commented Apr 10, 2024

elasticsearchmachine commented Apr 10, 2024

alex-spies commented Apr 11, 2024

costin left a comment

costin Apr 15, 2024

costin Apr 15, 2024

alex-spies Apr 16, 2024

costin Apr 15, 2024

alex-spies Apr 16, 2024

bpintea left a comment

bpintea Apr 16, 2024

bpintea Apr 16, 2024

alex-spies Apr 17, 2024

alex-spies Apr 17, 2024

astefan left a comment

elasticsearchmachine commented Apr 17, 2024

alex-spies commented Apr 18, 2024

	combineUpperGroupingsAndLowerProjections((List<? extends Attribute>) (List<?>) a.groupings(), p.projections()),
	combineUpperGroupingsAndLowerProjections((List<? extends Attribute>) a.groupings(), p.projections()),

ESQL: Fix missing refs due to pruning renamed grouping columns #107328

ESQL: Fix missing refs due to pruning renamed grouping columns #107328

Conversation

alex-spies commented Apr 10, 2024

elasticsearchmachine commented Apr 10, 2024

elasticsearchmachine commented Apr 10, 2024

alex-spies commented Apr 11, 2024

costin 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

bpintea 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

astefan left a comment

Choose a reason for hiding this comment

elasticsearchmachine commented Apr 17, 2024

💔 Backport failed

alex-spies commented Apr 18, 2024

💚 All backports created successfully

Questions ?