Doris-Breakwater commented on issue #69491: URL: https://github.com/apache/doris/issues/69491#issuecomment-6355734029
Breakwater-GitHub-Analysis-Slot: slot_84e2af81c0a7 **Initial assessment:** the guard/rewrite shape mismatch is present in the source, but the reported SQL failure is not yet independently confirmed. There is an earlier normalization step that matters to the proposed root cause. Suggested triage: Nereids / query correctness, with reproduction and plan evidence needed. **Verified from source** I inspected the configured local checkout and cross-checked the relevant files against upstream `master` at `cc49069836975cd3a37bcdc9ba3e35a1fca717d3`. - `checkAgg` recursively finds `Count` and excludes other aggregate functions and distinct counts. It does not require a direct `Alias(Count(...))`. Both the upper `Sum0` replacement and the special per-child count replacement require that direct shape. Thus, an **unnormalized** aggregate output such as `Alias(COUNT(k) + 10)` passes a guard that is broader than the transformation supports. [PushCountIntoUnionAll.java](https://github.com/apache/doris/blob/cc49069836975cd3a37bcdc9ba3e35a1fca717d3/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushCountIntoUnionAll.java#L125-L169) - However, `Analyzer` runs `NormalizeAggregate` before the rewrite stage, and `PushCountIntoUnionAll` runs later in the eager-aggregation rewrite topic. Normalization extracts aggregate functions into aggregate outputs and replaces their occurrences in an upper projection with slots. For this query, the source therefore predicts a schematic shape of `Project(k, c + 10 AS res) -> Aggregate(GROUP BY k, outputs=[k, COUNT(k) AS c]) -> UnionAll`. The direct count alias can then be rewritten to `SUM0` over branch counts, with `+ 10` evaluated once above it. [Analyzer](https://github.com/apache/doris/blob/cc49069836975cd3a37bcdc9ba3e35a1fca717d3/fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/executor/Analyzer.java#L189), [normalization](https://github.com/apache/doris/blob/cc49069836975cd3a37bcdc9ba3e35a1fca717d3/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/NormalizeAggregate.java#L271-L296), [output substitution](https://github.com/apache/doris/blob/cc4906983 6975cd3a37bcdc9ba3e35a1fca717d3/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/NormalizeAggregate.java#L485-L500), [rewrite registration](https://github.com/apache/doris/blob/cc49069836975cd3a37bcdc9ba3e35a1fca717d3/fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/executor/Rewriter.java#L681-L689). **What remains unverified** If an unnormalized compound output reaches this rule, the described loss of multiplicity is consistent with its implementation: the generic traversal still remaps `k` inside the compound expression, each branch emits one aggregate row per key, and the unchanged upper `COUNT(k) + 10` can count those branch rows rather than the original six rows. For the supplied two nonempty branches, that explains `12`. What is missing is evidence that the ordinary SQL planning path actually supplies that unnormalized output. I did not execute the SQL or run Doris tests, so this is a conditional source-level explanation, not a reproduced engine result. Please provide: 1. The exact build/commit (`SELECT VERSION()` and FE build revision); `master` alone does not identify the tested code. 2. Actual query output, `SHOW CREATE TABLE` for both tables, and per-table row counts from a fresh run of the supplied inserts. 3. `EXPLAIN ANALYZED PLAN` and `EXPLAIN REWRITTEN PLAN` for both the compound and plain-count queries, plus planner settings, especially `disable_nereids_rules`. If available, include the rewrite trace immediately before/after `PUSH_COUNT_INTO_UNION_ALL`. 4. Whether `12` came from a running Doris query or a test that directly constructs a logical plan and applies this rule. For the latter, include the test and its input plan. **Recommended next steps** - First check whether normalization was skipped or a later transformation reintroduced the compound aggregate output. As a diagnostic comparison, rerun with `PUSH_COUNT_INTO_UNION_ALL` added to the existing disabled-rule list, preserving and then restoring the original setting. Compare both results and plans; this is not yet a validated workaround. - If tightening the guard, retain supported grouping-slot outputs such as `k` and direct non-distinct count aliases, while rejecting compound aggregate outputs the rewrite cannot handle. Requiring *every* output to be `Alias(Count(...))` would also disable valid grouped cases. A recursive implementation needs separate partial-count slots and mappings for each count; summing branch-level `COUNT(k) + 10` would incorrectly add the constant per branch. - Add an end-to-end regression through normal analysis/rewrite for the supplied SQL, expecting `(10, 16)`, alongside the plain count expecting `(10, 6)`. Add a rule-level test for the unnormalized compound shape. Include multiple counts, nullable keys/arguments, empty branches/global empty input, and preservation of the existing distinct-count and mixed-aggregate exclusions. No repository code was changed. -- 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]
