test: cover array_agg(DISTINCT) on dictionaries and bounded retract_batch memory#23873
Open
alamb wants to merge 2 commits into
Open
test: cover array_agg(DISTINCT) on dictionaries and bounded retract_batch memory#23873alamb wants to merge 2 commits into
array_agg(DISTINCT) on dictionaries and bounded retract_batch memory#23873alamb wants to merge 2 commits into
Conversation
github-actions
Bot
added
sqllogictest
alamb
force-pushed
the
test/array_agg_distinct_coverage
branch
from
July 24, 2026 20:29
5da1f62 to
79fc2c4
Compare
… memory Adds two pieces of coverage for `DistinctArrayAggAccumulator` that are missing today: 1. `array_agg(DISTINCT ...)` over a dictionary encoded column. There is currently no test anywhere that aggregates a dictionary with DISTINCT, so nothing pins the requirement that the returned list keeps its declared `List(Dictionary(..))` element type. Covers the plain, nested-in-a-struct and zero-input cases. 2. A `retract_batch` test asserting that `size()` does not grow with the number of rows that have passed through the accumulator. The existing sliding window cases in `array_agg_sliding_window.slt` use five row tables, which is too small to notice an accumulator that never reclaims retracted values. Both tests pass on main and describe behavior that is already correct. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
alamb
force-pushed
the
test/array_agg_distinct_coverage
branch
from
July 24, 2026 20:33
79fc2c4 to
f6fa5cf
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23873 +/- ##
==========================================
- Coverage 80.71% 80.70% -0.02%
==========================================
Files 1090 1090
Lines 370370 370358 -12
Branches 370370 370358 -12
==========================================
- Hits 298954 298887 -67
- Misses 53595 53648 +53
- Partials 17821 17823 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
alamb
commented
Jul 24, 2026
| OVER (ROWS BETWEEN 4 PRECEDING AND CURRENT ROW) | ||
| FROM aggregate_test_100 | ||
|
|
||
| # array agg can use order by |
Contributor
Author
There was a problem hiding this comment.
I moved these tests verbatim to datafusion/sqllogictest/test_files/array_agg.slt and added several new ones at the end
|
|
||
| query error Error during planning: ORDER BY and WITHIN GROUP clauses cannot be used together in the same aggregate function | ||
| SELECT array_agg(a_varchar order by a_varchar) WITHIN GROUP (ORDER BY a_varchar) | ||
| FROM (VALUES ('a'), ('d'), ('c'), ('a')) t(a_varchar); |
Contributor
Author
There was a problem hiding this comment.
New Coverage starts here
alamb
marked this pull request as ready for review
July 24, 2026 21:19
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
array_agg()performance improvements #23716Rationale for this change
It adds test coverage for two gaps found while reviewing #23716.
What changes are included in this PR?
Tests only, no functional change.
Note I moved
array_aggcases outaggregate.sltas it is already more than 9k lines longAre these changes tested?
They are only tests
Are there any user-facing changes?
No. Tests only, no public API changes.