gmhelmold commented on PR #23486: URL: https://github.com/apache/datafusion/pull/23486#issuecomment-4992697393
Thanks — I did consider it, and I think it's the wrong layer to fix this one at. Hoisting the alias into a Projection would have to live in the DataFrame `aggregate()` builder, and it'd change the emitted plan shape for *every* DataFrame-API aggregate that carries an alias — an extra Projection node on all of them, unconditionally — just to make this single rule fire. That `Alias(AggregateFunction)` shape is the DF-API's intended output today; the rest of the optimizer and the physical planner already handle it fine, and several rules deliberately tolerate it by unwrapping one alias layer (e.g. `decorrelate.rs`'s `unalias()`, plus `common_subexpr_eliminate`, `optimize_projections`, and a couple of analyzer passes). `SingleDistinctToGroupBy` was just the outlier that only matched the bare shape. So this PR follows that established idiom: peel one alias layer in the matcher and the rewrite, scoped to this rule, no other plan changes. Bare-aggregate snapshots stay byte-identical, and the user alias is preserved by the projection the rewrite already rebuilds from the aggregate schema. Canonicalizing DF-API alias placement to match SQL is a reasonable idea, but it's a much broader normalization with its own blast radius (perf + plan-shape/back-compat across all DF aggregates) and better as its own discussion than as the fix for this bug. Happy to be wrong though — do you see a shape mismatch this local unwrap doesn't cover? -- 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]
