namanjain24-sudo opened a new pull request, #25146:
URL: https://github.com/apache/datafusion/pull/25146
## Which issue does this PR close?
- Closes #25100.
## Rationale for this change
The Substrait producer never sets `phase` on the aggregate and window
function
calls it emits, so every call carries `AGGREGATION_PHASE_UNSPECIFIED`.
That is not the same as leaving the field out. The spec gives the value a
meaning, and it is not the one these plans need:
```protobuf
enum AggregationPhase {
// Implies `INTERMEDIATE_TO_RESULT`.
AGGREGATION_PHASE_UNSPECIFIED = 0;
...
// A complete invocation: the function should aggregate the given set of
// inputs to yield a single return value. This style must be used for
// aggregate or window functions that are not decomposable.
AGGREGATION_PHASE_INITIAL_TO_RESULT = 3;
```
Both `AggregateFunction.phase` and `Expression.WindowFunction.phase` are
documented as `Required. Must be set to INITIAL_TO_RESULT for ... that are
not
decomposable.`
A `LogicalPlan::Aggregate` is always a complete aggregation over its input
rows. The partial/final split is a physical planning concern, and the logical
producer has no notion of it, so `INITIAL_TO_RESULT` is the phase these plans
should declare. What they declare instead carries the spec meaning
`INTERMEDIATE_TO_RESULT`: that the arguments are already intermediate state
to
be combined.
This stays invisible to a DataFusion-to-DataFusion round trip because the
consumer never reads the field (#24967). A consumer that does honour the
declaration reads a complete aggregation as one whose arguments are already
partial state.
## What changes are included in this PR?
Set `phase` to `AGGREGATION_PHASE_INITIAL_TO_RESULT` at the two producer call
sites that emit it:
- `from_aggregate_function` in `producer/expr/aggregate_function.rs`
- `make_substrait_window_function` in `producer/expr/window_function.rs`
No other producer site emits the field, and the consumer does not read it, so
nothing else changes.
## What is the testing strategy for this PR?
New test `aggregate_and_window_functions_declare_initial_to_result` in
`datafusion/substrait/tests/cases/serialize.rs`. It produces plans for a bare
aggregate, a grouped aggregate, and a window function, then walks the
produced
protobuf directly and asserts the phase on every aggregate and window
function
call, rather than round-tripping through the consumer (which ignores the
field, so a round trip could not catch this).
Verified the test fails without the producer change:
```
assertion `left == right` failed: phase for `SELECT sum(a) FROM data`
left: 0
right: 3
```
## Are there any user-facing changes?
Plans produced by `to_substrait_plan` now declare
`AGGREGATION_PHASE_INITIAL_TO_RESULT` instead of
`AGGREGATION_PHASE_UNSPECIFIED` on aggregate and window function calls. This
is a fix to the emitted protobuf rather than a Rust API change. Consumers
that
read `phase` will now see the correct declaration; consumers that ignore it,
including DataFusion's own, are unaffected.
--
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]
