namanjain24-sudo opened a new pull request, #25191:
URL: https://github.com/apache/datafusion/pull/25191
## Which issue does this PR close?
- Closes #25190.
## Rationale for this change
Substrait's `IfThen` has no base expression. Every `IfClause` is a standalone
boolean condition, and `then` is the value that clause yields.
The producer used `IfThen` for `CASE <base> WHEN <value> THEN ...` anyway, by
pushing a leading `IfClause` that carries the base expression in `if` and
leaves
`then` unset, then one clause per WHEN whose `if` is the raw WHEN operand.
For
```sql
SELECT CASE a WHEN 1 THEN 'x' WHEN 2 THEN 'y' ELSE 'z' END FROM data
```
that emits three clauses whose conditions are `a`, `1` and `2`. All three are
`i64`, and the first has no result at all.
The convention is private to DataFusion: the consumer reads a `then`-less
first
clause back as the base expression, so a DataFusion-to-DataFusion round trip
is
unaffected and no existing test failed. An engine that reads the plan as
Substrait defines it sees clauses it cannot evaluate.
## What changes are included in this PR?
`from_case` in `producer/expr/if_then.rs` now emits one clause per WHEN, with
`<base> = <value>` as the condition when a base expression is present. This
is
the same desugaring `from_between`, in the same crate, already applies to
`BETWEEN`. The searched form, which was already correct, is unchanged.
DataFusion matches a base expression with `=` semantics (`compare_with_eq`
uses
Arrow's `eq`), so the plan keeps its meaning, including a NULL WHEN operand
never matching.
Two things I deliberately did not do, and would rather handle separately:
- **`SwitchExpression`.** Substrait does have a switch construct, but its
`IfValue.if` is a `Literal`, so it cannot express `CASE a WHEN b + 1 THEN
...`,
and our consumer currently answers `not_impl_err!("Switch expression not
supported")`. Emitting it would need consumer support and would still need
this desugaring as the fallback, so the correctness fix is worth having on
its
own.
- **The consumer.** It still accepts a `then`-less first clause as a base
expression. Removing that would break reading plans written by older
DataFusion versions, so it seemed better left to its own discussion.
One trade-off worth calling out: a base expression is now repeated once per
WHEN
arm in the emitted plan, and the searched `CASE` it round trips back into
evaluates it per arm. For a column reference that is free; for an expensive
base
expression it is not. `SwitchExpression` support would remove the
duplication for
the all-literal subset.
## What is the testing strategy for this PR?
- `case_with_base_expression_emits_equality_conditions` in
`tests/cases/serialize.rs` walks the produced protobuf directly and
asserts one
clause per WHEN, every clause having both a condition and a `then`, and
every
condition being a call to the registered `equal` function. It inspects the
protobuf rather than round-tripping because the consumer understands the
old
encoding, so a round trip cannot catch this. Verified it fails on `main`:
```
assertion failed: !equal_anchors.is_empty()
no `equal` function registered
```
- `case_with_base_expression` in `tests/cases/roundtrip_logical_plan.rs`
moves
from `roundtrip` to `assert_expected_plan`, recording that a base `CASE`
now
comes back as the equivalent searched `CASE`. It still asserts the schema
is
unchanged; the projection keeps its original name via an alias.
- `cargo test -p datafusion-substrait` passes (272 tests), as does
`cargo xtask ci step test substrait`, and `./ci/scripts/rust_clippy.sh`,
`rust_fmt.sh`, `typos_check.sh` and `rust_docs.sh` are clean.
## Are there any user-facing changes?
Plans produced by `to_substrait_plan` for a `CASE` with a base expression now
carry boolean `equal` conditions instead of the previous encoding. No Rust
API
changes.
Consumers that implemented DataFusion's `then`-less convention will see the
new
form; it is valid Substrait, so a spec-conforming consumer reads it
correctly.
Within DataFusion, such a plan now round trips into the equivalent searched
`CASE` rather than the base form, with the same schema and results.
--
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]
