You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "huymq1710 (via GitHub)" <gi...@apache.org> on 2026/08/07 06:18:09 UTC
[PR] fix(substrait): preserve nested nullability [datafusion]
huymq1710 opened a new pull request, #24158:
URL: https://github.com/apache/datafusion/pull/24158
## Which issue does this PR close?
- Closes #22095
## 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(substrait): preserve nested nullability [datafusion]
Posted by "huymq1710 (via GitHub)" <gi...@apache.org>.
huymq1710 commented on PR #24158:
URL: https://github.com/apache/datafusion/pull/24158#issuecomment-6304686039
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(substrait): preserve nested nullability [datafusion]
Posted by "bvolpato (via GitHub)" <gi...@apache.org>.
bvolpato commented on code in PR #24158:
URL: https://github.com/apache/datafusion/pull/24158#discussion_r3771919383
##########
datafusion/substrait/src/logical_plan/consumer/expr/literal.rs:
##########
@@ -583,13 +700,144 @@ pub(crate) fn from_substrait_literal(
_ => return not_impl_err!("Unsupported literal_type: {:?}", lit.literal_type),
};
+ if let Some(expected_field) = expected_field
+ && scalar_value.data_type() != *expected_field.data_type()
+ {
+ return substrait_err!(
+ "Literal type mismatch: expected {:?} but found {:?}",
+ expected_field.data_type(),
+ scalar_value.data_type()
+ );
+ }
+
Ok(scalar_value)
}
+fn expected_list_item(
+ expected_field: Option<&Field>,
+ type_variation_reference: u32,
+) -> datafusion::common::Result<Option<&FieldRef>> {
+ let Some(expected_field) = expected_field else {
+ return Ok(None);
+ };
+
+ match (expected_field.data_type(), type_variation_reference) {
+ (DataType::List(item), DEFAULT_CONTAINER_TYPE_VARIATION_REF)
+ | (DataType::LargeList(item), LARGE_CONTAINER_TYPE_VARIATION_REF) => {
+ Ok(Some(item))
+ }
+ (expected, _) => substrait_err!(
+ "Expected List literal to have List or LargeList type, found {expected:?}"
+ ),
+ }
+}
+
+fn make_list_scalar(
+ elements: Vec<ScalarValue>,
+ expected_item: &FieldRef,
+ type_variation_reference: u32,
+) -> datafusion::common::Result<ScalarValue> {
+ let values = if elements.is_empty() {
+ new_empty_array(expected_item.data_type())
+ } else {
+ ScalarValue::iter_to_array(elements)?
+ };
+
+ match type_variation_reference {
+ DEFAULT_CONTAINER_TYPE_VARIATION_REF => {
+ Ok(ScalarValue::List(Arc::new(ListArray::new(
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(substrait): preserve nested nullability [datafusion]
Posted by "huymq1710 (via GitHub)" <gi...@apache.org>.
huymq1710 commented on PR #24158:
URL: https://github.com/apache/datafusion/pull/24158#issuecomment-5215802184
I see, Codecov was newly added
I added more test in [9418567](https://github.com/apache/datafusion/pull/24158/commits/94185672c4076557df8e75d12f7018a4696d1370)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(substrait): preserve nested nullability [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24158:
URL: https://github.com/apache/datafusion/pull/24158#issuecomment-5213535612
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24158?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `82.69231%` with `63 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.03%. Comparing base ([`f05869b`](https://app.codecov.io/gh/apache/datafusion/commit/f05869b0e6828e6524b06cde71e697bfb84fd2ee?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`391d0a2`](https://app.codecov.io/gh/apache/datafusion/commit/391d0a2dd75fe8984d2754049c71571024d4b856?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/24158?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...ubstrait/src/logical\_plan/consumer/expr/literal.rs](https://app.codecov.io/gh/apache/datafusion/pull/24158?src=pr&el=tree&filepath=datafusion%2Fsubstrait%2Fsrc%2Flogical_plan%2Fconsumer%2Fexpr%2Fliteral.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9zdWJzdHJhaXQvc3JjL2xvZ2ljYWxfcGxhbi9jb25zdW1lci9leHByL2xpdGVyYWwucnM=) | 82.95% | [25 Missing and 13 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24158?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ubstrait/src/logical\_plan/producer/expr/literal.rs](https://app.codecov.io/gh/apache/datafusion/pull/24158?src=pr&el=tree&filepath=datafusion%2Fsubstrait%2Fsrc%2Flogical_plan%2Fproducer%2Fexpr%2Fliteral.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9zdWJzdHJhaXQvc3JjL2xvZ2ljYWxfcGxhbi9wcm9kdWNlci9leHByL2xpdGVyYWwucnM=) | 82.43% | [2 Missing and 11 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24158?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ubstrait/src/logical\_plan/consumer/rel/read\_rel.rs](https://app.codecov.io/gh/apache/datafusion/pull/24158?src=pr&el=tree&filepath=datafusion%2Fsubstrait%2Fsrc%2Flogical_plan%2Fconsumer%2Frel%2Fread_rel.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9zdWJzdHJhaXQvc3JjL2xvZ2ljYWxfcGxhbi9jb25zdW1lci9yZWwvcmVhZF9yZWwucnM=) | 84.09% | [6 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24158?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...usion/substrait/src/logical\_plan/producer/types.rs](https://app.codecov.io/gh/apache/datafusion/pull/24158?src=pr&el=tree&filepath=datafusion%2Fsubstrait%2Fsrc%2Flogical_plan%2Fproducer%2Ftypes.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9zdWJzdHJhaXQvc3JjL2xvZ2ljYWxfcGxhbi9wcm9kdWNlci90eXBlcy5ycw==) | 84.21% | [0 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24158?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...usion/substrait/src/logical\_plan/consumer/types.rs](https://app.codecov.io/gh/apache/datafusion/pull/24158?src=pr&el=tree&filepath=datafusion%2Fsubstrait%2Fsrc%2Flogical_plan%2Fconsumer%2Ftypes.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9zdWJzdHJhaXQvc3JjL2xvZ2ljYWxfcGxhbi9jb25zdW1lci90eXBlcy5ycw==) | 50.00% | [0 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24158?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #24158 +/- ##
========================================
Coverage 81.02% 81.03%
========================================
Files 1106 1106
Lines 380692 380965 +273
Branches 380692 380965 +273
========================================
+ Hits 308468 308722 +254
+ Misses 53997 53989 -8
- Partials 18227 18254 +27
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24158?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(substrait): preserve nested nullability [datafusion]
Posted by "huymq1710 (via GitHub)" <gi...@apache.org>.
huymq1710 closed pull request #24158: fix(substrait): preserve nested nullability
URL: https://github.com/apache/datafusion/pull/24158
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org