fix: preserve UPPER and LOWER input nullability - #25381
Conversation
|
Thanks @osipovartem , here is a suggestion The unit tests only call statement ok
CREATE TABLE case_nullability (a VARCHAR NOT NULL, b VARCHAR) AS VALUES ('Ab', NULL);
statement ok
CREATE TABLE case_nullability_out AS
SELECT upper(a) ua, lower(a) la, upper(b) ub FROM case_nullability;
query TTT
DESCRIBE case_nullability_out;
----
ua Utf8View NO
la Utf8View NO
ub Utf8View YES
query T
SELECT upper(t.a) FROM (SELECT 1 k) x LEFT JOIN case_nullability t ON false;
----
NULL |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25381 +/- ##
==========================================
- Coverage 81.93% 81.93% -0.01%
==========================================
Files 1136 1136
Lines 428959 429190 +231
Branches 428959 429190 +231
==========================================
+ Hits 351467 351656 +189
- Misses 56469 56488 +19
- Partials 21023 21046 +23 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
5c0e952 to
62d185a
Compare
|
Thanks for the suggestion. I added an end-to-end SQLLogicTest covering both sides of the nullability contract: UPPER/LOWER preserve non-nullable and nullable inputs in a CTAS schema, while a LEFT JOIN widens the input and UPPER correctly remains nullable. The focused SLT records and both UDF unit tests pass locally. I also rebased the branch onto the latest main. |
|
|
||
| statement ok | ||
| CREATE TABLE case_nullability_out AS | ||
| SELECT upper(a) AS ua, lower(a) AS la, upper(b) AS ub FROM case_nullability |
There was a problem hiding this comment.
you can use
> describe select upper('aaa');
+--------------------+-----------+-------------+
| column_name | data_type | is_nullable |
+--------------------+-----------+-------------+
| upper(Utf8("aaa")) | Utf8 | YES |
+--------------------+-----------+-------------+
There was a problem hiding this comment.
Thanks. I replaced the CTAS-based schema check with direct DESCRIBE SELECT coverage. The test now checks both ordinary input nullability and nullability widened by a LEFT JOIN; it also retains the execution assertion that the widened value evaluates to NULL.
comphead
left a comment
There was a problem hiding this comment.
Thanks @osipovartem lgtm
|
Thank you all! |
Which issue does this PR close?
Rationale for this change
UPPERandLOWERcurrently use the default scalar UDF result field, which is always nullable. Both functions return a non-null value for a non-null input, so this loses a provable schema property and exposes inaccurate result metadata to downstream consumers.What changes are included in this PR?
return_field_from_argsforUpperFuncandLowerFunc.The vectorized execution kernels are unchanged. This adds no per-row dispatch, conversion, allocation, or materialization.
Are these changes tested?
Yes:
cargo +1.95.0 test -p datafusion-functions preserves_input_nullabilitycargo +1.95.0 clippy -p datafusion-functions --lib --tests --all-features --no-deps -- -D warningscargo +1.95.0 fmt --all -- --checkAre there any user-facing changes?
Planned result schemas now retain non-nullability through
UPPERandLOWER. Runtime values and public APIs are unchanged.