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:
|
|
Hi @namanjain24-sudo, thanks for the PR! Could I ask to summarize and condense a bit the PR description? Feel free to keep anything that is not already implicit in the code itself, aim for a 2 or 3 min read summary. That would help reviewers better digest the PR. Thanks! |
|
Thanks, trimmed it. What I cut (the local test log, and the longer reasoning for leaving On the red check, I don't think it's this PR: it's |
Which issue does this PR close?
Rationale for this change
Substrait's
IfThenhas no base expression. The spec writes every clause asif <boolean expression> then <result expression>.The producer used
IfThenforCASE <base> WHEN <value>anyway, through a convention private to DataFusion: a first clause holding the base inifwiththenleft unset, then one clause perWHENcarrying the rawWHENoperand.SELECT CASE a WHEN 1 THEN 'x' WHEN 2 THEN 'y' ELSE 'z' ENDcame out as three clauses whose conditions are the field referenceaand the literals1and2. None is boolean, and the first has no result. Our consumer reads the convention back, so round trips were unaffected and no test failed. Another engine sees clauses it cannot evaluate.What changes are included in this PR?
from_casenow emits one clause perWHEN, with<base> = <value>as the condition. That is the desugaringfrom_betweenalready uses forBETWEEN, and it is how the spec describes a switch: an if expression whose conditions are all equality against the same value. The searched form is unchanged. Meaning is preserved, because DataFusion matches a baseCASEwith the same equality kernels=lowers to.Left out deliberately:
SwitchExpressionitself. ItsIfValue.ifis aLiteral, so it cannot holdCASE a WHEN b + 1, and our consumer answersnot_impl_err!("Switch expression not supported").The cost is that the base expression is repeated once per
WHENarm, and evaluated per arm.What is the testing strategy for this PR?
case_with_base_expression_emits_equality_conditionsinspects the produced protobuf instead of round-tripping, because the consumer understands the old encoding and a round trip cannot catch this.case_with_base_expressionmoves toassert_expected_planto record the new shape, still asserting the schema is unchanged.Are there any user-facing changes?
A base
CASEnow serialises toequalcalls with a boolean output type. No Rust API changes. Inside DataFusion the plan round trips into the equivalent searchedCASE, with the same schema and results. A consumer that implemented the old convention sees the new form, which is valid Substrait.