alamb opened a new pull request, #23873: URL: https://github.com/apache/datafusion/pull/23873
## Which issue does this PR close? - Closes #. This PR does not close an issue. It adds test coverage for two gaps found while reviewing https://github.com/apache/datafusion/pull/23716. ## Rationale for this change While reviewing #23716 (which rewrites `DistinctArrayAggAccumulator` to use the arrow row format) I found two behaviors of `array_agg(DISTINCT ...)` that are correct on `main` today but that **nothing in the test suite pins down**: 1. **Dictionary encoded input.** There is no test anywhere in the repository that runs `array_agg(DISTINCT ...)` over a dictionary encoded column, so nothing checks that the returned list keeps its declared `List(Dictionary(..))` element type. Any change that round-trips values through a representation which flattens dictionaries silently produces a list whose child type no longer matches the schema the aggregate declared, and the query fails at execution time with ``` Arrow error: Invalid argument error: column types must match schema types, expected List(Dictionary(Int32, Utf8)) but found List(Utf8) at column index 0 ``` 2. **Memory used by `retract_batch`.** The sliding window coverage in `array_agg_sliding_window.slt` is good on *correctness*, but every table in it has five rows. That is far too small to notice an accumulator that never reclaims the storage of values which have been fully retracted out of the window frame: its footprint becomes a function of how many rows have streamed through it rather than of the frame's distinct cardinality. Both of these are things a reviewer has to notice by hand right now. Adding the tests makes CI notice them instead. ## What changes are included in this PR? Tests only, no functional change. **`datafusion/sqllogictest/test_files/aggregate.slt`** — a new `array_agg_distinct_dict_table` block next to the existing `array_agg_distinct_list_table` one, covering `array_agg(DISTINCT ...)` over a dictionary column: - values, for both `Dictionary(Int32, Utf8)` and `Dictionary(Int8, Int64)`, including `IGNORE NULLS` - `arrow_typeof(...)` of the result, so the element type is asserted directly rather than only implied - a dictionary nested inside a `struct` - the zero-input case, where the accumulator returns a null list — the element type has to agree with the non-empty case **`datafusion/functions-aggregate/src/array_agg.rs`** — a new `distinct_retract_memory_is_bounded` unit test. It emulates `ROWS BETWEEN CURRENT ROW AND CURRENT ROW` over 21,000 rows drawn from 10 distinct values, so every row enters the frame and immediately leaves it again and the live set never holds more than one value. It takes a baseline `size()` after a 1,000 row warm-up and asserts the accumulator has not more than doubled after another 20,000 rows. ## Are these changes tested? This PR is the tests. Both pass on `main`. To confirm they are not vacuous, I ran them against #23716, where each fails and localizes the corresponding problem: ``` 1. query failed: DataFusion error: Arrow error: Invalid argument error: column types must match schema types, expected List(Dictionary(Int32, Utf8)) but found List(Utf8) at column index 0 2. query failed: DataFusion error: Arrow error: Invalid argument error: column types must match schema types, expected List(Dictionary(Int32, Utf8)) but found List(Utf8) at column index 0 3. query failed: DataFusion error: Arrow error: Invalid argument error: column types must match schema types, expected List(Struct("f": Dictionary(Int32, Utf8))) but found List(Struct("f": Utf8)) at column index 0 ``` ``` size() must not grow with the number of retracted rows: 26809 bytes after 1000 rows, 820409 bytes after 21000 rows ``` ## Are there any user-facing changes? No. Tests only, no public API changes. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
