Repository navigation
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? - Closes apache#25380. ## Rationale for this change `UPPER` and `LOWER` currently 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? - Implement `return_field_from_args` for `UpperFunc` and `LowerFunc`. - Preserve the input string type and nullability. - Add nullable and non-nullable unit cases for each function. 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_nullability` - `cargo +1.95.0 clippy -p datafusion-functions --lib --tests --all-features --no-deps -- -D warnings` - `cargo +1.95.0 fmt --all -- --check` ## Are there any user-facing changes? Planned result schemas now retain non-nullability through `UPPER` and `LOWER`. Runtime values and public APIs are unchanged.
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.