ESQL: Retain aggregate when grouping #126598
Conversation
elasticsearchmachine
added
needs:triage
limotova
added
:Analytics/ES|QL
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
buildkite test this |
There was a problem hiding this comment.
Thanks @kanoshiou, I had a first look a the PR and I added a couple of comments
| @@ -84,7 +86,8 @@ public LogicalPlan apply(LogicalPlan plan) { | |||
| ); | |||
| } else { | |||
| // Aggs cannot produce pages with 0 columns, so retain one grouping. | |||
| remaining = List.of(Expressions.attribute(aggregate.groupings().get(0))); | |||
| Attribute attribute = Expressions.attribute(aggregate.groupings().getFirst()); | |||
| remaining = aggregate.aggregates().stream().map(v -> new Alias(v.source(), v.name(), attribute)).toList(); | |||
There was a problem hiding this comment.
Is there a specific reason why we are creating a list the same size as the original one?
Couldn't it just be
NamedExpression firstAggregate = aggregate.aggregates().getFirst();
remaining = List.of(new Alias(firstAggregate.source(), firstAggregate.name(), attribute));
It won't make a big difference at execution time, but a high number of attributes makes a difference at planning time
There was a problem hiding this comment.
You are right, I did't think that far.
x-pack/plugin/esql/qa/testFixtures/src/main/resources/eval.csv-spec
Outdated
Show resolved
Hide resolved
|
buildkite test this |
|
Apparently the CI is not happy, Also, probably we should preserve the NameID of the Alias, something like It won't make a huge difference probably, as that alias is not further referenced in the plan, but at least we avoid to create a new NameID |
|
Thank you for reviewing @luigidellaquila! Fix is on the way😊. |
|
buildkite test this |
luigidellaquila
added
auto-backport
There was a problem hiding this comment.
LGTM, thanks @kanoshiou!
It will only need a merge with main to resolve conflicts
# 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 # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java
|
Thank you @luigidellaquila! Conflicts have now been resolved. |
|
buildkite test this |
|
This is weird. I have iterated the test over 100 times and can't reproduce it. |
| | stats avg = avg(salary) by gender | ||
| | keep avg | ||
| | eval avg = 12 |
There was a problem hiding this comment.
nitpicky suggestion: the added tests 1. keep one or several aggregates and then 2. override them. We could also add tests that keep groupings in step 1, and consider aliased groupings for good measure.
E.g.
from employees
| stats avg(salary) by g = gender
| keep g
| eval g = 12
There was a problem hiding this comment.
Thank you for the helpful suggestion. I hadn’t considered keeping the aliased groupings, but I’ll update those tests.
There was a problem hiding this comment.
Thanks @kanoshiou ! I think a test case that doesn't keep any of the aggregates would be nice, too, like the one I suggested above - but this PR is already in a very good spot and can happily be merged as-is (once the small test regression is fixed that @luigidellaquila mentioned).
|
buildkite test this |
|
Thanks @kanoshiou, the PR is almost ready to merge; it just needs a little fix to a regression introduced in |
oops🥺 |
|
sorry, needs another merge with main due to conflicts 😓 |
No worries, let me handle it!😊 |
# Conflicts: # x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/generative/GenerativeRestTest.java
|
buildkite test this |
💔 Backport failed
You can use sqren/backport to manually backport by running |
|
This will need a manual backport to 8.x branch. |
|
Thank you @luigidellaquila @alex-spies ! |
Co-authored-by: kanoshiou <[email protected]>
This PR resolves #126026.