perf: add a GroupsAccumulator for array_agg(DISTINCT) by zhuqi-lucas · Pull Request #26357 · apache/datafusion

zhuqi-lucas · 2026-10-15T06:03:52Z

Which issue does this PR close?

Closes Add a GroupsAccumulator for array_agg(DISTINCT) #26347.

Rationale for this change

array_agg(DISTINCT x) had no GroupsAccumulator. Grouped queries fell back to GroupsAccumulatorAdapter, which keeps one DistinctArrayAggAccumulator per group, each with its own hash table and encoded rows. With many groups this dominates memory and CPU.

What changes are included in this PR?

New DistinctArrayAggGroupsAccumulator:

Each distinct value is encoded once with a RowConverter into a Rows buffer shared by all groups, so a value that appears in many groups is stored once.
Groups record (group_index, value_index) pairs, deduplicated in one hash set.
evaluate builds the ListArray with a counting sort by group.
It also covers array_agg(DISTINCT x ORDER BY x), sorting each group by the encoded rows.
EmitTo::First drops values only referenced by emitted groups.

Small refactors: the DISTINCT / ORDER BY check is shared by the row and groups paths (distinct_sort_options), and the one-row-per-list convert_to_state is shared with ArrayAggGroupsAccumulator.

state_compat now compares list results as multisets for DISTINCT without ORDER BY, since the element order is not defined there. The row accumulator returns hash table order.

Are these changes tested?

Unit tests cover dedup across batches, nulls with and without IGNORE NULLS, FILTER, ORDER BY DESC, EmitTo::First, state/merge, convert_to_state, dictionary types, and a differential check against DistinctArrayAggAccumulator.
state_compat passes with the new accumulator included.
array_agg.slt gains grouped cases mixing DISTINCT with count(*), FILTER, ORDER BY and IGNORE NULLS. The existing aggregate slt files pass.

Benchmarks

Measured locally with a criterion benchmark (not included here, happy to add it): one 8192-row Utf8 batch, update_batch plus evaluate, the new accumulator against GroupsAccumulatorAdapter over DistinctArrayAggAccumulator. Apple M-series:

groups	input	groups accumulator	adapter (before)	speedup
100	~25 distinct	154 µs	251 µs	1.6x
100	~7800 distinct	384 µs	969 µs	2.5x
1000	~25 distinct	173 µs	1.42 ms	8.2x
1000	~7800 distinct	382 µs	2.26 ms	5.9x
8192	~25 distinct	279 µs	10.2 ms	36x
8192	~7800 distinct	414 µs	10.4 ms	25x

Live heap after update_batch, measured with a counting global allocator: 65 536 Utf8 rows in 8 batches. size() tracks these within ~10% for both.

groups	input	groups accumulator	adapter (before)
100	~25 distinct	11 KB	720 KB
1000	~7800 distinct	2.2 MB	9.9 MB
8192	~25 distinct	1.7 MB	18.3 MB
65536	~7800 distinct	2.2 MB	100.7 MB

Memory now grows with the number of distinct values instead of the number of groups.

Are there any user-facing changes?

No API changes. array_agg(DISTINCT x) without ORDER BY may return elements in a different (still unspecified) order.

array_agg(DISTINCT x) had no GroupsAccumulator, so grouped queries went through GroupsAccumulatorAdapter with one DistinctArrayAggAccumulator (and its own hash table and encoded rows) per group. DistinctArrayAggGroupsAccumulator encodes each distinct value once with a RowConverter into a Rows buffer shared by all groups, and deduplicates (group_index, value_index) pairs in a single hash set. It also covers array_agg(DISTINCT x ORDER BY x), sorting each group by the encoded rows. state_compat now compares list results as multisets for DISTINCT without ORDER BY, where the element order is not defined. Closes apache#26347.

Copilot

🟡 Changes recommended

Missing DISTINCT instrumentation breaks existing metric expectations, and unnecessary encoding introduces CPU and memory regressions.

3 open findings

What changed in this PR

Adds a native grouped accumulator for array_agg(DISTINCT) to reduce per-group memory and CPU overhead.

Changes:

Shares distinct-value storage across groups and supports ordering and partial emission.
Reuses state-conversion helpers and adjusts unordered DISTINCT result comparisons.
Adds unit and SQL coverage for grouped DISTINCT aggregation.

File	Description
datafusion/sqllogictest/test_files/array_agg.slt	Adds grouped DISTINCT query coverage.
datafusion/functions-aggregate/tests/state_compat.rs	Compares unordered DISTINCT lists as multisets.
datafusion/functions-aggregate/src/array_agg.rs	Implements the native accumulator, shared helpers, and unit tests.

🧠 Review effort: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+impl GroupsAccumulator for DistinctArrayAggGroupsAccumulator {
+    fn update_batch(


+        if rows.is_empty() {
+            return Ok(());
+        }
+        self.insert(input, rows)


+        if rows.is_empty() {
+            return Ok(());
+        }
+        self.insert(lists.values(), rows)


…DISTINCT) - Report the DISTINCT submetric from the native groups accumulator, and keep it across a full emit. - Gather filtered or null-skipped rows before encoding them, and encode only the offset span of a sliced state list when merging. - Build the adapter explicitly in the tests that cover its metrics, and give the spilling test enough groups to spill with the native accumulator.

codecov-commenter · 2026-10-15T08:42:05Z

Codecov Report

❌ Patch coverage is 88.86827% with 60 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.88%. Comparing base (5009f42) to head (d29b125).

Files with missing lines	Patch %	Lines
datafusion/functions-aggregate/src/array_agg.rs	88.59%	8 Missing and 52 partials ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #26357    +/-   ##
========================================
  Coverage   82.87%   82.88%            
========================================
  Files        1153     1153            
  Lines      457101   457617   +516     
  Branches   457101   457617   +516     
========================================
+ Hits       378818   379278   +460     
- Misses      55268    55273     +5     
- Partials    23015    23066    +51

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

jayzhan211

Thanks @zhuqi-lucas , overall LGTM

jayzhan211 · 2026-10-16T12:34:46Z

+            + self.values.as_ref().map_or(0, Rows::size)
+            + self.value_map_size
+            + self.pairs.capacity() * size_of::<(u32, u32)>()
+            + self.pair_set.capacity() * (size_of::<(u32, u32)>() + 1)


Suggested change

+ self.pair_set.capacity() * (size_of::<(u32, u32)>() + 1)

+ estimate_memory_size::<(u32, u32)>(self.pair_set.capacity(), 0).unwrap_or_default()

Should we use this one?

Copilot AI balanced review requested due to automatic review settings October 15, 2026 06:03

github-actions Bot added sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation labels Oct 15, 2026

zhuqi-lucas marked this pull request as draft October 15, 2026 06:04

Copilot AI reviewed Oct 15, 2026

View reviewed changes

Copilot started reviewing on behalf of zhuqi-lucas October 15, 2026 07:27

View session

github-actions Bot added core Core DataFusion crate physical-plan Changes to the physical-plan crate labels Oct 15, 2026

zhuqi-lucas marked this pull request as ready for review October 16, 2026 06:04

Merge branch 'main' into array-agg-distinct-groups-accumulator

d29b125

zhuqi-lucas requested review from alamb and kosiew October 16, 2026 07:15

zhuqi-lucas mentioned this pull request Oct 16, 2026

Add a GroupsAccumulator for array_agg with ORDER BY #26409

Open

zhuqi-lucas requested a review from jayzhan211 October 16, 2026 07:18

jayzhan211 approved these changes Oct 16, 2026

View reviewed changes

Navigation Menu

Repository navigation

perf: add a GroupsAccumulator for array_agg(DISTINCT) - #26357

perf: add a GroupsAccumulator for array_agg(DISTINCT)#26357
zhuqi-lucas wants to merge 3 commits into
apache:mainfrom
zhuqi-lucas:array-agg-distinct-groups-accumulator

zhuqi-lucas commented Oct 15, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

codecov-commenter commented Oct 15, 2026 •

edited

Loading

Uh oh!

jayzhan211 left a comment

Uh oh!

jayzhan211 Oct 16, 2026

Uh oh!

jayzhan211 Oct 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		impl GroupsAccumulator for DistinctArrayAggGroupsAccumulator {
		fn update_batch(

	+ self.pair_set.capacity() * (size_of::<(u32, u32)>() + 1)
	+ estimate_memory_size::<(u32, u32)>(self.pair_set.capacity(), 0).unwrap_or_default()

Conversation

zhuqi-lucas commented Oct 15, 2026

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Benchmarks

Are there any user-facing changes?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

codecov-commenter commented Oct 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jayzhan211 left a comment

Choose a reason for hiding this comment

Uh oh!

jayzhan211 Oct 16, 2026

Choose a reason for hiding this comment

Uh oh!

jayzhan211 Oct 16, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Oct 15, 2026 •

edited

Loading