namanjain24-sudo opened a new issue, #25190:
URL: https://github.com/apache/datafusion/issues/25190
### Describe the bug
Substrait's `IfThen` has no notion of a base expression. Every `IfClause` is
a
standalone boolean condition, and `then` is the value that clause yields:
```proto
message IfThen {
// A list of one or more IfClauses
repeated IfClause ifs = 1;
// The returned Expression if no IfClauses are satisified
Expression else = 2;
message IfClause {
Expression if = 1;
Expression then = 2;
}
}
```
The producer nevertheless uses `IfThen` for `CASE <base> WHEN <value> THEN
...`,
encoding the base expression as a leading `IfClause` whose `then` is left
unset
([`from_case`](https://github.com/apache/datafusion/blob/517ac6d53133621c99a9a7dda470549a26348f6a/datafusion/substrait/src/logical_plan/producer/expr/if_then.rs#L36-L50)):
```rust
// Parse base
if let Some(e) = expr {
// Base expression exists
ifs.push(IfClause {
r#if: Some(producer.handle_expr(e, schema)?),
then: None,
});
}
// Parse `when`s
for (r#if, then) in when_then_expr {
ifs.push(IfClause {
r#if: Some(producer.handle_expr(r#if, schema)?),
then: Some(producer.handle_expr(then, schema)?),
});
}
```
The following clauses then carry the raw WHEN operand in `if`. Neither the
leading clause nor the WHEN operands are boolean, so the emitted `IfThen` is
not
something a spec-conforming consumer can evaluate.
That convention is private to DataFusion. The consumer decodes it back
([`from_if_then`](https://github.com/apache/datafusion/blob/517ac6d53133621c99a9a7dda470549a26348f6a/datafusion/substrait/src/logical_plan/consumer/expr/if_then.rs#L29-L43)):
```rust
// If the first element does not have a `then` part, then we can assume it's
a base expression
```
so a DataFusion-to-DataFusion round trip is unaffected and no existing test
fails. An engine that reads the plan as Substrait defines it sees something
different.
### To Reproduce
```sql
SELECT CASE a WHEN 1 THEN 'x' WHEN 2 THEN 'y' ELSE 'z' END FROM data
```
Produce the plan with `to_substrait_plan` and inspect the `IfThen` in the
projection directly, rather than round-tripping it through the consumer:
```
IfThen with 3 ifs, else present: true
ifs[0]: if_set=true then_set=false
if = Selection(FieldReference { ... StructField { field: 0 } ... })
// a, an i64
then = None
ifs[1]: if_set=true then_set=true
if = Literal(I64(1))
then = Literal(String("x"))
ifs[2]: if_set=true then_set=true
if = Literal(I64(2))
then = Literal(String("y"))
```
Three clauses, with conditions `a`, `1` and `2`. All three are `i64`, and the
first has no result at all.
For comparison, the searched form `CASE WHEN a = 1 THEN 'x' ELSE 'z' END` is
emitted correctly, as a single clause whose condition is an `equal` call with
`output_type` boolean.
### Expected behavior
Every emitted `IfClause` should have a boolean condition and a `then` value.
Substrait has no switch-style construct that fits this case in general:
`SwitchExpression` exists, but its `IfValue.if` is a `Literal`, so it cannot
express `CASE a WHEN b + 1 THEN ...`, and DataFusion's own consumer currently
rejects it with `not_impl_err!("Switch expression not supported")`. The
general
translation is therefore to desugar the base expression, emitting `<base> =
<value>` as each clause condition:
```
ifs[0]: if = equal(a, 1), then = 'x'
ifs[1]: if = equal(a, 2), then = 'y'
else : 'z'
```
DataFusion matches a base expression with `=` semantics
([`compare_with_eq`](https://github.com/apache/datafusion/blob/517ac6d53133621c99a9a7dda470549a26348f6a/datafusion/physical-expr-common/src/datum.rs#L138)
uses Arrow's `eq`), so this preserves the plan's meaning, including a NULL
WHEN
operand never matching. The producer already applies exactly this kind of
desugaring to `BETWEEN` in `from_between`.
A base `CASE` would then round trip as the equivalent searched `CASE`,
keeping
its projection name and schema.
### Additional context
- Distinct from the round-trip failures collected under #16248: this round
trip
succeeds. The problem is only visible by reading the emitted protobuf,
because
producer and consumer share the private convention.
- Same shape as #25100 (`phase` left at `AGGREGATION_PHASE_UNSPECIFIED`): a
producer field whose value is wrong by the spec but invisible to
DataFusion's
own consumer.
- Adding `SwitchExpression` support, on both sides, would preserve the
base-CASE
structure for the subset where every WHEN operand is a literal. That is
worth
doing separately; it does not cover the general case, and the emitted plan
should be valid regardless.
- Verified against the `algebra.proto` shipped in the pinned `substrait`
0.63.0
crate.
--
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]
