feat(physical-expr): port Literal to try_to_proto / try_from_proto hooks#22636
feat(physical-expr): port Literal to try_to_proto / try_from_proto hooks#22636koopatroopa787 wants to merge 1 commit into
Conversation
Move protobuf serialization/deserialization for Literal out of the central downcast chains in to_proto.rs and from_proto.rs into dedicated try_to_proto / try_from_proto hooks on Literal itself, following the pattern established for NotExpr, NegativeExpr, IsNullExpr, and IsNotNullExpr. Closes apache#22427
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Refactors the protobuf serialization/deserialization of Literal physical expressions to use the per-expression try_to_proto / try_from_proto hook pattern (instead of being centrally handled in to_proto.rs / from_proto.rs).
Changes:
- Removed the inline
Literalarm fromserialize_physical_expr_with_converterinto_proto.rsand dropped the unusedLiteralimport. - Routed
ExprType::Literaldecoding throughLiteral::try_from_protoinfrom_proto.rs. - Added
try_to_protoimpl andtry_from_protoconstructor onLiteral, plus unit tests under aproto_testsmodule.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| datafusion/proto/src/physical_plan/to_proto.rs | Removed the inline literal serialization branch and unused Literal import. |
| datafusion/proto/src/physical_plan/from_proto.rs | Delegates literal decoding to Literal::try_from_proto. |
| datafusion/physical-expr/src/expressions/literal.rs | Adds try_to_proto/try_from_proto methods and proto tests for Literal. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
@koopatroopa787
Thanks for the fix. I took a look and did not find any blocking issues. I have one small suggestion that could strengthen the test coverage.
| .unwrap() | ||
| .expect("null Literal should encode to Some(node)"); | ||
|
|
||
| assert!(matches!( |
There was a problem hiding this comment.
Nice test. One thing that might make it a bit stronger is to verify the decoded payload as well, not just the proto variant.
Since the contract is that this round-trips as a null Int32 literal, could we also decode the literal payload and assert that it equals ScalarValue::Int32(None)? That would help ensure both the variant and the encoded value are preserved correctly.
Which issue does this PR close?
Closes #22427
Rationale for this change
Literalserialization/deserialization lived in the central downcast chains into_proto.rsandfrom_proto.rs. This PR moves it into self-containedtry_to_proto/try_from_protohooks onLiteralitself, following the pattern established forNotExpr,NegativeExpr,IsNullExpr, andIsNotNullExpr.What changes are included in this PR?
datafusion/physical-expr/src/expressions/literal.rs#[cfg(feature = "proto")] fn try_to_proto(...)insideimpl PhysicalExpr for Literal#[cfg(feature = "proto")] impl Literal { pub fn try_from_proto(...) }proto_testsmodule with encode, null-literal, roundtrip, and reject-wrong-variant testsdatafusion/proto/src/physical_plan/to_proto.rsLiteraldowncast arm; removedLiteralfrom import listdatafusion/proto/src/physical_plan/from_proto.rsExprType::Literalarm withLiteral::try_from_proto(proto, &decode_ctx)?Are there any user-facing changes?
No. Serialization behaviour is identical; only the code location changed.
🤖 Generated with Claude Code