Skip to content

fix(substrait): preserve nested nullability - #24158

Closed
huymq1710 wants to merge 3 commits into
apache:mainfrom
huymq1710:substrait-preserve-nested-nullability
Closed

fix(substrait): preserve nested nullability#24158
huymq1710 wants to merge 3 commits into
apache:mainfrom
huymq1710:substrait-preserve-nested-nullability

Conversation

@huymq1710

@huymq1710 huymq1710 commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

Nested literal types could mismatch the declared VirtualTable schema, causing valid Values plans to fail

What changes are included in this PR?

  • Convert Values literals using expected schema fields
  • Preserve nested Struct, List, and Map nullability
  • Keep schema-less literal conversion unchanged

Are these changes tested?

Yes,
Added unit, round-trip, deprecated Values, and integration tests

Are there any user-facing changes?

Substrait Values plans now preserve nested nullability and avoid schema/literal type mismatches

@github-actions github-actions Bot added the substrait Changes to the substrait crate label Aug 7, 2026
@huymq1710 huymq1710 mentioned this pull request Aug 7, 2026
Open
@codecov-commenter

codecov-commenter commented Aug 7, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.34615% with 45 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.05%. Comparing base (e64e3f7) to head (9418567).
⚠️ Report is 311 commits behind head on main.

Files with missing lines Patch % Lines
...ubstrait/src/logical_plan/consumer/expr/literal.rs 94.47% 8 Missing and 10 partials ⚠️
...ubstrait/src/logical_plan/producer/expr/literal.rs 82.43% 2 Missing and 11 partials ⚠️
...ubstrait/src/logical_plan/consumer/rel/read_rel.rs 90.72% 4 Missing and 5 partials ⚠️
...usion/substrait/src/logical_plan/producer/types.rs 84.21% 0 Missing and 3 partials ⚠️
...usion/substrait/src/logical_plan/consumer/types.rs 50.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #24158      +/-   ##
==========================================
+ Coverage   81.02%   81.05%   +0.02%     
==========================================
  Files        1106     1106              
  Lines      380718   381147     +429     
  Branches   380718   381147     +429     
==========================================
+ Hits       308489   308923     +434     
+ Misses      54001    53967      -34     
- Partials    18228    18257      +29

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

@huymq1710

huymq1710 commented Aug 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I see, Codecov was newly added
I added more test in 22cf4bc commit

bvolpato
bvolpato approved these changes Aug 13, 2026
View reviewed changes

@bvolpato bvolpato 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.

LGTM. Expected-field conversion cleanly preserves nested nullability across Values literals and schema round trips. One non-blocking panic-hardening note inline.

Comment thread datafusion/substrait/src/logical_plan/consumer/expr/literal.rs

match type_variation_reference {
DEFAULT_CONTAINER_TYPE_VARIATION_REF => {
Ok(ScalarValue::List(Arc::new(ListArray::new(

@bvolpato bvolpato Aug 13, 2026

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.

Non-blocking: could we use ListArray::try_new / LargeListArray::try_new here and propagate the error? A Values plan can declare required list items but still contain NULL; conversion then builds a nullable child array, so new unwraps Arrow validation error and panics instead of returning a DataFusion error.

@huymq1710

huymq1710 commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

Let me close the PR

https://andrew.nerdnetworks.org/blog/2026/10/19/my-plan-for-reviewing-prs-in-open-source-projects-in-the-age-of-ai/

From actual users: They have the context to identify useful changes and evaluate their agent’s output, and the motivation to iterate on it until it is correct and merged.

To avoid placing significant pressure on the reviewers, I will focus on high-priority issues

@huymq1710 huymq1710 closed this Oct 21, 2026
@huymq1710
huymq1710 deleted the substrait-preserve-nested-nullability branch October 21, 2026 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@bvolpato bvolpato bvolpato approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

substrait Changes to the substrait crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Substrait literal conversion should preserve nested nullability

3 participants

@huymq1710 @codecov-commenter @bvolpato