Doris-Breakwater commented on issue #66044: URL: https://github.com/apache/doris/issues/66044#issuecomment-5078098319
Breakwater-GitHub-Analysis-Slot: slot_2f0ad5858350 ## Initial assessment **Confirmed planner safety issue, with high confidence; the proposed fix is directionally correct but incomplete for the issue's stated “one or more `count(distinct ...)`” scope.** The issue currently has no labels, assignee, or linked pull request. Suggested triage is Bug + Nereids/optimizer (and memory/OOM, if such a label exists in the repository taxonomy). ## Code findings 1. In `DistinctAggStrategySelector.shouldUseMultiDistinct`, the scalar branch rejects MultiDistinct when a distinct argument has absent/unknown statistics or NDV at least `inputRows * MID_CARDINALITY_THRESHOLD`. The grouped branch does neither check. It only handles a skew hint, group-key statistics, and group NDV; if group-key statistics are unknown it returns `true` immediately. This directly confirms the selector asymmetry described here on both the local master and branch-4.0 snapshots. 2. The memory mechanism is also confirmed. `MultiDistinctFunctionStrategy` converts the aggregate to `multi_distinct_*`; the BE implementation in `aggregate_function_uniq.h` stores an exact `flat_hash_set` in each aggregate state and merges the sets. A global grouped aggregation is partitioned by the group key, so the complete state for a hot/low-cardinality group is ultimately owned by one execution instance. A near-unique distinct argument can therefore make one per-group state grow with the group's row count. 3. The proposed input-slot validation is necessary for expressions. `ExpressionEstimation.visitIf` constructs a fresh statistic with NDV at least 2 and does not propagate the children's unknown flag. Consequently, checking only the estimated statistic for `if(cond, col, null)` can classify the expression as known even when `col` has absent/unknown statistics. Every input `Slot` collected from each distinct argument must also have a present, known statistic. ## Scope gap that should be resolved before the PR `DistinctAggStrategySelector` only processes aggregates with more than one distinct argument group (`distinctArgumentGroupCountUpToTwo(...) > 1` on current master). A single distinct argument group passes through to `DistinctAggregateRewriter`. That second rewriter has the same safety exposure: - on current master, `DistinctAggregateRewriter.chooseStrategy` returns `MULTI_STRATEGY` when group-key or distinct-argument statistics are unknown; - it also returns `MULTI_STRATEGY` specifically for low group-key NDV plus high distinct-key NDV; - branch 4.0 has equivalent behavior in `shouldUseMultiDistinct`; - both paths convert to the same exact-set `multi_distinct_*` implementation. This is not only a separate single-distinct case. CTE splitting turns each distinct argument group into its own one-distinct aggregate branch, and the later CTE-child rewrite can run `DistinctAggregateRewriter` on those branches. With unknown or low-group/high-distinct statistics, that rewriter can convert a CTE branch back to MultiDistinct and reintroduce the same exact-set risk. Therefore, changing only `DistinctAggStrategySelector` does not reliably close the OOM path and does not fix the single-distinct example unless the ellipsis contains another DISTINCT argument group. Please either: 1. apply the safety checks to both strategy selectors, preferably through shared helper logic; or 2. narrow the issue and reproduction explicitly to multiple distinct argument groups and track the single-distinct path separately. There is also a physical-plan condition to verify. CTE decomposition enables the normal DISTINCT multi-phase plans, including lower shuffles containing the distinct key. However, `SplitAggMultiPhase` can expose both group-key and distinct-key partition alternatives in some unknown-statistics cases. The regression must assert the final physical plan, not only the presence of `LogicalCTEAnchor`, and confirm that the risky low/unknown group-cardinality case actually selects a lower shuffle containing the distinct argument. Otherwise the CTE rewrite alone is not a proof that one large group cannot still be concentrated. ## Missing runtime evidence The code is sufficient to confirm the unsafe planner decision, so an OOM profile is not required to accept this as a planner bug. To validate impact and make the reproduction unambiguous, it would still help to attach: - the complete SQL, showing whether there is one or multiple distinct argument groups; - `EXPLAIN SHAPE PLAN` (or full EXPLAIN) plus `multi_distinct_strategy`, `agg_phase`, row count, and available column statistics; - the BE memory-limit error and relevant query-profile/aggregate memory trackers, if an actual OOM was observed; - input row count, group-key NDV, distinct-argument NDV, and the largest group's row/NDV count. ## Recommended implementation and tests - Evaluate distinct-argument risk before the grouped/scalar heuristic: reject MultiDistinct if an argument estimate is absent/unknown, any input slot has absent/unknown statistics, or estimated NDV is at least `inputRows * MID_CARDINALITY_THRESHOLD`. - Reuse the same risk predicate in `DistinctAggStrategySelector` and `DistinctAggregateRewriter` so their behavior cannot diverge again. - Preserve or explicitly document the precedence of forced modes (`multi_distinct_strategy`, `agg_phase`) and the existing `sourceRepeat`/GROUPING SETS constraints. - Treat “unknown group stats: one key => CTE, two or more keys => MultiDistinct” as a performance heuristic, not a cardinality guarantee. It is only reached after distinct arguments are considered safe and needs plan-shape coverage. - Add regression cases for: high-NDV direct slots; unknown direct slots; `if(cond, unanalyzed_col, null)`; known low-NDV arguments; unknown one-key and multi-key group-by statistics; unchanged scalar behavior; explicit strategy overrides; and both one and multiple distinct argument groups. - Verify master and branch 4.0 separately: their selector entry conditions and `DistinctAggregateRewriter` APIs differ, so the backport will need branch-specific adaptation. Given the reporter's willingness to submit a PR, the next maintainer action should be to confirm the intended single-distinct scope and the forced-mode precedence, then invite a PR with the above physical-plan regression coverage. -- 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]
