Repository navigation
fix: preserve ProjectionExec metadata during serialization - #25009
goutamadwant wants to merge 7 commits into
Conversation
github-actions
Bot
added
proto
|
Thank you for opening this pull request! Reviewer note: 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25009 +/- ##
==========================================
+ Coverage 82.73% 82.74% +0.01%
==========================================
Files 1147 1147
Lines 449509 449798 +289
Branches 449509 449798 +289
==========================================
+ Hits 371893 372205 +312
+ Misses 54944 54920 -24
- Partials 22672 22673 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jayzhan211
left a comment
jayzhan211
left a comment
There was a problem hiding this comment.
Thanks @goutamadwant !
gene-bordegaray
left a comment
gene-bordegaray
left a comment
There was a problem hiding this comment.
One concern, other than that thank you 👍
| 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 { |
There was a problem hiding this comment.
This conditioins shouldnt just be if we are overriding metadata I believe. Rather we should be checking if we have metadata in general.
There was a problem hiding this 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.
| )); | ||
| let plan = Arc::new(ProjectionExec::try_new( | ||
| vec![(col("value", &input_schema)?, "value".to_string())], | ||
| Arc::new(EmptyExec::new(Arc::clone(&input_schema))), |
There was a problem hiding this 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 👍
There was a problem hiding this 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.
gene-bordegaray
left a comment
gene-bordegaray
left a comment
There was a problem hiding this comment.
this is good to me now. @jayzhan211 and @timsaucer I know you guys were involved on this as well 👍
| 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 |
There was a problem hiding this comment.
what does self-contained here mean
There was a problem hiding this 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!
|
@jayzhan211 bumping this guy for visibility don't want it to get lost |
|
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? |
jayzhan211
left a comment
jayzhan211
left a comment
There was a problem hiding this comment.
One non-blocking suggestion — this can go in as is 🚀
| 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 |
There was a problem hiding this 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
- 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()?;Signed-off-by: goutamadwant <workwithgoutam@gmail.com>
gene-bordegaray
left a comment
gene-bordegaray
left a comment
There was a problem hiding this comment.
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 👍
Signed-off-by: goutamadwant <workwithgoutam@gmail.com>
|
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? |
Signed-off-by: goutamadwant <workwithgoutam@gmail.com>
@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. |
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 that does, in this case I think its ok to keep consition as: let schema = if *overrides_metadata
|| output_schema
.fields()
.iter()
.any(|field| !field.metadata().is_empty())
{
Some(output_schema.as_ref().try_into()?)
} else {
None
};Since we will be able to derive from children and like Tim said this should create a identical result assuming that all nodes are serialized correctly. This will lower the byte foot print and keep behavior if chilren keep their metadata correctly (which they should) |
|
Sorry for the back and forth on this @goutamadwant |
5 participants
Which issue does this PR close?
Rationale for this change
Physical-plan serialization can lose projection field and schema metadata. The projection payload should preserve its output metadata, including inherited metadata, without relying on the child to reconstruct it.
What changes are included in this PR?
ProjectionExecNode, emitted when the projection has schema or output-field metadata, or explicitly clears input metadata.try_new_with_schema_metadata, while deriving field names, data types, and nullability from the expressions.This preserves the projection's metadata independently of child metadata. It does not make the entire execution plan independent of its children. Omitting duplicated inherited metadata is left for a follow-up once the serialization contract is agreed.
What is the testing strategy for this PR?
Binary and JSON round trips cover inherited and overridden metadata, schema-only and field-only metadata, extension metadata, explicit clearing, metadata-free projections, and older payloads. Child-independent reconstruction and field-count validation remain covered.
./dev/rust_lint.sh.Are there any user-facing changes?
Projection schema and output-field metadata survive physical-plan serialization. Nested field definitions remain part of the expression-derived data types. The generated
ProjectionExecNodeRust struct gains an optionalschemafield, affecting exhaustive struct literals. New readers accept older payloads. This targets main, not a 55.1 backport.