alamb commented on PR #23873: URL: https://github.com/apache/datafusion/pull/23873#issuecomment-5074498907
### What is new vs. what was just moved To make this easier to review: **the only new tests are the last 52 lines of `array_agg.slt`** (the `array_agg_distinct_dict_table` block) plus one unit test in `array_agg.rs`. Everything else is relocation. | File | What changed | |---|---| | `datafusion/sqllogictest/test_files/aggregate.slt` | Pure deletion — 510 lines, eight separate regions, no edits | | `datafusion/sqllogictest/test_files/array_agg.slt` | New file: those deleted regions verbatim, regrouped into sections, plus the `aggregate_test_100` setup and the new dictionary block at the end | | `datafusion/functions-aggregate/src/array_agg.rs` | New `distinct_retract_memory_is_bounded` unit test | I verified the relocation mechanically: every line in `array_agg.slt` above the dictionary block — excluding the license header, the file-level comment and the four section headers — appears verbatim in the pre-move `aggregate.slt`, with zero lines unaccounted for. Query/statement directive counts go 1471 → 1388 + 84; the +1 is the duplicated `aggregate_test_100` setup statement. #### Why the two new tests Both cover behavior that is correct on `main` today but that nothing pins down, found while reviewing #23716: 1. **Dictionary encoded input.** No test anywhere ran `array_agg(DISTINCT ...)` over a dictionary column, so nothing checked that the returned list keeps its declared `List(Dictionary(..))` element type. Anything that round-trips values through a representation which flattens dictionaries produces a child type that no longer matches the declared schema, and the query fails at execution with `column types must match schema types, expected List(Dictionary(Int32, Utf8)) but found List(Utf8)`. 2. **`retract_batch` memory.** `array_agg_sliding_window.slt` covers sliding-frame *correctness* well, but every table in it has five rows — far too small to notice an accumulator that never reclaims values fully retracted out of the frame. Against #23716 the dictionary block produces 3 failures and the unit test reports `size() must not grow with the number of retracted rows: 26809 bytes after 1000 rows, 820409 bytes after 21000 rows`. #### Cases deliberately left in `aggregate.slt` Where `array_agg` is the vehicle rather than the subject: the `SingleDistinctToGroupBy` two-phase rewrite trio (one of the three uses `sum`, so splitting the group would read worse), `aggregate_duration_array_agg` (depends on the timestamp table built much earlier in the file), the `stream_test` ordered aggregates (cover `array_agg`/`first_value`/`last_value`/`nth_value`/`string_agg` together), and the nested-aggregate rejection case in the `nested_agg_t` group. Sliding window frames stay in `array_agg_sliding_window.slt`. -- 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]
