alexandrefimov opened a new issue, #24967: URL: https://github.com/apache/datafusion/issues/24967
### Describe the bug `AggregateFunction.phase` is never read: the string `phase` does not appear anywhere under `datafusion/substrait/src/logical_plan/consumer/`. A measure declared `INITIAL_TO_INTERMEDIATE` is planned as an ordinary aggregate, and the plan is accepted with no error. Measured on `3266eaa91`. The plans read a named table `t_avg(c0 i64 NOT NULL)` holding rows `1` and `2`, and carry one `avg` measure over `c0`: | plan | phase | declared output | DataFusion returns | | --- | --- | --- | --- | | `PlanRel.rel` | `INITIAL_TO_INTERMEDIATE` | `STRUCT<i64,i64>` | `avg(t_avg.c0):Float64?`, `1.5` | | `PlanRel.rel` | `INITIAL_TO_RESULT` | `i64?` | `avg(t_avg.c0):Float64?`, `1.5` | | `PlanRel.root` | `INITIAL_TO_INTERMEDIATE` | `STRUCT<i64,i64>` | rejected: `Names list must match exactly to nested schema, but found 1 uses for 3 names` | Two different phases, one answer — and that answer is neither declared type. `functions_arithmetic.yaml` gives `avg:i64` `return: i64?` and says it truncates for integral types, so `1.5` is DataFusion's own `avg`: not the intermediate struct the first plan asks for, and not the final value the second one declares. The third row is why this stays out of sight. A struct-returning measure needs three names depth-first in `RelRoot` — the column, then the struct's two fields — and that plan is rejected on names, so anyone writing the rooted form reads an error about something else. ### Expected behavior Producing the intermediate state does not look available here: `LogicalPlan::Aggregate` carries no phase, and partial/final is the physical planner's business. Rejecting what cannot be honoured may be the whole of the fix, but the boundary matters. `phase` defaults to 0 (`UNSPECIFIED`), and that is what DataFusion's own producer writes (`producer/expr/aggregate_function.rs:68`), so a rule has to accept 0 alongside `INITIAL_TO_RESULT` and refuse the rest. Worth knowing before you pick the rule: `algebra.proto` in spec v0.102.0 comments that constant `// Implies INTERMEDIATE_TO_RESULT`, so reading 0 by its documented meaning would reject the plans your own producer emits. That contradiction is the spec's, not yours. Window functions carry the same field. I have not tested them. ### To Reproduce Three protobuf-JSON plans of eighty to a hundred lines, plus the one-column table registered above. Glad to open a PR adding them as consumer tests if that is where you would want them. -- 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]
