kosiew commented on code in PR #25191:
URL: https://github.com/apache/datafusion/pull/25191#discussion_r4034043991
##########
datafusion/substrait/src/logical_plan/producer/expr/if_then.rs:
##########
@@ -32,19 +32,35 @@ 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.as_ref().filter(|base| base.is_volatile()) {
Review Comment:
I think there is still a volatility edge case here. `Expr::is_volatile()`
only walks the expression tree and does not inspect the logical plan inside a
`ScalarSubquery`.
For example, a base like `CASE (SELECT call_counter()) WHEN 2 THEN ... WHEN
1 THEN ... END` can pass this guard. During serialization, though, that scalar
subquery is cloned into each `equal(base, when)` condition. The original
`CaseExpr` evaluates the base once, while the emitted `IfThen` may execute the
volatile subquery once per arm.
Could we either reject scalar-subquery bases here or add plan-aware
volatility detection? It would also be good to add a regression test with a
volatile UDF inside a scalar-subquery base.
--
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]
