Skip to content

fix: Emit equality conditions for Substrait CASE base expressions - #25191

Open
namanjain24-sudo wants to merge 1 commit into
apache:mainfrom
namanjain24-sudo:fix-substrait-case-base-expression
Open

fix: Emit equality conditions for Substrait CASE base expressions#25191
namanjain24-sudo wants to merge 1 commit into
apache:mainfrom
namanjain24-sudo:fix-substrait-case-base-expression

Conversation

@namanjain24-sudo

Copy link
Copy Markdown

Which issue does this PR close?

Rationale for this change

Substrait's IfThen has no base expression. Every IfClause is a standalone
boolean condition, and then is the value that clause yields.

The producer used IfThen for CASE <base> WHEN <value> THEN ... anyway, by
pushing a leading IfClause that carries the base expression in if and leaves
then unset, then one clause per WHEN whose if is the raw WHEN operand. For

SELECT CASE a WHEN 1 THEN 'x' WHEN 2 THEN 'y' ELSE 'z' END FROM data

that emits three clauses whose conditions are a, 1 and 2. All three are
i64, and the first has no result at all.

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 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_case in producer/expr/if_then.rs now emits one clause per WHEN, with
<base> = <value> as the condition when a base expression is present. This is
the same desugaring from_between, in the same crate, already applies to
BETWEEN. The searched form, which was already correct, is unchanged.

DataFusion matches a base expression with = semantics (compare_with_eq uses
Arrow's eq), so the plan keeps its meaning, including a NULL WHEN operand
never matching.

Two things I deliberately did not do, and would rather handle separately:

  • SwitchExpression. Substrait does have a switch construct, but its
    IfValue.if is a Literal, so it cannot express CASE 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 need
    this desugaring as the fallback, so the correctness fix is worth having on its
    own.
  • The consumer. It still accepts a then-less first clause as a base
    expression. 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 CASE it round trips back into
evaluates it per arm. For a column reference that is free; for an expensive base
expression it is not. SwitchExpression support would remove the duplication for
the all-literal subset.

What is the testing strategy for this PR?

  • case_with_base_expression_emits_equality_conditions in
    tests/cases/serialize.rs walks the produced protobuf directly and asserts one
    clause per WHEN, every clause having both a condition and a then, and every
    condition being a call to the registered equal function. It inspects the
    protobuf rather than round-tripping because the consumer understands the old
    encoding, so a round trip cannot catch this. Verified it fails on main:

    assertion failed: !equal_anchors.is_empty()
    no `equal` function registered
    
  • case_with_base_expression in tests/cases/roundtrip_logical_plan.rs moves
    from roundtrip to assert_expected_plan, recording that a base CASE now
    comes back as the equivalent searched CASE. It still asserts the schema is
    unchanged; the projection keeps its original name via an alias.

  • cargo test -p datafusion-substrait passes (272 tests), as does
    cargo xtask ci step test substrait, and ./ci/scripts/rust_clippy.sh,
    rust_fmt.sh, typos_check.sh and rust_docs.sh are clean.

Are there any user-facing changes?

Plans produced by to_substrait_plan for a CASE with a base expression now
carry boolean equal conditions instead of the previous encoding. No Rust API
changes.

Consumers that implemented DataFusion's then-less convention will see the new
form; it is valid Substrait, so a spec-conforming consumer reads it correctly.
Within DataFusion, such a plan now round trips into the equivalent searched
CASE rather than the base form, with the same schema and results.

@github-actions github-actions Bot added the substrait Changes to the substrait crate label Sep 11, 2026
@namanjain24-sudo

Copy link
Copy Markdown
Author

@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 cargo test -p datafusion-substrait (272 tests), cargo xtask ci step test substrait, and the rust_clippy.sh, rust_fmt.sh, typos_check.sh and rust_docs.sh steps of the lint suite.

On the change itself: the one judgement call is desugaring CASE <base> WHEN <value> into <base> = <value> conditions rather than emitting SwitchExpression. I went that way because SwitchExpression.IfValue.if is a Literal, so it cannot express a non-literal WHEN operand, and our consumer currently answers not_impl_err!("Switch expression not supported"), so emitting it would break our own round trip. The trade-off is that the base expression is now repeated once per WHEN arm. I described both in #25190 and would be glad to take the PR in a different direction if you'd prefer.

@namanjain24-sudo
namanjain24-sudo force-pushed the fix-substrait-case-base-expression branch from b9ff008 to 2848da2 Compare September 11, 2026 16:02
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.
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.93%. Comparing base (f8cc678) to head (b87b857).

Files with missing lines Patch % Lines
...ubstrait/src/logical_plan/producer/expr/if_then.rs 75.00% 0 Missing and 2 partials ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

substrait Changes to the substrait crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Substrait producer emits non-boolean IfThen clauses for CASE with a base expression

2 participants