perf: avoid scanning array_agg state in size - #24097
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24097 +/- ##
==========================================
- Coverage 80.90% 80.90% -0.01%
==========================================
Files 1102 1102
Lines 376807 377137 +330
Branches 376807 377137 +330
==========================================
+ Hits 304874 305140 +266
- Misses 53775 53793 +18
- Partials 18158 18204 +46 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
run benchmarks array_agg |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/array-agg-constant-time-size (5199f58) to 31ffab1 (merge-base) diff Run configurationrun benchmark array_aggResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing perf/array-agg-constant-time-size (5199f58) to 31ffab1 (merge-base) diff Run configurationrun benchmark array_aggCPU Details (lscpu)Details
Resource Usagearray_agg — base (merge-base)
array_agg — branch
File an issue against this benchmark runner |
|
@lyne7-sc Looks like the benchmarks show some significant regressions. I'm guessing this is because they don't call |
Which issue does this PR close?
Rationale for this change
ArrayAggAccumulator::size()andArrayAggGroupsAccumulator::size()scanned all retained batches on every call. Since aggregate operators callsize()for each input batch, memory accounting became increasingly expensive as the accumulated state grew.What changes are included in this PR?
Track retained array and entry memory incrementally when accumulator state is updated, retracted, cleared, merged, or partially emitted.
This avoids repeatedly scanning accumulated state in
size()while preserving the existing memory estimation behavior.Are these changes tested?
Yes. Unit tests cover incremental memory tracking.
Are there any user-facing changes?
No. This only improves internal memory accounting.