You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "feichai0017 (via GitHub)" <gi...@apache.org> on 2026/03/14 14:38:33 UTC
[PR] fix: avoid extraneous casts for equivalent nested types [datafusion]
feichai0017 opened a new pull request, #20945:
URL: https://github.com/apache/datafusion/pull/20945
## Summary
This PR avoids inserting extraneous casts during function argument coercion when two nested types are structurally equivalent but differ only in nested field names or metadata.
Specifically, it:
- treats equivalent nested `DataType`s as matching during UDF argument coercion
- avoids rewriting such arguments with unnecessary `CAST`s
- adds regression coverage in both `datafusion-expr` and `datafusion-optimizer`
Closes #19943.
## Tests
- `cargo test -p datafusion-optimizer`
- `./dev/rust_lint.sh`
- `cargo test -p datafusion-expr` currently fails on an existing snapshot mismatch in `logical_plan::plan::tests::test_display_pg_json` on the current `main` baseline, unrelated to this 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] fix: avoid extraneous casts for equivalent nested types [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey merged PR #20945:
URL: https://github.com/apache/datafusion/pull/20945
--
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: avoid extraneous casts for equivalent nested types [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #20945:
URL: https://github.com/apache/datafusion/pull/20945#discussion_r3346366902
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -586,6 +586,50 @@ fn get_valid_types(
arguments: &[ArrayFunctionArgument],
array_coercion: Option<&ListCoercion>,
) -> Result<Vec<Vec<DataType>>> {
+ fn rebuild_array_type(
Review Comment:
probably good idea to leave a comment explaining why we do this, e.g.
```rust
// try maintain original fields where possible (avoid extraneous casts in cast metadata/field names don't match)
```
--
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: avoid extraneous casts for equivalent nested types [datafusion]
Posted by "feichai0017 (via GitHub)" <gi...@apache.org>.
feichai0017 commented on code in PR #20945:
URL: https://github.com/apache/datafusion/pull/20945#discussion_r2937697649
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -307,6 +307,34 @@ fn try_coerce_types(
)
}
+fn data_types_match(valid_types: &[DataType], current_types: &[DataType]) -> bool {
Review Comment:
Thanks for mention.
The original reproducer does go through a `Map`, but the actual mismatch at the coercion point is on the extracted map value (`List<Struct<...>>`), not on the `Map` type itself. That’s why I kept the fast-path relaxation narrow.
I did try a broader `equals_datatype` approach first, but it was too permissive in this path and regressed existing cases where runtime kernels still require exact type identity, especially around `Struct`. I agree `ListView` / `LargeListView` should be handled consistently with `List` / `LargeList`, and I’ve updated the matcher for that.
--
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: avoid extraneous casts for equivalent nested types [datafusion]
Posted by "feichai0017 (via GitHub)" <gi...@apache.org>.
feichai0017 commented on PR #20945:
URL: https://github.com/apache/datafusion/pull/20945#issuecomment-4071991070
> What about `Struct(a: List(x: Int64))` vs `Struct(a: List(y: Int64))` -- should we try to elide the cast in this case as well?那 `Struct（a： List（x： Int64））` 和 `Struct（a： List（y： Int64））呢——` 在这种情况下我们也应该尝试省略角色吗？
>
> It might be helpful to add an SLT test to verify the end-user behavior that the extraneous casts are indeed omitted.添加一个 SLT 测试可能会有帮助，以验证终端用户行为是否确实省略了多余的铸造。
Thanks, I updated the matcher to make that boundary explicit.
The fast-path match now:
- ignores wrapper field names for list-like types and maps, where the wrapper name does not affect the physical representation
- recurses through `Struct`, while still requiring struct field names/order/nullability to match exactly
That means cases like `Struct(a: List(x: Int64))` vs `Struct(a: List(y: Int64))` now avoid the extra cast, but broader `Struct` reordering still does not.
I also added:
- analyzer regressions for both `Struct(...List(...))` and `Map(...)`
- an `EXPLAIN VERBOSE` sqllogictest in `spark/array/array.slt` to verify the user-visible behavior that no extra cast is introduced during type coercion
--
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: avoid extraneous casts for equivalent nested types [datafusion]
Posted by "feichai0017 (via GitHub)" <gi...@apache.org>.
feichai0017 commented on code in PR #20945:
URL: https://github.com/apache/datafusion/pull/20945#discussion_r3310829576
##########
datafusion/sqllogictest/test_files/spark/array/array.slt:
##########
@@ -85,3 +85,43 @@ query ?
SELECT array(arrow_cast(array(1,2), 'LargeList(Int64)'), array(3));
----
[[1, 2], [3]]
+
+query TT
Review Comment:
Sry fot this part. Those SLT cases were not exercising the actual bug, so I removed that approach.
The coverage now directly targets the real failure mode:
- a unit test for `get_valid_types(..., Signature::array_and_index(...))`
- a focused analyzer regression for a parquet-style `List(Field("element", ...))` input
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -236,7 +236,7 @@ pub fn data_types(
get_valid_types(function_name.as_ref(), type_signature, current_types)?;
if valid_types
.iter()
- .any(|data_type| data_type == current_types)
+ .any(|data_type| data_types_match(data_type, current_types))
Review Comment:
Agreed, thanks. I removed the change to the deprecated `data_types(...)` path and kept the fix only on the path that is actually used by the analyzer.
--
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: avoid extraneous casts for equivalent nested types [datafusion]
Posted by "feichai0017 (via GitHub)" <gi...@apache.org>.
feichai0017 commented on code in PR #20945:
URL: https://github.com/apache/datafusion/pull/20945#discussion_r3310830490
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -757,6 +802,10 @@ fn maybe_data_types(
for (i, valid_type) in valid_types.iter().enumerate() {
let current_type = ¤t_types[i];
+ // Keep exact equality here. Some kernels such as `make_array`
+ // require nested field names/order to match exactly at runtime.
+ // Structural-equivalence short-circuiting is handled earlier by
+ // `data_types_match`.
Review Comment:
Thanks, you were right that the previous reasoning was too indirect.
I reworked the fix to target the root cause instead: `get_valid_types()` was rebuilding array signature types in a way that normalized away the original list child field name. The fix now preserves that field when deriving the expected array type, so the existing exact equality in `fields_with_udf` works as intended.
--
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: avoid extraneous casts for equivalent nested types [datafusion]
Posted by "feichai0017 (via GitHub)" <gi...@apache.org>.
feichai0017 commented on code in PR #20945:
URL: https://github.com/apache/datafusion/pull/20945#discussion_r3346504019
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -586,6 +586,50 @@ fn get_valid_types(
arguments: &[ArrayFunctionArgument],
array_coercion: Option<&ListCoercion>,
) -> Result<Vec<Vec<DataType>>> {
+ fn rebuild_array_type(
Review Comment:
Done in 06800baed: added a brief comment explaining that we preserve the original list field to avoid unnecessary casts caused by field name / metadata differences.
--
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: avoid extraneous casts for equivalent nested types [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #20945:
URL: https://github.com/apache/datafusion/pull/20945#discussion_r3145038441
##########
datafusion/sqllogictest/test_files/spark/array/array.slt:
##########
@@ -85,3 +85,43 @@ query ?
SELECT array(arrow_cast(array(1,2), 'LargeList(Int64)'), array(3));
----
[[1, 2], [3]]
+
+query TT
Review Comment:
These tests already pass on main, so what are they testing here?
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -236,7 +236,7 @@ pub fn data_types(
get_valid_types(function_name.as_ref(), type_signature, current_types)?;
if valid_types
.iter()
- .any(|data_type| data_type == current_types)
+ .any(|data_type| data_types_match(data_type, current_types))
Review Comment:
We don't need to change this function, it's deprecated and not used anywhere
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -757,6 +802,10 @@ fn maybe_data_types(
for (i, valid_type) in valid_types.iter().enumerate() {
let current_type = ¤t_types[i];
+ // Keep exact equality here. Some kernels such as `make_array`
+ // require nested field names/order to match exactly at runtime.
+ // Structural-equivalence short-circuiting is handled earlier by
+ // `data_types_match`.
Review Comment:
I don't quite understand this reasoning and how it doesn't apply to the fix applied in `fields_with_udf` directly? As in, `fields_with_udf` first tries to see if the input fields already match with the valid types of the UDF and returns early if so (and this is where the PR applies a fix for equality). Then if it doesn't match exactly it'll try to coerce, which is this code path here. So why does coercion have this catch of nested fields needing to match exactly at runtime when the exact path doesn't?
--
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: avoid extraneous casts for equivalent nested types [datafusion]
Posted by "feichai0017 (via GitHub)" <gi...@apache.org>.
feichai0017 commented on code in PR #20945:
URL: https://github.com/apache/datafusion/pull/20945#discussion_r3346504019
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -586,6 +586,50 @@ fn get_valid_types(
arguments: &[ArrayFunctionArgument],
array_coercion: Option<&ListCoercion>,
) -> Result<Vec<Vec<DataType>>> {
+ fn rebuild_array_type(
Review Comment:
Done in 06800baed: added a brief comment explaining that we preserve the original list field to avoid unnecessary casts caused by field name / metadata differences.
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -586,6 +586,50 @@ fn get_valid_types(
arguments: &[ArrayFunctionArgument],
array_coercion: Option<&ListCoercion>,
) -> Result<Vec<Vec<DataType>>> {
+ fn rebuild_array_type(
Review Comment:
ok, let me add it
--
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: avoid extraneous casts for equivalent nested types [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #20945:
URL: https://github.com/apache/datafusion/pull/20945#issuecomment-4620961309
thanks @feichai0017 & @neilconway
--
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: avoid extraneous casts for equivalent nested types [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #20945:
URL: https://github.com/apache/datafusion/pull/20945#discussion_r2937712156
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -307,6 +307,34 @@ fn try_coerce_types(
)
}
+fn data_types_match(valid_types: &[DataType], current_types: &[DataType]) -> bool {
Review Comment:
Ah, got it.
Just to help me understand, can you point at an SLT test (e.g., involving structs) that would regress if we used `equals_datatype`? Or if such an SLT test doesn't already exist, it would probably be a good idea to add one as a sanity check.
--
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: avoid extraneous casts for equivalent nested types [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #20945:
URL: https://github.com/apache/datafusion/pull/20945#discussion_r2941421186
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -307,6 +307,34 @@ fn try_coerce_types(
)
}
+fn data_types_match(valid_types: &[DataType], current_types: &[DataType]) -> bool {
Review Comment:
Thank you! That makes sense: the key point is that some Arrow kernels depend on struct field ordering, but the "field name" of a list has no influence on the representation of the data. Can we add a brief comment to `data_type_matches` to explain the rationale for the kinda-structural-equality we are implementing?
It seems like `Map` has the same behavior as the `List` variants: the "field name" does not impact the representation of the data. Should we handle that as well, for completeness?
--
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: avoid extraneous casts for equivalent nested types [datafusion]
Posted by "feichai0017 (via GitHub)" <gi...@apache.org>.
feichai0017 commented on PR #20945:
URL: https://github.com/apache/datafusion/pull/20945#issuecomment-4083574124
I dropped the `logical_plan_only` config toggles and added additional SLT coverage for `map`, nested `struct`, and `FixedSizeList`
--
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: avoid extraneous casts for equivalent nested types [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #20945:
URL: https://github.com/apache/datafusion/pull/20945#discussion_r2937013729
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -307,6 +307,34 @@ fn try_coerce_types(
)
}
+fn data_types_match(valid_types: &[DataType], current_types: &[DataType]) -> bool {
Review Comment:
Seems like we aren't handling Map, Struct, or ListView -- is there a reason for that? In fact, the original bug report uses Map.
I wonder if we can simplify this to use `equals_datatype` from Arrow, as suggested by the bug reporter?
--
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: avoid extraneous casts for equivalent nested types [datafusion]
Posted by "feichai0017 (via GitHub)" <gi...@apache.org>.
feichai0017 commented on code in PR #20945:
URL: https://github.com/apache/datafusion/pull/20945#discussion_r2937754435
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -307,6 +307,34 @@ fn try_coerce_types(
)
}
+fn data_types_match(valid_types: &[DataType], current_types: &[DataType]) -> bool {
Review Comment:
Yes, I locally verified that a broader `equals_datatype`-style matcher regresses existing SLTs.
In particular:
- `/datafusion/datafusion/sqllogictest/test_files/struct.slt`
`select [{a: 1, b: 2}, {b: 3, a: 4}];`
- `/datafusion/datafusion/sqllogictest/test_files/spark/array/array.slt`
`SELECT array(arrow_cast(array(1,2), 'LargeList(Int64)'), array(3));`
With the broader matching, both end up failing in array construction (`MutableArrayData`) because those paths still require exact runtime type identity. That was the main reason I kept this matcher narrower than `equals_datatype`, especially around `Struct`.
I agree it would be useful to make that boundary explicit, so I can also add a focused sanity-check regression test in this PR.
--
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: avoid extraneous casts for equivalent nested types [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #20945:
URL: https://github.com/apache/datafusion/pull/20945#issuecomment-5286220468
This appears to have caused a regression (exposed an existing issue) in the 55 release
- https://github.com/apache/datafusion/issues/24341
--
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