Doris-Breakwater commented on issue #66044: URL: https://github.com/apache/doris/issues/66044#issuecomment-5078099137
Initial triage: **valid FE/Nereids planner safety issue, but the proposed fix is not yet complete end to end**. The issue currently has no labels; it should be triaged under the repository's bug/FE-optimizer category. ### Code findings I checked local master (`27cb451229c7a42dd089c5248847828eb0536d02`) and `upstream/branch-4.0`; both have the relevant behavior. * `DistinctAggStrategySelector.shouldUseMultiDistinct` checks unknown/high-NDV distinct arguments only in the scalar (no-group-by) branch. In the group-by branch it checks only the group keys, and unknown group-key statistics return `true` immediately. This asymmetry is real. * The BE implementation confirms the memory mechanism: `AggregateFunctionUniqExactData` owns a `flat_hash_set`, and `AggregateFunctionUniq.add_batch` selects the set through the aggregate-state `place` for each row/group. Memory for a group therefore grows with that group's distinct values, and merging also materializes those keys in the destination set. * This selector only runs its strategy decision when there are at least two distinct-argument groups (`distinctArgumentGroupCountUpToTwo(agg) > 1`). A query with one `COUNT(DISTINCT payment_id)` plus ordinary aggregates bypasses this selector. Its strategy is controlled by `DistinctAggregateRewriter`, so the example and the phrase "one or more" need this second code path to be covered explicitly. ### Important gap in the proposed fix / PR #66045 Routing the outer multi-distinct aggregate to `SplitMultiDistinctStrategy` does **not by itself guarantee redistribution on the distinct key**. The newly created per-distinct CTE aggregate branches are subsequently processed by `DistinctAggregateRewriter`: * on unknown group-key or distinct-key statistics, `DistinctAggregateRewriter` chooses `MULTI_STRATEGY`; * with known statistics, it also chooses `MULTI_STRATEGY` when group NDV is below 0.1% of input rows and distinct NDV is above 10%. Consequently, an unknown-statistics branch, or the genuinely near-unique/low-cardinality-group case described in this issue, can be converted back to `multi_distinct_*` inside each CTE consumer. The CTE may separate multiple sets into different branches, but each branch can still keep a whole group's distinct set on one node; that does not establish the claimed OOM protection. The current PR regression uses `d_20` over about 1,003 rows (roughly 2% NDV). That crosses the selector's 1% `MID_CARDINALITY_THRESHOLD` but remains below `DistinctAggregateRewriter`'s 10% `HIGH_CARDINALITY_THRESHOLD`, so the downstream rewriter happens to choose a distributed split. It does not exercise a near-unique argument above 10%. The no-statistics expected plan has only one aggregate per CTE consumer, consistent with the downstream `MULTI_STRATEGY` decision. There is another unsafe assumption in the proposed unknown-group-key fallback: the number of group-by expressions is not evidence of joint cardinality. Two unknown, constant, or strongly correlated keys can still produce one large group. If safety is the goal, `groupByExpressions().size() >= 2` should not be treated as sufficient without statistics; either use the conservative split or document and validate this as a performance heuristic rather than an OOM guarantee. ### Recommended implementation direction Coordinate both strategy selectors rather than changing only `DistinctAggStrategySelector`: 1. Centralize the high/unknown distinct-argument risk test so the multi-distinct selector and `DistinctAggregateRewriter` cannot make contradictory decisions. 2. For a risky high/unknown-NDV argument, force `SPLIT_IN_REWRITE`/`SPLIT_IN_CASCADES` in each single-distinct branch (or carry an explicit "must split" property into the generated CTE aggregates). The final physical plan must shuffle/deduplicate on a key set containing the distinct key and must not contain `multi_distinct_*` for the protected branch. 3. Apply the same policy to the single-distinct group-by path, since it bypasses `DistinctAggStrategySelector`. 4. Decide whether scanning every input slot is intentionally conservative. `getInputSlots()` also includes condition slots and inputs to bounded transforms such as `YEAR`; missing statistics there do not necessarily make the result high-NDV. Fixing unknown provenance in expression estimation, or checking only value-producing inputs where possible, would reduce unnecessary CTE plans. Before merging #66045, add final-plan tests (not only helper/selector tests) for: * two distinct arguments with group NDV below 0.1% and distinct NDV well above 10%; * unknown distinct statistics, including `IF(cond, col, NULL)`; * one distinct argument plus an ordinary aggregate; * multiple unknown/correlated group keys if the `>= 2` fallback is retained; * assertions that the optimized plan contains no `multi_distinct_*` in the protected branches and that the lower distribution includes the distinct key. ### Missing runtime evidence The code is sufficient to confirm the planner gap, but the reported production OOM and its severity cannot be validated from the issue as written. Please add: * the complete SQL (the `...` is material because the selector requires multiple distinct-argument groups); * exact build commit/version, table DDL/distribution, backend count, and relevant session variables (`multi_distinct_strategy`, `agg_phase`, parallelism); * `EXPLAIN VERBOSE` or `EXPLAIN SHAPE PLAN`, row counts, and group/distinct column statistics; * the BE OOM/error log and query runtime profile showing the responsible aggregation operator and its peak memory. As a temporary diagnostic for the multiple-distinct case, test `multi_distinct_strategy=2` together with `agg_phase=3` or `4`, then verify in `EXPLAIN` that the distinct branches use distributed distinct phases rather than `multi_distinct_*`. For a single-distinct query, validate `agg_phase=3/4` separately because `multi_distinct_strategy` does not route that selector path. Breakwater-GitHub-Analysis-Slot: slot_2f0ad5858350 -- 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]
