Skip to content

fix: preserve UPPER and LOWER input nullability - #25381

Merged
kumarUjjawal merged 3 commits into
apache:mainfrom
Embucket:upstream-string-case-nullability
Sep 17, 2026
Merged

kumarUjjawal merged 3 commits into
apache:mainfrom
Embucket:upstream-string-case-nullability

Conversation

@osipovartem

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

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.

@github-actions github-actions Bot added the functions Changes to functions implementation label Sep 16, 2026
@jayzhan211

Copy link
Copy Markdown
Contributor

Thanks @osipovartem , here is a suggestion

The unit tests only call return_field_from_args directly. Consider an end-to-end .slt case (e.g. in test_files/string/string_query.slt.part or functions.slt) so the planned schema and the outer-join case are covered:

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-commenter

codecov-commenter commented Sep 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.73684% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.93%. Comparing base (ecb0fea) to head (be7d666).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/functions/src/string/lower.rs 94.73% 0 Missing and 1 partial ⚠️
datafusion/functions/src/string/upper.rs 94.73% 0 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@osipovartem
osipovartem force-pushed the upstream-string-case-nullability branch from 5c0e952 to 62d185a Compare September 16, 2026 15:14
@osipovartem

osipovartem commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

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.

@github-actions github-actions Bot added the sqllogictest SQL Logic Tests (.slt) label Sep 16, 2026

statement ok
CREATE TABLE case_nullability_out AS
SELECT upper(a) AS ua, lower(a) AS la, upper(b) AS ub FROM case_nullability

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use

> describe select upper('aaa');
+--------------------+-----------+-------------+
| column_name        | data_type | is_nullable |
+--------------------+-----------+-------------+
| upper(Utf8("aaa")) | Utf8      | YES         |
+--------------------+-----------+-------------+

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 comphead left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @osipovartem lgtm

@kumarUjjawal

Copy link
Copy Markdown
Contributor

Thank you all!

@kumarUjjawal
kumarUjjawal added this pull request to the merge queue Sep 17, 2026
Merged via the queue into apache:main with commit b300cea Sep 17, 2026
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

functions Changes to functions implementation sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

UPPER and LOWER always report nullable result fields

5 participants