You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "timsaucer (via GitHub)" <gi...@apache.org> on 2026/08/26 13:35:23 UTC
[PR] [branch-55] fix: adapt input batches with stricter nested nullability to planned schema in aggregation (#24394) [datafusion]
timsaucer opened a new pull request, #24699:
URL: https://github.com/apache/datafusion/pull/24699
This is a back port of #24394 for `branch-55`.
The original issue is https://github.com/apache/datafusion/issues/24069
Note that the original PR was marked as an auto-detected api change, but I believe that is a false positive.
--
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] [branch-55] fix: adapt input batches with stricter nested nullability to planned schema in aggregation (#24394) [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #24699:
URL: https://github.com/apache/datafusion/pull/24699#discussion_r3870189952
##########
datafusion/common/src/nested_struct.rs:
##########
@@ -215,6 +275,17 @@ pub fn cast_column(
target_value_type,
cast_options,
),
+ (
+ DataType::Union(source_fields, source_mode),
+ DataType::Union(target_fields, target_mode),
+ ) => cast_union_column(
+ source_col,
+ source_fields,
+ source_mode,
+ target_fields,
+ target_mode,
+ cast_options,
+ ),
_ => Ok(cast_with_options(source_col, target_type, cast_options)?),
Review Comment:
Could we add a focused `MemoryStream` regression for a contained `Map<..., Struct>` where the runtime nested field is non-nullable and the declared nested field is nullable? My understanding is that this should already work because shapes accepted by `Schema::contains` retain the same Map entry/key/value structure, and Arrow's generic Map cast should recursively cast the value Struct and rebuild it with the target fields. A regression here would confirm that assumption for this branch.
```
If it fails, we should either backport the minimal Map prerequisite from
https://github.com/apache/datafusion/pull/23914 or explicitly constrain
the branch-55 adapter to shapes this branch can safely normalize.
```
as suggested by @patrickswedish
--
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] [branch-55] fix: adapt input batches with stricter nested nullability to planned schema in aggregation (#24394) [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24699:
URL: https://github.com/apache/datafusion/pull/24699#issuecomment-5426745749
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24699?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 `90.45872%` with `52 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.18%. Comparing base ([`045b590`](https://app.codecov.io/gh/apache/datafusion/commit/045b590c4822fdbd2ce47eac2f7a5a55b28cde45?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`d9f53b3`](https://app.codecov.io/gh/apache/datafusion/commit/d9f53b3ab677180d9a74a9273e61ce242d8c49a2?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/24699?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/common/src/nested\_struct.rs](https://app.codecov.io/gh/apache/datafusion/pull/24699?src=pr&el=tree&filepath=datafusion%2Fcommon%2Fsrc%2Fnested_struct.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9jb21tb24vc3JjL25lc3RlZF9zdHJ1Y3QucnM=) | 90.37% | [28 Missing and 16 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24699?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/physical-plan/src/memory.rs](https://app.codecov.io/gh/apache/datafusion/pull/24699?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Fmemory.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9tZW1vcnkucnM=) | 90.90% | [3 Missing and 5 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24699?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 @@
## branch-55 #24699 +/- ##
=============================================
+ Coverage 81.16% 81.18% +0.01%
=============================================
Files 1110 1110
Lines 386911 387456 +545
Branches 386911 387456 +545
=============================================
+ Hits 314050 314545 +495
- Misses 54370 54399 +29
- Partials 18491 18512 +21
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24699?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] [branch-55] fix: adapt input batches with stricter nested nullability to planned schema in aggregation (#24394) [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on PR #24699:
URL: https://github.com/apache/datafusion/pull/24699#issuecomment-5426526467
FYI @patrickswedish as the original author, would you mind reviewing this back port?
--
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] [branch-55] fix: adapt input batches with stricter nested nullability to planned schema in aggregation (#24394) [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #24699:
URL: https://github.com/apache/datafusion/pull/24699#discussion_r3870189952
##########
datafusion/common/src/nested_struct.rs:
##########
@@ -215,6 +275,17 @@ pub fn cast_column(
target_value_type,
cast_options,
),
+ (
+ DataType::Union(source_fields, source_mode),
+ DataType::Union(target_fields, target_mode),
+ ) => cast_union_column(
+ source_col,
+ source_fields,
+ source_mode,
+ target_fields,
+ target_mode,
+ cast_options,
+ ),
_ => Ok(cast_with_options(source_col, target_type, cast_options)?),
Review Comment:
Could we add a focused `MemoryStream` regression for a contained `Map<..., Struct>` where the runtime nested field is non-nullable and the declared nested field is nullable? My understanding is that this should already work because shapes accepted by `Schema::contains` retain the same Map entry/key/value structure, and Arrow's generic Map cast should recursively cast the value Struct and rebuild it with the target fields. A regression here would confirm that assumption for this branch.
```
If it fails, we should either backport the minimal Map prerequisite from https://github.com/apache/datafusion/pull/23914 or explicitly constrain the branch-55 adapter to shapes this branch can safely normalize.
```
as suggested by @patrickswedish
--
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] [branch-55] fix: adapt input batches with stricter nested nullability to planned schema in aggregation (#24394) [datafusion]
Posted by "patrickswedish (via GitHub)" <gi...@apache.org>.
patrickswedish commented on PR #24699:
URL: https://github.com/apache/datafusion/pull/24699#issuecomment-5427286804
> FYI @patrickswedish as the original author, would you mind reviewing this back port?
Thanks for asking me to review the backport. I compared it against the merged #24394 and the actual branch-55 base rather than treating it as a mechanical cherry-pick.
The Struct/Union path looks correctly carried over, and moving normalization to the MemoryStream producer boundary is still the right invariant.
One backport-specific gap stood out: #24394 originally merged on top of #23914, which had already added recursive Map schema adaptation in nested_struct. branch-55 does not have that prerequisite.
In this backport, adapt_batch_to_schema can now run for contained runtime-schema mismatches, but branch-55's cast_column still has no DataType::Map adaptation arm. So a stricter nested Map schema accepted by Schema::contains may fall through to Arrow's generic cast path rather than the recursive schema-conformance path present under the original PR.
Before approval, I think it would be worth adding one focused regression:
declared schema: Map<..., Struct<field nullable>>
runtime batch: Map<..., Struct<field non-nullable>>
confirm Schema::contains accepts it
exercise adapt_batch_to_schema / MemoryStream
If Arrow 56 already handles this case correctly, then no extra code is needed. If it fails, we should either backport the minimal Map prerequisite from #23914 or explicitly constrain the branch-55 adapter to shapes this branch can safely normalize.
Everything else I checked looks aligned with the original fix direction.
--
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] [branch-55] fix: adapt input batches with stricter nested nullability to planned schema in aggregation (#24394) [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #24699:
URL: https://github.com/apache/datafusion/pull/24699#discussion_r3870189952
##########
datafusion/common/src/nested_struct.rs:
##########
@@ -215,6 +275,17 @@ pub fn cast_column(
target_value_type,
cast_options,
),
+ (
+ DataType::Union(source_fields, source_mode),
+ DataType::Union(target_fields, target_mode),
+ ) => cast_union_column(
+ source_col,
+ source_fields,
+ source_mode,
+ target_fields,
+ target_mode,
+ cast_options,
+ ),
_ => Ok(cast_with_options(source_col, target_type, cast_options)?),
Review Comment:
Could we add a focused `MemoryStream` regression for a contained `Map<..., Struct>` where the runtime nested field is non-nullable and the declared nested field is nullable? My understanding is that this should already work because shapes accepted by `Schema::contains` retain the same Map entry/key/value structure, and Arrow's generic Map cast should recursively cast the value Struct and rebuild it with the target fields. A regression here would confirm that assumption for this branch. The Map-specific adapter in #23914 covers broader schema evolution, such as nested Struct add/drop/reorder and sliced or null-backed Map compaction, which I don't think this backport needs to support.
##########
datafusion/physical-plan/src/memory.rs:
##########
@@ -673,4 +684,128 @@ mod lazy_memory_tests {
Review Comment:
The new Union adaptation has good coverage at the helper level, but I think it would also be useful to exercise it through `MemoryStream`, where we enforce the producer-side schema invariant. Could we add a small Union case with a nullable declared child and a non-nullable runtime child, then assert that `emitted_batch.schema() == stream.schema()`? That would give us coverage for the Union reconstruction path at the actual integration boundary.
--
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] [branch-55] fix: adapt input batches with stricter nested nullability to planned schema in aggregation (#24394) [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer merged PR #24699:
URL: https://github.com/apache/datafusion/pull/24699
--
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