Doris-Breakwater commented on issue #70383: URL: https://github.com/apache/doris/issues/70383#issuecomment-6728871997
Source inspection confirms the allocation-before-deduplication problem. This is a BE aggregation memory-amplification issue that can cause query memory-limit failures with duplicate-heavy input. The evidence supports retaining unnecessary bytes during the arena's lifetime; it does not establish a leak after arena destruction or a result-correctness failure. Verified from source: - At [`41cd2b473e9`, string `collect_set` add/merge](https://github.com/apache/doris/blob/41cd2b473e9d9bb26be089f302cdfdfe6e107eaf/be/src/exprs/aggregate/aggregate_function_collect.h#L135-L155), `arena.insert` precedes set insertion. The outer `add` checks the distinct-count limit first, but duplicates keep allocating while cardinality remains below that limit. Repeated overlapping merges also copy keys already present in the destination. - The [generic DISTINCT states](https://github.com/apache/doris/blob/41cd2b473e9d9bb26be089f302cdfdfe6e107eaf/be/src/exprs/aggregate/aggregate_function_distinct.h#L140-L248) have the reported behavior: single-argument add copies before insertion in both stable and unstable modes; multi-argument add serializes every row without rolling back rejected duplicates. Generic merge copies before insertion and is supported only in unstable mode. `deserialize_and_merge` routes through that merge, so fixing add alone is insufficient. - [`Arena::insert`, `rollback`, and chunk management](https://github.com/apache/doris/blob/41cd2b473e9d9bb26be089f302cdfdfe6e107eaf/be/src/core/arena.h#L146-L286) explain the retention: failed hash insertion does not undo allocation. Thus add-path payload consumption follows submitted row bytes, while merge-path consumption follows incoming state-key bytes, even when destination cardinality stops growing. - I compared the five affected method bodies against tag `4.1.4` (commit `ad35a140c7fd`), tag `4.1.5-release` (`917b20be8387`), and the current `branch-4.1` revision `3f4002e778bd`: their allocation/insertion logic matches the reported master revision. This confirms source exposure, not independently reproduced runtime measurements. The proposed fix is consistent with the ownership requirements. For single keys and merge, probe using the borrowed bytes and copy into the destination arena only inside the successful `lazy_emplace` constructor callback. Stored keys must still own arena-backed bytes after the input column, source state, or serialized buffer disappears. For multi-column keys, serialize first and roll back only on failed insertion, in both stable modes. Preserve the first-insertion index in the stable map. The existing [`AggregateFunctionUniqVariant::add`](https://github.com/apache/doris/blob/41cd2b473e9d9bb26be089f302cdfdfe6e107eaf/be/src/exprs/aggregate/aggregate_function_uniq.h#L295-L307) provides the rollback pattern, including checking that the returned pointer equals the serialized key's start. Rollback makes the latest range reusable; it does not free arena chunks, and serialization that crosses chunks can leave copied prefixes behind. Please open the offered PR with the unit tests. Review should cover duplicate-heavy add, repeated overlapping merge and deserialize-and-merge, source-buffer lifetime, stable ordering, multiple arguments, NULL/empty strings, limit boundaries, all-unique input, and keys that force arena chunk growth. Check both retained allocation and result semantics; use `arena.used_size()` for focused payload assertions rather than expecting total query peak memory to equal distinct payload size. One reproduction detail needs clarification before accepting the grouped benchmark numbers: `count(*)` over the grouped subquery does not consume its aggregate result. [Column pruning](https://github.com/apache/doris/blob/41cd2b473e9d9bb26be089f302cdfdfe6e107eaf/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/ColumnPruning.java#L310-L315) can remove unused aggregates. Please provide `EXPLAIN` for the measured queries and repeat with the result consumed, for example `SELECT g, size(collect_set(s, 100)) FROM t GROUP BY g`, and the grouped `group_concat` outputs directly. Also, this dataset has one distinct `s` per `g`, because 1000 is divisible by 10; the ten distinct strings describe the global input. The report already supplies enough version, SQL, and source detail to proceed with a fix review. Remaining evidence needed is the PR/test output and exact before/after build revisions and plans. Audit records or profiles tied to those runs would substantiate the reported peak-memory/CPU deltas and identify the residual memory consumers. I have not executed the workload or reviewed a profile, so those measurements and the attribution of all remaining peak memory to scan/hash tables remain unverified. Breakwater-GitHub-Analysis-Slot: slot_54cba2593129 -- 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]
