You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "goutamadwant (via GitHub)" <gi...@apache.org> on 2026/09/07 06:51:13 UTC
[PR] fix: preserve ProjectionExec metadata during serialization [datafusion]
goutamadwant opened a new pull request, #25009:
URL: https://github.com/apache/datafusion/pull/25009
## Which issue does this PR close?
- Closes #24695.
## Rationale for this change
Physical-plan serialization loses projection field and schema metadata when it differs from metadata derived from the input and expressions.
## What changes are included in this PR?
- Add an optional schema to `ProjectionExecNode`, emitted only when the projection overrides metadata.
- Restore metadata through `try_new_with_schema_metadata`, keeping field names, types, and nullability derived from the expressions.
- Preserve decoding of older payloads without the optional field.
## What is the testing strategy for this PR?
- `roundtrip_projection_metadata` reproduces the loss before the fix and passes afterward.
- Added binary and JSON coverage for overrides, explicit clearing, extension metadata, and older payloads. `decode_projection_schema_only_replaces_metadata` checks expression-derived field properties and invalid field counts.
- The required extended workspace test command passes, including 2,014 physical-plan tests, 268 protobuf integration tests, and 511 SQL logic-test files.
- All-target, all-feature Clippy passes for the three affected crates; the contributor guide's documented lint steps also pass.
- Full-workspace Clippy with all features enabled hits the existing PostgreSQL decimal-formatting lint in #24974; the affected source is unchanged here.
## Are there any user-facing changes?
Projection metadata survives physical-plan serialization. The generated `ProjectionExecNode` Rust struct gains an optional `schema` field. New readers continue to accept older payloads. This targets main, not a 55.1 backport.
--
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: preserve ProjectionExec metadata during serialization [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25009:
URL: https://github.com/apache/datafusion/pull/25009#issuecomment-5566585127
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25009?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 `77.77778%` with `6 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.75%. Comparing base ([`e1ca94f`](https://app.codecov.io/gh/apache/datafusion/commit/e1ca94fb11ab0f2ead9bddd2b5be6666e5d75c62?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`d58dcb7`](https://app.codecov.io/gh/apache/datafusion/commit/d58dcb752c8c5a18765ca39ca0595d415568375b?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/25009?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/physical-plan/src/projection.rs](https://app.codecov.io/gh/apache/datafusion/pull/25009?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Fprojection.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9wcm9qZWN0aW9uLnJz) | 78.57% | [0 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25009?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/pbjson.rs](https://app.codecov.io/gh/apache/datafusion/pull/25009?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=) | 76.92% | [1 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25009?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 #25009 +/- ##
==========================================
+ Coverage 81.69% 81.75% +0.05%
==========================================
Files 1127 1127
Lines 415471 415680 +209
Branches 415471 415680 +209
==========================================
+ Hits 339424 339844 +420
+ Misses 56108 55850 -258
- Partials 19939 19986 +47
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25009?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: preserve ProjectionExec metadata during serialization [datafusion]
Posted by "goutamadwant (via GitHub)" <gi...@apache.org>.
goutamadwant commented on code in PR #25009:
URL: https://github.com/apache/datafusion/pull/25009#discussion_r4002707877
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -673,20 +673,25 @@ impl ExecutionPlan for ProjectionExec {
metrics: _,
// Derived plan properties, recomputed on decode.
cache: _,
- // Derived metadata comparison, recomputed with the projector.
- overrides_metadata: _,
+ overrides_metadata,
} = self;
let projection_exprs = projector.projection().as_ref();
let input = ctx.encode_child(input)?;
let expr = ctx.encode_expressions(projection_exprs.iter().map(|p| &p.expr))?;
let expr_name = projection_exprs.iter().map(|p| p.alias.clone()).collect();
+ let schema = if *overrides_metadata {
Review Comment:
@gene-bordegaray Updated the condition to encode schema or output-field metadata even when inherited. The override check remains so an empty metadata map can still explicitly clear input metadata.
--
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: preserve ProjectionExec metadata during serialization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #25009:
URL: https://github.com/apache/datafusion/pull/25009#discussion_r3958383080
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -673,20 +673,25 @@ impl ExecutionPlan for ProjectionExec {
metrics: _,
// Derived plan properties, recomputed on decode.
cache: _,
- // Derived metadata comparison, recomputed with the projector.
- overrides_metadata: _,
+ overrides_metadata,
} = self;
let projection_exprs = projector.projection().as_ref();
let input = ctx.encode_child(input)?;
let expr = ctx.encode_expressions(projection_exprs.iter().map(|p| &p.expr))?;
let expr_name = projection_exprs.iter().map(|p| p.alias.clone()).collect();
+ let schema = if *overrides_metadata {
Review Comment:
This conditioins shouldnt just be if we are overriding metadata I believe. Rather we should be checking if we have metadata in general.
##########
datafusion/proto/tests/cases/plans/exprs.rs:
##########
@@ -43,9 +43,176 @@ use datafusion_proto::physical_plan::{
};
use datafusion_proto::protobuf;
use datafusion_proto::protobuf::PhysicalPlanNode;
+use prost::Message;
+use std::collections::HashMap;
use std::sync::Arc;
use std::vec;
+#[test]
+fn roundtrip_projection_metadata() -> Result<()> {
+ let input_schema = Arc::new(Schema::new(vec![Field::new(
+ "value",
+ DataType::Int32,
+ false,
+ )]));
+ let projected_schema = Schema::new_with_metadata(
+ vec![Field::new("value", DataType::Int32, false).with_metadata(
+ [("field-key".to_string(), "field-value".to_string())].into(),
+ )],
+ [("schema-key".to_string(), "schema-value".to_string())].into(),
+ );
+ let plan = Arc::new(ProjectionExec::try_new_with_schema_metadata(
+ vec![(col("value", &input_schema)?, "value".to_string())],
+ Arc::new(EmptyExec::new(input_schema)),
+ &projected_schema,
+ )?);
+ let ctx = SessionContext::new();
+ let codec = DefaultPhysicalExtensionCodec {};
+ let converter = DefaultPhysicalProtoConverter {};
+ let decoded = roundtrip_test_and_return(plan, &ctx, &codec, &converter)?;
+ assert_eq!(decoded.schema().as_ref(), &projected_schema);
+ Ok(())
+}
+
+#[test]
+fn roundtrip_projection_metadata_overrides() -> Result<()> {
+ let field_metadata = [("field-key".to_string(), "field-value".to_string())].into();
+ let extension_metadata = [
+ ("ARROW:extension:name".to_string(), "arrow.uuid".to_string()),
+ ("ARROW:extension:metadata".to_string(), String::new()),
+ ]
+ .into();
+ for (input_field, output_field, input_metadata) in [
+ (
+ Field::new("value", DataType::Int32, false).with_metadata(field_metadata),
+ Field::new("value", DataType::Int32, false),
+ [("input-schema".to_string(), "input-value".to_string())].into(),
+ ),
+ (
+ Field::new("value", DataType::FixedSizeBinary(16), true),
+ Field::new("value", DataType::FixedSizeBinary(16), true)
+ .with_metadata(extension_metadata),
+ HashMap::new(),
+ ),
+ ] {
+ let input_schema =
+ Arc::new(Schema::new_with_metadata(vec![input_field], input_metadata));
+ let projected_schema = Schema::new(vec![output_field]);
+ let plan = Arc::new(ProjectionExec::try_new_with_schema_metadata(
+ vec![(col("value", &input_schema)?, "value".to_string())],
+ Arc::new(EmptyExec::new(input_schema)),
+ &projected_schema,
+ )?);
+ let codec = DefaultPhysicalExtensionCodec {};
+ let ctx = SessionContext::new();
+ let node = PhysicalPlanNode::try_from_physical_plan(plan, &codec)?;
+ let Some(protobuf::physical_plan_node::PhysicalPlanType::Projection(projection)) =
+ node.physical_plan_type.as_ref()
+ else {
+ unreachable!("expected ProjectionExecNode")
+ };
+ assert!(projection.schema.is_some());
+ let node = PhysicalPlanNode::decode(node.encode_to_vec().as_slice()).unwrap();
+ #[cfg(feature = "json")]
+ let node: PhysicalPlanNode =
+ serde_json::from_str(&serde_json::to_string(&node).unwrap()).unwrap();
+ let decoded = node.try_into_physical_plan(&ctx.task_ctx(), &codec)?;
+ assert_eq!(decoded.schema().as_ref(), &projected_schema);
+ }
+ Ok(())
+}
+
+#[test]
+fn roundtrip_projection_without_metadata_override() -> Result<()> {
+ let input_schema = Arc::new(Schema::new_with_metadata(
+ vec![Field::new("value", DataType::Int32, false).with_metadata(
+ [("field-key".to_string(), "field-value".to_string())].into(),
+ )],
+ [("schema-key".to_string(), "schema-value".to_string())].into(),
+ ));
+ let plan = Arc::new(ProjectionExec::try_new(
+ vec![(col("value", &input_schema)?, "value".to_string())],
+ Arc::new(EmptyExec::new(Arc::clone(&input_schema))),
Review Comment:
this is preserving the metadata through the child after decoding, not a proojection that needs to rederive the metadata completely from itself.
Could we add a test that forces the projection to completely rederive the metadata from its own proto after roundtrip đź‘Ť
--
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: preserve ProjectionExec metadata during serialization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on PR #25009:
URL: https://github.com/apache/datafusion/pull/25009#issuecomment-5749301597
actually this may be agood question for @timsaucer . For serialization do we usually want to uphold that any node can be reconstructed completely independently of the rest of the plan or is it ok for a node to rely on a child's protobuf for infromation such as metadata in this case?
--
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: preserve ProjectionExec metadata during serialization [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #25009:
URL: https://github.com/apache/datafusion/pull/25009#issuecomment-6060821743
Thanks @goutamadwant @gene-bordegaray , I think this is ready to go!
--
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: preserve ProjectionExec metadata during serialization [datafusion]
Posted by "goutamadwant (via GitHub)" <gi...@apache.org>.
goutamadwant commented on code in PR #25009:
URL: https://github.com/apache/datafusion/pull/25009#discussion_r4060126866
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -673,20 +673,34 @@ impl ExecutionPlan for ProjectionExec {
metrics: _,
// Derived plan properties, recomputed on decode.
cache: _,
- // Derived metadata comparison, recomputed with the projector.
- overrides_metadata: _,
+ overrides_metadata,
} = self;
let projection_exprs = projector.projection().as_ref();
let input = ctx.encode_child(input)?;
let expr = ctx.encode_expressions(projection_exprs.iter().map(|p| &p.expr))?;
let expr_name = projection_exprs.iter().map(|p| p.alias.clone()).collect();
+ let output_schema = projector.output_schema();
+ // Keep inherited metadata self-contained, and retain empty overrides
Review Comment:
@gene-bordegaray “Self-contained” refers to restoring output field/schema metadata even when the child does not preserve it. The child and expressions still determine names, types, and nullability. I've clarified this boundary in the description. let me know. thanks!
--
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: preserve ProjectionExec metadata during serialization [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on PR #25009:
URL: https://github.com/apache/datafusion/pull/25009#issuecomment-5794024710
> Yes I see. I am mostly asking tim if the condition I am concerned about which is for serialization and deserilization is it typically good practice in this repo to have all nodes be able to deserialize without any dependency to children nodes. I was under the assumption that this was the case but here it causes us to have to serialize more over the wire then we would need to in some cases so we may not actually want to if not needed.
In my opinion, the litmus test is this: For any given variation, does serialization + deserialization produce an identical result? Second to that, can either approach demonstrably do this faithful reproduction with a smaller byte footprint? I haven't gone through and considered all of the permutations of this, but I think you do want to do a little testing around metadata at different levels and demonstrate the faithful reproduction. Does that answer the question?
--
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: preserve ProjectionExec metadata during serialization [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #25009:
URL: https://github.com/apache/datafusion/pull/25009#issuecomment-5566338986
<!-- 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-physical-plan v55.0.0 (current)
Built [ 40.801s] (current)
Parsing datafusion-physical-plan v55.0.0 (current)
Parsed [ 0.157s] (current)
Building datafusion-physical-plan v55.0.0 (baseline)
Built [ 39.944s] (baseline)
Parsing datafusion-physical-plan v55.0.0 (baseline)
Parsed [ 0.157s] (baseline)
Checking datafusion-physical-plan v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 0.693s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 83.677s] datafusion-physical-plan
Building datafusion-proto v55.0.0 (current)
Built [ 55.621s] (current)
Parsing datafusion-proto v55.0.0 (current)
Parsed [ 0.018s] (current)
Building datafusion-proto v55.0.0 (baseline)
Built [ 54.730s] (baseline)
Parsing datafusion-proto v55.0.0 (baseline)
Parsed [ 0.019s] (baseline)
Checking datafusion-proto 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 [ 111.737s] datafusion-proto
Building datafusion-proto-models v55.0.0 (current)
Built [ 25.712s] (current)
Parsing datafusion-proto-models v55.0.0 (current)
Parsed [ 0.131s] (current)
Building datafusion-proto-models v55.0.0 (baseline)
Built [ 25.619s] (baseline)
Parsing datafusion-proto-models v55.0.0 (baseline)
Parsed [ 0.134s] (baseline)
Checking datafusion-proto-models v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 1.711s] 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 ProjectionExecNode.schema in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:2207
field ProjectionExecNode.schema in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:2207
Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [ 54.657s] 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: preserve ProjectionExec metadata during serialization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on PR #25009:
URL: https://github.com/apache/datafusion/pull/25009#issuecomment-5816750254
> > Yes I see. I am mostly asking tim if the condition I am concerned about which is for serialization and deserilization is it typically good practice in this repo to have all nodes be able to deserialize without any dependency to children nodes. I was under the assumption that this was the case but here it causes us to have to serialize more over the wire then we would need to in some cases so we may not actually want to if not needed.
>
> In my opinion, the litmus test is this: For any given variation, does serialization + deserialization produce an identical result? Second to that, can either approach demonstrably do this faithful reproduction with a smaller byte footprint? I haven't gone through and considered all of the permutations of this, but I think you do want to do a little testing around metadata at different levels and demonstrate the faithful reproduction. Does that answer the question?
Yes this does help thank you. I do believe that our checks are necessary so that these nodes can be reconstructured independent of other nodes in the plan. This will add some bytes in cases but will make this safer.
--
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: preserve ProjectionExec metadata during serialization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on PR #25009:
URL: https://github.com/apache/datafusion/pull/25009#issuecomment-5762652647
> > I the last commit this makes the condition different and incorrect as inherited metadata will be dropped.
> > Also the last commit deleted the test that covered the case that wold catch this, so let's keep that đź‘Ť
>
> @gene-bordegaray retained the restored inherited metadata and child-independent reconstruction test. Fixed the Arrow 60 compilation failure with explicit metadata maps.
Yes I see. I am mostly asking tim if the condition I am concerned about which is for serialization and deserilization is it typically good practice in this repo to have all nodes be able to deserialize without any dependency to children nodes. I was under the assumption that this was the case but here it causes us to have to serialize more over the wire then we would need to in some cases so we may not actually want to if not needed.
--
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: preserve ProjectionExec metadata during serialization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on PR #25009:
URL: https://github.com/apache/datafusion/pull/25009#issuecomment-5730527686
@jayzhan211 bumping this guy for visibility don't want it to get lost
--
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: preserve ProjectionExec metadata during serialization [datafusion]
Posted by "goutamadwant (via GitHub)" <gi...@apache.org>.
goutamadwant commented on PR #25009:
URL: https://github.com/apache/datafusion/pull/25009#issuecomment-5757089510
> I the last commit this makes the condition different and incorrect as inherited metadata will be dropped.
>
> Also the last commit deleted the test that covered the case that wold catch this, so let's keep that đź‘Ť
@gene-bordegaray retained the restored inherited metadata and child-independent reconstruction test. Fixed the Arrow 60 compilation failure with explicit metadata maps.
--
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: preserve ProjectionExec metadata during serialization [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #25009:
URL: https://github.com/apache/datafusion/pull/25009#discussion_r4047300298
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -673,20 +673,34 @@ impl ExecutionPlan for ProjectionExec {
metrics: _,
// Derived plan properties, recomputed on decode.
cache: _,
- // Derived metadata comparison, recomputed with the projector.
- overrides_metadata: _,
+ overrides_metadata,
} = self;
let projection_exprs = projector.projection().as_ref();
let input = ctx.encode_child(input)?;
let expr = ctx.encode_expressions(projection_exprs.iter().map(|p| &p.expr))?;
let expr_name = projection_exprs.iter().map(|p| p.alias.clone()).collect();
+ let output_schema = projector.output_schema();
+ // Keep inherited metadata self-contained, and retain empty overrides
+ // that explicitly clear metadata from the input.
+ let schema = if *overrides_metadata
Review Comment:
The `|| !metadata.is_empty()` arms encode a full `Schema` for plain `try_new` projections, whose metadata decode already re-derives from the child. Measured on a 200-col identity projection with one metadata key per field: the projection node grows by ~6.9 KB (plan 10,354 → 17,248 bytes), repeated for every projection in a stack. `roundtrip_projection_metadata_without_child_metadata` only passes by swapping the child inside the proto, which encode never produces; `overrides_metadata` alone fixes #24695 and the explicit-clear case.
Fine as a follow-up
```diff
- let schema = if *overrides_metadata
- || !output_schema.metadata().is_empty()
- || output_schema
- .fields()
- .iter()
- .any(|field| !field.metadata().is_empty())
- {
- Some(output_schema.as_ref().try_into()?)
- } else {
- None
- };
+ // Inherited metadata is re-derived from the child on decode.
+ let schema = overrides_metadata
+ .then(|| projector.output_schema().as_ref().try_into())
+ .transpose()?;
```
--
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: preserve ProjectionExec metadata during serialization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on PR #25009:
URL: https://github.com/apache/datafusion/pull/25009#issuecomment-5817053835
Sorry for the back and forth on this @goutamadwant
--
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: preserve ProjectionExec metadata during serialization [datafusion]
Posted by "goutamadwant (via GitHub)" <gi...@apache.org>.
goutamadwant commented on code in PR #25009:
URL: https://github.com/apache/datafusion/pull/25009#discussion_r4002711274
##########
datafusion/proto/tests/cases/plans/exprs.rs:
##########
@@ -43,9 +43,176 @@ use datafusion_proto::physical_plan::{
};
use datafusion_proto::protobuf;
use datafusion_proto::protobuf::PhysicalPlanNode;
+use prost::Message;
+use std::collections::HashMap;
use std::sync::Arc;
use std::vec;
+#[test]
+fn roundtrip_projection_metadata() -> Result<()> {
+ let input_schema = Arc::new(Schema::new(vec![Field::new(
+ "value",
+ DataType::Int32,
+ false,
+ )]));
+ let projected_schema = Schema::new_with_metadata(
+ vec![Field::new("value", DataType::Int32, false).with_metadata(
+ [("field-key".to_string(), "field-value".to_string())].into(),
+ )],
+ [("schema-key".to_string(), "schema-value".to_string())].into(),
+ );
+ let plan = Arc::new(ProjectionExec::try_new_with_schema_metadata(
+ vec![(col("value", &input_schema)?, "value".to_string())],
+ Arc::new(EmptyExec::new(input_schema)),
+ &projected_schema,
+ )?);
+ let ctx = SessionContext::new();
+ let codec = DefaultPhysicalExtensionCodec {};
+ let converter = DefaultPhysicalProtoConverter {};
+ let decoded = roundtrip_test_and_return(plan, &ctx, &codec, &converter)?;
+ assert_eq!(decoded.schema().as_ref(), &projected_schema);
+ Ok(())
+}
+
+#[test]
+fn roundtrip_projection_metadata_overrides() -> Result<()> {
+ let field_metadata = [("field-key".to_string(), "field-value".to_string())].into();
+ let extension_metadata = [
+ ("ARROW:extension:name".to_string(), "arrow.uuid".to_string()),
+ ("ARROW:extension:metadata".to_string(), String::new()),
+ ]
+ .into();
+ for (input_field, output_field, input_metadata) in [
+ (
+ Field::new("value", DataType::Int32, false).with_metadata(field_metadata),
+ Field::new("value", DataType::Int32, false),
+ [("input-schema".to_string(), "input-value".to_string())].into(),
+ ),
+ (
+ Field::new("value", DataType::FixedSizeBinary(16), true),
+ Field::new("value", DataType::FixedSizeBinary(16), true)
+ .with_metadata(extension_metadata),
+ HashMap::new(),
+ ),
+ ] {
+ let input_schema =
+ Arc::new(Schema::new_with_metadata(vec![input_field], input_metadata));
+ let projected_schema = Schema::new(vec![output_field]);
+ let plan = Arc::new(ProjectionExec::try_new_with_schema_metadata(
+ vec![(col("value", &input_schema)?, "value".to_string())],
+ Arc::new(EmptyExec::new(input_schema)),
+ &projected_schema,
+ )?);
+ let codec = DefaultPhysicalExtensionCodec {};
+ let ctx = SessionContext::new();
+ let node = PhysicalPlanNode::try_from_physical_plan(plan, &codec)?;
+ let Some(protobuf::physical_plan_node::PhysicalPlanType::Projection(projection)) =
+ node.physical_plan_type.as_ref()
+ else {
+ unreachable!("expected ProjectionExecNode")
+ };
+ assert!(projection.schema.is_some());
+ let node = PhysicalPlanNode::decode(node.encode_to_vec().as_slice()).unwrap();
+ #[cfg(feature = "json")]
+ let node: PhysicalPlanNode =
+ serde_json::from_str(&serde_json::to_string(&node).unwrap()).unwrap();
+ let decoded = node.try_into_physical_plan(&ctx.task_ctx(), &codec)?;
+ assert_eq!(decoded.schema().as_ref(), &projected_schema);
+ }
+ Ok(())
+}
+
+#[test]
+fn roundtrip_projection_without_metadata_override() -> Result<()> {
+ let input_schema = Arc::new(Schema::new_with_metadata(
+ vec![Field::new("value", DataType::Int32, false).with_metadata(
+ [("field-key".to_string(), "field-value".to_string())].into(),
+ )],
+ [("schema-key".to_string(), "schema-value".to_string())].into(),
+ ));
+ let plan = Arc::new(ProjectionExec::try_new(
+ vec![(col("value", &input_schema)?, "value".to_string())],
+ Arc::new(EmptyExec::new(Arc::clone(&input_schema))),
Review Comment:
@gene-bordegaray added a regression that removes metadata from the encoded child and checks the decoded projection independently. Let me know if it looks good now.
--
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: preserve ProjectionExec metadata during serialization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #25009:
URL: https://github.com/apache/datafusion/pull/25009#discussion_r4017913922
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -673,20 +673,34 @@ impl ExecutionPlan for ProjectionExec {
metrics: _,
// Derived plan properties, recomputed on decode.
cache: _,
- // Derived metadata comparison, recomputed with the projector.
- overrides_metadata: _,
+ overrides_metadata,
} = self;
let projection_exprs = projector.projection().as_ref();
let input = ctx.encode_child(input)?;
let expr = ctx.encode_expressions(projection_exprs.iter().map(|p| &p.expr))?;
let expr_name = projection_exprs.iter().map(|p| p.alias.clone()).collect();
+ let output_schema = projector.output_schema();
+ // Keep inherited metadata self-contained, and retain empty overrides
Review Comment:
what does self-contained here mean
--
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: preserve ProjectionExec metadata during serialization [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on PR #25009:
URL: https://github.com/apache/datafusion/pull/25009#issuecomment-5730711676
LGTM but it looks like you've got at least one unanswered question from your review. @goutamadwant are you able to address the question about self contained?
--
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: preserve ProjectionExec metadata during serialization [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 merged PR #25009:
URL: https://github.com/apache/datafusion/pull/25009
--
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