fix: align physical CASE nullability through casts#23844
Conversation
github-actions
Bot
added
physical-expr
| END AS endpoint | ||
| FROM foo | ||
| ) | ||
| GROUP BY endpoint |
There was a problem hiding this comment.
Is there a simpler version of this e.g. without a groupby / aggregation?
There was a problem hiding this comment.
This seems to be the simplest repro. At the sql level, a plain projection still executes because nothing consumes the conflicting nullability. The aggregate is what exposes the logical/physical schema mismatch
adriangb
left a comment
adriangb
left a comment
There was a problem hiding this comment.
Nice fix — mirroring the logical unwrap_certainly_null_expr exactly is the right call, since the failure mode is precisely the logical and physical sides disagreeing. Two small committable suggestions below (minimal repro + a keep-in-sync doc note).
One thing that isn't inline-commentable because the file isn't in this diff: it'd be worth adding the reciprocal note on the logical side too — a // keep in sync with the physical-expr version in datafusion/physical-expr/src/expressions/case.rs on unwrap_certainly_null_expr in datafusion/expr/src/expr_schema.rs — so the contract is visible from both ends.
Optional extra coverage: a case pinning TRY_CAST as intentionally not unwrapped (i.e. WHEN x IS NOT NULL THEN TRY_CAST(x AS ...) stays nullable on both sides) would guard the sync invariant against someone later unwrapping it on only one side; and a nested-wrapper case (e.g. CAST(-x)) would lock in the recursion.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23844 +/- ##
==========================================
- Coverage 80.75% 80.71% -0.04%
==========================================
Files 1089 1090 +1
Lines 368866 370384 +1518
Branches 368866 370384 +1518
==========================================
+ Hits 297871 298949 +1078
- Misses 53241 53611 +370
- Partials 17754 17824 +70 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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 <noreply@anthropic.com>
|
A couple suggested tests: pydantic#65 |
test: cover nested wrappers and TRY_CAST in physical CASE nullability
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Head branch was pushed to by a user without write access
3 participants
Rationale for this change
Logical
CASEnullability unwraps null preserving casts before analyzing guarded branches, but physicalCASEnullability did not. Type coercion could therefore produce conflicting schemas and cause valid aggregation queries to fail during planning