kosiew commented on code in PR #25191:
URL: https://github.com/apache/datafusion/pull/25191#discussion_r4021881861
##########
datafusion/substrait/tests/cases/serialize.rs:
##########
@@ -321,6 +322,75 @@ mod tests {
Ok(())
}
+ /// Substrait's `IfThen` has no base expression: every `IfClause` is a
+ /// standalone boolean condition and `then` is the value that clause
yields.
+ /// A `CASE <base> WHEN <value> ...` must therefore be emitted as
conditions
+ /// over `<base> = <value>`. A round trip cannot catch a regression here,
+ /// because the consumer reads back whatever the producer writes.
+ #[tokio::test]
+ async fn case_with_base_expression_emits_equality_conditions() ->
Result<()> {
+ let ctx = create_context().await?;
+ let sql = "SELECT CASE a WHEN 1 THEN 'x' WHEN 2 THEN 'y' ELSE 'z' END
FROM data";
+
+ let plan = ctx.sql(sql).await?.into_optimized_plan()?;
+ let proto = to_substrait_plan(&plan, &ctx.state())?;
+
+ let equal_anchors: Vec<u32> = proto
+ .extensions
+ .iter()
+ .filter_map(|e| match e.mapping_type.as_ref().unwrap() {
+ MappingType::ExtensionFunction(f) if f.name == "equal" => {
+ Some(f.function_anchor)
+ }
+ _ => None,
+ })
+ .collect();
+ assert!(!equal_anchors.is_empty(), "no `equal` function registered");
+
+ let root = match proto.relations.first().unwrap().rel_type.as_ref() {
+ Some(RelType::Root(root)) => root.input.as_ref().unwrap(),
+ _ => panic!("expected Root"),
+ };
+ let Some(rel::RelType::Project(project)) = root.rel_type.as_ref() else
{
+ panic!("expected Project")
+ };
+
+ let if_thens: Vec<&IfThen> = project
+ .expressions
+ .iter()
+ .filter_map(|expr| match expr.rex_type.as_ref() {
+ Some(RexType::IfThen(if_then)) => Some(if_then.as_ref()),
+ _ => None,
+ })
+ .collect();
+ assert_eq!(if_thens.len(), 1, "expected one IfThen for `{sql}`");
+ let if_then = if_thens[0];
+
+ // One clause per WHEN, with no extra clause carrying the base
expression.
+ assert_eq!(if_then.ifs.len(), 2);
+ assert!(if_then.r#else.is_some());
+
+ for (i, clause) in if_then.ifs.iter().enumerate() {
Review Comment:
Nice to have the protobuf-level check here. Could we make this a little
stronger by asserting the two equality arguments as well? In particular, verify
that each condition is `equal(base_field_a, expected_when_literal)` for `1` and
`2` in order. As written, the test would still pass if the operands were
swapped, the wrong literal were used, or the equality referred to unrelated
expressions.
##########
datafusion/substrait/src/logical_plan/producer/expr/if_then.rs:
##########
@@ -32,19 +32,24 @@ 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,
- });
- }
- // Parse `when`s
- for (r#if, then) in when_then_expr {
+
+ // 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.
+ 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 this changes the semantics for volatile base expressions.
DataFusion's physical `CaseExpr` evaluates the base once and then compares each
`WHEN` against that retained value
(`datafusion/physical-expr/src/expressions/case.rs:792-798, 849-859`). With
this desugaring, `CASE <volatile> WHEN ...` becomes a sequence of independently
evaluated `<volatile> = <when>` predicates, so the base can produce a different
value for each arm and select a different branch or fall through to `ELSE`.
DataFusion defines volatility as allowing a different result on repeated
evaluation and already avoids duplicating volatile expressions in other places
(`datafusion/expr/src/expr.rs:2164-2179`;
`datafusion/substrait/src/logical_plan/consumer/rel/mod.rs:93-99`). I think we
need to preserve single evaluation at the serialization boundary.
`SwitchExpression` looks like it could represent literal `WHEN` values, but
the DataFusion consumer currently rejects it, so using it would also need
consumer support and roundtrip coverage. For base `CASE` forms that cannot be
represented without duplicating evaluation, I think volatile bases should be
rejected unless there is another semantics-preserving Substrait encoding.
Could you also add a regression using an instrumented volatile UDF or
counter rather than `random()`? That would make the duplicate evaluation and
changed result deterministic.
--
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]
