goutamadwant commented on PR #25045:
URL: https://github.com/apache/datafusion/pull/25045#issuecomment-5659993764
> Not a committer, so this is not a binding review. I have been working in
the same corner of the producer, so I read this closely.
>
> I checked that the two gates cover every path: `from_substrait_agg_func`
is reached only from `rel/aggregate_rel.rs:98`, and `from_window_function` only
from `substrait_consumer.rs:332`, so there is no third place where a phase
could slip past. The `-1` and `12345` cases in `aggregate_invalid_phase` are a
good touch, since `try_from` is the only thing standing between an unknown enum
value and silent acceptance.
>
> **One thing worth pinning down in the code, not just in review.** The
accept-list treats `UNSPECIFIED` as a complete call and rejects
`INTERMEDIATE_TO_RESULT`. On the spec's own terms those are the same value:
>
> ```proto
> enum AggregationPhase {
> // Implies `INTERMEDIATE_TO_RESULT`.
> AGGREGATION_PHASE_UNSPECIFIED = 0;
> ```
>
> That text is identical in the pinned 0.63.0 crate and on substrait main
today, so the rule as written accepts a value the spec defines as meaning
exactly the thing the next arm rejects.
>
> I think the carve-out is the right call anyway, because DataFusion's own
producer has been emitting `UNSPECIFIED` for every aggregate and window call,
and plans written by released versions have to keep loading. But that is a
deliberate, DataFusion-specific exception rather than a reading of the spec,
and the comment currently reads as though the two were the same thing.
Something like "the spec says UNSPECIFIED implies INTERMEDIATE_TO_RESULT; we
accept it regardless because DataFusion's own producer emitted it, so rejecting
it would break plans written by released versions" would stop the next reader
from treating it as spec-derived.
>
> It also leaves a residual hole worth naming in the comment: a
non-DataFusion producer that emits `UNSPECIFIED` meaning intermediate state
still gets executed as a complete aggregation, which is the failure #24967
describes. This PR closes it for the three explicit phases and leaves it open
for the default one.
>
> For disclosure, #25146 is mine, and it stops the producer from emitting
`UNSPECIFIED`. It does not conflict with this; if anything the two fit
together, since after it the carve-out here covers only plans from older
versions rather than everything DataFusion currently writes. If this lands
first I am happy to rebase onto it.
>
> Minor, take or leave: `validate_aggregation_phase` lives in
`expr/aggregate_function.rs` and is pulled into the window path via `use
super::aggregate_function::...`. Since both callers are peers it might read
better next to the shared consumer helpers, but that is taste.
>
> The branch is 65 commits behind main now, so it will want a rebase before
it can go in.
Clarified that substrait defines UNSPECIFIED as INTERMEDIATE_TO_RESULT,
while this consumer retains the compatibility exception for existing DataFusion
plans. The comment now explicitly acknowledges the ambiguity for external
producers. The producer change in #25146 is complementary and is not duplicated
here.
--
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]
