namanjain24-sudo commented on issue #25100:
URL: https://github.com/apache/datafusion/issues/25100#issuecomment-5641220445
@alexandrefimov's reading is what the code does — I checked it rather than
take it on trust, and the
exact lines are worth pinning because this one never throws.
substrait-java `v0.103.0`,
`spark/src/main/scala/io/substrait/spark/expression/ToAggregateFunction.scala:76`:
```scala
def toSpark(phase: SExpression.AggregationPhase): AggregateMode = phase
match {
case SExpression.AggregationPhase.UNSPECIFIED =>
Final // UNSPECIFIED implies INTERMEDIATE_TO_RESULT
case SExpression.AggregationPhase.INITIAL_TO_INTERMEDIATE => Partial
case SExpression.AggregationPhase.INTERMEDIATE_TO_INTERMEDIATE =>
PartialMerge
case SExpression.AggregationPhase.INTERMEDIATE_TO_RESULT => Final
case SExpression.AggregationPhase.INITIAL_TO_RESULT => Complete
}
```
So what we export as `AGGREGATION_PHASE_UNSPECIFIED` arrives as Spark
`Final`, meaning "the inputs
are partial aggregation buffers", when what we actually exported is a
complete aggregation over raw
input rows, which is Spark `Complete`. Nothing is rejected. It is read as a
different plan.
`algebra.proto` agrees at both ends: `AGGREGATION_PHASE_UNSPECIFIED = 0` is
documented *"Implies
`INTERMEDIATE_TO_RESULT`"*, and `INITIAL_TO_RESULT` is *"A complete
invocation: the function should
aggregate the given set of inputs to yield a single return value"*. The
field's own comment says
`Required`.
Why no test here catches it: our consumer never reads `phase`. There are
zero occurrences of it
anywhere under `logical_plan/consumer/`, so producer and consumer agree on
the omission and every
DataFusion-to-DataFusion round trip passes.
On the fix in #25146 — setting both sites to `INITIAL_TO_RESULT`
unconditionally is correct for us
and does not need a case analysis, because the Substrait producer converts
*logical* plans and
`LogicalPlan::Aggregate` has no mode at all (`input`, `group_expr`,
`aggr_expr`, `schema`).
`AggregateMode`, with its `Partial` and `Final` variants, lives in
`physical-plan`. A logical
aggregate is therefore always a complete aggregation, which is exactly
`INITIAL_TO_RESULT`. I
confirmed the diff does set both sites, the aggregate and the window
function.
Correcting myself on one point: I wrote on #25090 that this issue was a
spec-conformance argument
rather than a reproduction. That was wrong, and the mapping above is why. A
silent misread is the
worse of the two failures, not the milder one.
--
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]
