-
Notifications
You must be signed in to change notification settings - Fork 25.3k
ESQL: Retain aggregate when grouping #126598
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 9 commits
b808bba
46faa07
cab7c3e
7745195
3cabf9d
65791cc
a18e167
ca6f19b
9f46ed3
df357aa
44916c0
4d34c51
8b93e2a
82198fc
100ce40
f1b5524
5d021b0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
pr: 126598 | ||
summary: "ESQL: Retain aggregate when grouping" | ||
area: ES|QL | ||
type: bug | ||
issues: | ||
- 126026 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -636,4 +636,55 @@ row foo = "Bar" | where foo rlike "(?i)Ba.*" | |
foo:keyword | ||
; | ||
|
||
evalAfterAvgGroupingUsingSameName | ||
required_capability: retain_aggregate_when_grouping | ||
from employees | ||
| stats avg = avg(salary) by gender | ||
| keep avg | ||
| eval avg = 12 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). |
||
; | ||
|
||
avg:integer | ||
12 | ||
12 | ||
12 | ||
; | ||
|
||
evalAfterGroupingUsingSameName | ||
required_capability: retain_aggregate_when_grouping | ||
row foo = [10,11,9], bar = [1,2,3] | ||
| mv_expand foo | ||
| mv_expand bar | ||
| stats this = max(foo) by bar | ||
| keep this | ||
| eval this = 12 | ||
; | ||
|
||
this:integer | ||
12 | ||
12 | ||
12 | ||
; | ||
|
||
evalAfterGroupingUsingSameName2 | ||
required_capability: retain_aggregate_when_grouping | ||
from employees | ||
| stats count = count(emp_no) by gender, is_rehired | ||
| keep count | ||
| rename count as x | ||
| keep x | ||
| eval x = 12 | ||
; | ||
|
||
x:integer | ||
12 | ||
12 | ||
12 | ||
12 | ||
12 | ||
12 | ||
12 | ||
12 | ||
12 | ||
; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,8 @@ | |
import org.elasticsearch.compute.data.Block; | ||
import org.elasticsearch.compute.data.BlockUtils; | ||
import org.elasticsearch.index.IndexMode; | ||
import org.elasticsearch.xpack.esql.core.expression.Alias; | ||
import org.elasticsearch.xpack.esql.core.expression.Attribute; | ||
import org.elasticsearch.xpack.esql.core.expression.AttributeSet; | ||
import org.elasticsearch.xpack.esql.core.expression.EmptyAttribute; | ||
import org.elasticsearch.xpack.esql.core.expression.Expressions; | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a specific reason why we are creating a list the same size as the original one?
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right, I did't think that far. |
||
p = aggregate.with(aggregate.groupings(), remaining); | ||
} | ||
} else { | ||
|
Uh oh!
There was an error while loading. Please reload this page.