You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "pepijnve (via GitHub)" <gi...@apache.org> on 2026/09/07 12:53:08 UTC
[PR] Add special case in validate_struct_compatibility for empty structs [datafusion]
pepijnve opened a new pull request, #25023:
URL: https://github.com/apache/datafusion/pull/25023
## Which issue does this PR close?
- Closes #25022.
## Rationale for this change
`nested_struct::validate_struct_compatibility` checks for at least one common field between the source and target field sets. This does not take into account that the source field set might be empty. This can cause issue during query planning.
For instance, when querying an Iceberg table's manifest with no partitioning information the following error is produced:
```
Execution error: Cannot cast column 'data_file' from 'Struct("content": non-null Int32, "file_path": non-null Utf8, "file_format": non-null Utf8, "partition": non-null Struct(), metadata: {"avro.name": "r102"}, "record_count": non-null Int64, "file_size_in_bytes": non-null Int64, "column_sizes": List(non-null Struct("key": non-null Int32, "value": non-null Int64), metadata: {"avro.name": "k117_v118"}), "value_counts": List(non-null Struct("key": non-null Int32, "value": non-null Int64), metadata: {"avro.name": "k119_v120"}), "null_value_counts": List(non-null Struct("key": non-null Int32, "value": non-null Int64), metadata: {"avro.name": "k121_v122"}), "nan_value_counts": List(non-null Struct("key": non-null Int32, "value": non-null Int64), metadata: {"avro.name": "k138_v139"}), "lower_bounds": List(non-null Struct("key": non-null Int32, "value": non-null Binary), metadata: {"avro.name": "k126_v127"}), "upper_bounds": List(non-null Struct("key": non-null Int32, "value": non-null B
inary), metadata: {"avro.name": "k129_v130"}), "key_metadata": Binary, "split_offsets": List(non-null Int64), metadata: {"element-id": "133"}, "equality_ids": List(non-null Int32), metadata: {"element-id": "136"}, "sort_order_id": Int32, "referenced_data_file": Utf8)' (physical data type) to 'Struct("content": non-null Int32, "file_path": non-null Utf8, "file_format": non-null Utf8, "partition": non-null Struct(), "record_count": non-null Int64, "file_size_in_bytes": non-null Int64, "column_sizes": List(non-null Struct("key": non-null Int32, "value": non-null Int64)), "value_counts": List(non-null Struct("key": non-null Int32, "value": non-null Int64)), "null_value_counts": List(non-null Struct("key": non-null Int32, "value": non-null Int64)), "nan_value_counts": List(non-null Struct("key": non-null Int32, "value": non-null Int64)), "lower_bounds": List(non-null Struct("key": non-null Int32, "value": non-null Binary)), "upper_bounds": List(non-null Struct("key": non-null Int32, "val
ue": non-null Binary)), "key_metadata": Binary, "split_offsets": List(non-null Int64), "equality_ids": List(non-null Int32), "sort_order_id": Int32, "referenced_data_file": Utf8)' (logical data type): Error during planning: Cannot cast struct with 0 fields to 0 fields because there is no field name overlap
```
## What changes are included in this PR?
- Adds an early success return when `source_fields.is_empty()`
## What is the testing strategy for this PR?
- Added unit tests
- Verified SLTs still pass
## Are there any user-facing changes?
No
--
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] Add special case in validate_struct_compatibility for empty structs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on PR #25023:
URL: https://github.com/apache/datafusion/pull/25023#issuecomment-5575514472
Clippy warning fixed.
--
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] Add special case in validate_struct_compatibility for empty structs [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on PR #25023:
URL: https://github.com/apache/datafusion/pull/25023#issuecomment-5572699236
> > The documentation .. is no longer accurate. It says structs without overlapping field names are rejected, but this change allows an empty source struct to cast to an empty or fully nullable target struct. Could we update this sentence to document the empty source exception?
>
> I reviewed the documentation and I'm now wondering if this function should actually be even more flexible. It states
>
> > structs should always be allowed to cast to other structs
>
> but then requires at least one overlapping field by name. Should we just drop the overlap check and let the nullability check that comes after it deal with the case where mapping is not possible instead?
I think it was introduced in [PR #19955](https://github.com/apache/datafusion/pull/19955) and was DataFusion 53 breaking change because unrelated structs could otherwise silently lose data. For example, casting {left: 1} to {alpha: INT NULL} would discard left and produce {alpha: NULL}. The nullability check only protects nonnullable target fields, so it cannot replace the overlap check.
The empty source case is different because there are no source fields to discard. I suggest keeping the current exception and updating the documentation around [nested_struct.rs lines 39 to 45](https://github.com/apache/datafusion/blob/7127cf6c8956c641cb3f3dbfaf907c6b5c4e882b/datafusion/common/src/nested_struct.rs#L39-L45) to say that a nonempty source requires at least one overlapping field name, while an empty source can cast to an empty or fully nullable target.
--
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] Add special case in validate_struct_compatibility for empty structs [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25023:
URL: https://github.com/apache/datafusion/pull/25023#issuecomment-5571601230
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25023?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 81.71%. Comparing base ([`46bbf0d`](https://app.codecov.io/gh/apache/datafusion/commit/46bbf0db1accffb81822e4a7a374818b36b70def?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`483dd11`](https://app.codecov.io/gh/apache/datafusion/commit/483dd11daab0353f0a8e2349c24d2ad6bdd14563?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 5 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #25023 +/- ##
========================================
Coverage 81.70% 81.71%
========================================
Files 1127 1127
Lines 416001 416095 +94
Branches 416001 416095 +94
========================================
+ Hits 339902 340016 +114
+ Misses 56125 56091 -34
- Partials 19974 19988 +14
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25023?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] Add special case in validate_struct_compatibility for empty structs [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal merged PR #25023:
URL: https://github.com/apache/datafusion/pull/25023
--
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] Add special case in validate_struct_compatibility for empty structs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on code in PR #25023:
URL: https://github.com/apache/datafusion/pull/25023#discussion_r3950836612
##########
datafusion/common/src/nested_struct.rs:
##########
@@ -1551,6 +1553,46 @@ mod tests {
assert!(result.is_ok());
}
+ #[test]
+ fn test_validate_struct_compatibility_empty_source_and_target() {
Review Comment:
Thanks for catching this. I had fixed the validation, then hit the arrow-rs error in the Avro decoder and didn't think of checking the cast implementation itself 🤦♂️
I adjusted the cast implementation and added tests covering the change.
--
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] Add special case in validate_struct_compatibility for empty structs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on code in PR #25023:
URL: https://github.com/apache/datafusion/pull/25023#discussion_r3950836612
##########
datafusion/common/src/nested_struct.rs:
##########
@@ -1551,6 +1553,46 @@ mod tests {
assert!(result.is_ok());
}
+ #[test]
+ fn test_validate_struct_compatibility_empty_source_and_target() {
Review Comment:
Thanks for catching this. I had fixed the validation, then hit the arrow-rs error and didn't think of checking the cast implementation itself 🤦♂️
I adjusted the cast implementation and added tests covering the change.
--
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] Add special case in validate_struct_compatibility for empty structs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on PR #25023:
URL: https://github.com/apache/datafusion/pull/25023#issuecomment-5572515078
> The documentation .. is no longer accurate. It says structs without overlapping field names are rejected, but this change allows an empty source struct to cast to an empty or fully nullable target struct. Could we update this sentence to document the empty source exception?
I reviewed the documentation and I'm, now wonder if this function should actually be more flexible. It states
> structs should always be allowed to cast to other structs
but then requires at least one overlapping field by name. Should we just drop the overlap check and let the nullability check that comes after it deal with the case where mapping is not possible instead?
--
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] Add special case in validate_struct_compatibility for empty structs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on PR #25023:
URL: https://github.com/apache/datafusion/pull/25023#issuecomment-5572936030
> I think it was introduced in [PR #19955](https://github.com/apache/datafusion/pull/19955) and was DataFusion 53 breaking change because unrelated structs could otherwise silently lose data. For example, casting {left: 1} to {alpha: INT NULL} would discard left and produce {alpha: NULL}. The nullability check only protects nonnullable target fields, so it cannot replace the overlap check.
Thanks for the pointer to the PR. The comment made by @adriangb that this matches DuckDB behaviour makes sense, and as you said, coming from an empty struct there's no information to lose anyway. I'll have a go at revising the doc comment.
--
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] Add special case in validate_struct_compatibility for empty structs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on PR #25023:
URL: https://github.com/apache/datafusion/pull/25023#issuecomment-5573060201
I've updated the documentation a bit and removed the duplication between the validation and cast functions.
--
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] Add special case in validate_struct_compatibility for empty structs [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on code in PR #25023:
URL: https://github.com/apache/datafusion/pull/25023#discussion_r3950569235
##########
datafusion/common/src/nested_struct.rs:
##########
@@ -1551,6 +1553,46 @@ mod tests {
assert!(result.is_ok());
}
+ #[test]
+ fn test_validate_struct_compatibility_empty_source_and_target() {
Review Comment:
This test only verifies validation. After validation succeeds, `cast_struct_column` still calls `StructArray::try_new` with empty `fields` and `arrays`, which Arrow rejects because it cannot infer the row count. I reproduced this with a two row `StructArray::new_empty_fields(2, None)`.
Please use `try_new_with_length` or `new_empty_fields` and add a `cast_column` regression that verifies the output length and null validity. Otherwise, the reported Iceberg query moves past planning but still fails during execution.
--
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