namanjain24-sudo commented on issue #25190:
URL: https://github.com/apache/datafusion/issues/25190#issuecomment-5641220048
Yes, compatibility, and this one reproduces on a DataFusion plan with
nothing worked around. The
engine is substrait-java `io.substrait:core:0.103.0`.
`SELECT CASE a WHEN 1 THEN 'x' WHEN 2 THEN 'y' ELSE 'z' END FROM data`, one
nullable `i64` column,
through `to_substrait_plan`, produces an `IfThen` shaped like this:
```
clause 0: if = SELECTION (column a) then = <unset>
clause 1: if = LITERAL i64 1 then = "x"
clause 2: if = LITERAL i64 2 then = "y"
else "z"
```
Handing that protobuf straight to `ProtoPlanConverter`:
```
java.lang.IllegalArgumentException: Unknown type: REXTYPE_NOT_SET
at
io.substrait.expression.proto.ProtoExpressionConverter.from(ProtoExpressionConverter.java:364)
at
io.substrait.expression.proto.ProtoExpressionConverter.lambda$from$1(ProtoExpressionConverter.java:212)
```
Line 212 is `ExpressionCreator.ifThenClause(from(t.getIf()),
from(t.getThen()))`. It converts `then`
unconditionally and clause 0 does not have one. This plan registers no
functions at all, so unlike
#25049 there is no extension-URN problem in the way: it is a straight
failure on what we emit today.
Two things are wrong here and only one of them throws, which is worth
separating:
1. **Clause 0 carries the base expression with no `then`.** That is the hard
failure above.
2. **The conditions are not boolean.** They are `a`, then `1`, then `2`, all
`i64`. The spec's if
expression is `if <boolean expression> then ...`. I checked whether
substrait-java objects to
that on its own — drop clause 0 and feed it the remaining two clauses,
and it **accepts** them.
It does not type-check `if`. So that half is a silent divergence, not an
error, which is also why
no round trip anywhere would surface it.
With #25191 every clause becomes `equal(a, <value>)` carrying `output_type:
bool` and a `then`, and
the same query is accepted, giving `Struct{nullable=false,
fields=[Str{nullable=false}]}`. That plan
does need the #11545 workaround, because it now registers the `equal`
function and we still write
`extension_urn_reference: u32::MAX`.
On `SwitchExpression`, since it is the obvious alternative and I should say
why I did not use it:
`SwitchExpression.IfValue.if` is a `Literal`, so it cannot express `CASE a
WHEN b + 1`, and our own
consumer answers `not_impl_err!("Switch expression not supported")`
(`logical_plan/consumer/substrait_consumer.rs:348`). Emitting it would need
consumer work first and
would still need this desugaring as the fallback, so I left it as a
follow-up rather than folding it
into the same change.
--
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]
