Repository navigation
Closed
Labels
good first issueGood for newcomersGood for newcomers
Description
Sub-issue of #22418. Port CaseExpr to own its protobuf (de)serialization via the try_to_proto / try_from_proto hooks added in #21929, removing it from the central downcast chain.
Reference: copy the Column migration in #21929. Mind the asymmetry — try_to_proto is a trait override, try_from_proto is an inherent fn (see steps).
Where CaseExpr lives
datafusion/physical-expr/src/expressions/case.rs (crate datafusion-physical-expr).
Steps
- Add
try_to_protoinside theimpl PhysicalExpr for CaseExprblock — it overrides a trait method, so it must live there, feature-gated#[cfg(feature = "proto")].
⚠️ Do not put it in an inherentimpl CaseExprblock. The serializer calls it through&dyn PhysicalExpr, so an inherent method is silently never reached and serialization keeps using the old chain. This is the most common review bounce on these PRs. - Add
CaseExpr::try_from_proto(node, ctx)as an inherent associated fn (#[cfg(feature = "proto")] impl CaseExpr) and wire it into the matchingExprType::…arm indatafusion/proto/src/physical_plan/from_proto.rs. - In this same PR, delete the
CaseExprarm fromdatafusion/proto/src/physical_plan/to_proto.rsand the oldExprType::…decode arm infrom_proto.rs. This is not cleanup-for-later — it is the only thing that proves your hook is actually reached. If the roundtrip tests still pass after deleting the arm, it works; if you leave the arm, the tests pass through dead code. - Keep the wire format byte-for-byte identical.
expr_idisNonehere (onlyDynamicFilterPhysicalExprsets it). - Keep
use datafusion_proto_models::protobuf;inside the method bodies (matches theColumnreference).
Tests
- Existing
roundtrip_physical_plan/roundtrip_physical_exprmust still pass:cargo test -p datafusion-proto --test proto_integration. - Add a direct hook test in
datafusion/physical-expr/src/expressions/case.rscovering both directions and a bad-input case (wrongexpr_typevariant / missing child → clean error, not panic). Reviewers have asked for this on every reviewed PR. - Put
#[cfg(test)] mod testsat the end of the file orclippy -D warningsfails (items-after-test-module).
Before you push
- Fill in every section of the PR template.
cargo fmt --allandcargo clippy --all-targets --all-features -- -D warningsmust pass.
Checklist
-
try_to_protoinimpl PhysicalExpr for CaseExpr(not inherent),#[cfg(feature="proto")] -
CaseExpr::try_from_protowired intofrom_proto.rs - central
to_proto.rs+from_proto.rsarms deleted in this PR - direct hook test incl. bad-input case; test module at end of file
- roundtrip tests pass;
fmt+clippy -D warningsclean; PR template filled in
Metadata
Metadata
Assignees
Labels
good first issueGood for newcomersGood for newcomers
Activity
chakkk309 commentedon May 28, 2026
take
adriangb commentedon May 28, 2026
Please base your PR on the helpers documented in #22418 (added in #22596). If you've already written this against the older inline pattern, the migration is small — see the second commit of #22596 for examples.
adriangb commentedon Jun 8, 2026
@chakkk309 are you working on this?
chakkk309 commentedon Jun 9, 2026
Yes, I opened #22838 for this. It ports CaseExpr to the try_to_proto / try_from_proto hooks and follows the helper pattern from #22418 / #22596.
adriangb commentedon Jun 9, 2026
THanks @chakkk309
refactor: Port CaseExpr proto serialization hooks (apache#22838)
refactor: Port CaseExpr proto serialization hooks (apache#22838)