chore(ordered-partial-aggregate): move OrderedPartialAggregateStream to generators for readability - #23951
chore(ordered-partial-aggregate): move OrderedPartialAggregateStream to generators for readability#23951rluvaton wants to merge 3 commits into
OrderedPartialAggregateStream to generators for readability#23951Conversation
…` to generators for readability
|
@2010YOUY01 can you please run the benchmark that you think are the most appropriate here and review |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #23951 +/- ##
==========================================
+ Coverage 80.67% 80.69% +0.02%
==========================================
Files 1095 1095
Lines 372376 372447 +71
Branches 372376 372447 +71
==========================================
+ Hits 300397 300563 +166
+ Misses 54052 53913 -139
- Partials 17927 17971 +44 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thank you, this looks much better now. It should be good to go after verifying the benchmarks.
There is no benchmark suite targeting ordered aggregation now, but previously we have found regressions for ordered aggregation with Additionally, I tried a simple query, the performance number is good. select v1, avg(v1+1)
from generate_series(1000000000) as t1(v1)
group by v1; |
|
@adriangb Could you grant me access to the benchmark bot? Thanks. |
|
run benchmark tpcds |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing move-ordered-partial-stream-to-async (5c3ef5c) to 1c3232c (merge-base) diff using: tpcds File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
@2010YOUY01 you should make a pr like adriangb/datafusion-benchmarking#16 |
|
@2010YOUY01 can you please review? |
Which issue does this PR close?
N/A
Rationale for this change
Simplify the code by making it linear and with less state to hold in mind
What changes are included in this PR?
change
OrderedPartialAggregateStreaminto async generatorsAre these changes tested?
existing tests
Are there any user-facing changes?
no