apache / apache/datafusion

Substrait consumer ignores AggregateFunction.phase, so an intermediate aggregate runs as a complete one

Aperta
#24,967 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub
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

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.