Skip to content

refactor: Migrate ScalarSubqueryExpr to self-serialization proto pattern#23130

Open
mattp5657 wants to merge 1 commit into
apache:mainfrom
mattp5657:add_proto_conversion_scalar_subquery
Open

refactor: Migrate ScalarSubqueryExpr to self-serialization proto pattern#23130
mattp5657 wants to merge 1 commit into
apache:mainfrom
mattp5657:add_proto_conversion_scalar_subquery

Conversation

@mattp5657

@mattp5657 mattp5657 commented Jun 23, 2026

Copy link
Copy Markdown

Move ScalarSubqueryExpr proto encode/decode into the expression itself via the try_to_proto / try_from_proto hooks, removing the centralized downcast branch in to_proto.rs and the inline construction in from_proto.rs. Because the ScalarSubqueryResults container is runtime-only shared state and not part of the wire format, from_proto.rs still fetches it from the decode context and threads it into try_from_proto.

Follows the pattern established in #21929.

Which issue does this PR close?

Rationale for this change

datafusion-proto serializes ScalarSubqueryExpr by downcasting in to_proto.rs and rebuilding it inline in from_proto.rs, which forces its internal fields to be exposed as public accessors just for the proto crate.

PR #21929 introduced a self-serialization pattern (try_to_proto / try_from_proto) where each expression owns its own wire format and the central dispatch just delegates. Other expressions like InListExpr and DynamicFilterPhysicalExpr already use it. This change moves ScalarSubqueryExpr to the same pattern, removing its bespoke proto branches and the proto-only public accessors.

One wrinkle: ScalarSubqueryExpr's ScalarSubqueryResults is runtime-only shared state, not part of the wire format, so try_from_proto takes it as an extra argument that from_proto.rs threads in from the decode context.

What changes are included in this PR?

  • try_to_proto in impl PhysicalExpr for ScalarSubqueryExpr (not inherent), #[cfg(feature = "proto")]
  • ScalarSubqueryExpr::try_from_proto wired into from_proto.rs
  • central to_proto.rs + from_proto.rs arms deleted in this PR
  • direct hook test incl. bad-input case; test module at end of file
  • roundtrip tests pass; fmt + clippy -D warnings clean; PR template filled in

Are these changes tested?

  • Unit tests (scalar_subquery.rs, #[cfg(all(test, feature = "proto"))] module at end of file) exercise the hooks directly:
    • round_trips_through_proto — encode via try_to_proto, decode via try_from_proto, asserting data_type/nullable/index and shared-results identity survive.
    • rejects_non_scalar_subquery_node — wrong ExprType variant errors cleanly.
    • rejects_missing_data_type — missing required field errors cleanly.
  • Round-trip integration tests (datafusion-proto, --all-features) cover the full plan path: roundtrip_scalar_subquery_exec, roundtrip_nested_scalar_subquery_exec_scopes_results, and roundtrip_scalar_subquery_exec_with_default_converter_executes.

All pass, and cargo fmt --all + cargo clippy --all-features --all-targets -- -D warnings are clean.

Are there any user-facing changes?

No

@github-actions github-actions Bot added physical-expr Changes to the physical-expr crates proto Related to proto crate labels Jun 23, 2026
Move ScalarSubqueryExpr proto encode/decode into the expression itself
via the try_to_proto / try_from_proto hooks, removing the centralized
downcast branch in to_proto.rs and the inline construction in
from_proto.rs. Because the ScalarSubqueryResults container is runtime-only
shared state and not part of the wire format, from_proto.rs still fetches
it from the decode context and threads it into try_from_proto.

Follows the pattern established in apache#21929.
@mattp5657 mattp5657 force-pushed the add_proto_conversion_scalar_subquery branch from 690d82c to 4abb2f5 Compare June 23, 2026 18:06
@mattp5657 mattp5657 marked this pull request as ready for review June 23, 2026 18:13
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.

1 participant