You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "buraksenn (via GitHub)" <gi...@apache.org> on 2026/09/08 08:53:42 UTC
[PR] fix(proto): preserve physical cast expression state [datafusion]
buraksenn opened a new pull request, #25056:
URL: https://github.com/apache/datafusion/pull/25056
## Which issue does this PR close?
- Closes #24615.
## Rationale for this change
Physical cast protobuf hooks did not serialize `CastExpr` options or explicit target-field metadata. A round trip could therefore change cast behavior, nullability, or metadata. The hooks also accessed fields individually, so future fields could be omitted silently.
## What changes are included in this PR?
- Exhaustively destructure `CastExpr`, `TryCastExpr`, and their protobuf messages.
- Preserve explicit target fields for `CAST` and `TRY_CAST` while keeping legacy type-only payload behavior.
- Preserve all serializable Arrow cast/format options; reject custom formatter factories instead of silently dropping them.
- Store decoded format strings in owned cast options.
- Regenerate protobuf models and document the `CastExpr::cast_options` API adjustment.
The protobuf changes are additive and backward compatible.
## What is the testing strategy for this PR?
Added unit coverage for:
- target-field and cast-option round trips
- legacy payloads without the new fields
- explicit default-shaped targets
- malformed target-field types
- unsupported formatter factories
Validated with:
- `cargo test -p datafusion-physical-expr --features proto expressions::cast::`
- `cargo test -p datafusion-physical-expr --features proto expressions::try_cast::`
- `cargo clippy -p datafusion-physical-expr -p datafusion-physical-expr-adapter --all-features --tests -- -D warnings`
- `cargo check -p datafusion-proto --all-features`
- `./ci/scripts/doc_prettier_check.sh --write --allow-dirty`
## Are there any user-facing changes?
Cast expressions now retain their options and explicit output-field semantics across protobuf round trips. `CastExpr::cast_options()` returns a borrowing `CastOptions<'_>` value rather than `&CastOptions<'static>`; the upgrade guide documents migration for code rebuilding casts.
--
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(proto): preserve physical cast expression state [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #25056:
URL: https://github.com/apache/datafusion/pull/25056#discussion_r4237350405
##########
datafusion/physical-expr/src/expressions/cast.rs:
##########
@@ -2160,6 +2478,102 @@ mod proto_tests {
.expect("cast type should be encoded");
let data_type: DataType = arrow_type.try_into().unwrap();
assert_eq!(data_type, Int64);
+ assert!(cast_node.target_field.is_none());
+ assert!(cast_node.cast_options.is_none());
+ }
+
+ #[test]
+ fn cast_target_field_survives_proto_round_trip() {
+ let schema = Schema::new(vec![Field::new("a", Int32, true)]);
+ let target = Arc::new(Field::new("target", Int64, false).with_metadata(
+ Metadata::from([("extension".to_string(), "value".to_string())]),
+ ));
+ let cast = CastExpr::new_with_target_field(
+ col("a", &schema).unwrap(),
+ Arc::clone(&target),
+ None,
+ );
+
+ assert!(encode_cast(&cast).target_field.is_some());
+ let decoded = round_trip_cast(&cast, &schema);
+
+ assert_eq!(decoded.target_field(), &target);
+ assert_eq!(decoded.target_metadata(), Some(target.metadata()));
+ assert_eq!(decoded.target_nullable(), Some(false));
+ assert!(!decoded.return_field(&schema).unwrap().is_nullable());
+ }
+
+ #[test]
+ fn explicit_default_shaped_cast_target_is_encoded() {
+ let schema = Schema::new(vec![Field::new("a", Int32, false).with_metadata(
+ Metadata::from([("source".to_string(), "value".to_string())]),
+ )]);
+ let cast = CastExpr::new_with_target_field(
+ col("a", &schema).unwrap(),
+ Int64.into_nullable_field_ref(),
+ None,
+ );
+
+ assert!(encode_cast(&cast).target_field.is_some());
+ let decoded = round_trip_cast(&cast, &schema);
+ assert_eq!(decoded.target_metadata(), Some(&Metadata::default()));
+ assert_eq!(decoded.target_nullable(), Some(true));
+ let output = decoded.return_field(&schema).unwrap();
+ assert!(output.metadata().is_empty());
+ assert!(output.is_nullable());
+ }
+
+ #[test]
+ fn type_only_cast_target_is_omitted_and_remains_type_only() {
+ let metadata = Metadata::from([("source".to_string(), "value".to_string())]);
+ let schema = Schema::new(vec![
+ Field::new("a", Int32, false).with_metadata(metadata.clone()),
+ ]);
+ let cast = CastExpr::new(col("a", &schema).unwrap(), Int64, None);
+
+ assert!(encode_cast(&cast).target_field.is_none());
+ let decoded = round_trip_cast(&cast, &schema);
+
+ assert_eq!(decoded.target_metadata(), None);
+ assert_eq!(decoded.target_nullable(), None);
+ let output = decoded.return_field(&schema).unwrap();
+ assert_eq!(output.metadata(), &metadata);
+ assert!(!output.is_nullable());
+ }
+
+ #[test]
+ fn cast_options_survive_proto_round_trip() {
Review Comment:
Could we add a wire round-trip test in `datafusion/proto/tests/cases/plans/` using a `ProjectionExec` that casts an invalid string like "9.1" from Utf8 to Int32? It would be useful to serialize and deserialize the plan with `physical_plan_to_bytes` and `physical_plan_from_bytes`, then verify that both plans return NULL with `safe=true` and an error with `safe=false`. The existing test checks option equality but not execution behavior across the public serialization path. This can be a follow-up.
--
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(proto): preserve physical cast expression state [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #25056:
URL: https://github.com/apache/datafusion/pull/25056#issuecomment-5583274387
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/cargo-semver-checks) reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).
<details>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion-common v55.0.0 (current)
Built [ 35.254s] (current)
Parsing datafusion-common v55.0.0 (current)
Parsed [ 0.058s] (current)
Building datafusion-common v55.0.0 (baseline)
Built [ 32.564s] (baseline)
Parsing datafusion-common v55.0.0 (baseline)
Parsed [ 0.059s] (baseline)
Checking datafusion-common v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 0.929s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 70.815s] datafusion-common
Building datafusion-expr-common v55.0.0 (current)
Built [ 19.460s] (current)
Parsing datafusion-expr-common v55.0.0 (current)
Parsed [ 0.018s] (current)
Building datafusion-expr-common v55.0.0 (baseline)
Built [ 18.945s] (baseline)
Parsing datafusion-expr-common v55.0.0 (baseline)
Parsed [ 0.019s] (baseline)
Checking datafusion-expr-common v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 0.299s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 39.756s] datafusion-expr-common
Building datafusion-physical-expr v55.0.0 (current)
Built [ 28.433s] (current)
Parsing datafusion-physical-expr v55.0.0 (current)
Parsed [ 0.047s] (current)
Building datafusion-physical-expr v55.0.0 (baseline)
Built [ 28.645s] (baseline)
Parsing datafusion-physical-expr v55.0.0 (baseline)
Parsed [ 0.049s] (baseline)
Checking datafusion-physical-expr v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 0.459s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 58.769s] datafusion-physical-expr
Building datafusion-physical-expr-adapter v55.0.0 (current)
Built [ 31.830s] (current)
Parsing datafusion-physical-expr-adapter v55.0.0 (current)
Parsed [ 0.014s] (current)
Building datafusion-physical-expr-adapter v55.0.0 (baseline)
Built [ 31.764s] (baseline)
Parsing datafusion-physical-expr-adapter v55.0.0 (baseline)
Parsed [ 0.010s] (baseline)
Checking datafusion-physical-expr-adapter v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 0.115s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 65.251s] datafusion-physical-expr-adapter
Building datafusion-proto-models v55.0.0 (current)
Built [ 24.305s] (current)
Parsing datafusion-proto-models v55.0.0 (current)
Parsed [ 0.126s] (current)
Building datafusion-proto-models v55.0.0 (baseline)
Built [ 24.524s] (baseline)
Parsing datafusion-proto-models v55.0.0 (baseline)
Parsed [ 0.124s] (baseline)
Checking datafusion-proto-models v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 2.350s] 223 checks: 222 pass, 1 fail, 0 warn, 31 skip
--- failure constructible_struct_adds_field: struct exhaustively constructible through public API adds field ---
Description:
A pub struct that could be exhaustively constructed with a literal using only public API has a new pub field, breaking existing exhaustive literals.
ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/constructible_struct_adds_field.ron
Failed in:
field PhysicalCastNode.target_field in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:1863
field PhysicalCastNode.cast_options in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:1866
field PhysicalCastNode.target_field in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:1863
field PhysicalCastNode.cast_options in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:1866
field PhysicalTryCastNode.target_field in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:1852
field PhysicalTryCastNode.target_field in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:1852
Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [ 52.906s] datafusion-proto-models
```
</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(proto): preserve physical cast expression state [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew merged PR #25056:
URL: https://github.com/apache/datafusion/pull/25056
--
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(proto): preserve physical cast expression state [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25056:
URL: https://github.com/apache/datafusion/pull/25056#issuecomment-5583641182
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25056?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 `56.39464%` with `358 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.68%. Comparing base ([`92746a9`](https://app.codecov.io/gh/apache/datafusion/commit/92746a993c66eb4d97ea044336bff27420800426?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`b0e2236`](https://app.codecov.io/gh/apache/datafusion/commit/b0e22360215e17d119169eb7350560cf3288d28d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25056?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/proto-models/src/generated/pbjson.rs](https://app.codecov.io/gh/apache/datafusion/pull/25056?src=pr&el=tree&filepath=datafusion%2Fproto-models%2Fsrc%2Fgenerated%2Fpbjson.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9wcm90by1tb2RlbHMvc3JjL2dlbmVyYXRlZC9wYmpzb24ucnM=) | 0.00% | [315 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25056?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/physical-expr/src/expressions/cast.rs](https://app.codecov.io/gh/apache/datafusion/pull/25056?src=pr&el=tree&filepath=datafusion%2Fphysical-expr%2Fsrc%2Fexpressions%2Fcast.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1leHByL3NyYy9leHByZXNzaW9ucy9jYXN0LnJz) | 92.56% | [20 Missing and 7 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25056?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/proto-models/src/generated/prost.rs](https://app.codecov.io/gh/apache/datafusion/pull/25056?src=pr&el=tree&filepath=datafusion%2Fproto-models%2Fsrc%2Fgenerated%2Fprost.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9wcm90by1tb2RlbHMvc3JjL2dlbmVyYXRlZC9wcm9zdC5ycw==) | 0.00% | [11 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25056?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...tafusion/physical-expr/src/expressions/try\_cast.rs](https://app.codecov.io/gh/apache/datafusion/pull/25056?src=pr&el=tree&filepath=datafusion%2Fphysical-expr%2Fsrc%2Fexpressions%2Ftry_cast.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1leHByL3NyYy9leHByZXNzaW9ucy90cnlfY2FzdC5ycw==) | 96.03% | [2 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25056?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 #25056 +/- ##
==========================================
- Coverage 81.74% 81.68% -0.06%
==========================================
Files 1128 1128
Lines 416645 417425 +780
Branches 416645 417425 +780
==========================================
+ Hits 340580 340993 +413
- Misses 55998 56357 +359
- Partials 20067 20075 +8
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25056?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(proto): preserve physical cast expression state [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on code in PR #25056:
URL: https://github.com/apache/datafusion/pull/25056#discussion_r4245525963
##########
datafusion/physical-expr/src/expressions/cast.rs:
##########
@@ -2160,6 +2478,102 @@ mod proto_tests {
.expect("cast type should be encoded");
let data_type: DataType = arrow_type.try_into().unwrap();
assert_eq!(data_type, Int64);
+ assert!(cast_node.target_field.is_none());
+ assert!(cast_node.cast_options.is_none());
+ }
+
+ #[test]
+ fn cast_target_field_survives_proto_round_trip() {
+ let schema = Schema::new(vec![Field::new("a", Int32, true)]);
+ let target = Arc::new(Field::new("target", Int64, false).with_metadata(
+ Metadata::from([("extension".to_string(), "value".to_string())]),
+ ));
+ let cast = CastExpr::new_with_target_field(
+ col("a", &schema).unwrap(),
+ Arc::clone(&target),
+ None,
+ );
+
+ assert!(encode_cast(&cast).target_field.is_some());
+ let decoded = round_trip_cast(&cast, &schema);
+
+ assert_eq!(decoded.target_field(), &target);
+ assert_eq!(decoded.target_metadata(), Some(target.metadata()));
+ assert_eq!(decoded.target_nullable(), Some(false));
+ assert!(!decoded.return_field(&schema).unwrap().is_nullable());
+ }
+
+ #[test]
+ fn explicit_default_shaped_cast_target_is_encoded() {
+ let schema = Schema::new(vec![Field::new("a", Int32, false).with_metadata(
+ Metadata::from([("source".to_string(), "value".to_string())]),
+ )]);
+ let cast = CastExpr::new_with_target_field(
+ col("a", &schema).unwrap(),
+ Int64.into_nullable_field_ref(),
+ None,
+ );
+
+ assert!(encode_cast(&cast).target_field.is_some());
+ let decoded = round_trip_cast(&cast, &schema);
+ assert_eq!(decoded.target_metadata(), Some(&Metadata::default()));
+ assert_eq!(decoded.target_nullable(), Some(true));
+ let output = decoded.return_field(&schema).unwrap();
+ assert!(output.metadata().is_empty());
+ assert!(output.is_nullable());
+ }
+
+ #[test]
+ fn type_only_cast_target_is_omitted_and_remains_type_only() {
+ let metadata = Metadata::from([("source".to_string(), "value".to_string())]);
+ let schema = Schema::new(vec![
+ Field::new("a", Int32, false).with_metadata(metadata.clone()),
+ ]);
+ let cast = CastExpr::new(col("a", &schema).unwrap(), Int64, None);
+
+ assert!(encode_cast(&cast).target_field.is_none());
+ let decoded = round_trip_cast(&cast, &schema);
+
+ assert_eq!(decoded.target_metadata(), None);
+ assert_eq!(decoded.target_nullable(), None);
+ let output = decoded.return_field(&schema).unwrap();
+ assert_eq!(output.metadata(), &metadata);
+ assert!(!output.is_nullable());
+ }
+
+ #[test]
+ fn cast_options_survive_proto_round_trip() {
Review Comment:
thanks for the review I've added the round trip test
--
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(proto): preserve physical cast expression state [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #25056:
URL: https://github.com/apache/datafusion/pull/25056#issuecomment-6142148717
🚀
@buraksenn
Thank you for your contribution.
--
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