Substrait consumer ignores AggregateFunction.phase, so an intermediate aggregate runs as a complete one
- Lingua principale
- Rust
- Stelle
- 9.3k
- Fork
- 2.4k
- Merge medio
- 3g 11h
- PR unite (30g)
- 360
Descrizione
### 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` | `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` | 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
The consumer should honor the requested phase or reject it if unsupported. `LogicalPlan::Aggregate` has no phase field; partial/final aggregation is handled by the physical planner, so rejecting unsupported intermediate phases may be sufficient here.
There is also a producer compatibility issue. At the measured revision, DataFusion's producer writes phase 0 (`UNSPECIFIED`) for complete aggregates (`producer/expr/aggregate_function.rs:68`). However, [spec v0.102.0](https://github.com/substrait-io/substrait/blob/v0.102.0/proto/substrait/algebra.proto) defines `UNSPECIFIED` as `INTERMEDIATE_TO_RESULT`. Treating 0 as `INITIAL_TO_RESULT` would be a compatibility exception for those producer plans. Whether to retain that exception or update the producer should be decided explicitly.
Window functions carry the same field. I have not tested them.
### To Reproduce
The three plans are at [`probe/phase-cases`](https://github.com/alexandrefimov/substrait-conformance-cases/tree/v0.1.0/probe/phase-cases) — protobuf-JSON with the `.bin` alongside, 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.
Guida per i contributori
Apri la guida per i contributori
Direzione di ricerca
Inizia in datafusion/substrait/src/logical_plan/consumer/ e analizza come vengono consumate le misure aggregate, quindi esamina producer/expr/aggregate_function.rs:68. Riproduci i piani da probe/phase-cases e aggiungi test del consumer se appropriato. Il lavoro è completato quando le fasi intermedie vengono rispettate o rifiutate esplicitamente, e la decisione sulla compatibilità del producer di phase-0 è documentata nei test o nella discussione dell’issue.
Scritto dal modello di indicizzazione a partire dal testo della issue.
Valutazione
- Stack tecnologico
- rust
- Ambito
- databases
- Tipo di issue
- Bug
- Difficoltà
- 4/5
- Tempo stimato
- 3-5 giorni
- Stato di attività
- Attiva
- Chiarezza
- Abbastanza chiara
- Idoneità per principianti
- 48/100