adriangb commented on code in PR #23844:
URL: https://github.com/apache/datafusion/pull/23844#discussion_r3639419068
##########
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
Review Comment:
The GROUP BY and the second column aren't load-bearing here — the failure is
the aggregate's *input*-schema nullability check (`schema_satisfied_by`), which
fires regardless of grouping. All that's essential is the subquery (so the
`CASE` lands in a projection that becomes the aggregate's input schema) plus an
aggregate that references the column (so projection pushdown keeps it). A bare
`count(endpoint)` still errors with the exact `(physical) true vs (logical)
false` mismatch on `main`:
```suggestion
query I
SELECT count(endpoint)
FROM (
SELECT CASE
WHEN a IS NOT NULL THEN CAST(a AS BIGINT)
ELSE CAST(0 AS BIGINT)
END AS endpoint
FROM foo
)
----
6
```
##########
datafusion/physical-expr/src/expressions/case.rs:
##########
@@ -1537,6 +1543,19 @@ fn replace_with_null(
Ok(with_null)
}
+/// Returns the innermost [`PhysicalExpr`] that is provably null if `expr` is
null.
Review Comment:
Worth a keep-in-sync note here: this is a line-for-line mirror of the
logical `unwrap_certainly_null_expr` in `datafusion/expr/src/expr_schema.rs`,
and if the two ever disagree on which wrappers are null-preserving, logical vs
physical `CASE` nullability diverges — which is exactly this class of schema
mismatch. A cross-reference makes the contract explicit:
```suggestion
/// 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).
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
