array_agg() performance improvements#23716
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23716 +/- ##
==========================================
- Coverage 80.69% 80.69% -0.01%
==========================================
Files 1089 1089
Lines 368525 368602 +77
Branches 368525 368602 +77
==========================================
+ Hits 297392 297447 +55
- Misses 53414 53426 +12
- Partials 17719 17729 +10 ☔ 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_aggregate (33c5bb1) to 1e58928 (merge-base) diff using: array_agg File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagearray_agg — base (merge-base)
array_agg — branch
File an issue against this benchmark runner |
|
Seems like there's no impact in those benchmarks. Do you know if there are any other benchmarks that are using |
|
Let's ship the benchmarks in a preliminary PR, and then come back to this one. |
33c5bb1 to
1cd1305
Compare
|
run benchmarks array_agg |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/array_aggregate (1cd1305) to 1fcdef2 (merge-base) diff using: array_agg File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagearray_agg — base (merge-base)
array_agg — branch
File an issue against this benchmark runner |
|
Nice! |
Which issue does this PR close?
array_agg()performance #23715.Rationale for this change
Queries using
array_agg(DISTINCT col)were significantly slower than expected.Profiling revealed that
DistinctArrayAggAccumulator::update_batchwas allocating aheap-owned
Stringon every single input row — even for rows whose value was alreadypresent in the accumulator. For a typical low-cardinality workload (e.g. a column of ~25
database names across thousands of rows), this meant paying the full allocation cost for
every duplicate, which dominated the runtime.
What changes are included in this PR?
This PR applies the same deduplication strategy already used by
AggregateExecforGROUP BY: duplicate rows now cost only a hash probe with no heap allocation, and newdistinct values are appended to a single shared buffer rather than allocated individually.
The fix applies to all column types, not just strings, and
retract_batchsupport(required for sliding window frames such as
ROWS BETWEEN N PRECEDING AND CURRENT ROW)is fully preserved.
Are these changes tested?
Four unit tests were added to
DistinctArrayAggAccumulator— one each forUtf8,Int64,Float64, andDate32— to pin the deduplication contract across the mostcommon column types and serve as a regression guard for future changes. The existing
sliding window sqllogictest suite (
array_agg_sliding_window.slt) coversretract_batchcorrectness end-to-end and passes unchanged.
Two
update_batchmicro-benchmarks were added to measure the before/after on realisticdata: one with low cardinality (~25 distinct database names in 8 192 rows, modelling the
common production case) and one with high cardinality (~7 800 distinct values, modelling
the worst case where almost every row is new). Results on an 8 192-row batch:
Are there any user-facing changes?
No user-facing changes
No breaking changes to public APIs