namanjain24-sudo commented on PR #25090:
URL: https://github.com/apache/datafusion/pull/25090#issuecomment-5640878571
Fair question, and my PR description was the reason for it — it argued from
the spec and only said a
consumer "can reject such plans", without naming one. Here is the concrete
failure, measured rather
than asserted.
**The engine is substrait-java** (`io.substrait:core:0.103.0`). It reads the
field unconditionally:
```java
// ProtoAggregateFunctionConverter.java:82
.outputType(protoTypeConverter.from(measure.getOutputType()))
```
```java
// ProtoTypeConverter.java:125
case KIND_NOT_SET:
throw new UnsupportedOperationException("Type is not set: " + type);
```
There is no `hasOutputType()` guard on that path, so every aggregate measure
we export hits it.
**Reproduction, with no DataFusion consumer in the loop.** I built `SELECT
sum(i) FROM t` over one
non-null `i64` column, called `to_substrait_plan`, wrote the protobuf to a
file, and handed that file
to substrait-java's `ProtoPlanConverter`.
One wrinkle that matters if you try this, and that I should have led with: a
DataFusion plan as
emitted today does not even reach the `output_type` check. It fails earlier
with
```
IllegalStateException: Function 'sum' references URN anchor -1, but no URN
is registered at that anchor
```
because we still write `extension_urn_reference: u32::MAX` for every
function declaration
(`extensions.rs:120`), which is the known gap tracked in #11545. To isolate
*this* bug I registered
`extension:io.substrait:functions_arithmetic` on the plan and used the
compound key `sum:i64`. I
changed nothing else in either plan.
| plan | #11545 worked around | substrait-java 0.103.0 |
| --- | --- | --- |
| `main` | no | `IllegalStateException: … URN anchor -1` |
| `main` | yes | `UnsupportedOperationException: Type is not set:` |
| this PR | no | `IllegalStateException: … URN anchor -1` |
| this PR | yes | **accepted** — `Struct{nullable=false,
fields=[I64{nullable=true}]}` |
So `output_type` is a second, independent gate sitting behind #11545. Fixing
this alone does not
make DataFusion plans readable by substrait-java. Leaving it unfixed means
they still will not be
once #11545 is. The type substrait-java recovers, nullable `i64`, is exactly
what the optimized
DataFusion plan's own schema says `sum(t.i)` is, so the value written is
right and not merely
present.
**Why nothing in our suite catches it.** Our consumer never reads this field
— the only `output_type`
read anywhere under `logical_plan/consumer/` is in `cast.rs`. Producer and
consumer therefore agree
on a shape the spec does not describe, and every DataFusion-to-DataFusion
round trip passes.
**Spec wording**, identical in the pinned `substrait` 0.63.0 crate, in
upstream `v0.87.0` and on
`main`:
> Must be set to the return type of the function, exactly as derived using
the declaration in the
> extension.
**Precedent.** This is the same defect and the same fix as #15831, fixed by
#20597, which set
`output_type` from `Expr::ScalarFunction(..).to_field(schema)` for scalar
functions. This PR uses
that identical path for aggregates.
One correction to my own description while I am here: it lists
`from_in_list` as a remaining
follow-up site, which is wrong. `from_in_list` emits `SingularOrList`, which
has no `output_type`
field at all. The sites that genuinely still omit it are window functions
(`window_function.rs:110`), the LIKE and NOT LIKE paths
(`scalar_function.rs:296` and `:312`), and
the `not()` wrapper in `utils.rs:106`. I have fixed that sentence in the
description.
I will answer the same question on #25100 and #25190 on their own threads.
Worth saying up front that
they are not all the same kind of claim: substrait-java accepts
`AGGREGATION_PHASE_UNSPECIFIED`
without complaint, so #25100 is a spec-conformance argument, not a
reproduction like this 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]
