From f13963276df9fdecf6c775f6bae00119e43adcdc Mon Sep 17 00:00:00 2001 From: Matthew Kim <38759997+friendlymatthew@users.noreply.github.com> Date: Thu, 23 Jul 2026 10:15:29 -0400 Subject: [PATCH 1/6] fix: align physical CASE nullability through casts --- .../physical-expr/src/expressions/case.rs | 43 ++++++++++++++++++- datafusion/sqllogictest/test_files/case.slt | 18 ++++++++ 2 files changed, 59 insertions(+), 2 deletions(-) diff --git a/datafusion/physical-expr/src/expressions/case.rs b/datafusion/physical-expr/src/expressions/case.rs index 8a0f15467c47b..864f85a933220 100644 --- a/datafusion/physical-expr/src/expressions/case.rs +++ b/datafusion/physical-expr/src/expressions/case.rs @@ -19,7 +19,9 @@ mod literal_lookup_table; use super::{Column, Literal}; use crate::PhysicalExpr; -use crate::expressions::{LambdaVariable, lit, try_cast}; +use crate::expressions::{ + CastExpr, LambdaVariable, NegativeExpr, NotExpr, lit, try_cast, +}; use arrow::array::*; use arrow::compute::kernels::zip::zip; use arrow::compute::{ @@ -1278,7 +1280,11 @@ impl PhysicalExpr for CaseExpr { // it would evaluate to null. // Replace the `then` expression with `NULL` in the `when` expression - let with_null = match replace_with_null(w, t.as_ref(), input_schema) { + let with_null = match replace_with_null( + w, + unwrap_certainly_null_expr(t.as_ref()), + input_schema, + ) { Err(e) => return Some(Err(e)), Ok(e) => e, }; @@ -1537,6 +1543,19 @@ fn replace_with_null( Ok(with_null) } +/// Returns the innermost [`PhysicalExpr`] that is provably null if `expr` is null. +fn unwrap_certainly_null_expr(expr: &dyn PhysicalExpr) -> &dyn PhysicalExpr { + if let Some(expr) = expr.downcast_ref::() { + unwrap_certainly_null_expr(expr.arg().as_ref()) + } else if let Some(expr) = expr.downcast_ref::() { + unwrap_certainly_null_expr(expr.arg().as_ref()) + } else if let Some(expr) = expr.downcast_ref::() { + unwrap_certainly_null_expr(expr.expr.as_ref()) + } else { + expr + } +} + /// Create a CASE expression pub fn case( expr: Option>, @@ -2577,10 +2596,20 @@ mod tests { let zero = lit(0); let foo_eq_zero = binary(Arc::clone(&foo), Operator::Eq, Arc::clone(&zero), &schema)?; + let cast_foo = cast(Arc::clone(&foo), &schema, DataType::Int64)?; + let negative_foo = expressions::negative(Arc::clone(&foo), &schema)?; assert_not_nullable(when_then_else(&foo_is_not_null, &foo, &zero)?, &schema); assert_not_nullable(when_then_else(¬_foo_is_null, &foo, &zero)?, &schema); assert_not_nullable(when_then_else(&foo_eq_zero, &foo, &zero)?, &schema); + assert_not_nullable( + when_then_else(&foo_is_not_null, &cast_foo, &lit(0i64))?, + &schema, + ); + assert_not_nullable( + when_then_else(&foo_is_not_null, &negative_foo, &zero)?, + &schema, + ); assert_not_nullable( when_then_else( @@ -2702,6 +2731,16 @@ mod tests { &schema, ); + let boolean_schema = + Schema::new(vec![Field::new("predicate", DataType::Boolean, true)]); + let predicate = col("predicate", &boolean_schema)?; + let predicate_is_not_null = is_not_null(Arc::clone(&predicate))?; + let not_predicate = expressions::not(Arc::clone(&predicate))?; + assert_not_nullable( + when_then_else(&predicate_is_not_null, ¬_predicate, &lit(false))?, + &boolean_schema, + ); + Ok(()) } diff --git a/datafusion/sqllogictest/test_files/case.slt b/datafusion/sqllogictest/test_files/case.slt index 3953878ceb666..e70bb1bbc9e06 100644 --- a/datafusion/sqllogictest/test_files/case.slt +++ b/datafusion/sqllogictest/test_files/case.slt @@ -41,6 +41,24 @@ NULL 6 7 +# CASE nullability remains consistent through type coercion +query II rowsort +SELECT endpoint, count(*) +FROM ( + SELECT CASE + WHEN a IS NOT NULL THEN CAST(a AS BIGINT) + ELSE CAST(0 AS BIGINT) + END AS endpoint + FROM foo +) +GROUP BY endpoint +---- +0 2 +1 1 +3 1 +5 1 +6 1 + # column or explicit null query I SELECT CASE WHEN a > 2 THEN b ELSE null END FROM foo From 253ab099b4f20b6ab86e535e6619cf4d15766ea1 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Thu, 23 Jul 2026 10:58:17 -0500 Subject: [PATCH 2/6] test: cover nested wrappers and TRY_CAST in physical CASE nullability Adds unit tests that lock in two properties of unwrap_certainly_null_expr: - Nested null-preserving wrappers (e.g. CAST(-foo)) are unwrapped recursively, so the guarded branch is still proven unreachable-as-null. - TRY_CAST is intentionally NOT unwrapped: it can yield NULL on a failed cast even for a non-null input, so a guarded TRY_CAST branch keeps the CASE nullable. This guards the logical/physical sync invariant against someone later unwrapping TRY_CAST on only one side. Co-Authored-By: Claude Opus 4.8 --- .../physical-expr/src/expressions/case.rs | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/datafusion/physical-expr/src/expressions/case.rs b/datafusion/physical-expr/src/expressions/case.rs index 864f85a933220..fd2303d4f6eeb 100644 --- a/datafusion/physical-expr/src/expressions/case.rs +++ b/datafusion/physical-expr/src/expressions/case.rs @@ -2611,6 +2611,31 @@ mod tests { &schema, ); + // Nested null-preserving wrappers must be unwrapped recursively. `CAST(-foo)` + // still collapses `foo IS NOT NULL` to `false`, so the branch is + // unreachable-as-null and the `CASE` is not nullable. + let cast_negative_foo = cast( + expressions::negative(Arc::clone(&foo), &schema)?, + &schema, + DataType::Int64, + )?; + assert_not_nullable( + when_then_else(&foo_is_not_null, &cast_negative_foo, &lit(0i64))?, + &schema, + ); + + // `TRY_CAST` is intentionally NOT treated as null-preserving: it yields + // NULL on a failed cast even for a non-null input, so a guarded `TRY_CAST` + // branch is still reachable-as-null and the `CASE` stays nullable. This must + // stay consistent with the logical planner (`unwrap_certainly_null_expr` in + // `datafusion/expr/src/expr_schema.rs`); unwrapping it on only one side would + // reintroduce a logical/physical schema mismatch. + let try_cast_foo = try_cast(Arc::clone(&foo), &schema, DataType::Int64)?; + assert_nullable( + when_then_else(&foo_is_not_null, &try_cast_foo, &lit(0i64))?, + &schema, + ); + assert_not_nullable( when_then_else( &binary( @@ -2741,6 +2766,13 @@ mod tests { &boolean_schema, ); + // Nested `NOT` is likewise unwrapped recursively. + let not_not_predicate = expressions::not(Arc::clone(¬_predicate))?; + assert_not_nullable( + when_then_else(&predicate_is_not_null, ¬_not_predicate, &lit(false))?, + &boolean_schema, + ); + Ok(()) } From 430c6635e0132ceae2a6da3f78450aa097f92b7d Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Thu, 23 Jul 2026 13:31:23 -0500 Subject: [PATCH 3/6] Apply suggestions from code review Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> --- datafusion/physical-expr/src/expressions/case.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/datafusion/physical-expr/src/expressions/case.rs b/datafusion/physical-expr/src/expressions/case.rs index fd2303d4f6eeb..02f2b5c11fab6 100644 --- a/datafusion/physical-expr/src/expressions/case.rs +++ b/datafusion/physical-expr/src/expressions/case.rs @@ -1544,6 +1544,12 @@ fn replace_with_null( } /// Returns the innermost [`PhysicalExpr`] that is provably null if `expr` is null. +/// +/// Keep this in sync with the logical-plan equivalent, `unwrap_certainly_null_expr` +/// in `datafusion/expr/src/expr_schema.rs`. If the two disagree on which wrappers +/// are null-preserving, `CASE` nullability computed by the logical and physical +/// planners can diverge and cause a schema mismatch during planning (the bug this +/// PR fixes). fn unwrap_certainly_null_expr(expr: &dyn PhysicalExpr) -> &dyn PhysicalExpr { if let Some(expr) = expr.downcast_ref::() { unwrap_certainly_null_expr(expr.arg().as_ref()) From cea9b11b5663af1492c29074c7deba5416a7dc09 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Thu, 23 Jul 2026 13:32:14 -0500 Subject: [PATCH 4/6] Apply suggestion from @adriangb --- datafusion/sqllogictest/test_files/case.slt | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/datafusion/sqllogictest/test_files/case.slt b/datafusion/sqllogictest/test_files/case.slt index e70bb1bbc9e06..f7ae380242942 100644 --- a/datafusion/sqllogictest/test_files/case.slt +++ b/datafusion/sqllogictest/test_files/case.slt @@ -42,8 +42,8 @@ NULL 7 # CASE nullability remains consistent through type coercion -query II rowsort -SELECT endpoint, count(*) +query I +SELECT count(endpoint) FROM ( SELECT CASE WHEN a IS NOT NULL THEN CAST(a AS BIGINT) @@ -51,13 +51,8 @@ FROM ( END AS endpoint FROM foo ) -GROUP BY endpoint ---- -0 2 -1 1 -3 1 -5 1 -6 1 +6 # column or explicit null query I From 12e722ca26308b88d4df1f11d44b59f9c1695071 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Thu, 23 Jul 2026 14:58:12 -0500 Subject: [PATCH 5/6] Apply suggestion from @adriangb --- datafusion/physical-expr/src/expressions/case.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/datafusion/physical-expr/src/expressions/case.rs b/datafusion/physical-expr/src/expressions/case.rs index 02f2b5c11fab6..7e0f9e9b54a93 100644 --- a/datafusion/physical-expr/src/expressions/case.rs +++ b/datafusion/physical-expr/src/expressions/case.rs @@ -1548,8 +1548,8 @@ fn replace_with_null( /// Keep this in sync with the logical-plan equivalent, `unwrap_certainly_null_expr` /// in `datafusion/expr/src/expr_schema.rs`. If the two disagree on which wrappers /// are null-preserving, `CASE` nullability computed by the logical and physical -/// planners can diverge and cause a schema mismatch during planning (the bug this -/// PR fixes). +/// planners can diverge and cause a schema mismatch during planning. +/// See https://github.com/apache/datafusion/pull/23844 for rationale. fn unwrap_certainly_null_expr(expr: &dyn PhysicalExpr) -> &dyn PhysicalExpr { if let Some(expr) = expr.downcast_ref::() { unwrap_certainly_null_expr(expr.arg().as_ref()) From 49e1b1238b15ff7bb00418a4b3c9db5960b3e592 Mon Sep 17 00:00:00 2001 From: Matthew Kim <38759997+friendlymatthew@users.noreply.github.com> Date: Fri, 24 Jul 2026 10:38:10 -0400 Subject: [PATCH 6/6] fix: use rustdoc autolink for PR reference --- datafusion/physical-expr/src/expressions/case.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datafusion/physical-expr/src/expressions/case.rs b/datafusion/physical-expr/src/expressions/case.rs index 7e0f9e9b54a93..17288a9737699 100644 --- a/datafusion/physical-expr/src/expressions/case.rs +++ b/datafusion/physical-expr/src/expressions/case.rs @@ -1549,7 +1549,7 @@ fn replace_with_null( /// in `datafusion/expr/src/expr_schema.rs`. If the two disagree on which wrappers /// are null-preserving, `CASE` nullability computed by the logical and physical /// planners can diverge and cause a schema mismatch during planning. -/// See https://github.com/apache/datafusion/pull/23844 for rationale. +/// See for rationale. fn unwrap_certainly_null_expr(expr: &dyn PhysicalExpr) -> &dyn PhysicalExpr { if let Some(expr) = expr.downcast_ref::() { unwrap_certainly_null_expr(expr.arg().as_ref())