namanjain24-sudo commented on code in PR #25191:
URL: https://github.com/apache/datafusion/pull/25191#discussion_r4040052572
##########
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:
You're right again, thanks. I reproduced it before changing anything:
`tree_node.rs:83` lists `Expr::ScalarSubquery` among the arms that return
`Continue` without visiting children, so `is_volatile` cannot see into the
plan. Running your example through the producer, `CASE (SELECT call_counter())
WHEN 2 THEN 20 WHEN 1 THEN 10 ELSE 99 END` was accepted and `call_counter`
appeared twice in the emitted protobuf, once per condition.
Fixed in 62a8970 with plan-aware detection rather than by rejecting subquery
bases, so that a base like `(SELECT max(a) FROM data)` still works:
```rust
fn is_volatile_including_subqueries(expr: &Expr) -> Result<bool> {
expr.exists(|expr| match expr {
Expr::ScalarSubquery(subquery)
| Expr::Exists(Exists { subquery, .. })
| Expr::InSubquery(InSubquery { subquery, .. })
| Expr::SetComparison(SetComparison { subquery, .. }) => {
plan_is_volatile(&subquery.subquery)
}
expr => Ok(expr.is_volatile_node()),
})
}
```
`plan_is_volatile` walks the plan's expressions and calls back into this, so
a subquery nested inside a subquery is covered as well. The other three
variants are there because a base is not restricted to a scalar subquery.
The test you asked for is in the same case as the earlier one, using the
counter UDF:
- `CASE (SELECT call_counter()) WHEN 2 ... WHEN 1 ... END` is rejected;
- `CASE (SELECT max(a) FROM data) WHEN 2 ... END` still serializes, so the
guard has not become a blanket ban on subquery bases.
With the check reverted to `is_volatile()`, that first assertion fails and
the plan it produces carries the scalar subquery in both `IfClause` conditions,
which is the duplication you described.
--
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]
