Skip to content

fix: derive string function nullability from inputs - #26209

Open
VVKot wants to merge 1 commit into
apache:mainfrom
VVKot:vkot/string-nullability-main
Open

VVKot wants to merge 1 commit into
apache:mainfrom
VVKot:vkot/string-nullability-main

Conversation

@VVKot

@VVKot VVKot commented Oct 11, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Part of the work towards #15914

Rationale for this change

#26205 (review)

What changes are included in this PR?

Deriving nullability correctly for soundex, space, and native concat_ws.

What is the testing strategy for this PR?

New unit + SLT tests.

Are there any user-facing changes?

No.

@github-actions github-actions Bot added sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation spark labels Oct 11, 2026
@VVKot

VVKot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor Author

@neilconway addressing #26205 (review)

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.29412% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.80%. Comparing base (454ca08) to head (72bcd53).

Files with missing lines Patch % Lines
datafusion/spark/src/function/string/space.rs 81.39% 0 Missing and 8 partials ⚠️
datafusion/functions/src/string/concat_ws.rs 92.30% 0 Missing and 1 partial ⚠️
datafusion/spark/src/function/string/soundex.rs 91.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #26209   +/-   ##
=======================================
  Coverage   82.80%   82.80%           
=======================================
  Files        1149     1149           
  Lines      452213   452281   +68     
  Branches   452213   452281   +68     
=======================================
+ Hits       374473   374531   +58     
+ Misses      54945    54942    -3     
- Partials    22795    22808   +13     

☔ 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.

@VVKot

VVKot commented Oct 13, 2026

Copy link
Copy Markdown
Contributor Author

@Jefffrey any chance you have a sec to review this one please?

.collect::<Vec<_>>();
Ok(Field::new(
self.name(),
self.return_type(&arg_types)?,

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.

Suggested change
self.return_type(&arg_types)?,
deduce_return_type(&arg_types)?,

then can leave return_type to return a not implemented error, same pattern we follow for other UDFs that implement return_field_from_args and not return_type

let input = &args.arg_fields[0];
Ok(Arc::new(Field::new(
self.name(),
self.return_type(&arg_types)?,

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.

same here

let input = &args.arg_fields[0];
// Retained dictionaries can have null values even with non-null keys.
let nullable = input.is_nullable()
|| matches!(input.data_type(), DataType::Dictionary(_, _));

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.

why does space have this extra dictionary logic compared to soundex

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants