kosiew commented on code in PR #25191:
URL: https://github.com/apache/datafusion/pull/25191#discussion_r4044041532
##########
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:
I think there is still a semantic mismatch here when the CASE base is NULL.
`CaseExpr::case_when_with_expr` removes rows with a NULL base before
evaluating the `when_expr`
(`datafusion/physical-expr/src/expressions/case.rs:792-846`). With this
conversion, `equal(base, when)` evaluates both operands of the equality
(`BinaryExpr::evaluate`,
`datafusion/physical-expr/src/expressions/binary.rs:542-544`).
For example, `CASE nullable_a WHEN call_counter() THEN ... ELSE ... END`
does not invoke `call_counter()` on rows where `a` is NULL in the native base
CASE, but the emitted equality does. An erroring non-constant WHEN expression
is more concerning: something like division by a zero-valued column could now
fail on a row where the native CASE would skip the WHEN expression and return
ELSE.
The volatile-base check does not cover this because the extra evaluation is
in the WHEN operand rather than the base. Could we either use an encoding that
preserves this short circuit, or reject base CASE forms that cannot be
represented safely, including non-literal WHEN operands? It would also be good
to add NULL-base regression coverage using a counter or erroring WHEN
expression.
--
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]
