chore(ordered-partial-aggregate): move OrderedPartialAggregateStream to generators for readability - #23951
Conversation
…` 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.75% +0.08%
==========================================
Files 1095 1096 +1
Lines 372376 373506 +1130
Branches 372376 373506 +1130
==========================================
+ Hits 300397 301619 +1222
+ Misses 54052 53875 -177
- Partials 17927 18012 +85 ☔ 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? |
Co-authored-by: Yongting You <2010youy01@gmail.com>
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