fix: Emit equality conditions for Substrait CASE base expressions - #25191
fix: Emit equality conditions for Substrait CASE base expressions#25191namanjain24-sudo wants to merge 1 commit into
Conversation
|
@gabotechs @kosiew when you have a moment, would one of you be able to trigger the workflows on this PR? The contributor guide notes a committer has to do that for a new contributor, and no checks have run here yet. Locally this passes On the change itself: the one judgement call is desugaring |
b9ff008 to
2848da2
Compare
Substrait's `IfThen` has no base expression. Every `IfClause` is a standalone boolean condition, and `then` is the value that clause yields. The producer instead encoded `CASE <base> WHEN <value> THEN ...` by pushing a leading `IfClause` that carries the base expression in `if` and leaves `then` unset, followed by one clause per WHEN whose `if` is the raw WHEN operand. For `CASE a WHEN 1 THEN 'x' WHEN 2 THEN 'y' ELSE 'z' END` that emits three clauses whose conditions are `a`, `1` and `2`, none of which is boolean, and a first clause with no result. The convention is private to DataFusion: the consumer reads a `then`-less first clause back as the base expression, so a DataFusion-to-DataFusion round trip is unaffected. Any other engine sees clauses it cannot evaluate. Emit `<base> = <value>` as each clause condition instead, the same desugaring `from_between` already applies to `BETWEEN`. DataFusion matches a base expression with `=` semantics, so the plan keeps its meaning, including a NULL WHEN operand never matching. A base `CASE` now round trips as the equivalent searched `CASE`, keeping its original projection name and schema.
2848da2 to
b87b857
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25191 +/- ##
==========================================
- Coverage 81.93% 81.93% -0.01%
==========================================
Files 1133 1133
Lines 423529 423529
Branches 423529 423529
==========================================
- Hits 347028 347018 -10
- Misses 55910 55920 +10
Partials 20591 20591 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Which issue does this PR close?
Rationale for this change
Substrait's
IfThenhas no base expression. EveryIfClauseis a standaloneboolean condition, and
thenis the value that clause yields.The producer used
IfThenforCASE <base> WHEN <value> THEN ...anyway, bypushing a leading
IfClausethat carries the base expression inifand leavesthenunset, then one clause per WHEN whoseifis the raw WHEN operand. Forthat emits three clauses whose conditions are
a,1and2. All three arei64, and the first has no result at all.The convention is private to DataFusion: the consumer reads a
then-less firstclause back as the base expression, so a DataFusion-to-DataFusion round trip is
unaffected and no existing test failed. An engine that reads the plan as
Substrait defines it sees clauses it cannot evaluate.
What changes are included in this PR?
from_caseinproducer/expr/if_then.rsnow emits one clause per WHEN, with<base> = <value>as the condition when a base expression is present. This isthe same desugaring
from_between, in the same crate, already applies toBETWEEN. The searched form, which was already correct, is unchanged.DataFusion matches a base expression with
=semantics (compare_with_equsesArrow's
eq), so the plan keeps its meaning, including a NULL WHEN operandnever matching.
Two things I deliberately did not do, and would rather handle separately:
SwitchExpression. Substrait does have a switch construct, but itsIfValue.ifis aLiteral, so it cannot expressCASE a WHEN b + 1 THEN ...,and our consumer currently answers
not_impl_err!("Switch expression not supported"). Emitting it would need consumer support and would still needthis desugaring as the fallback, so the correctness fix is worth having on its
own.
then-less first clause as a baseexpression. Removing that would break reading plans written by older
DataFusion versions, so it seemed better left to its own discussion.
One trade-off worth calling out: a base expression is now repeated once per WHEN
arm in the emitted plan, and the searched
CASEit round trips back intoevaluates it per arm. For a column reference that is free; for an expensive base
expression it is not.
SwitchExpressionsupport would remove the duplication forthe all-literal subset.
What is the testing strategy for this PR?
case_with_base_expression_emits_equality_conditionsintests/cases/serialize.rswalks the produced protobuf directly and asserts oneclause per WHEN, every clause having both a condition and a
then, and everycondition being a call to the registered
equalfunction. It inspects theprotobuf rather than round-tripping because the consumer understands the old
encoding, so a round trip cannot catch this. Verified it fails on
main:case_with_base_expressionintests/cases/roundtrip_logical_plan.rsmovesfrom
roundtriptoassert_expected_plan, recording that a baseCASEnowcomes back as the equivalent searched
CASE. It still asserts the schema isunchanged; the projection keeps its original name via an alias.
cargo test -p datafusion-substraitpasses (272 tests), as doescargo xtask ci step test substrait, and./ci/scripts/rust_clippy.sh,rust_fmt.sh,typos_check.shandrust_docs.share clean.Are there any user-facing changes?
Plans produced by
to_substrait_planfor aCASEwith a base expression nowcarry boolean
equalconditions instead of the previous encoding. No Rust APIchanges.
Consumers that implemented DataFusion's
then-less convention will see the newform; it is valid Substrait, so a spec-conforming consumer reads it correctly.
Within DataFusion, such a plan now round trips into the equivalent searched
CASErather than the base form, with the same schema and results.