Skip to content

refactor: port Literal physical expr proto hooks#22492

Closed
officialasishkumar wants to merge 1 commit into
apache:mainfrom
officialasishkumar:refactor/literal-proto-hooks
Closed

refactor: port Literal physical expr proto hooks#22492
officialasishkumar wants to merge 1 commit into
apache:mainfrom
officialasishkumar:refactor/literal-proto-hooks

Conversation

@officialasishkumar

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

DataFusion is migrating built-in physical expressions to own their protobuf serialization and deserialization through the try_to_proto / try_from_proto hooks added in #21929. Literal still used the central physical expression proto match.

What changes are included in this PR?

This ports Literal to the per-expression proto hook pattern and updates physical expression decoding to delegate literal reconstruction to Literal::try_from_proto. The protobuf wire format is unchanged.

Are these changes tested?

Yes.

  • cargo fmt --all -- --check
  • cargo test -p datafusion-proto --test proto_integration cases::roundtrip_physical_plan::roundtrip_filter_with_not_and_in_list
  • cargo test -p datafusion-proto --test proto_integration cases::roundtrip_physical_plan::roundtrip_hash_table_lookup_expr_to_lit
  • cargo test -p datafusion-proto --test proto_integration cases::roundtrip_physical_plan::roundtrip_call_null_scalar_struct_dict
  • cargo test -p datafusion-proto --test proto_integration cases::roundtrip_physical_plan::test_round_trip_human_display
  • git diff --check

Are there any user-facing changes?

No. This is an internal serialization refactor with the same serialized representation.

@github-actions github-actions Bot added physical-expr Changes to the physical-expr crates proto Related to proto crate labels May 24, 2026

#[cfg(feature = "proto")]
impl Literal {
/// Reconstruct a [`Literal`] from its protobuf representation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A single round trip unit test in a test mod like this one would be great:

#[cfg(all(test, feature = "proto"))]
mod proto_tests {

@adriangb

Copy link
Copy Markdown
Contributor

@officialasishkumar please resolve conflicts 🙏🏻

@adriangb

Copy link
Copy Markdown
Contributor

#22596 adds an expect_expr_variant! macro and a require_proto_field helper that together collapse the outer match and any non-expression "missing required field" ok_or_else to one line each. Could you rebase onto it and adopt them before final review?

@adriangb

Copy link
Copy Markdown
Contributor

Thanks for the contribution @officialasishkumar . This was completed in #22636 so I'll close this PR.

@adriangb adriangb closed this Jun 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-expr Changes to the physical-expr crates proto Related to proto crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Port Literal to use try_to_proto / try_from_proto

2 participants