namanjain24-sudo commented on code in PR #25191:
URL: https://github.com/apache/datafusion/pull/25191#discussion_r4044312290
##########
datafusion/substrait/src/logical_plan/producer/expr/if_then.rs:
##########
@@ -32,19 +34,37 @@ pub fn from_case(
when_then_expr,
else_expr,
} = case;
- let mut ifs: Vec<IfClause> = vec![];
- // Parse base
- if let Some(e) = expr {
- // Base expression exists
- ifs.push(IfClause {
- r#if: Some(producer.handle_expr(e, schema)?),
- then: None,
- });
+
+ // Substrait's `IfThen` has no notion of a base expression: every
`IfClause`
+ // is a standalone boolean condition. A `CASE <base> WHEN <value> THEN ...`
+ // is therefore emitted as `IfClause`s over `<base> = <value>`, the same
+ // desugaring `from_between` applies to `BETWEEN`. DataFusion matches a
base
+ // expression with `=` semantics, so this preserves the plan's meaning,
+ // including a `NULL` `<value>` never matching.
+ //
+ // The base is written once per WHEN, which a volatile base would then
+ // evaluate once per arm. `CaseExpr` evaluates it once and compares every
+ // WHEN against that one value, so such a plan has no faithful `IfThen`
+ // encoding and is rejected instead.
+ if let Some(base) = expr
+ && is_volatile_including_subqueries(base)?
+ {
+ return not_impl_err!(
+ "Substrait does not support a volatile CASE base expression:
{base}"
+ );
}
- // Parse `when`s
- for (r#if, then) in when_then_expr {
+
+ let mut ifs: Vec<IfClause> = Vec::with_capacity(when_then_expr.len());
+ for (when, then) in when_then_expr {
+ let condition = match expr {
+ Some(base) => {
+ let eq = Expr::eq(*base.clone(), *when.clone());
Review Comment:
You're right, and I reproduced it before changing anything.
`case_when_with_expr` fills the result for rows whose base is NULL from ELSE
(`case.rs:812`) and then removes them from the remainder batch
(`case.rs:837-842`), so the first WHEN never sees them. `<base> = <when>`
evaluates both operands, so the emitted plan did:
`SELECT CASE a WHEN 10 / b THEN 'x' ELSE 'y' END FROM (VALUES (1, 1), (NULL,
0)) AS t(a, b)`
| plan | result |
| --- | --- |
| base CASE | `y`, `y` |
| emitted, before | `Arrow error: Divide by zero error` |
| emitted, after | `y`, `y` |
Fixed in 908ea39 by emitting that skip as a leading clause, `<base> IS NULL`
yielding the ELSE value, rather than by rejecting. The clause is exactly what
`CaseExpr` does for those rows, so the conditions keep their order and meaning.
It is only added where it can matter: a nullable base whose WHEN operands
are not all literals or columns. Reading a literal or a column on those rows
cannot fail and has no side effect, so `CASE <base> WHEN <literal> ...` keeps
the exact encoding it had, and the existing round trip and protobuf tests are
unchanged. That is also why I did not reject non-literal WHEN operands outright
— `CASE a WHEN b THEN ...` over columns is safe as it stands, and rejecting it
would drop plans that round trip correctly today. If you would rather have one
uniform encoding, emitting the guard for every nullable base is a one line
change.
On the ELSE appearing twice: that matches `CaseExpr`, which evaluates the
else expression separately for the NULL base rows before the remainder falls
through to it. Measured with a counter UDF as the ELSE over rows `1, NULL, 7`:
two invocations natively and two after the round trip, same results.
Coverage, next to the earlier tests:
- `case_with_null_base_does_not_evaluate_when_operands` — the erroring WHEN
above, native and round trip compared; the same CASE with no ELSE, where both
return NULL; and a negative control whose erroring row has a non-NULL base,
which must still fail on both sides.
- `case_with_null_base_emits_guard_clause` and
`case_with_non_nullable_base_emits_no_null_guard` — the protobuf shape, with
the guard present and absent.
With the guard removed both new tests fail, the first with
`ArrowError(DivideByZero)`. `cargo test -p datafusion-substrait` (215 + 58 + 3)
and `./ci/scripts/rust_clippy.sh` pass, and the branch is rebased onto main.
--
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]
