Support GroupsAccumulator for Avg duration
#15748
Conversation
github-actions
bot
added
sqllogictest
shruti2522
force-pushed
the
avg_duration_ga
branch
from
d833b56 to
2506180
Compare
shruti2522
force-pushed
the
avg_duration_ga
branch
from
293be12 to
2f07061
Compare
|
Ready for review @alamb |
There was a problem hiding this comment.
Thanks @shruti2522. I found that the tests will be passed in the current main branch. Could you double-check if we still need this change?
| # avg_duration (GroupsAccumulator) | ||
|
|
||
| statement ok | ||
| create table duration as values | ||
| (arrow_cast(10, 'Duration(Second)'), arrow_cast(100, 'Duration(Millisecond)'), 'a', 1), | ||
| (arrow_cast(20, 'Duration(Second)'), arrow_cast(200, 'Duration(Millisecond)'), 'a', 2), | ||
| (arrow_cast(30, 'Duration(Second)'), arrow_cast(300, 'Duration(Millisecond)'), 'b', 1), | ||
| (arrow_cast(40, 'Duration(Second)'), arrow_cast(400, 'Duration(Millisecond)'), 'b', 2), | ||
| (arrow_cast(50, 'Duration(Second)'), arrow_cast(500, 'Duration(Millisecond)'), 'c', 1), | ||
| (arrow_cast(60, 'Duration(Second)'), arrow_cast(600, 'Duration(Millisecond)'), 'c', 2); | ||
|
|
||
| query T??I | ||
| SELECT column3, avg(column1), avg(column2), column4 FROM duration GROUP BY column3, column4 ORDER BY column3, column4; |
There was a problem hiding this comment.
Actually, the following tests will be passed after #15468 in the current main branch.
|
I think the groups accumulator will result in faster performance, not new functionality. @shruti2522 maybe you can verify this with |
Thanks for the explanation. If it's a performance improvement, it's better to run some benchmark or add the benchmark case in |
|
Got it @alamb @goldmedal, will test and share results soon. |
|
I wrote up a ticket about how to make a benchmark: |
Thanks, @alamb. I've been a bit caught up with semester exams lately, but I’ll get it done by tomorrow. |
Thank you so much! I totally re: school (which should take top priority for sure) |
Which issue does this PR close?
AVGfor durations #15458 .Rationale for this change
What changes are included in this PR?
Implement
GroupsAccumulatorfor avg duration calculationAre these changes tested?
Yes
Are there any user-facing changes?
Yes